Calling Erc1155Quest.withdrawRemainingTokens function for an ERC1155 quest can possibly withdraw rewards that are associated with minted RabbitHole receipts

[code423n4]

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Erc1155Quest.sol#L54-L63

Vulnerability details

Impact

The Erc20Quest.withdrawRemainingTokens function prevents the Erc20Quest contract's owner from withdrawing the unclaimed reward token amounts that are associated with the minted RabbitHole receipts after the quest is ended. However, this is not the case for the following Erc1155Quest.withdrawRemainingTokens function. After the ERC1155 quest is ended, if the rewards for some minted RabbitHole receipts are not withdrawn yet, calling the Erc1155Quest.withdrawRemainingTokens function by the Erc1155Quest contract's owner can transfer these unclaimed reward token amounts to the specified to_ address even though such owner has no malicious intent. Afterwards, regaining these deserved reward token amounts can be troublesome to the affected RabbitHole receipt holders, especially these who need to use their reward tokens in a timely manner.

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Erc1155Quest.sol#L54-L63

    function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);
        IERC1155(rewardToken).safeTransferFrom(
            address(this),
            to_,
            rewardAmountInWeiOrTokenId,
            IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId),
            '0x00'
        );
    }

Proof of Concept

Please append the following test in the withdrawRemainingTokens() describe block in quest-protocol\test\Erc1155Quest.spec.ts. This test will pass to demonstrate the described scenario.

    it.only('Calling withdrawRemainingTokens function for an ERC1155 quest can possibly withdraw rewards that are associated with minted RabbitHole receipts', async () => {
      // quest contract owns 100 reward tokens and owner owns no reward tokens
      expect(await deployedSampleErc1155Contract.balanceOf(deployedQuestContract.address, rewardAmount)).to.equal(100)
      expect(await deployedSampleErc1155Contract.balanceOf(owner.address, rewardAmount)).to.equal(0)

      // firstAddress owns a RabbitHole receipt but does not claim the associated rewards yet
      await deployedRabbitholeReceiptContract.mint(firstAddress.address, questId)
      await deployedQuestContract.start()

      await ethers.provider.send('evm_increaseTime', [20000])

      // owner calls withdrawRemainingTokens function after quest is ended
      await deployedQuestContract.connect(owner).withdrawRemainingTokens(owner.address)

      // all of the 100 reward tokens are transferred from quest contract to owner while firstAddress has not claimed any rewards yet
      expect(await deployedSampleErc1155Contract.balanceOf(deployedQuestContract.address, rewardAmount)).to.equal(0)
      expect(await deployedSampleErc1155Contract.balanceOf(owner.address, rewardAmount)).to.equal(100)

      // firstAddress is unable to claim rewards for its RabbitHole receipt
      await expect(
        deployedQuestContract.connect(firstAddress).claim()
      ).to.be.revertedWith('ERC1155: insufficient balance for transfer')

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

Tools Used

VSCode

Recommended Mitigation Steps

The Erc1155Quest.withdrawRemainingTokens function can be updated to not allow any withdrawals of the unclaimed reward token amounts that are associated with the minted RabbitHole receipts so it becomes consistent with the Erc20Quest.withdrawRemainingTokens function.

[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)