All tokens in Erc1155Quest should not be withdrawn before users claim

[code423n4]

Lines of code

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

Vulnerability details

Impact

There are 2 type of Quest, Erc20Quest and Erc1155Quest. Both contracts have common property that they should hold enough reward token for totalParticipants. However, if number of receipt minted is less than totalParticipants, owner is allowed to withdraw remaining token to an address he specified. This check is implemented correctly in Erc20Quest where owner can only withdraw nonClaimableTokens.

function withdrawRemainingTokens(address to_) public override onlyOwner {
    super.withdrawRemainingTokens(to_);

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

However, it did not have the same check in Erc1155Quest.

function withdrawRemainingTokens(address to_) public override onlyOwner { 
  // @audit owner should not be able to withdraw all tokens before users claimed
    super.withdrawRemainingTokens(to_);
    IERC1155(rewardToken).safeTransferFrom(
        address(this),
        to_,
        rewardAmountInWeiOrTokenId,
        IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId),
        '0x00'
    );
}

As the result, all tokens in Erc1155Quest can be withdraw before any user claim their receipts.

Proof of Concept

Please add this test to Erc1155Quest.spec.ts

it.only('revert claiming when owner withdraw all tokens', async () => {
  await deployedRabbitholeReceiptContract.mint(owner.address, questId)
  await deployedQuestContract.start()

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

  expect(await deployedSampleErc1155Contract.balanceOf(owner.address, rewardAmount)).to.equal(0)

  const totalTokens = await deployedRabbitholeReceiptContract.getOwnedTokenIdsOfQuest(questId, owner.address)
  expect(totalTokens.length).to.equal(1)

  expect(await deployedQuestContract.isClaimed(1)).to.equal(false)
  // owner withdraw
  await deployedQuestContract.connect(owner).withdrawRemainingTokens(owner.address)

  // will revert here
  await deployedQuestContract.claim()
  expect(await deployedSampleErc1155Contract.balanceOf(owner.address, rewardAmount)).to.equal(1)
  await ethers.provider.send('evm_increaseTime', [-86400])
})

Tools Used

Manual Review

Recommended Mitigation Steps

Consider only allowing owner to withdraw non claimable tokens in Erc1155Quest

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