RabbitHole Quest Protocol contest - lukris02's results

Lines of code

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

Vulnerability details

Impact

It is possible to call the function withdrawFee() many times. Note that even though the function has the modifier onlyAdminWithdrawAfterEnd, actually anyone (not only admin) can call the function. So, when quest has ended and the Erc20Quest_contrcat_balance > 0 (possible if there are unclaimedTokens left and/or the owner did not withdraw remaining tokens), attacker can withdraw fees until the balance of the contract runs out. Although the attacker will not directly benefit, the cost of the attack is relatively small, and the owner of the quest contract and users who have not yet claimed the reward can lose their assets.

Proof of Concept

For example:

• the function protocolFee() returns 5
• 12 users did not claim reward (assume rewardAmountInWei = 1) ( => Quest contract should have balance at least 12)
• Imagine there are 2 more tokens that can be withdrawn by owner

So, contract balance = 14.

1. Attacker calls the function withdrawFee() 2 times. The contract balance becomes 4.
2. Attacker can deposit 1 token and call the function withdrawFee() one more time. The contract balance becomes 0.

As a result, 12 users can not claim rewards and owner can not withdraw any tokens.

Tools Used

Manual review.

Recommended Mitigation Steps

Add the state variable bool isFeeWithdrawn to Erc20Quest.sol and change the function:

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

Lines of code

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

Vulnerability details

Impact

The function mintReceipt has no check that block.timestamp < quest endTime. So, it is possible to mint receipts after the quest has ended which can lead to loss of assets. If the owner of the quest contract withdraws the remaining tokens immediately after the end of the quest, assets can be lost by users, who did not claim rewards, or the protocol.

Proof of Concept

Several scenarios are possible:

1. The quest has ended (but quest.numberMinted < quest.totalParticipants), all users claimed rewards (unclaimedTokens = 0), protocol withdrawn fees, owner withdrawn remaining tokens. Contract balance = 0.
   User mints Receipt and can not claim rewards because there are no tokens left on quest contract balance.
2. The quest has ended (but quest.numberMinted < quest.totalParticipants), not all users claimed rewards, protocol withdrawn fees, owner withdrawn remaining tokens.
   User mints Receipt and claim rewards but one of the users who has not yet claimed a reward will not be able to receive a reward because there are not enough tokens.
3. The quest has ended (but quest.numberMinted < quest.totalParticipants), all users claimed rewards (unclaimedTokens = 0), protocol did not withdraw fees, owner withdrawn remaining tokens. User mints Receipt and claim rewards but protocol will not be able to receive fees because there are not enough tokens.

Tools Used

Manual review.

Recommended Mitigation Steps

Restrict the ability to mintReceipt when block.timestamp > quest.endTime.

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

Impact

If a user has many ERC-721 tokens and needs to claim a lot of quest tokens, the claim() function can be DOSed due to out-of-gas error. User's rewards will be locked until they sell some tokens.

Proof of Concept

The function claim() contains gas consuming loop, and also it calls two gas consuming functions: _setClaimed (has 1 loop) and getOwnedTokenIdsOfQuest() (has 2 loops).

Tools Used

Manual review.

Recommended Mitigation Steps

Consider allowing users to specify what token ids they want to claim and just check that they own these tokens instead of iterating and filtering all user's tokens.

QA Report for RabbitHole Quest Protocol contest

Overview

During the audit, 8 low and 7 non-critical issues were found.

Low Risk Findings(8)

L-1. Misleading modifier

Description

The modifier onlyAdminWithdrawAfterEnd() does not check that only admin can call a fucntion, actually anyone can call it.

Instances

• Link:

    modifier onlyAdminWithdrawAfterEnd() {
        if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
        _;
    }

Recommendation

Add a check in modifier or rename it.

L-2. Owner can renounce ownership

Description

Openzeppelin's Ownable.sol implements renounceOwnership() function which leaves the contract without an owner and removes any functionality that is only available to the them.

Instances

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

Recommendation

Consider reimplementing the function to disable it.

L-3. Event should be emitted

Description

The governor functions that change critical parameters should emit events. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes and allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services. The alternative of directly querying on-chain contract state for such changes is not considered practical for most users/usages.

Instances

• function changeCreateQuestRole(address account_, bool canCreateQuest_) public onlyOwner {
• function setClaimSignerAddress(address claimSignerAddress_) public onlyOwner {
• function setProtocolFeeRecipient(address protocolFeeRecipient_) public onlyOwner {
• function setRabbitHoleReceiptContract(address rabbitholeReceiptContract_) public onlyOwner {
• function setRewardAllowlistAddress(address rewardAddress_, bool allowed_) public onlyOwner {
• function setQuestFee(uint256 questFee_) public onlyOwner {
• function setReceiptRenderer(address receiptRenderer_) public onlyOwner {
• function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner {
• function setQuestFactory(address questFactory_) public onlyOwner {
• function mint(address to_, string memory questId_) public onlyMinter {
• function start() public virtual onlyOwner {
• function pause() public onlyOwner onlyStarted {
• function unPause() public onlyOwner onlyStarted {
• function setTicketRenderer(address ticketRenderer_) public onlyOwner {
• function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner {
• function mint(address to_, uint256 id_, uint256 amount_, bytes memory data_) public onlyMinter {
• function mintBatch(
• function start() public override {
• function start() public override {

Recommendation

Consider adding events for these functions.

L-4. Quest startTime is not checked

Description

The function mintReceipt() does not check that quest has started.

Instances

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

Recommendation

Consider adding the check.

L-5. ECDSAUpgradeable.recover return value is not checked

Description

ECDSAUpgradeable.recover may return address(0) if invoked with an invalid signature. So, it is possible to mintReceipt() with invalid signature, if claimSignerAddress accidentally will be set to address(0). claimSignerAddress can be equal to address(0) if it is stay unset in initialize() or incorrectly set in setClaimSignerAddress() function.

Instances

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

Recommendation

ECDSAUpgradeable.recover return value must be checked in the function recoverSigner() to ensure that it is not zero address.

L-6. Max limit can be set

Description

royaltyFee is unlimited.

Instances

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

Recommendation

It is better to limit the maximum royaltyFee to increase transparency.

L-7. Sign change

Description

It is possible that some users will participate in the quest at the last second, so the < sign needs to be change to <=.

Instances

• if (block.timestamp < endTime) revert NoWithdrawDuringClaim();

Recommendation

Consider changing to: if (block.timestamp <= endTime) revert NoWithdrawDuringClaim();.

L-8. Checks can be added

Description

In Quest constructor it can be checked that:

• uint256 totalParticipants_> 0,
• endTime_ - startTime_ > minimum duration,
• endTime_ - startTime_ < maximum duration.

Instances

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

Recommendation

Consider adding more check.

Non-Critical Risk Findings(7)

NC-1. Unused named return variables

Description

Both named return variable(s) and return statement are used.

Instances

• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L185
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L114

Recommendation

To improve clarity use only named return variables.
For example, change:

function functionName() returns (uint id) {
    return x;

to

function functionName() returns (uint id) {
    id = x;

NC-2. Order of Functions

Description

According to Style Guide, ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier.
Functions should be grouped according to their visibility and ordered:

1. constructor
2. receive function (if exists)
3. fallback function (if exists)
4. external
5. public
6. internal
7. private

Instances

• function withdrawRemainingTokens(address to_) public override onlyOwner { (public function should be placed before internal)

Recommendation

Reorder functions where possible.

NC-3. Order of Layout

Description

According to Order of Layout, inside each contract, library or interface, use the following order:

1. Type declarations
2. State variables
3. Events
4. Modifiers
5. Functions

Instances

• modifier onlyAdminWithdrawAfterEnd() {
• modifier onlyStarted() {
• modifier onlyQuestActive() {

Recommendation

Place modifiers before constructor.

NC-4. Missing leading underscores

Description

Internal and private state variables and functions should have a leading underscore:

Instances

• function grantDefaultAdminAndCreateQuestRole(address account_) internal {
• mapping(uint256 => bool) private claimedList;

Recommendation

Add leading underscores where needed.

NC-5. Typos

Instances

• /// @dev set or remave a contract address to be used as a reward => remove

NC-6. Open TODOs

Instances

• // TODO clean this whole thing up

Recommendation

Resolve issues.

NC-7. Maximum line length exceeded

Description

According to Style Guide, maximum suggested line length is 120 characters. Longer lines make the code harder to read.

Instances

• return ReceiptRendererContract.generateTokenURI(tokenId_, questId, totalParticipants, claimed, rewardAmount, rewardAddress);

Recommendation

Make the lines shorter.

Gas Optimizations Report for RabbitHole Quest Protocol contest

Overview

During the audit, 2 gas issues were found.
Total savings ~2415.

Gas Optimizations Findings(2)

G-1. Extra sstore

Description

There is no need to make isPaused false because it can not be true before quest start.

Instances

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

Saved

2100 gas

G-2. Use unchecked blocks for incrementing

Description

In Solidity 0.8+, there’s a default overflow and underflow check on unsigned integers. In the loops or similar cases, "i" will not overflow because the loop will run out of gas before that or max value never be reached.

Instances

• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L101
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L132
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L226
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L117
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L121
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L128
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L131
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L70
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L104

Recommendation

Change:

for (uint256 i; i < n; ++i) {
 // ...
}

to:

for (uint256 i; i < n;) { 
 // ...
 unchecked { ++i; }
}

Saved

This saves ~30-40 gas per iteration.
So, ~35*9 = 315