code-423n4 / 2023-01-rabbithole-findings

1 stars 2 forks source link

Upgraded Q -> 3 from #619 [1675724566035] #693

Closed c4-judge closed 1 year ago

c4-judge commented 1 year ago

Judge has assessed an item in Issue #619 as 3 risk. The relevant finding follows:

The function `withdrawRemainingTokens can be changed in a safer way to handle the withdraw from the owner and the protocol fee as well. This prevent risks allocated with the protocol fees. By the docs this function is called in two different scenarios, if a quest is full and receipt redeemers equals the max amount of total participants allowed in the quest - only withdrawFee is called. If a quest doesn't hit the max total participants, first the owner calls the function withdrawRemainingTokens to withdraw the remaining tokens and then the fee should be paid with the function withdrawFee.

Overall the best solution of this problem is that the function withdrawRemainingTokens, both does the withdrawing part to the owner and pays the fee to the protocol as well. This is considered the safest way:

First - if the receipt redeemers are below the totalParticipants, can withdraw the remaining tokens and pay the fee at the same time, second if the quest is full and receipt redemeers hits the total amount of people allowed, only the fee will be paid to the protocol and will skip the withdraw remaining rewards part.

function withdrawRemainingTokens(address to) public override onlyOwner { super.withdrawRemainingTokens(to);

    if (receiptRedeemers() < totalParticipants) {

    uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
    uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
    IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);

    IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());

    } else {

    IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());

    }
}
c4-judge commented 1 year ago

kirk-baird marked the issue as duplicate of #42

c4-judge commented 1 year ago

This auto-generated issue was withdrawn by kirk-baird

c4-judge commented 1 year ago

This previously downgraded issue has been upgraded by kirk-baird

c4-judge commented 1 year ago

kirk-baird marked the issue as satisfactory

c4-judge commented 1 year ago

kirk-baird changed the severity to 2 (Med Risk)