RabbitHole Quest Protocol contest - HollaDieWaldfee's results

Lines of code

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

Vulnerability details

Impact

The Erc20Quest.withdrawFee function is used to withdraw the protocol fee to the protocolFeeRecipient (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L102-L104).

The protocolFee is a percentage of the total rewards.

This function has the onlyAdminWithdrawAfterEnd modifier which other than it name suggests does not check that the msg.sender is the admin. It only checks that the function is called after the quest has ended (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L76-L79).

Also there is no check that the function is only called once.

So to summarize, the Erc20Quest.withdrawFee function can be called by any user multiple times.

This means that this can be used to withdraw all funds in the contract to the protocolFeeRecipient.

This situation is made worse by the fact that users can claim their rewards at any point in time. Even when the quest has ended. This was confirmed by the sponsor to be intended. So the end time of the quest is only really relevant to withdrawing fees (i.e. fees must be withdrawn after the end).

This means the users that have not yet claimed their rewards can lose them because all funds can be sent to the protocolFeeRecipient. The fact that users lose their rewards makes me estimate this issue to be "High" severity.

Proof of Concept

Assume the following situation:

1. User A has a pending reward in quest 1. Quest 1 reaches its end time but the reward is not claimed yet.
2. According to the sponsor this is perfectly fine and User A should be able to claim his rewards at any point in time
3. The attacker calls Erc20Quest.withdrawFee multiple times, until as much funds as possible are sent to the protocolFeeRecipient
4. User A can now not claim his rewards anymore

Tools Used

VSCode

Recommended Mitigation Steps

The Erc20Quest.withdrawFee function should only be called once. This can easily be implemented by adding a isFeeWitdrawn flag.

function withdrawFee() public onlyAdminWithdrawAfterEnd {
        if (isFeeWithdrawn) revert FeeAlreadyWithdrawn;
        IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
        isFeeWithdrawn = true;
    }

Lines of code

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

Vulnerability details

Impact

When the endTime is reached, any unclaimed tokens can be withdrawn from the Erc20Quest contract via the withdrawRemainingTokens function (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L81-L87).

The endTime is checked in the onlyAdminWithdrawAfterEnd modifier in the parent function (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L150).

This is the calculation to determine the amount to withdraw:

uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);

You can see that the rewards for any minted receipts that are not yet redeemed will stay in the contract.

The issue with this is that there is no check in the QuestFactory.mintReceipt function that makes sure that receipts cannot be minted after the endTime of a quest is reached (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L219-L229).

Therefore when Erc20Quest.withdrawRemainingTokens is called and then a new receipt is minted, there won't be enough rewards in the Erc20Quest contract for everyone to redeem.

The sponsor provided the information that the claimSigner will sign mintReceipt requests only when the endTime is not yet reached.

I think however that this is not sufficient and the endTime check must occur on-chain in the QuestFactory.mintReceipt function.

This is because a mintReceipt request might be signed off-chain in time (i.e. before the endTime) but is only processed on-chain after the endTime.

Proof of Concept

Think of the following scenario:

1. The claimSigner signs a mintReceipt request for User A before the endTime is reached
2. Now the endTime is reached and all remaining funds are withdrawn from the Erc20Quest contract. Say there is funds for one receipt left in the contract which can be redeemed by User B
3. Now the signed mintReceipt request is processed on-chain and User A redeems his reward
4. User B can now no longer redeem his reward because there are no funds in the Erc20Quest contract

You can see from this scenario how - when a mintReceipt request is processed on-chain after the endTime - users can miss out on their rewards.

Tools Used

VSCode

Recommended Mitigation Steps

The QuestFactory contract should have access to the endTime of each Quest and not mint receipts after the endTime has been reached.

This can be achieved by extending the Quest struct to include the endTime:

struct Quest {
        mapping(address => bool) addressMinted;
        address questAddress;
        uint totalParticipants;
        uint numberMinted;
        uint endTime;
    }

The endTime can then be checked in the QuestFactory.mintReceipt function:

if (quests[questId_].endTime <= block.timestamp) revert QuestExpired();

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc1155Quest.sol#L54-L63

Vulnerability details

Impact

The Erc1155Quest.withdrawRemainingTokens function is used to withdraw the remaining funds from the contract after the quest has ended (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc1155Quest.sol#L54-L63).

It is intended that users that have a valid receipt can claim their reward even after the quest has ended. This was confirmed by the sponsor.

The issue is that the Erc1155Quest.withdrawRemainingTokens function withdraws ALL remaining tokens and does not leave unclaimed tokens in the contract. This means that any users that want to claim their token after Erc1155Quest.withdrawRemainingTokens was called cannot do so anymore because there are no tokens left.

You can see in the Erc20Quest.withdrawRemainingTokens function (which is implemented correctly) how it should work:

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

uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);

The unclaimed tokens should remain in the contract.

Proof of Concept

1. A user has a valid receipt to withdraw a reward token from the Erc1155Quest contract
2. The endTime is reached and the owner calls Erc1155Quest.withdrawRemainingTokens which transfers all reward tokens to the to address
3. The user should still be able to claim his reward token but there are no reward tokens left in the contract

Tools Used

VSCode

Recommended Mitigation Steps

The Erc1155Quest.withdrawRemainingTokens function should make sure that receiptRedeemers() - redeemedTokens tokens remain in the contract.

Therefore it also needs to implement the receiptReedemers() function which can be the same as in the Erc20Quest contract.

Lines of code

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

Vulnerability details

Impact

The Erc20Quest.withdrawRemainingTokens function (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L81-L87) is used to withdraw all remaining reward tokens once the endTime is reached.

The issue is that this function incorporates the protocolFee into its calculation.

The protocolFee however can already be paid out when the Erc20Quest.withdrawRemainingTokens function is called. There is no mechanism by which Erc20Quest.withdrawFee() must be called after Erc20Quest.withdrawRemainingTokens.

At the very least this results in a withdrawn amount that is too small if protocol fees have been withdrawn before.

However the fact that protocol fees are subtracted that are not in the contract anymore can also make the calculation underflow which causes an amount up to the protocol fees to be stuck in the contract.

Proof of Concept

The calculation to determine the amount of tokens to withdraw is this:

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

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

Assume the following:

IERC20(rewardToken).balanceOf(address(this)) = 40 USDC
protocolFee() = 50 USDC
unclaimedTokens = 0 USDC

In this situation all 40 USDC should be withdrawn because there are no unclaimedTokens and the protocolFee is already paid out.

However the calculation reverts which causes the 40 USDC to be stuck in the contract.

Tools Used

VSCode

Recommended Mitigation Steps

I discussed this issue with the sponsor and it was decided that probably the best solution is to save the yet to be paid fees in a uint variable that is subtracted from when fees are paid out.

This variable is added to when receipts are minted.

This solution also requires a modification of the data flow, i.e. the quest contract must know when a new receipt is minted.

The sponsor mentioned that this will be investigated as part of a broader refactoring of the data flow.

RabbitHole Quest Protocol Low Risk and Non-Critical Issues

Summary

[L-01] Unlocked pragma

Currently the Solidity source files will compile with any version >=0.8.15 and <0.9.0.
It is best practice to use a fixed compiler version.

There are 10 instances of this issue:

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/interfaces/IQuest.sol#L2

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/interfaces/IQuestFactory.sol#L2

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

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

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

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

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

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

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

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

[L-02] Ownable + OwnableUpgradeable: Owner can renounce ownership

Multiple of the in-scope contracts inherit from openzeppelin's Ownable or OwnableUpgradeable contract.
Both contracts contain a renounceOwnership function that allows the owner of the contract to transfer ownership to the zero address. So ownership of the contract is lost.

You should consider implementing this function in the child contracts to disable it, i.e. override it and revert when it is called.

There are 4 instances of this issue:

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

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

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L15-L22

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L11-L17

[L-03] Ownable + OwnableUpgradeable: Does not implement 2-Step-Process for transferring ownership

Multiple of the in-scope contracts inherit from openzeppelin's Ownable or OwnableUpgradeable contract.
This contract does not implement a 2-Step-Process for transferring ownership.
So ownership of a contract can easily be lost when making a mistake when transferring ownership.

Consider using the Ownable2Step and Ownable2StepUpgradeable contracts instead.

Note:
Switching to Ownable2Step and Ownable2StepUpgradeable does not solve the [L-02] issue. So also override and disable the renounceOwnership function.

There are 4 instances of this issue:

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

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

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L15-L22

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L11-L17

[L-04] questIdCount should start at 0

The questIdCount is set to 1 in the constructor (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L49).

And it is incremented every time a quest is created.

This means after the first quest is created it is set to 2, after the second quest is created it is set to 3 and so on.

It was determined with the sponsor that this variable should count the number of quests created.

Therefore the constructor should not set it to 1 but instead leave it at its default value which is 0.

[L-05] Check value of royaltyFee

The RabbitHoleReceipt.setRoyaltyFee function does not check the value that royaltyFee is set to (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L90-L93).

It should be checked that the value is not greater than 10,000 which is 100%.

[L-06] initialize function should call setters

The RabbitHoleReceipt.initialize function sets the royaltyFee and minterAddress directly (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L43-L56).

However it should call setMinterAddress and setRoyaltyFee such that the MinterAddressSet and RoyaltyFeeSet events are emitted.

[L-07] rabbitHoleReceiptContract should not be mutable

The QuestFactory.setRabbitHoleReceiptContract function (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L172-L174) allows to set the rabbitHoleReceiptContract variable.

This is dangerous because all quest contracts rely on this for their operation.

I propose that the rabbitHoleReceiptContract should only be set once in the initialize function.

[N-01] Lines too long

The maximum line length should be 164 characters.
That's because GiHub will not display lines longer than that on the screen without scrolling.

There are 3 instances of this:

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/interfaces/IQuestFactory.sol#L16

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

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

[N-02] Remove TODO comments

TODO comments should be resolved and not be included in production code.

There is 1 instance of this:

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/interfaces/IQuest.sol#L4

[N-03] Remove unnecessary imports

Solidity files should only import the dependencies that are necessary to make the code cleaner and make it easier to understand.

There is 1 instance of this:

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

[N-04] questId can be immutable

The questId is only set in the constructor, so it can be declared immutable.

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