RabbitHole Quest Protocol contest - Iurii3's results

Lines of code

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

Vulnerability details

Impact

If someone calls withdrawFee many time before, there won't be enough tokens on the contract to claim by participants

Proof of Concept

Function WithdrawFee() is a public function, so anyone may call it. While it has modifier onlyAdminWithdrawAfterEnd that would probably intended to limit it's usage to only Admin, actually this modifier only checks if the end time passed

/// @notice Prevents reward withdrawal until the Quest has ended
modifier onlyAdminWithdrawAfterEnd() {
    if (block.timestamp < endTime) revert NoWithdrawDuringClaim(); 
    _;
}

In this case after the end of the quest anyone can call this function and withdraw all funds from the Quest to protocolFeeRecipient. Moreover, as there is no check if the protocolFee was already withdraw anyone may drain quest contract after it ending by calling this function many times. And thus participant won't be able to claim their rewards

Recommended Mitigation Steps

Add check if protocol fee was collected before executing this function

Lines of code

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

Vulnerability details

Impact

If ProtocolFee were collected from ERC20 quest before calling withdrawRemainingTokens function, the amount of tokens to withdraw by withdrawRemainingTokens will be calculated wrong and tokens will be stuck on the contract.

Proof of Concept

Smart contract designed in a such way that creator of the quest may withdraw unused funds from the Quest after its ending. The unused funds is calculates as Total balance on the contract less Unclaimed by participant less ProtocolFee.

 uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;

While amount of unclaimed tokens calculated at the moment of withdrawal, nature of ProtocolFee is a bit different. ProtocolFee is static number calculated as percentage of the total amount of granted rewards. Contract does not check whether they were already withdrew from the contract or not.

So, if ProtocolFee were collected, their amount already subtracted from the balance of the ERC20 quest contract. And thus while calculating nonClaimableTokens they would be subtracted second time and won't be withdrew from contract. As these tokens are "free" and not claimable by participant, they would be stuck there forever.

Recommended Mitigation Steps

Before calculating nonClaimableTokens check whether protocolFee was already collected or not

[NC-01] Creator of the quest may pause it forever

Creator of the quest has a right to stop (pause forever) quest. In case there is an unclaimed reward at that time, participant won't be able to claim it. While quest creator role is limited it should not be a big problem. However, in case of opening it for broader public, current approach should be reconsidered.

Quest.sol#L57 Quest.sol#L97

[NC-02] minterAddress must be equal to QuestFactory address

Under the RabbitHoleReceipt.sol contract there are two separate state variables - minterAddress and QuestFactoryContract assigned by relevant function.

RabbitHoleReceipt.sol#L77 RabbitHoleReceipt.sol#L83

However, for proper functioning of the mintReceipt function minterAddress must be set to QuestFactoryContract address.

QuestFactory.sol#L219

To avoid confusion we may combine setQuestFactory and setMinterAddress in one function that uses QuestFactory address as an argument

[NC-03] Missing contract initialization

RabbitHoleReceipt.sol does not initialize ERC721EnumerableUpgradeable RabbitHoleTickets.sol does not initialize IERC2981Upgradeable

[NC-04] Make code more readable and self-explanatory

super.withdrawRemainingTokens(to_) may be changed to modifier onlyAdminWithdrawAfterEnd to be clearer

Erc20Quest.sol#L82 Erc1155Quest.sol#L55

modifier onlyAdminWithdrawAfterEnd() actually does not check if admin calls it, it should be renamed to onlyAfterEnd() Quest.sol#L76

wrong variable name, should be claimingAddress_Balance instead of msgSenderBalance RabbitHoleReceipt.sol#L113

[NC-05] WithdrawRemainingTokens function on the ERC1155Quest does not count on unclaimed tokens

Erc1155Quest.sol#L60

WithdrawRemainingTokens on ERC1155 Quest does not check if there are remaining unclaimed tokens by participants and withdraw full balance on the contract. In this case it must be used only after all participants claim their rewards that may require additional checks from the team.

[G-01] Unnecessary check

If the second check would be passed (case quests[questId_].addressMinted[msg.sender] == false) the first would be always passed as well. May leave the second one only.

QuestFactory.sol#L220-L221

[G-02] Unnecessary variable assignment

isPaused is false by default and there is no way to un-pause it before calling start() function

Quest.sol#L51

[G-03] Unused argument of a function

RabbitHoleTickets.sol#L110

[G-04] Mapping not used

There is a mapping between timestamp and receipt token id. However it is not clear how we would use it without any view function to return information from it. Consider deleting or adding view function

RabbitHoleReceipt.sol#L34

[G-05] May assign the same role twice

There is no check if caller provided right bool canCreateQuest_ as an argument. We may accidentally grant/revoke role second time and lose gas.

QuestFactory.sol#L142

We can add check by updating if statement from

if (canCreateQuest_)

to

if (canCreateQuest_ && hasRole(CREATE_QUEST_ROLE, account_) ) 

[G-06] Possible quest Id collision

While creating quest any sting may be used as it's ID. Using the same ID second time will be reverted as QuestIdUsed(). However, there is no direct way to retrieve if current current Quest Id used or not (it is possible to use questInfo() function, but it will retrieve (0, 0, 0) rather then bool if the current id is available.

It might be useful to create view function that checks if current quest id is available.

QuestFactory.sol#L68

[G-07] <x> += <y>  COSTS MORE GAS THAN <x> = <x> + <y>  FOR STATE VARIABLES

Using the addition operator instead of plus-equals saves gas

Quest.sol#L115

[G-08] CAN MAKE THE VARIABLE OUTSIDE THE LOOP TO SAVE GAS

Make it outside and only use it inside.

Quest.sol#L70 Quest.sol#L101 Quest.sol#L104

[G-09] Change i++ to ++i

Quest.sol#L70 Quest.sol#L104 Quest.sol#L106 QuestFactory.sol#L226 RabbitHoleReceipt.sol#L117 RabbitHoleReceipt.sol#L121 RabbitHoleReceipt.sol#L128 RabbitHoleReceipt.sol#L131