Receipt owners can cheat on marketplaces claiming before accepting a bid offer

[code423n4]

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_);
     }
 

[c4-judge]

kirk-baird marked the issue as duplicate of #201

[c4-judge]

kirk-baird marked the issue as satisfactory