withdrawRemainingTokens() in the Erc1155Quest withdraws all tokens and does not consider the amount of unclaimed tokens

[code423n4]

Lines of code

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

Vulnerability details

Impact

The withdrawRemainingTokens() function in the Erc1155Quest contract does not consider the amount of unclaimed tokens. When the owner calls the function when the quest has ended, all tokens belonging to the contract will be withdrawn. Any user who has not yet used their receipt to claim their token(s) will no longer be able to claim them since all tokens has been withdrawn by the owner, leaving their receipts useless.

Proof of Concept

The following test can be added in quest-protocol/test/Erc1155Quest.spec.ts to the claim() group (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/test/Erc1155Quest.spec.ts#L199)

it('leaves no tokens behind for users to claim', async () => {
      await deployedRabbitholeReceiptContract.mint(owner.address, questId)
      await deployedQuestContract.start()

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

      const beginningContractBalance = await deployedSampleErc1155Contract.balanceOf(
        deployedQuestContract.address,
        rewardAmount
      )

      expect(beginningContractBalance.toString()).to.equal('100')

      await deployedQuestContract.withdrawRemainingTokens(owner.address)

      await expect(deployedQuestContract.claim()).to.be.revertedWith('ERC1155: insufficient balance for transfer')

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

The command yarn hardhat test --grep "leaves no tokens behind" can be used to run the test

Recommended Mitigation Steps

Note that the Erc20Quest contract considers the unclaimed token amount and ensures this amount will be kept in the contract to allow users to claim their tokens even after withdrawRemainingTokens() has been called, as can be seen on lines 84-86 in Erc20Quest (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L84-L86)

 uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
 uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
 IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);

A similiar calculation should be introduced to withdrawRemainingTokens() in the Erc1155Quest contract.

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