Quest Admin/Owner may not be able to withdraw all ERC20 tokens that are non-claimable leaving excess in contract.

[code423n4]

Lines of code

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

Vulnerability details

Where a quest has ended and not all possible receipts were minted, there will be non-claimable rewards which the quest admin/owner can withdraw from the contract. The protocolFee can be withdrawn at multiple times after the quest has ended and therefore will affect alongside receipts redeems the contracts total balance of the ERC20 reward token.

Impact

As protocolFee returns the total overall fees that is due to protocol for this quest based on the number receipt redeems (as mentioned in my other bug finding), there is no global variable which states the amount of fees already withdrawn from the quest. If a withdrawal of fees has taken place before the quest admin/owner proceeds with a transaction to withdraw remaining tokens, the nonClaimableTokens value will be flawed as it takes into account in the calculation tokens that have already been withdrawn as reflected by contract's current balance (IERC20(rewardToken).balanceOf(address(this))) which would have decreased.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

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

Tools Used

Manual Code Review, Visual Studio

Recommended Mitigation Steps

• Track the amount of fees already withdrawn by protocol using a global variable for each quest (specifically ERC20 quests) which is updated in withdrawFee() just before safeTransfer
• Use this variable in calculation of nonClaimableTokens for example: uint256 unclaimedFeeTokens = protocolFee() - 'alreadyWithdrawn'; uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - unclaimedFeeTokens - unclaimedTokens;

[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 marked the issue as satisfactory

[c4-judge]

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