Erc20Quest#withdrawRemainingTokens() may count remaining tokens incorrectly if withdrawFee() is called first.

[code423n4]

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L81-L87
https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L91-L93
https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L96-L98

Vulnerability details

Summary

Erc20Quest#withdrawRemainingTokens() may count remaining tokens incorrectly if Erc20Quest#withdrawFee() is called first.

Impact

Quest deployer cannot get back the funds from those participants that did not successfully mint a ticket.

Proof of Concept

In Erc20Quest#withdrawRemainingTokens(), the owner can withdraw the remaining tokens that is left in the contract.

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


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

Firstly, the function tabulates the amount of unclaimed tokens, which is the total amount of people that has minted a receipt minus those that already claimed, multiplied by the rewardAmount.

Next, the function calculates the nonClaimableTokens, which is the total amount in the contract minus protocolFee() and unclaimedTokens. ProtocolFee() is calculated by taking the total amount of people that has minted a receipt * amount * questFee and divided by BPS.

function protocolFee() public view returns (uint256) {
    return (receiptRedeemers() * rewardAmountInWeiOrTokenId * questFee) / 10_000;
}

However, the function did not take into account that the protocolFee() might already been withdrawn.

    uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;

If the protocolFee() is already withdrawn, then the nonClimableTokens should not account for the protocolFee anymore. Otherwise, the quest deployer will not be able to withdraw their reward.

Eg There is 1500 USDC in the contract. 500 USDC is already distributed to those that minted and claimed.

There is now 1000 USDC in the contract. 200 USDC is protocol fee, 700 USDC is unclaimed tickets and 100 USDC is leftover for failed quests. Quest deployer calls withdrawRemainingTokens and gets 100 USDC back.

However, if protocolFees is withdrawn already (1000-200 = 800), there will be 800 USDC in the contract (address(this)). 200 USDC is protocol fee (protocolFee), 700 USDC is unclaimed tickets (unclaimedTokens), 100 USDC is leftover. Quest deployer calls withdrawRemainingTokens and attempt to withdraw 100 USDC . However, function will result in an overflow because 800 - 200 - 700 = -100.

Tools Used

Manual Review

Recommended Mitigation Steps

The function should check whether the protocol fee is withdrawn already. A suggestion is to create a modifier in the protocolFee with a boolean, and check that the protocolFee() is not withdrawn yet before calling withdrawRemainingTokens(). Another suggestion would be to put withdrawFee() and withdrawRemainingTokens() in the same function

[c4-judge]

kirk-baird marked the issue as duplicate of #42

[c4-judge]

kirk-baird marked the issue as not a duplicate

[c4-judge]

kirk-baird marked the issue as duplicate of #61

[c4-judge]

kirk-baird changed the severity to 3 (High Risk)

[c4-judge]

kirk-baird marked the issue as satisfactory

[c4-judge]

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