RabbitHole Quest Protocol contest - KmanOfficial's results

Lines of code

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

Vulnerability details

The protocol allows the admin to withdraw the protocolFee after a quest's end time which is calculated based on the number of redeemed receipts times by the reward amount where the questFee is applied to this (initially 20%).

The admin can call protocolFee() several times and this value increases as more users redeem their receipt for the reward, entitling the protocol to collect more fees for hosting the quest.

Impact

The modifier onlyAdminCanWithdraw does not have any functionality stopping the function withdrawFee() from being executed on chain multiple times. There is also no variable that keeps track of how much has been withdrawn in terms of protocol fees. This means the value returned by protocolFee() can be abused and allow for the multiple withdrawals of the protocol fee value if the quest contract's reward token balance is sufficient enough.

Proof of Concept

Provide direct links to all referenced code in GitHub. https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L89-L104

Potential example:

A quest which paid 100 USDC for completing an on-chain task. Max total participants of 50 After the quest end time, only 25 receipts were redeemed for a receipt so far. The contract balance is initially 6000 USDC. After receipt redeems, the balance is 3500.

If an admin called the withdrawFee() function in a transaction, protocolFee() would be calculated to equate to 500 USDC, transferring that amount to the protocolFeeRecipient. As there is no variable holding the amount of fees withdrawn, the admin could call this 6 more times draining the quest contract of any USDC as the reward token to pay the other 25 receipt holders the reward they are due.

Tools Used

Manual Code Review, Visual Studio

Recommended Mitigation Steps

• Create a global variable for ERC20 Quests that keep tracked of how much protocol fees have already been withdrawn which is updated in withdrawFee() just before safeTransfer.
• in withdrawFee(), the amount sent to the protocolFeeReceipt should be protocolFee() - 'alreadyWithdrawn'
• Apply this variable to all the calculations in the various functions that will be affected by this such as: withdrawFee(), withdrawRemainingTokens()

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;