RabbitHole Quest Protocol contest - 0xRobocop's results

Lines of code

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

Vulnerability details

Impact

When block.timestamp is greater than endTime anyone can call the function withdrawFee from the ERC20Quest.sol contract.

The problem is that this function can be called multiple times until draining all the funds from the contract, causing people not been able to claim their receipts.

Proof of Concept

Let's say the quest have the following parameters:

fee == 20% total participants == 2 reward amount == 1000 units

Both participants have minted their receipt, hence receiptRedeemers returns 2.

Before anyone claiming either receipts or fees, the contract must hold at least 2,400 units of tokens. 2000 (participants * rewardamount) + 400 (20% fee).

Let's say participant 1 claim at time t before endTime. Now the contract holds 1,400 units. After endTime someone call withdrawFee making the contract send 400 units of tokens to protocolFeeRecipient. This can be repeated until leaving the contract with 200 units of tokens. After this, participant number 2 wont be able to claim his receipt.

Tools Used

Manual.

Recommended Mitigation Steps

withdrawFee should only be called once.

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L219 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L98 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L81

Vulnerability details

Impact

The purpose of withdrawRemainingTokens is to withdraw all the tokens that wont be claimed by participants, because endTime has passed and some receipts were not minted. Leaving on the contract the exact amount for minted but not claimed receipts and for the protocol fee given the amount of minted receipts.

So the function has an assumption that receipts cannot be minted after endTime but following the control flow, there is no check that ensures this, nor a deadline for the signatures to be valid.

If someone mints a receipt after the owner called withdrawRemainingTokens then the contract wont have enough tokens to pay all the mintedd receipts nor the protocol fee.

Proof of Concept

Maybe the Front End wont stop giving signatures after endTime but the user can request a signature before endTime, and then cancelling the transaction but copying the valid signature to be used later.

Tools Used

Manual

Recommended Mitigation Steps

Make signatures have a deadline to be used or not allow minting after endTime

Judge has assessed an item in Issue #251 as 2 risk. The relevant finding follows:

[L-03] The claim function might use an amount of gas greater than the block gas limit. Description:

The claim function at the Quest.sol contract can consume an amount of gas greater than the block gas limit if the user has too many receipts.

Mitigation:

Make users know they might need to split their receipts accross other wallets in this case.

[L-01] Use Ownable2StepUpgradeable and Ownable2Step instead of OwnableUpgradeable and Ownable.

Description:

Using a 1-step transfer of ownership is dangerous. If an incorrect address is passed as the new owner, there is no coming back.

Better use the 2-step pattern from OZ. Ownable2Step.sol and Ownable2StepUpgradeable.sol.

Found at :

Quest.sol QuestFactory.sol RabbitHoleReceipt.sol

[L-02] Use a nonReentrant mutex to avoid reentrancy issues.

Description:

When using _safeMint() of the ERC721 OZ implementation there is a call to the address of the _to paramenter to make sure the account can receive an ERC-721 token. This can be used to re-enter the contract causing unexpected behaviors.

This can happen also when using erc-777 tokens instead of erc-20 tokens.

Use a mutex like the OZ nonReentrant

Found at:

RabbitHoleReceipt.sol.

[L-03] The claim function might use an amount of gas greater than the block gas limit.

Description:

The claim function at the Quest.sol contract can consume an amount of gas greater than the block gas limit if the user has too many receipts.

Mitigation:

Make users know they might need to split their receipts accross other wallets in this case.

[L-04] Use EIP-712 for signatures.

For a better usability of off-chain message signing for use on-chain use the EIP-712 standard.

[L-05] ERC-20 Tokens stucked if protocolFeeReceipt cannot handle them.

There is a non-zero risk of making a mistake of setting protocolFeeReceipt address to an account not been able to manage ERC-20 tokens. Effectively losing all the tokens send to it through the protocol fee.