RabbitHole Quest Protocol contest - manikantanynala97's results

Lines of code

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

Vulnerability details

Impact

 In the ERC20Quest Contract, the withdrawFee() function can be called after the quest is ended and if the contract has funds that are not claimed 
by the claimers, by that time this function can be called and claim all the remaining funds in the contract.

Proof of Concept

  For example, suppose there are 101 tokens in the contract so 100 tokens are been allocated to claimers and 1 token to the protocolFeeRecipient 
  and after the Quest Ends, they are 50 tokens unclaimed by claimers and 1 token for protocolFeeRecipient, and now the withdrawFee() function 
  can be called a few times and all 51 tokens can be claimed by the protocolFeeRecipient.

Tools Used

 Pen and paper

Recommended Mitigation Steps

 Have a check that the function should only be called once basically like having a variable like bool  protocolFeeRecipientClaimedFees =  false 
 initially and once the  withdrawFee() function is called after the quest then make it protocolFeeRecipientClaimedFees =  true and basically if it 
 called again revert it because the Fee is already claimed.
  

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L96-L118 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L106-L135

Vulnerability details

Impact

 If they are many tokens to be claimed by the claim() function this will actually exceed the block.gaslimit and the claimer cannot claim any token. 
 This  is  because the claim function calls the getOwnedTokenIdsOfQuest() function which has two loops going on it, the claim() function checks in a 
 loop whether the tokens are claimed and also it calls the _setClaimed() function which consists of a loop.

Proof of Concept

  Suppose they are 50 tokens to be claimed by the claimer now the function will surely revert due to block.gaslimit and as mentioned above why will 
  it happens.

Tools Used

 Pen and Paper

Recommended Mitigation Steps

 It is always recommended to have a maximum claiming of rewards per transaction instead of claiming all the tokens at a time. For example, it is 
 better to claim rewards for 10 tokens at a time if the claimer has 50 tokens so that the person can call the function 5 times and claim all the tokens.

Precision Loss varies according to the token for the given Formula ie basically for USDC the decimal points are 6 and whereas for ETH it is 18

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

Check the nonClaimableTokens before transferring by calling the safe transfer function.

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