After ERC1155 quest ends, admin can withdraw rewards that are still meant to be claimed

[code423n4]

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc1155Quest.sol#L54-L62

Vulnerability details

Impact

It should be possible for users to claim rewards after quest ends. However, for ERC1155 Quests owner can withdraw all tokens from the contract as soon as the quest ends by calling withdrawRemainingTokens. Then, if someone tries to claim a reward with a valid receipt, the execution will revert due to insufficient balance.

Proof of Concept

The following test reverts due to 'ERC1155: insufficient balance for transfer'. This test:

• mints a receipt for an address
• starts a quest
• rewinds time to end the quest
• initiates withdrawRemainingTokens by the owner
• claims the reward with the receipt minted in the beginning

diff --git a/test/Erc1155Quest.orig.ts b/test/Erc1155Quest.spec.ts
index 8897a21..4630b70 100644
--- a/test/Erc1155Quest.orig.ts
+++ b/test/Erc1155Quest.spec.ts
@@ -232,6 +232,16 @@ describe('Erc1155Quest', () => {
       await ethers.provider.send('evm_increaseTime', [-1000])
     })
 
+    it('should transfer after contest ends', async () => {
+      await deployedRabbitholeReceiptContract.mint(owner.address, questId)
+      await deployedQuestContract.start()
+      await ethers.provider.send('evm_increaseTime', [10000])
+      await deployedQuestContract.withdrawRemainingTokens(owner.address)
+      await deployedQuestContract.claim()
+      expect(await deployedSampleErc1155Contract.balanceOf(owner.address, rewardAmount)).to.equal(1)
+      await ethers.provider.send('evm_increaseTime', [-10000])
+    })
+
     it('should only transfer the correct amount of rewards', async () => {
       await deployedRabbitholeReceiptContract.mint(owner.address, questId)
       await deployedQuestContract.start()

Tools Used

Manual Review

Recommended Mitigation Steps

Similar to ERC20 Quest, I recommend calculating how many receipts haven't been claimed yet and leaving corresponding amount of tokens on the contract.

    function receiptRedeemers() public view returns (uint256) {
        return questFactoryContract.getNumberMinted(questId);
    }

    /// @dev Withdraws the remaining tokens from the contract. Only able to be called by owner
    /// @param to_ The address to send the remaining tokens to
    function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);

        uint unclaimedTokens = receiptRedeemers() - redeemedTokens;

        IERC1155(rewardToken).safeTransferFrom(
            address(this),
            to_,
            rewardAmountInWeiOrTokenId,
            IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId) - unclaimedTokens,
            '0x00'
        );
    }

[c4-judge]

kirk-baird marked the issue as duplicate of #42

[c4-judge]

kirk-baird changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by kirk-baird

[c4-judge]

kirk-baird marked the issue as satisfactory

[c4-judge]

kirk-baird changed the severity to 2 (Med Risk)