Erc20Quest.sol: withdrawRemainingTokens function calculates wrong amount after protocol fees are withdrawn

[code423n4]

Lines of code

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

Vulnerability details

Impact

The Erc20Quest.withdrawRemainingTokens function (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L81-L87) is used to withdraw all remaining reward tokens once the endTime is reached.

The issue is that this function incorporates the protocolFee into its calculation.

The protocolFee however can already be paid out when the Erc20Quest.withdrawRemainingTokens function is called. There is no mechanism by which Erc20Quest.withdrawFee() must be called after Erc20Quest.withdrawRemainingTokens.

At the very least this results in a withdrawn amount that is too small if protocol fees have been withdrawn before.

However the fact that protocol fees are subtracted that are not in the contract anymore can also make the calculation underflow which causes an amount up to the protocol fees to be stuck in the contract.

Proof of Concept

The calculation to determine the amount of tokens to withdraw is this:

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L85

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

Assume the following:

IERC20(rewardToken).balanceOf(address(this)) = 40 USDC
protocolFee() = 50 USDC
unclaimedTokens = 0 USDC

In this situation all 40 USDC should be withdrawn because there are no unclaimedTokens and the protocolFee is already paid out.

However the calculation reverts which causes the 40 USDC to be stuck in the contract.

Tools Used

VSCode

Recommended Mitigation Steps

I discussed this issue with the sponsor and it was decided that probably the best solution is to save the yet to be paid fees in a uint variable that is subtracted from when fees are paid out.

This variable is added to when receipts are minted.

This solution also requires a modification of the data flow, i.e. the quest contract must know when a new receipt is minted.

The sponsor mentioned that this will be investigated as part of a broader refactoring of the data flow.

[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 primary issue

[c4-sponsor]

waynehoover marked the issue as sponsor confirmed

[c4-judge]

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

[c4-judge]

kirk-baird marked issue #122 as primary and marked this issue as a duplicate of 122

[c4-judge]

kirk-baird marked the issue as satisfactory

[c4-judge]

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