RabbitHole Quest Protocol contest - thekmj's results

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L58-L61 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L47-L50

Vulnerability details

Details

The current onlyMinter modifier is implemented as shown

modifier onlyMinter() {
    msg.sender == minterAddress;
    _;
}

This modifier does not perform any checks nor reverting. The sponsor may have intended this to be an access control mechanism.

As such, anyone can mint a receipt, as the contract is missing the important access control. This affects both RabbitHoleTicket and RabbitHoleReceipt.

Tools Used

Manual review

Recommended Mitigation Steps

Correct the modifier with an appropriate require statement

modifier onlyMinter() {
    require(msg.sender == minterAddress, "Only minter");
    _;
}

Alternatively, the sponsor may opt to use custom errors for better gas saving.

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L96-L118 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L109-L135

Vulnerability details

Disclaimer: The warden has submitted a separate issue on the access control of the mint() function, which would've made this issue to be of High risk. In this report, we assume that said issue has been mitigated.

Summary

The claim() function will iterate over all owned receipts by a user, and also performs numerous storage writes. This opens up a self-DOS possibility that can lock funds under certain circumstances.

This report aims to give an estimation of the gas usage of the function, an analysis on the likelihood of a self-DOS happening, as well as the impact caused.

Gas usage analysis

Let us first analyze the gas usage of storage accesses from the claim() function in Quest.sol contract:

• The function first fetches all receipt tokens owned by the user.

uint[] memory tokens = rabbitHoleReceiptContract.getOwnedTokenIdsOfQuest(questId, msg.sender);

Since the getOwnedTokenIdsOfQuest() is a view function, the gas cost can be estimated to be (Storage read * balance of user) multiplied by a constant factor. The length of the loop is bounded by the number of receipts said user owns across all quests.

• The function then sets claim status for all of said tokens

_setClaimed(tokens);

This will cost significantly more gas than the former operation, since this is a storage write. The length of the loop is bounded by the number of receipts said user owns from this quest only.

Self-DOS likelihood

We can see that the gas usage directly correlates to the number of receipts owned by the claiming user. The two likely scenario where a self-DOS happens might be:

• A user has lots of unclaimed receipts for a particular quest, and tries to claim. This is a problem because a claimed storage write for an unclaimed token incurs a Gsset operation, instead of a cheaper Gsreset. This will DOS the _setClaimed() loop.
• A user has lots of receipts in general, across all quests. This will DOS the rabbitHoleReceiptContract.getOwnedTokenIdsOfQuest() loop.
  • This can also be from completed quests as well, as completed receipts stay in the user's wallet without being moved out, thus the likelihood of this being a DOS vector increases with time and protocol usage.

Impact

The problem arises when the claim() from Quests does not have a backdoor mechanism for claiming by specific tokens:

• There is no outgoing path for tokens, rendering them effectively locked for a particular user.
• A user can technically transfer their receipts to another account and claim from there, however since there is no batch transfer mechanism without using up many transactions, this will cost the user a hefty amount of gas, which can be considered a heavy leak of value.

The result is locking of rewards for a user, which can get costly to recover.

It is worth noting that, token ownership information from getOwnedTokenIdsOfQuest() can technically be obtained from emitted events as well, making the claim() loop to be the only one with a severity worth mentioning.

Tools Used

Manual review.

Recommended Mitigation Steps

We propose mitigation methods for both unbounded loops.

• For the _setClaimed() loop problem, we recommend adding a claimByID(uint256[] calldata tokenIDs) function, which will claim rewards from all of the given tokens, performing appropriate ownership checks. This will create a recovery backdoor for the existing problem.
• For the getOwnedTokenIdsOfQuest(), we propose to burn the token from the user's wallet upon a successful claim.

[L-01] Missing sanity check for royalty fee.

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

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

According to the function royaltyInfo, the royalty fee is measured in BPS. However there is no sanity check for the amount of fee, meaning the owner can set it arbitrarily high. This is recognized as an issue given there is such a check on QuestFactory.

[N-01] Useful information in events should be indexed

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

The following event can use an indexed field into address account. Although indexed fields will incur slightly more gas (375 gas per field per event), it will make it easy to query useful info specific to the field (e.g. querying how many claims has this user done, total amounts, and such)

event Claimed(address account, uint256 amount);

[N-02] Don't compare a boolean with true

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

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

A boolean expression in an if-condition is straightforwardly evaluated. There is no need to check if (condition == true).

Removing the check will also save some gas.

[N-03] Define a constant instead of using a magic value

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

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

uint256 royaltyPayment = (salePrice_ * royaltyFee) / 10_000;

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

if (questFee_ > 10_000) revert QuestFeeTooHigh();

[G-01] Tightly pack storage for better gas saving

This finding is divided into several findings due to distinction, but the main technique of storage packing is applied in all

[G-01.1] QuestFactory contract storage

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

The current storage layout is as follow:

address public claimSignerAddress; // one slot
address public protocolFeeRecipient; // one slot
mapping(string => Quest) public quests; // one slot
RabbitHoleReceipt public rabbitholeReceiptContract; // one slot
mapping(address => bool) public rewardAllowlist; // one slot
uint public questFee; // one slot
uint public questIdCount; // one slot

We can notice that questFee and questIdCount cannot be too large (with questFee not exceeding 10000, and questIdCount incremented one by one). We can use smaller value types and better pack storage as follow:

address public claimSignerAddress;
// one slot
address public protocolFeeRecipient; 
uint48 public questFee;
uint48 public questIdCount;
// end of slot
mapping(string => Quest) public quests;
RabbitHoleReceipt public rabbitholeReceiptContract;
mapping(address => bool) public rewardAllowlist;

Since all of these storage variables are accessed every time a quest is created, this replaces two instances of Gwarmaccess (2100 gas) with Gcoldsload (100 gas), saving a total of 4000 gas per quest created.

This will also save some deployment gas, due to having to set up fewer storage slots than usual.

[G-01.2] struct Quest

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

The current layout for Quest struct is as follow:

struct Quest {
    mapping(address => bool) addressMinted; // 1 slot
    address questAddress; // 2 slot
    uint totalParticipants; // 3 slot
    uint numberMinted; // 4 slot
}

Note that this is used in storage as the quests mapping, so this is relevant.

There is room for optimization in totalParticipants and numberMinted:

• totalParticipants: Is the number of participants.
• numberMinted: Is incremental.

We can thus reason that the values will be reasonably small. Thus the following layout is recommended:

struct Quest {
    mapping(address => bool) addressMinted; // 1 slot
    address questAddress; // 2 slot
    uint48 totalParticipants; // 2 slot
    uint48 numberMinted; // 2 slot
}

The change brings the following optimizations:

• For each quest created, converts one Gsset (20000 gas) into one Gsreset (2900 gas) by totalParticipants not having to write to an empty storage. Equates to 17100 gas saved per quest.
• For the first receipt minted per quest, numberMinted doesn't have to write to a zero storage slot. Thus another 17100 gas is saved in the same manner, per quest.

The total gas saving is 34200 per quest created.

[G-01.3] Quest contract storage

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

The storage layout is as follow

bool public hasStarted; // slot 1
bool public isPaused; // slot 1
string public questId; // slot 2
uint256 public redeemedTokens; // slot 3

The redeemedTokens is a count of the number of tokens redeemed by the user, which cannot be too large by definition. Thus we can tightly pack the storage as follow:

bool public hasStarted; // slot 1
bool public isPaused; // slot 1
uint240 public redeemedTokens; // slot 1
string public questId; // slot 2

The effect are as follow:

• Since both hasStarted and redeemedTokens is accessed whenever claim() is (successfully) called, converts one Gcoldsload (2100 gas) into Gwarmaccess (100 gas) for 2000 gas in total.
• Since hasStarted has to be already True for the function to not revert, a write to redeemedTokens will not use a Gsset (20000 gas), but rather use a Gsreset (2900 gas), saving 17100 gas for the first claim made by the user per quest.

This saves a total of 19100 gas.

[G-02] Don't initialize storage with default value

Affected instances:

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

redeemedTokens = 0; // @audit this is redundant

The given storage variable is guaranteed by logic to be default. Setting it with the same value will incur a redundant Gsreset (2900 gas).

This finding is similar to [GAS-4] of C4udit, but was not found by the automated tool.

[G-03] Redundant condition check

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

Note the condition check at the Quest constructor:

if (endTime_ <= block.timestamp) revert EndTimeInPast();
if (startTime_ <= block.timestamp) revert StartTimeInPast();
if (endTime_ <= startTime_) revert EndTimeLessThanOrEqualToStartTime();

The first condition is redundant. That is, if the latter two conditions are true, then the first condition is always true. Thus we can remove the first check entirely.

Saves 25 gas when run on hardhat gas report.

[G-04] Don't compare a boolean with true

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

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

There is no need to check if (condition == true), but simply if (condition) will suffice.

Saves 3 gas per instance.