RabbitHole Quest Protocol contest - joestakey's results

Lines of code

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

Vulnerability details

mintReceipt currently does not have any check that the questId passed as an argument corresponds to an active quest.

As the signature will be valid without a deadline, a user can call mintReceipt after the end of a quest, when there is no more funds in the Quest contract - ie when all users have already claimed their rewards and the owner has called ERC20Quest.withdrawRemainingTokens().

The user has hence minted a useless token.

As per the gas reports, the cost of mintReceipt is around $40. This is a non-negligeable amount and the function should ensure the user does not mint an obsolete token

Impact

Medium

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check in mintReceipt to ensure Quest(quests[questId_].questAddress).endTime() > block.timestamp

Summary

Low Risk

Non-Critical

Low

[L‑01] Additional sanity checks

Add a sanity check in setRoyaltyFee: royaltyFee_ < 10000.

RabbitHoleReceipt.sol

93: function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner {
94:         royaltyFee = royaltyFee_;
95:         emit RoyaltyFeeSet(royaltyFee_);
96:     }

[L‑02] Wrong comment

RabbitHoleReceipt.sol

177: /// @dev See {IERC165-royaltyInfo} //@audit - IERC2981
178:     /// @param tokenId_ the token id
179:     /// @param salePrice_ the sale price
180:     function royaltyInfo(

Non-critical

[N-01] Redundant computation

isPaused is already false when start is called. Quest.sol

50: function start() public virtual onlyOwner {
51:         isPaused = false; //@audit can be removed
52:         hasStarted = true;
53:     }

[N-02] Scientific notation can be used

QuestFactory.sol

186: function setQuestFee(uint256 questFee_) public onlyOwner {
187:         if (questFee_ > 10_000) revert QuestFeeTooHigh(); //@audit - change to 1e4
188:         questFee = questFee_;
189:     }

[N-03] Use named imports

QuestFactory.sol

10: import '@openzeppelin/contracts-upgradeable/utils/cryptography/ECDSAUpgradeable.sol'; //@audit import {ECDSAUpgradeable}

[N-04] Offset can be misleading

questIdCount should not be initialized to 1 in initialize, as it means querying this variable will always return the actual number of quests with an offset of 1.

QuestFactory.sol

49:         questIdCount = 1; //@audit consider removing this line

[N-05] Open TODOs

IQuest.sol

4: // TODO clean this whole thing up

[N-06] Events should be emitted in key setters

Emit an event in key setters

QuestFactory.sol

159: function setClaimSignerAddress(address claimSignerAddress_) public onlyOwner {
160:         claimSignerAddress = claimSignerAddress_;
161:     }

[N-07] Avoid floating pragmas

All the contracts in scope

2: pragma solidity ^0.8.15;

Gas Report

Table of Contents

Gas saving issues

• Redundant getters
• timestampForTokenId can be removed
• setQuestFactory and setMinter can be combined in one function

Summary

A few optimizations allow to save significant amounts of gas upon QuestFactory.createQuest() and QuestFactory.mintReceipt() calls, ie the most important user-facing functions of the protocol. The total gas saved on users function calls is 41,852 The total gas saved on admin function calls is 21,000 The total gas saved on deployment is 81,841

Redundant getters

Getters are automatically defined for public storage variables and constants. The functions Quest.getRewardAmount() and Quest.getRewardToken() are hence redundant and can be replaced to save gas upon deploying new Quests - ie when calling QuestFactory.createQuest()

Instances

Quest.sol

139:      /// @dev Returns the reward amount
140:      function getRewardAmount() public view returns (uint256) {
141:          return rewardAmountInWeiOrTokenId;
142:      }
143: 
144:      /// @dev Returns the reward token address
145:      function getRewardToken() public view returns (address) {
146:          return rewardToken;
147:      }

Tools Used

Manual Analysis, Hardhat

Mitigation

-139:      /// @dev Returns the reward amount
-140:      function getRewardAmount() public view returns (uint256) -{
-141:          return rewardAmountInWeiOrTokenId;
-142:      }
-143: 
-144:      /// @dev Returns the reward token address
-145:      function getRewardToken() public view returns (address) {
-146:          return rewardToken;
-147:      }

And in RabbitHoleReceipt.sol

-166:          IQuest questContract = IQuest(questAddress);
+166:          Quest questContract = Quest(questAddress);
167:
168:          bool claimed = questContract.isClaimed(tokenId_);
-169:         uint rewardAmount = questContract.getRewardAmount();
+169:         uint rewardAmount = questContract.rewardAmountInWeiOrTokenId();
-170:         address rewardAddress = questContract.getRewardToken();
+170:         address rewardAddress = questContract.rewardToken();

Note that you must also remove the two getters from IQuest()

• Gas costs before:

• Gas costs after:

This saves on average:

• 46,935 gas upon deployment for QuestFactory.sol
• 19,678 gas on every QuestFactory.createQuest call

timestampForTokenId can be removed

timestampForTokenId is used when minting a receipt in mint() to save the timestamp at which the token was minted. This information can also be retrieved by checking the timestamp of the block in which the event Transfer(address(0), tokenId, ..) (emitted upon minting) was included in. Given that timestampForTokenId is not used anywhere else, consider removing it to save gas upon deployment and on calls to mintReceipt().

Instances

RabbitHoleReceipt.sol

35:     mapping(uint => uint) public timestampForTokenId;

Tools Used

Manual Analysis, Hardhat

Mitigation

-35:     mapping(uint => uint) public timestampForTokenId;
...
104:         questIdForTokenId[newTokenID] = questId_;
-105:         timestampForTokenId[newTokenID] = block.timestamp;
106:         _safeMint(to_, newTokenID);

• Gas costs before:

• Gas costs after:

This saves on average:

• 16,810 gas upon deployment for RabbitHoleReceipt
• 22,174 gas on every QuestFactory.mintReceipt call

setQuestFactory and setMinter can be combined in one function

As per QuestFactory.mintReceipt comments:

215: /// @dev mint a RabbitHole Receipt. Note: this contract must be set as Minter on the receipt contract

This means that when the owner calls RabbitHoleReceipt.setQuestFactory, they must follow it with a call to RabbitHoleReceipt.setMinter. Given that the base number of gas needed for any transaction is 21,000 gas, combining these two setters into one can save 21,000 gas every time the owner need to change the QuestFactory address.

Instances

RabbitHoleReceipt.sol

76:     /// @dev set the quest factory contract
77:     /// @param questFactory_ the address of the quest factory contract
78:     function setQuestFactory(address questFactory_) public onlyOwner {
79:         QuestFactoryContract = IQuestFactory(questFactory_);
80:     }
81: 
82:     /// @dev set the minter address
83:     /// @param minterAddress_ the address of the minter
84:     function setMinterAddress(address minterAddress_) public onlyOwner {
85:         minterAddress = minterAddress_;
86:         emit MinterAddressSet(minterAddress_);
87:     }

Tools Used

Manual Analysis,

Mitigation

76:     /// @dev set the quest factory contract
77:     /// @param questFactory_ the address of the quest factory contract
78:     function setQuestFactory(address questFactory_) public onlyOwner {
79:         QuestFactoryContract = IQuestFactory(questFactory_);
+           minterAddress = questFactory_;
+           emit MinterAddressSet(questFactory_);
80:     }
81: 
-82:     /// @dev set the minter address
-83:     /// @param minterAddress_ the address of the minter
-84:     function setMinterAddress(address minterAddress_) public onlyOwner -{
-85:         minterAddress = minterAddress_;
-86:         emit MinterAddressSet(minterAddress_);
-87:     }

• Gas costs before:

• Gas costs after:

This saves on average:

• 18,096 gas upon deployment for RabbitHoleReceipt
• 21,000 gas on every setQuestFactory call and setMinter call (as they are combined in one)