RabbitHole Quest Protocol contest - ForkEth's results

Lines of code

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

Vulnerability details

Impact

The claim() function declared in the Quest.sol does not have a reentrancy guard. Since the "redeemedTokens" are only updated after the transference there is an opportunity to call the function "withdrawRemainingTokens" from ERC20Quest.sol via fallback without the redeemedTokens getting updated. The following statement, states that only non-necessary tokens should be available to be retrieved: "Every receipt minted should still be able to claim rewards". Using the reentrancy previously mentioned a mal-intentioned owner that has a complete Quest can withdraw more than the maximum amount of tokens leaving the contract without enough funds to pay everyone that has completed quests.

Proof of Concept

Malicious owner: claim() --> fallback() call onlyAdminWithdrawAfterEnd(owner_address)

last user that claims a quest: claim() //will fail since there are no more tokens in the contract

Recommended Mitigation Steps

Perform the "redeemedTokens += redeemableTokenCount;" before the _transferRewards.