RabbitHole Quest Protocol contest - evan's results

Lines of code

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

Vulnerability details

Impact

After a quest finishes, there will be some tokens left over. Beside the protocolFee, some of these tokens belong to the quest owner, and some of them belong to participants who have not claimed their reward.

However, withdrawFee() can be called multiple times, and protocolFee() does not decrease. So, anyone can call the withdrawFee function multiple times to drain all the tokens from the Erc20Quest contract to the protocolFeeRecipient.

Proof of Concept

Add the following test to Erc20Quest.spec.ts. It successfully calls withdrawFee twice, getting twice the protocolFee it's supposed to.

describe('withdrawFee()', async () => {
    it('withdrawFee can be called multiple times', async () => {
      const beginningContractBalance = await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)

      await deployedFactoryContract.connect(firstAddress).mintReceipt(questId, messageHash, signature)
      await deployedQuestContract.start()
      await ethers.provider.send('evm_increaseTime', [86400])
      expect(await deployedSampleErc20Contract.balanceOf(protocolFeeAddress)).to.equal(0)

      await deployedQuestContract.connect(firstAddress).claim()
      expect(await deployedSampleErc20Contract.balanceOf(firstAddress.address)).to.equal(1 * 1000)
      expect(beginningContractBalance).to.equal(totalRewardsPlusFee * 100)

      await ethers.provider.send('evm_increaseTime', [100001])
      await deployedQuestContract.withdrawFee()

      expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(
        totalRewardsPlusFee * 100 - 1 * 1000 - 200
      )
      expect(await deployedSampleErc20Contract.balanceOf(protocolFeeAddress)).to.equal(200)

      await deployedQuestContract.withdrawFee()

      expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(
        totalRewardsPlusFee * 100 - 1 * 1000 - 400
      )
      expect(await deployedSampleErc20Contract.balanceOf(protocolFeeAddress)).to.equal(400)


      await ethers.provider.send('evm_increaseTime', [-100001])
      await ethers.provider.send('evm_increaseTime', [-86400])
    })
  })

Tools Used

VSCode, hardhat

Recommended Mitigation Steps

Maintain a counter initialized to 0 (say protocolFeeCount). Set it to receiptRedeemers() at the end of withdrawFee().

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L97 Then, in protocolFee(), this line should be changed to return ((receiptRedeemers() - protocolFeeCount)* rewardAmountInWeiOrTokenId * questFee) / 10_000;

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L117-L123 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L99-L108

Vulnerability details

Impact

There are multiple unbounded for loops in the claim() function of Quest.sol.

The number of iterations that the loop runs depends on the caller's receipt balance. Since receipts can only be claimed once, claimed receipts are basically useless.

However, claimed receipts are not automatically burned, so a user can end up with many of these as they complete more quests. Eventually, the user will have to spend pay a significant amount of gas fee every time they claim their reward.

A malicious user can even speed up this process by transferring their claimed receipts to the victim.

Proof of Concept

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L99 Claim() calls getOwnedTokenIdsOfQuest().

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L117-L123 getOwnedTokenIdsOfQuest() loops through all the receipt tokens owned by the caller. Note that there are also other problematic for loops, but this is the one with the most number of iterations.

Tools Used

VSCode

Recommended Mitigation Steps

The claim function wastes a lot of gas iterating through all the receipts owned by the sender since many of them are for other quests or have already been claimed.

One potential solution is to let the user specify the receipt they would like to claim (i.e. claim(uint tokenId)) Note that now the function would now need to check that

• the sender owns the receipt token
• the receipt token is minted for this quest
• the receipt token hasn't been claimed

Lines of code

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

Vulnerability details

Impact

If withdrawFee() has been called, then the value of nonClaimableTokens in withdrawRemainingTokens() is incorrect. When this happens, the quest owner will get less tokens then they are supposed to when they call withdrawRemainingTokens()

Proof of Concept

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L85 Consider this line of code. Let the current balance = b.

If withdrawFee() is called immediately before withdrawRemainingTokens(), the caller (quest owner) will get b - protocolFee() - protocolFee() - unclaimedTokens tokens back (since balance after withdrawFee is b - protocolFee())

If withdrawRemainingTokens() is not preceded by withdrawFee(), the caller (quest owner) will get b - protocolFee() - unclaimedTokens tokens back

Tools Used

VSCode

Recommended Mitigation Steps

Maintain a counter initialized to 0 (say protocolFeeCount). Set it to receiptRedeemers() at the end of withdrawFee().

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L97 Then, in protocolFee(), this line should be changed to return ((receiptRedeemers() - protocolFeeCount)* rewardAmountInWeiOrTokenId * questFee) / 10_000;