Platform: Code4rena
Start Date: 25/01/2023
Pot Size: $36,500 USDC
Total HM: 11
Participants: 173
Period: 5 days
Judge: kirk-baird
Total Solo HM: 1
Id: 208
League: ETH
Rank: 18/173
Findings: 4
Award: $283.81
š Selected for report: 1
š Solo Findings: 0
š Selected for report: adriro
Also found by: 0xRobocop, 0xmrhoodie, 0xngndev, AkshaySrivastav, ArmedGoose, Atarpara, Bauer, CodingNameKiki, ElKu, Garrett, HollaDieWaldfee, IllIllI, Iurii3, KIntern_NA, KmanOfficial, Lotus, M4TZ1P, MiniGlome, Ruhum, SovaSlava, bin2chen, bytes032, carrotsmuggler, cccz, chaduke, codeislight, cryptonue, doublesharp, evan, fs0c, glcanvas, gzeon, hansfriese, hihen, hl_, holme, horsefacts, ladboy233, lukris02, mahdikarimi, manikantanynala97, martin, mert_eren, mrpathfindr, omis, peakbolt, peanuts, prestoncodes, rbserver, rvierdiiev, sashik_eth, timongty, tnevler, trustindistrust, usmannk, wait, yixxas, zadaru13, zaskoh
0.7512 USDC - $0.75
There are no checks of whether the Quest owner has already claimed the protocol fee
A user that is late to claim their reward may have it taken by an owner that calls withdrawFee() multiple times. The owner can be an arbitrary user (outside of the control of the protocol), so it's not safe to just trust them to do the right thing, even if they or someone in the same organization may have originally provided the funds.
withdrawRemainingTokens() prevents taking unclaimed tokens, so withdrawFee() should too
The fee can be withdrawn any number of times, as long as the contract has a remaining balance:
// File: contracts/Erc20Quest.sol : Erc20Quest.withdrawFee() #1 102 function withdrawFee() public onlyAdminWithdrawAfterEnd { 103 @> IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee()); 104: }
protocolFee() doesn't base its answer on whether the fee has been claimed already
Code inspection
Store a flag saying whether the protocol fee has been withdrawn yet, and use it to prevent more than one fee withdrawal
#0 - c4-judge
2023-02-05T05:27:22Z
kirk-baird marked the issue as duplicate of #23
#1 - c4-judge
2023-02-14T08:54:38Z
kirk-baird changed the severity to 3 (High Risk)
#2 - c4-judge
2023-02-14T08:59:55Z
kirk-baird marked the issue as satisfactory
š Selected for report: carlitox477
Also found by: 0xRobocop, 0xbepresent, ArmedGoose, Atarpara, IllIllI, Tointer, UdarTeam, adriro, betweenETHlines, cryptojedi88, evan, glcanvas, gzeon, horsefacts, ladboy233, libratus, lukris02, luxartvinsec, manikantanynala97, minhquanym, mookimgo, p4st13r4, simon135, thekmj, trustindistrust
18.6976 USDC - $18.70
The more Receipts a user has, the more storage slots the claim() function makes.
If the user holds too many, they may be unable to claim rewards until they transfer some of them to other addresses they own
An external call looks up every one of the held Receipts, incurring storage reads for each. Next each one incurs a second storage read for checking whether it's claimable. On top of this, each incurs a storage write to mark it as claimed. Finally, the transfer of the rewards themselves may be expensive:
// File: contracts/Quest.sol : Quest.claim() #1 96 function claim() public virtual onlyQuestActive { 97 if (isPaused) revert QuestPaused(); 98 99 @> uint[] memory tokens = rabbitHoleReceiptContract.getOwnedTokenIdsOfQuest(questId, msg.sender); 100 101 if (tokens.length == 0) revert NoTokensToClaim(); 102 103 uint256 redeemableTokenCount = 0; 104 for (uint i = 0; i < tokens.length; i++) { 105 if (!isClaimed(tokens[i])) { 106 @> redeemableTokenCount++; 107 } 108 } 109 110 if (redeemableTokenCount == 0) revert AlreadyClaimed(); 111 112 uint256 totalRedeemableRewards = _calculateRewards(redeemableTokenCount); 113 @> _setClaimed(tokens); 114 @> _transferRewards(totalRedeemableRewards); 115 redeemedTokens += redeemableTokenCount; 116 117: emit Claimed(msg.sender, totalRedeemableRewards);
Code inspection
Allow a variant of the claim() function which takes in an offset and count, and claims only that subset of the tokens held
#0 - c4-judge
2023-02-05T05:27:05Z
kirk-baird marked the issue as duplicate of #135
#1 - c4-judge
2023-02-14T09:18:00Z
kirk-baird marked the issue as satisfactory
š Selected for report: CodingNameKiki
Also found by: 0x1f8b, 0x4non, 0x5rings, 0xAgro, 0xMirce, 0xRobocop, 0xSmartContract, 0xackermann, AkshaySrivastav, ArmedGoose, Aymen0909, BClabs, Breeje, Dewaxindo, Diana, HollaDieWaldfee, IceBear, IllIllI, Iurii3, Josiah, Krayt, PaludoX0, Phenomana, PrasadLak, RaymondFam, Rolezn, SaeedAlipoor01988, SaharDevep, SleepingBugs, adriro, arialblack14, bin2chen, brevis, btk, carlitox477, carrotsmuggler, catellatech, chaduke, chrisdior4, cryptonue, cryptostellar5, csanuragjain, ddimitrov22, fellows, glcanvas, halden, hl_, horsefacts, jat, joestakey, kenta, libratus, lukris02, luxartvinsec, manikantanynala97, martin, matrix_0wl, nadin, nicobevi, oberon, peanuts, prestoncodes, rbserver, sakshamguruji, sashik_eth, sayan, seeu, thekmj, tnevler, trustindistrust, tsvetanovv, xAriextz, zadaru13, zaskoh
119.6013 USDC - $119.60
| Issue | Instances | |
|---|---|---|
| [Lā01] | image_data should be used for raw svg | 2 |
| [Lā02] | Don't use strings for numbers in JSON | 1 |
| [Lā03] | Users may self-DOS themselves | 1 |
| [Lā04] | startTime_ should be allowed to be the current block | 1 |
| [Lā05] | Upgradeable contract not initialized | 1 |
| [Lā06] | Owner can renounce while system is paused | 1 |
| [Lā07] | Use Ownable2Step rather than Ownable | 3 |
| [Lā08] | Common code should be a mixin | 1 |
Total: 11 instances over 8 issues
| Issue | Instances | |
|---|---|---|
| [Nā01] | Use abstract contracts with unimplemented virtual functions | 2 |
| [Nā02] | Non-exploitable re-entrancies | 2 |
| [Nā03] | Erc1155Quest doesn't have an option for a protocol fee | 1 |
| [Nā04] | _disableInitializers() is preferred for constructors | 1 |
| [Nā05] | Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions | 3 |
| [Nā06] | Import declarations should import specific identifiers, rather than the whole file | 23 |
| [Nā07] | override function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings | 1 |
| [Nā08] | constants should be defined rather than using magic numbers | 7 |
| [Nā09] | Missing event and or timelock for critical parameter change | 5 |
| [Nā10] | Events that mark critical parameter changes should contain both the old and the new value | 4 |
| [Nā11] | Expressions for constant values such as a call to keccak256(), should use immutable rather than constant | 1 |
| [Nā12] | Lines are too long | 3 |
| [Nā13] | Non-library/interface files should use fixed compiler versions, not floating ones | 8 |
| [Nā14] | Typos | 1 |
| [Nā15] | File is missing NatSpec | 2 |
| [Nā16] | NatSpec is incomplete | 10 |
| [Nā17] | Not using the named return variables anywhere in the function is confusing | 4 |
| [Nā18] | Contracts should have full test coverage | 1 |
| [Nā19] | Large or complicated code bases should implement fuzzing tests | 1 |
| [Nā20] | Function ordering does not follow the Solidity style guide | 9 |
| [Nā21] | Contract does not follow the Solidity style guide's suggested layout ordering | 5 |
Total: 94 instances over 21 issues
image_data should be used for raw svgimage is for urls. See this NFT project which properly uses the image_data key, and renders nicely on OpenSea
There are 2 instances of this issue:
File: /contracts/TicketRenderer.sol 25 '"image": "', 26: generateSVG(tokenId_),
File: /contracts/ReceiptRenderer.sol 69 '"image": "', 70: generateSVG(tokenId_, questId_),
For numbers, OpenSea has extra display capabilities, and different behavior for different display_types. When the field is a number, an number rather than a string should be provided
There is 1 instance of this issue:
File: /contracts/ReceiptRenderer.sol 58: generateAttribute('Reward Amount', rewardAmount_.toString()),
If a user holds too many tokens for a quest, this function may run out of gas, especially since it makes quite a few storage lookups and does external calls in a loop
There is 1 instance of this issue:
File: /contracts/RabbitHoleReceipt.sol 109 function getOwnedTokenIdsOfQuest( 110 string memory questId_, 111 address claimingAddress_ 112: ) public view returns (uint[] memory) {
startTime_ should be allowed to be the current blockonlyQuestActive() allows the timestamp boundary to be on the current block, so it's inconsistent for it to be excluded here
There is 1 instance of this issue:
File: /contracts/Quest.sol 36: if (startTime_ <= block.timestamp) revert StartTimeInPast();
Upgradeable contracts are initialized via an initializer function rather than by a constructor. Leaving such a contract uninitialized may lead to it being taken over by a malicious user
There is 1 instance of this issue:
File: contracts/RabbitHoleReceipt.sol /// @audit missing __IERC2981_init() 15: contract RabbitHoleReceipt is
The contract owner or single user with a role is not prevented from renouncing the role/ownership while the contract is paused, which would cause any user assets stored in the protocol, to be locked indefinitely
There is 1 instance of this issue:
File: contracts/Quest.sol 57 function pause() public onlyOwner onlyStarted { 58 isPaused = true; 59: }
Ownable2Step rather than OwnableOwnable2Step and Ownable2StepUpgradeable prevent the contract ownership from mistakenly being transferred to an address that cannot handle it (e.g. due to a typo in the address), by requiring that the recipient of the owner permissions actively accept via a contract call of its own.
There are 3 instances of this issue:
File: contracts/QuestFactory.sol 16: contract QuestFactory is Initializable, OwnableUpgradeable, AccessControlUpgradeable, IQuestFactory {
File: contracts/RabbitHoleReceipt.sol 15 contract RabbitHoleReceipt is 16 Initializable, 17 ERC721Upgradeable, 18 ERC721EnumerableUpgradeable, 19 ERC721URIStorageUpgradeable, 20 OwnableUpgradeable, 21: IERC2981Upgradeable
File: contracts/RabbitHoleTickets.sol 11 contract RabbitHoleTickets is 12 Initializable, 13 ERC1155Upgradeable, 14 OwnableUpgradeable, 15 ERC1155BurnableUpgradeable, 16: IERC2981Upgradeable
The code below is repeated for both RabbitHoleReceipt and RabbitHoleTickets, and instead should be refactored to a common mixin
There is 1 instance of this issue:
File: /contracts/RabbitHoleReceipt.sol 58 modifier onlyMinter() { 59 msg.sender == minterAddress; 60 _; 61 } 62 63 /// @dev set the receipt renderer contract 64 /// @param receiptRenderer_ the address of the receipt renderer contract 65 function setReceiptRenderer(address receiptRenderer_) public onlyOwner { 66 ReceiptRendererContract = ReceiptRenderer(receiptRenderer_); 67 } 68 69 /// @dev set the royalty recipient 70 /// @param royaltyRecipient_ the address of the royalty recipient 71 function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner { 72 royaltyRecipient = royaltyRecipient_; 73 } 74 75 /// @dev set the quest factory contract 76 /// @param questFactory_ the address of the quest factory contract 77 function setQuestFactory(address questFactory_) public onlyOwner { 78 QuestFactoryContract = IQuestFactory(questFactory_); 79 } 80 81 /// @dev set the minter address 82 /// @param minterAddress_ the address of the minter 83 function setMinterAddress(address minterAddress_) public onlyOwner { 84 minterAddress = minterAddress_; 85 emit MinterAddressSet(minterAddress_); 86 } 87 88 /// @dev set the royalty fee 89 /// @param royaltyFee_ the royalty fee 90 function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner { 91 royaltyFee = royaltyFee_; 92 emit RoyaltyFeeSet(royaltyFee_); 93: }
abstract contracts with unimplemented virtual functionsThe compiler currently warns about the examples below being unreachable code. If you instead change the contracts to be abstract, you can leave the functions unimplemented, and force the extending contract to have an implementation
There are 2 instances of this issue:
File: /contracts/Quest.sol 123: revert MustImplementInChild(); 130: revert MustImplementInChild();
Code should follow the best-practice of check-effects-interaction
There are 2 instances of this issue:
File: /contracts/QuestFactory.sol 100 newQuest.transferOwnership(msg.sender); 101: ++questIdCount; 131 newQuest.transferOwnership(msg.sender); 132: ++questIdCount;
Erc1155Quest doesn't have an option for a protocol feeIt's inconsistent for Erc20Quest to support fees, but for Erc1155Quest not to
There is 1 instance of this issue:
File: /contracts/Erc1155Quest.sol 11: contract Erc1155Quest is Quest, ERC1155Holder {
_disableInitializers() is preferred for constructorsUse _disableInitializers() in the constructor, rather than using the initializer modifier
There is 1 instance of this issue:
File: /contracts/QuestFactory.sol 35: constructor() initializer {}
__gap[50] storage variable to allow for new storage variables in later versionsSee this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.
There are 3 instances of this issue:
File: contracts/QuestFactory.sol 16: contract QuestFactory is Initializable, OwnableUpgradeable, AccessControlUpgradeable, IQuestFactory {
File: contracts/RabbitHoleReceipt.sol 15 contract RabbitHoleReceipt is 16 Initializable, 17 ERC721Upgradeable, 18 ERC721EnumerableUpgradeable, 19 ERC721URIStorageUpgradeable, 20 OwnableUpgradeable, 21: IERC2981Upgradeable
File: contracts/RabbitHoleTickets.sol 11 contract RabbitHoleTickets is 12 Initializable, 13 ERC1155Upgradeable, 14 OwnableUpgradeable, 15 ERC1155BurnableUpgradeable, 16: IERC2981Upgradeable
Using import declarations of the form import {<identifier_name>} from "some/file.sol" avoids polluting the symbol namespace making flattened files smaller, and speeds up compilation
There are 23 instances of this issue:
File: contracts/QuestFactory.sol 4: import '@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol'; 10: import '@openzeppelin/contracts-upgradeable/utils/cryptography/ECDSAUpgradeable.sol'; 11: import '@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol';
File: contracts/RabbitHoleReceipt.sol 4: import '@openzeppelin/contracts-upgradeable/token/ERC721/ERC721Upgradeable.sol'; 5: import '@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721URIStorageUpgradeable.sol'; 6: import '@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721EnumerableUpgradeable.sol'; 7: import '@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol'; 8: import '@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol'; 9: import '@openzeppelin/contracts-upgradeable/interfaces/IERC2981Upgradeable.sol'; 10: import '@openzeppelin/contracts-upgradeable/utils/CountersUpgradeable.sol'; 11: import './ReceiptRenderer.sol'; 12: import './interfaces/IQuestFactory.sol'; 13: import './interfaces/IQuest.sol';
File: contracts/RabbitHoleTickets.sol 4: import '@openzeppelin/contracts-upgradeable/token/ERC1155/ERC1155Upgradeable.sol'; 5: import '@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol'; 6: import '@openzeppelin/contracts-upgradeable/token/ERC1155/extensions/ERC1155BurnableUpgradeable.sol'; 7: import '@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol'; 8: import '@openzeppelin/contracts-upgradeable/interfaces/IERC2981Upgradeable.sol'; 9: import './TicketRenderer.sol';
File: contracts/ReceiptRenderer.sol 4: import '@openzeppelin/contracts/utils/Base64.sol'; 5: import '@openzeppelin/contracts/utils/Strings.sol';
File: contracts/TicketRenderer.sol 4: import '@openzeppelin/contracts/utils/Base64.sol'; 5: import '@openzeppelin/contracts/utils/Strings.sol';
override function arguments that are unused should have the variable name removed or commented out to avoid compiler warningsThere is 1 instance of this issue:
File: contracts/RabbitHoleTickets.sol 110: uint256 tokenId_,
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 7 instances of this issue:
File: contracts/Erc20Quest.sol /// @audit 10_000 53: return (maxTotalRewards() * questFee) / 10_000; /// @audit 10_000 97: return (receiptRedeemers() * rewardAmountInWeiOrTokenId * questFee) / 10_000;
File: contracts/QuestFactory.sol /// @audit 2_000 48: setQuestFee(2_000); /// @audit 10_000 187: if (questFee_ > 10_000) revert QuestFeeTooHigh();
File: contracts/RabbitHoleReceipt.sol /// @audit 10_000 184: uint256 royaltyPayment = (salePrice_ * royaltyFee) / 10_000;
File: contracts/RabbitHoleTickets.sol /// @audit 10_000 113: uint256 royaltyPayment = (salePrice_ * royaltyFee) / 10_000;
File: contracts/ReceiptRenderer.sol /// @audit 20 60: generateAttribute('Reward Address', Strings.toHexString(uint160(rewardAddress_), 20)),
Events help non-contract tools to track changes, and events prevent users from being surprised by changes
There are 5 instances of this issue:
File: contracts/QuestFactory.sol 159 function setClaimSignerAddress(address claimSignerAddress_) public onlyOwner { 160 claimSignerAddress = claimSignerAddress_; 161: } 165 function setProtocolFeeRecipient(address protocolFeeRecipient_) public onlyOwner { 166 if (protocolFeeRecipient_ == address(0)) revert AddressZeroNotAllowed(); 167 protocolFeeRecipient = protocolFeeRecipient_; 168: } 186 function setQuestFee(uint256 questFee_) public onlyOwner { 187 if (questFee_ > 10_000) revert QuestFeeTooHigh(); 188 questFee = questFee_; 189: }
File: contracts/RabbitHoleReceipt.sol 71 function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner { 72 royaltyRecipient = royaltyRecipient_; 73: }
File: contracts/RabbitHoleTickets.sol 60 function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner { 61 royaltyRecipient = royaltyRecipient_; 62: }
This should especially be done if the new value is not required to be different from the old value
There are 4 instances of this issue:
File: contracts/RabbitHoleReceipt.sol /// @audit setMinterAddress() 85: emit MinterAddressSet(minterAddress_); /// @audit setRoyaltyFee() 92: emit RoyaltyFeeSet(royaltyFee_);
File: contracts/RabbitHoleTickets.sol /// @audit setRoyaltyFee() 68: emit RoyaltyFeeSet(royaltyFee_); /// @audit setMinterAddress() 75: emit MinterAddressSet(minterAddress_);
keccak256(), should use immutable rather than constantWhile it doesn't save any gas because the compiler knows that developers often make this mistake, it's still best to use the right tool for the task at hand. There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts. constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.
There is 1 instance of this issue:
File: contracts/QuestFactory.sol 17: bytes32 public constant CREATE_QUEST_ROLE = keccak256('CREATE_QUEST_ROLE');
Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length
There are 3 instances of this issue:
File: contracts/Erc20Quest.sol 56: /// @notice Starts the quest by marking it ready to start at the contract level. Marking a quest ready to start does not mean that it is live. It also requires that the start time has passed 57: /// @dev Requires that the balance of the rewards in the contract is greater than or equal to the maximum amount of rewards that can be claimed by all users and the protocol
File: contracts/interfaces/IQuestFactory.sol 16: event QuestCreated(address indexed creator, address indexed contractAddress, string indexed questId, string contractType, address rewardTokenAddress, uint256 endTime, uint256 startTime, uint256 totalParticipants, uint256 rewardAmountOrTokenId);
There are 8 instances of this issue:
File: contracts/Erc1155Quest.sol 2: pragma solidity ^0.8.15;
File: contracts/Erc20Quest.sol 2: pragma solidity ^0.8.15;
File: contracts/QuestFactory.sol 2: pragma solidity ^0.8.15;
File: contracts/Quest.sol 2: pragma solidity ^0.8.15;
File: contracts/RabbitHoleReceipt.sol 2: pragma solidity ^0.8.15;
File: contracts/RabbitHoleTickets.sol 2: pragma solidity ^0.8.15;
File: contracts/ReceiptRenderer.sol 2: pragma solidity ^0.8.15;
File: contracts/TicketRenderer.sol 2: pragma solidity ^0.8.15;
There is 1 instance of this issue:
File: contracts/QuestFactory.sol /// @audit remave 176: /// @dev set or remave a contract address to be used as a reward
There are 2 instances of this issue:
File: contracts/interfaces/IQuestFactory.sol
File: contracts/interfaces/IQuest.sol
There are 10 instances of this issue:
File: contracts/QuestFactory.sol /// @audit Missing: '@return' 191 /// @dev return the number of minted receipts for a quest 192 /// @param questId_ The id of the quest 193: function getNumberMinted(string memory questId_) external view returns (uint) { /// @audit Missing: '@return' 197 /// @dev return data in the quest struct for a questId 198 /// @param questId_ The id of the quest 199: function questInfo(string memory questId_) external view returns (address, uint, uint) { /// @audit Missing: '@return' 208 /// @param hash_ The hash of the message 209 /// @param signature_ The signature of the hash 210: function recoverSigner(bytes32 hash_, bytes memory signature_) public pure returns (address) {
File: contracts/Quest.sol /// @audit Missing: '@return' 133 /// @notice Checks if a Receipt token id has been used to claim a reward 134 /// @param tokenId_ The token id to check 135: function isClaimed(uint256 tokenId_) public view returns (bool) {
File: contracts/RabbitHoleReceipt.sol /// @audit Missing: '@return' 107 /// @param questId_ the quest id 108 /// @param claimingAddress_ the address claiming to own the tokens 109 function getOwnedTokenIdsOfQuest( 110 string memory questId_, 111 address claimingAddress_ 112: ) public view returns (uint[] memory) { /// @audit Missing: '@return' 176 /// @param tokenId_ the token id 177 /// @param salePrice_ the sale price 178 function royaltyInfo( 179 uint256 tokenId_, 180 uint256 salePrice_ 181: ) external view override returns (address receiver, uint256 royaltyAmount) { /// @audit Missing: '@return' 188 /// @dev returns true if the supplied interface id is supported 189 /// @param interfaceId_ the interface id 190 function supportsInterface( 191 bytes4 interfaceId_ 192: ) public view virtual override(ERC721Upgradeable, ERC721EnumerableUpgradeable, IERC165Upgradeable) returns (bool) {
File: contracts/RabbitHoleTickets.sol /// @audit Missing: '@return' 107 /// @param tokenId_ the token id 108 /// @param salePrice_ the sale price 109 function royaltyInfo( 110 uint256 tokenId_, 111 uint256 salePrice_ 112: ) external view override returns (address receiver, uint256 royaltyAmount) { /// @audit Missing: '@return' 117 /// @dev returns true if the supplied interface id is supported 118 /// @param interfaceId_ the interface id 119 function supportsInterface( 120 bytes4 interfaceId_ 121: ) public view virtual override(ERC1155Upgradeable, IERC165Upgradeable) returns (bool) {
File: contracts/ReceiptRenderer.sol /// @audit Missing: '@return' 80 /// @param key The key for the attribute 81 /// @param value The value for the attribute 82: function generateAttribute(string memory key, string memory value) public pure returns (string memory) {
Consider changing the variable to be an unnamed one
There are 4 instances of this issue:
File: contracts/RabbitHoleReceipt.sol /// @audit receiver /// @audit royaltyAmount 178 function royaltyInfo( 179 uint256 tokenId_, 180 uint256 salePrice_ 181: ) external view override returns (address receiver, uint256 royaltyAmount) {
File: contracts/RabbitHoleTickets.sol /// @audit receiver /// @audit royaltyAmount 109 function royaltyInfo( 110 uint256 tokenId_, 111 uint256 salePrice_ 112: ) external view override returns (address receiver, uint256 royaltyAmount) {
While 100% code coverage does not guarantee that there are no bugs, it often will catch easy-to-find bugs, and will ensure that there are fewer regressions when the code invariably has to be modified. Furthermore, in order to get full coverage, code authors will often have to re-organize their code so that it is more modular, so that each component can be tested separately, which reduces interdependencies between modules and layers, and makes for code that is easier to reason about and audit.
There is 1 instance of this issue:
File: Various Files
Large code bases, or code with lots of inline-assembly, complicated math, or complicated interactions between multiple contracts, should implement fuzzing tests. Fuzzers such as Echidna require the test writer to come up with invariants which should not be violated under any circumstances, and the fuzzer tests various inputs and function calls to ensure that the invariants always hold. Even code with 100% code coverage can still have bugs due to the order of the operations a user performs, and fuzzers, with properly and extensively-written invariants, can close this testing gap significantly.
There is 1 instance of this issue:
File: Various Files
According to the Solidity style guide, functions should be laid out in the following order :constructor(), receive(), fallback(), external, public, internal, private, but the cases below do not follow this pattern
There are 9 instances of this issue:
File: contracts/Erc1155Quest.sol /// @audit _calculateRewards() came earlier 54: function withdrawRemainingTokens(address to_) public override onlyOwner {
File: contracts/Erc20Quest.sol /// @audit _calculateRewards() came earlier 81: function withdrawRemainingTokens(address to_) public override onlyOwner {
File: contracts/QuestFactory.sol /// @audit grantDefaultAdminAndCreateQuestRole() came earlier 159: function setClaimSignerAddress(address claimSignerAddress_) public onlyOwner { /// @audit setQuestFee() came earlier 193: function getNumberMinted(string memory questId_) external view returns (uint) {
File: contracts/Quest.sol /// @audit _setClaimed() came earlier 96: function claim() public virtual onlyQuestActive { /// @audit _transferRewards() came earlier 135: function isClaimed(uint256 tokenId_) public view returns (bool) {
File: contracts/RabbitHoleReceipt.sol /// @audit _beforeTokenTransfer() came earlier 158 function tokenURI( 159 uint tokenId_ 160: ) public view virtual override(ERC721Upgradeable, ERC721URIStorageUpgradeable) returns (string memory) { /// @audit tokenURI() came earlier 178 function royaltyInfo( 179 uint256 tokenId_, 180 uint256 salePrice_ 181: ) external view override returns (address receiver, uint256 royaltyAmount) {
File: contracts/RabbitHoleTickets.sol /// @audit uri() came earlier 109 function royaltyInfo( 110 uint256 tokenId_, 111 uint256 salePrice_ 112: ) external view override returns (address receiver, uint256 royaltyAmount) {
The style guide says that, within a contract, the ordering should be 1) Type declarations, 2) State variables, 3) Events, 4) Modifiers, and 5) Functions, but the contract(s) below do not follow this ordering
There are 5 instances of this issue:
File: contracts/Quest.sol /// @audit function _setClaimed came earlier 76 modifier onlyAdminWithdrawAfterEnd() { 77 if (block.timestamp < endTime) revert NoWithdrawDuringClaim(); 78 _; 79: }
File: contracts/RabbitHoleReceipt.sol /// @audit event MinterAddressSet came earlier 27: CountersUpgradeable.Counter private _tokenIds; /// @audit function initialize came earlier 58 modifier onlyMinter() { 59 msg.sender == minterAddress; 60 _; 61: }
File: contracts/RabbitHoleTickets.sol /// @audit event MinterAddressSet came earlier 22: address public royaltyRecipient; /// @audit function initialize came earlier 47 modifier onlyMinter() { 48 msg.sender == minterAddress; 49 _; 50: }
These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness
| Issue | Instances | |
|---|---|---|
| [Lā01] | Missing checks for address(0x0) when assigning values to address state variables | 12 |
| [Lā02] | abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256() | 6 |
Total: 18 instances over 2 issues
| Issue | Instances | |
|---|---|---|
| [Nā01] | public functions not called by the contract should be declared external instead | 27 |
| [Nā02] | Event is missing indexed fields | 1 |
Total: 28 instances over 2 issues
address(0x0) when assigning values to address state variablesThere are 12 instances of this issue:
File: contracts/Erc20Quest.sol /// @audit (valid but excluded finding) 39: protocolFeeRecipient = protocolFeeRecipient_;
File: contracts/QuestFactory.sol /// @audit (valid but excluded finding) 45: claimSignerAddress = claimSignerAddress_; /// @audit (valid but excluded finding) 160: claimSignerAddress = claimSignerAddress_;
File: contracts/Quest.sol /// @audit (valid but excluded finding) 40: rewardToken = rewardTokenAddress_;
File: contracts/RabbitHoleReceipt.sol /// @audit (valid but excluded finding) 52: royaltyRecipient = royaltyRecipient_; /// @audit (valid but excluded finding) 53: minterAddress = minterAddress_; /// @audit (valid but excluded finding) 72: royaltyRecipient = royaltyRecipient_; /// @audit (valid but excluded finding) 84: minterAddress = minterAddress_;
File: contracts/RabbitHoleTickets.sol /// @audit (valid but excluded finding) 41: royaltyRecipient = royaltyRecipient_; /// @audit (valid but excluded finding) 42: minterAddress = minterAddress_; /// @audit (valid but excluded finding) 61: royaltyRecipient = royaltyRecipient_; /// @audit (valid but excluded finding) 74: minterAddress = minterAddress_;
abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.
If all arguments are strings and or bytes, bytes.concat() should be used instead
There are 6 instances of this issue:
File: contracts/QuestFactory.sol /// @audit (valid but excluded finding) /// @audit (valid but excluded finding) 72: if (keccak256(abi.encodePacked(contractType_)) == keccak256(abi.encodePacked('erc20'))) { /// @audit (valid but excluded finding) /// @audit (valid but excluded finding) 105: if (keccak256(abi.encodePacked(contractType_)) == keccak256(abi.encodePacked('erc1155'))) { /// @audit (valid but excluded finding) 211: bytes32 messageDigest = keccak256(abi.encodePacked('\x19Ethereum Signed Message:\n32', hash_)); /// @audit (valid but excluded finding) 222: if (keccak256(abi.encodePacked(msg.sender, questId_)) != hash_) revert InvalidHash();
public functions not called by the contract should be declared external insteadContracts are allowed to override their parents' functions and change the visibility from external to public.
There are 27 instances of this issue:
File: contracts/Erc20Quest.sol /// @audit (valid but excluded finding) 102: function withdrawFee() public onlyAdminWithdrawAfterEnd {
File: contracts/QuestFactory.sol /// @audit (valid but excluded finding) 61 function createQuest( 62 address rewardTokenAddress_, 63 uint256 endTime_, 64 uint256 startTime_, 65 uint256 totalParticipants_, 66 uint256 rewardAmountOrTokenId_, 67 string memory contractType_, 68 string memory questId_ 69: ) public onlyRole(CREATE_QUEST_ROLE) returns (address) { /// @audit (valid but excluded finding) 142: function changeCreateQuestRole(address account_, bool canCreateQuest_) public onlyOwner { /// @audit (valid but excluded finding) 159: function setClaimSignerAddress(address claimSignerAddress_) public onlyOwner { /// @audit (valid but excluded finding) 172: function setRabbitHoleReceiptContract(address rabbitholeReceiptContract_) public onlyOwner { /// @audit (valid but excluded finding) 179: function setRewardAllowlistAddress(address rewardAddress_, bool allowed_) public onlyOwner { /// @audit (valid but excluded finding) 219: function mintReceipt(string memory questId_, bytes32 hash_, bytes memory signature_) public {
File: contracts/Quest.sol /// @audit (valid but excluded finding) 57: function pause() public onlyOwner onlyStarted { /// @audit (valid but excluded finding) 63: function unPause() public onlyOwner onlyStarted { /// @audit (valid but excluded finding) 140: function getRewardAmount() public view returns (uint256) { /// @audit (valid but excluded finding) 145: function getRewardToken() public view returns (address) {
File: contracts/RabbitHoleReceipt.sol /// @audit (valid but excluded finding) 43 function initialize( 44 address receiptRenderer_, 45 address royaltyRecipient_, 46 address minterAddress_, 47 uint royaltyFee_ 48: ) public initializer { /// @audit (valid but excluded finding) 65: function setReceiptRenderer(address receiptRenderer_) public onlyOwner { /// @audit (valid but excluded finding) 71: function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner { /// @audit (valid but excluded finding) 77: function setQuestFactory(address questFactory_) public onlyOwner { /// @audit (valid but excluded finding) 83: function setMinterAddress(address minterAddress_) public onlyOwner { /// @audit (valid but excluded finding) 90: function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner { /// @audit (valid but excluded finding) 98: function mint(address to_, string memory questId_) public onlyMinter { /// @audit (valid but excluded finding) 109 function getOwnedTokenIdsOfQuest( 110 string memory questId_, 111 address claimingAddress_ 112: ) public view returns (uint[] memory) {
File: contracts/RabbitHoleTickets.sol /// @audit (valid but excluded finding) 32 function initialize( 33 address ticketRenderer_, 34 address royaltyRecipient_, 35 address minterAddress_, 36 uint royaltyFee_ 37: ) public initializer { /// @audit (valid but excluded finding) 54: function setTicketRenderer(address ticketRenderer_) public onlyOwner { /// @audit (valid but excluded finding) 60: function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner { /// @audit (valid but excluded finding) 66: function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner { /// @audit (valid but excluded finding) 73: function setMinterAddress(address minterAddress_) public onlyOwner { /// @audit (valid but excluded finding) 83: function mint(address to_, uint256 id_, uint256 amount_, bytes memory data_) public onlyMinter { /// @audit (valid but excluded finding) 92 function mintBatch( 93 address to_, 94 uint256[] memory ids_, 95 uint256[] memory amounts_, 96 bytes memory data_ 97: ) public onlyMinter {
File: contracts/TicketRenderer.sol /// @audit (valid but excluded finding) 16 function generateTokenURI( 17 uint tokenId_ 18: ) public pure returns (string memory) {
indexed fieldsIndex event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
There is 1 instance of this issue:
File: contracts/interfaces/IQuest.sol /// @audit (valid but excluded finding) 8: event Claimed(address account, uint256 amount);
#0 - c4-sponsor
2023-02-08T04:08:36Z
jonathandiep marked the issue as sponsor confirmed
#1 - c4-judge
2023-02-16T07:03:42Z
kirk-baird marked the issue as grade-a
š Selected for report: IllIllI
Also found by: 0x1f8b, 0x4non, 0xAgro, 0xSmartContract, 0xhacksmithh, 0xngndev, Aymen0909, Bnke0x0, Breeje, Deivitto, Diana, Dug, Iurii3, LethL, MiniGlome, NoamYakov, RaymondFam, ReyAdmirado, Rolezn, SAAJ, adriro, ali, arialblack14, atharvasama, c3phas, carlitox477, catellatech, chaduke, cryptonue, cryptostellar5, ddimitrov22, dharma09, doublesharp, favelanky, georgits, glcanvas, gzeon, halden, horsefacts, jasonxiale, joestakey, karanctf, lukris02, matrix_0wl, nadin, navinavu, saneryee, shark, thekmj
144.7607 USDC - $144.76
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [Gā01] | Shorten the array rather than copying to a new one | 1 | - |
| [Gā02] | Hash shouldn't be re-calculated on every iteration of the for-loop | 1 | - |
| [Gā03] | Using calldata instead of memory for read-only arguments in external functions saves gas | 12 | 1440 |
| [Gā04] | Multiple accesses of a mapping/array should use a local variable cache | 9 | 378 |
| [Gā05] | The result of function calls should be cached rather than re-calling the function | 1 | - |
| [Gā06] | <x> += <y> costs more gas than <x> = <x> + <y> for state variables | 1 | 113 |
| [Gā07] | internal functions only called once can be inlined to save gas | 1 | 20 |
| [Gā08] | ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops | 4 | 240 |
| [Gā09] | Optimize names to save gas | 8 | 176 |
| [Gā10] | String literals passed to abi.encode()/abi.encodePacked() should not be split by commas | 5 | - |
| [Gā11] | Using private rather than public for constants, saves gas | 5 | - |
| [Gā12] | Don't compare boolean expressions to boolean literals | 3 | 27 |
| [Gā13] | Use custom errors rather than revert()/require() strings to save gas | 3 | - |
| [Gā14] | Functions guaranteed to revert when called by normal users can be marked payable | 2 | 42 |
Total: 56 instances over 14 issues with 2436 gas saved
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.
Inline-assembly can be used to shorten the array by changing the length slot, so that the entries don't have to be copied to a new, shorter array
There is 1 instance of this issue:
File: /contracts/RabbitHoleReceipt.sol 125 uint[] memory filteredTokens = new uint[](foundTokens); 126 uint filterTokensIndexTracker = 0; 127 128 for (uint i = 0; i < msgSenderBalance; i++) { 129 if (tokenIdsForQuest[i] > 0) { 130 filteredTokens[filterTokensIndexTracker] = tokenIdsForQuest[i]; 131 filterTokensIndexTracker++; 132 } 133: }
for-loopCalculate the hash outside of the loop, and use that value within the loop
There is 1 instance of this issue:
File: /contracts/RabbitHoleReceipt.sol 119: if (keccak256(bytes(questIdForTokenId[tokenId])) == keccak256(bytes(questId_))) {
calldata instead of memory for read-only arguments in external functions saves gasWhen a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Note that even if an interface defines a function as having memory arguments, it's still valid for implementation contracs to use calldata arguments instead.
If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one
Note that I've also flagged instances where the function is public but can be marked as external since it's not called by the contract, and cases where a constructor is involved
There are 12 instances of this issue:
File: contracts/QuestFactory.sol /// @audit contractType_ /// @audit questId_ 61 function createQuest( 62 address rewardTokenAddress_, 63 uint256 endTime_, 64 uint256 startTime_, 65 uint256 totalParticipants_, 66 uint256 rewardAmountOrTokenId_, 67 string memory contractType_, 68 string memory questId_ 69: ) public onlyRole(CREATE_QUEST_ROLE) returns (address) { /// @audit questId_ 193: function getNumberMinted(string memory questId_) external view returns (uint) { /// @audit questId_ 199: function questInfo(string memory questId_) external view returns (address, uint, uint) { /// @audit questId_ /// @audit signature_ 219: function mintReceipt(string memory questId_, bytes32 hash_, bytes memory signature_) public {
File: contracts/RabbitHoleReceipt.sol /// @audit questId_ 98: function mint(address to_, string memory questId_) public onlyMinter { /// @audit questId_ 109 function getOwnedTokenIdsOfQuest( 110 string memory questId_, 111 address claimingAddress_ 112: ) public view returns (uint[] memory) {
File: contracts/RabbitHoleTickets.sol /// @audit data_ 83: function mint(address to_, uint256 id_, uint256 amount_, bytes memory data_) public onlyMinter { /// @audit ids_ /// @audit amounts_ /// @audit data_ 92 function mintBatch( 93 address to_, 94 uint256[] memory ids_, 95 uint256[] memory amounts_, 96 bytes memory data_ 97: ) public onlyMinter {
The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations. Caching an array's struct avoids recalculating the array offsets into memory/calldata
There are 9 instances of this issue:
File: contracts/QuestFactory.sol /// @audit quests[questId_] on line 70 98: quests[questId_].questAddress = address(newQuest); /// @audit quests[questId_] on line 98 99: quests[questId_].totalParticipants = totalParticipants_; /// @audit quests[questId_] on line 129 130: quests[questId_].totalParticipants = totalParticipants_; /// @audit quests[questId_] on line 201 202: quests[questId_].totalParticipants, /// @audit quests[questId_] on line 202 203: quests[questId_].numberMinted /// @audit quests[questId_] on line 220 220: if (quests[questId_].numberMinted + 1 > quests[questId_].totalParticipants) revert OverMaxAllowedToMint(); /// @audit quests[questId_] on line 220 221: if (quests[questId_].addressMinted[msg.sender] == true) revert AddressAlreadyMinted(); /// @audit quests[questId_] on line 221 225: quests[questId_].addressMinted[msg.sender] = true; /// @audit quests[questId_] on line 225 226: quests[questId_].numberMinted++;
The instances below point to the second+ call of the function within a single function
There is 1 instance of this issue:
File: contracts/ReceiptRenderer.sol /// @audit tokenId_.toString() on line 52 66: tokenId_.toString(),
<x> += <y> costs more gas than <x> = <x> + <y> for state variablesUsing the addition operator instead of plus-equals saves 113 gas
There is 1 instance of this issue:
File: contracts/Quest.sol 115: redeemedTokens += redeemableTokenCount;
internal functions only called once can be inlined to save gasNot inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
There is 1 instance of this issue:
File: contracts/QuestFactory.sol 152: function grantDefaultAdminAndCreateQuestRole(address account_) internal {
++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loopsThe unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop
There are 4 instances of this issue:
File: contracts/Quest.sol 70: for (uint i = 0; i < tokenIds_.length; i++) { 104: for (uint i = 0; i < tokens.length; i++) {
File: contracts/RabbitHoleReceipt.sol 117: for (uint i = 0; i < msgSenderBalance; i++) { 128: for (uint i = 0; i < msgSenderBalance; i++) {
public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted
There are 8 instances of this issue:
File: contracts/Erc20Quest.sol /// @audit maxTotalRewards(), maxProtocolReward(), receiptRedeemers(), protocolFee(), withdrawFee() 11: contract Erc20Quest is Quest {
File: contracts/interfaces/IQuest.sol /// @audit isClaimed(), getRewardAmount(), getRewardToken() 6: interface IQuest {
File: contracts/QuestFactory.sol /// @audit initialize(), createQuest(), changeCreateQuestRole(), setClaimSignerAddress(), setProtocolFeeRecipient(), setRabbitHoleReceiptContract(), setRewardAllowlistAddress(), setQuestFee(), getNumberMinted(), questInfo(), recoverSigner(), mintReceipt() 16: contract QuestFactory is Initializable, OwnableUpgradeable, AccessControlUpgradeable, IQuestFactory {
File: contracts/Quest.sol /// @audit unPause(), claim(), isClaimed(), getRewardAmount(), getRewardToken(), withdrawRemainingTokens() 12: contract Quest is Ownable, IQuest {
File: contracts/RabbitHoleReceipt.sol /// @audit initialize(), setReceiptRenderer(), setRoyaltyRecipient(), setQuestFactory(), setMinterAddress(), setRoyaltyFee(), mint(), getOwnedTokenIdsOfQuest() 15: contract RabbitHoleReceipt is
File: contracts/RabbitHoleTickets.sol /// @audit initialize(), setTicketRenderer(), setRoyaltyRecipient(), setRoyaltyFee(), setMinterAddress(), mint(), mintBatch() 11: contract RabbitHoleTickets is
File: contracts/ReceiptRenderer.sol /// @audit generateTokenURI(), generateDataURI(), generateAttribute(), generateSVG() 10: contract ReceiptRenderer {
File: contracts/TicketRenderer.sol /// @audit generateTokenURI(), generateSVG() 10: contract TicketRenderer {
abi.encode()/abi.encodePacked() should not be split by commasString literals can be split into multiple parts and still be considered as a single string literal. Adding commas between each chunk makes it no longer a single string, and instead multiple strings. EACH new comma costs 21 gas due to stack operations and separate MSTOREs
There are 5 instances of this issue:
File: contracts/ReceiptRenderer.sol /// @audit 4 commas 63 bytes memory dataURI = abi.encodePacked( 64 '{', 65 '"name": "RabbitHole.gg Receipt #', 66 tokenId_.toString(), 67 '",', 68 '"description": "RabbitHole.gg Receipts are used to claim rewards from completed quests.",', 69 '"image": "', 70 generateSVG(tokenId_, questId_), 71 '",', 72 '"attributes": ', 73 attributes, 74 '}' 75: ); /// @audit 3 commas 83 bytes memory attribute = abi.encodePacked( 84 '{', 85 '"trait_type": "', 86 key, 87 '",', 88 '"value": "', 89 value, 90 '"', 91 '}' 92: ); /// @audit 5 commas 101 bytes memory svg = abi.encodePacked( 102 '<svg xmlns="http://www.w3.org/2000/svg" preserveAspectRatio="xMinYMin meet" viewBox="0 0 350 350">', 103 '<style>.base { fill: white; font-family: serif; font-size: 14px; }</style>', 104 '<rect width="100%" height="100%" fill="black" />', 105 '<text x="50%" y="40%" class="base" dominant-baseline="middle" text-anchor="middle">RabbitHole Quest #', 106 questId_, 107 '</text>', 108 '<text x="70%" y="40%" class="base" dominant-baseline="middle" text-anchor="middle">RabbitHole Quest Receipt #', 109 tokenId_, 110 '</text>', 111 '</svg>' 112: );
File: contracts/TicketRenderer.sol /// @audit 4 commas 19 bytes memory dataURI = abi.encodePacked( 20 '{', 21 '"name": "RabbitHole Tickets #', 22 tokenId_.toString(), 23 '",', 24 '"description": "A reward for completing quests within RabbitHole, with unk(no)wn utility",', 25 '"image": "', 26 generateSVG(tokenId_), 27 '"', 28 '}' 29: ); /// @audit 4 commas 37 bytes memory svg = abi.encodePacked( 38 '<svg xmlns="http://www.w3.org/2000/svg" preserveAspectRatio="xMinYMin meet" viewBox="0 0 350 350">', 39 '<style>.base { fill: white; font-family: serif; font-size: 14px; }</style>', 40 '<rect width="100%" height="100%" fill="black" />', 41 '<text x="50%" y="40%" class="base" dominant-baseline="middle" text-anchor="middle">RabbitHole Tickets #', 42 tokenId_.toString(), 43 '</text>', 44 '</svg>' 45: );
private rather than public for constants, saves gasIf needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table
There are 5 instances of this issue:
File: contracts/Erc20Quest.sol 13: uint256 public immutable questFee;
File: contracts/Quest.sol 15: uint256 public immutable endTime; 16: uint256 public immutable startTime; 17: uint256 public immutable totalParticipants; 18: uint256 public immutable rewardAmountInWeiOrTokenId;
if (<x> == true) => if (<x>), if (<x> == false) => if (!<x>)
There are 3 instances of this issue:
File: contracts/QuestFactory.sol 73: if (rewardAllowlist[rewardTokenAddress_] == false) revert RewardNotAllowed(); 221: if (quests[questId_].addressMinted[msg.sender] == true) revert AddressAlreadyMinted();
File: contracts/Quest.sol 136: return claimedList[tokenId_] == true;
revert()/require() strings to save gasCustom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas
There are 3 instances of this issue:
File: contracts/RabbitHoleReceipt.sol 161: require(_exists(tokenId_), 'ERC721URIStorage: URI query for nonexistent token'); 162: require(QuestFactoryContract != IQuestFactory(address(0)), 'QuestFactory not set'); 182: require(_exists(tokenId_), 'Nonexistent token');
payableIf a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are
CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost
There are 2 instances of this issue:
File: contracts/QuestFactory.sol 61 function createQuest( 62 address rewardTokenAddress_, 63 uint256 endTime_, 64 uint256 startTime_, 65 uint256 totalParticipants_, 66 uint256 rewardAmountOrTokenId_, 67 string memory contractType_, 68 string memory questId_ 69: ) public onlyRole(CREATE_QUEST_ROLE) returns (address) {
File: contracts/RabbitHoleTickets.sol 92 function mintBatch( 93 address to_, 94 uint256[] memory ids_, 95 uint256[] memory amounts_, 96 bytes memory data_ 97: ) public onlyMinter {
These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [Gā01] | <array>.length should not be looked up in every loop of a for-loop | 2 | 6 |
| [Gā02] | require()/revert() strings longer than 32 bytes cost extra gas | 1 | - |
| [Gā03] | Using bools for storage incurs overhead | 4 | 68400 |
| [Gā04] | ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) | 8 | 40 |
| [Gā05] | Using private rather than public for constants, saves gas | 1 | - |
| [Gā06] | Functions guaranteed to revert when called by normal users can be marked payable | 25 | 525 |
Total: 41 instances over 6 issues with 68971 gas saved
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.
<array>.length should not be looked up in every loop of a for-loopThe overheads outlined below are PER LOOP, excluding the first loop
MLOAD (3 gas)CALLDATALOAD (3 gas)Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
There are 2 instances of this issue:
File: contracts/Quest.sol /// @audit (valid but excluded finding) 70: for (uint i = 0; i < tokenIds_.length; i++) { /// @audit (valid but excluded finding) 104: for (uint i = 0; i < tokens.length; i++) {
require()/revert() strings longer than 32 bytes cost extra gasEach extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas
There is 1 instance of this issue:
File: contracts/RabbitHoleReceipt.sol /// @audit (valid but excluded finding) 161: require(_exists(tokenId_), 'ERC721URIStorage: URI query for nonexistent token');
bools for storage incurs overhead// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27
Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past
There are 4 instances of this issue:
File: contracts/QuestFactory.sol /// @audit (valid but excluded finding) 30: mapping(address => bool) public rewardAllowlist;
File: contracts/Quest.sol /// @audit (valid but excluded finding) 19: bool public hasStarted; /// @audit (valid but excluded finding) 20: bool public isPaused; /// @audit (valid but excluded finding) 24: mapping(uint256 => bool) private claimedList;
++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)Saves 5 gas per loop
There are 8 instances of this issue:
File: contracts/QuestFactory.sol /// @audit (valid but excluded finding) 226: quests[questId_].numberMinted++;
File: contracts/Quest.sol /// @audit (valid but excluded finding) 70: for (uint i = 0; i < tokenIds_.length; i++) { /// @audit (valid but excluded finding) 104: for (uint i = 0; i < tokens.length; i++) { /// @audit (valid but excluded finding) 106: redeemableTokenCount++;
File: contracts/RabbitHoleReceipt.sol /// @audit (valid but excluded finding) 117: for (uint i = 0; i < msgSenderBalance; i++) { /// @audit (valid but excluded finding) 121: foundTokens++; /// @audit (valid but excluded finding) 128: for (uint i = 0; i < msgSenderBalance; i++) { /// @audit (valid but excluded finding) 131: filterTokensIndexTracker++;
private rather than public for constants, saves gasIf needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table
There is 1 instance of this issue:
File: contracts/QuestFactory.sol /// @audit (valid but excluded finding) 17: bytes32 public constant CREATE_QUEST_ROLE = keccak256('CREATE_QUEST_ROLE');
payableIf a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are
CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost
There are 25 instances of this issue:
File: contracts/Erc1155Quest.sol /// @audit (valid but excluded finding) 54: function withdrawRemainingTokens(address to_) public override onlyOwner {
File: contracts/Erc20Quest.sol /// @audit (valid but excluded finding) 81: function withdrawRemainingTokens(address to_) public override onlyOwner { /// @audit (valid but excluded finding) 102: function withdrawFee() public onlyAdminWithdrawAfterEnd {
File: contracts/QuestFactory.sol /// @audit (valid but excluded finding) 142: function changeCreateQuestRole(address account_, bool canCreateQuest_) public onlyOwner { /// @audit (valid but excluded finding) 159: function setClaimSignerAddress(address claimSignerAddress_) public onlyOwner { /// @audit (valid but excluded finding) 165: function setProtocolFeeRecipient(address protocolFeeRecipient_) public onlyOwner { /// @audit (valid but excluded finding) 172: function setRabbitHoleReceiptContract(address rabbitholeReceiptContract_) public onlyOwner { /// @audit (valid but excluded finding) 179: function setRewardAllowlistAddress(address rewardAddress_, bool allowed_) public onlyOwner { /// @audit (valid but excluded finding) 186: function setQuestFee(uint256 questFee_) public onlyOwner {
File: contracts/Quest.sol /// @audit (valid but excluded finding) 50: function start() public virtual onlyOwner { /// @audit (valid but excluded finding) 57: function pause() public onlyOwner onlyStarted { /// @audit (valid but excluded finding) 63: function unPause() public onlyOwner onlyStarted { /// @audit (valid but excluded finding) 96: function claim() public virtual onlyQuestActive { /// @audit (valid but excluded finding) 150: function withdrawRemainingTokens(address to_) public virtual onlyOwner onlyAdminWithdrawAfterEnd {}
File: contracts/RabbitHoleReceipt.sol /// @audit (valid but excluded finding) 65: function setReceiptRenderer(address receiptRenderer_) public onlyOwner { /// @audit (valid but excluded finding) 71: function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner { /// @audit (valid but excluded finding) 77: function setQuestFactory(address questFactory_) public onlyOwner { /// @audit (valid but excluded finding) 83: function setMinterAddress(address minterAddress_) public onlyOwner { /// @audit (valid but excluded finding) 90: function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner { /// @audit (valid but excluded finding) 98: function mint(address to_, string memory questId_) public onlyMinter {
File: contracts/RabbitHoleTickets.sol /// @audit (valid but excluded finding) 54: function setTicketRenderer(address ticketRenderer_) public onlyOwner { /// @audit (valid but excluded finding) 60: function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner { /// @audit (valid but excluded finding) 66: function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner { /// @audit (valid but excluded finding) 73: function setMinterAddress(address minterAddress_) public onlyOwner { /// @audit (valid but excluded finding) 83: function mint(address to_, uint256 id_, uint256 amount_, bytes memory data_) public onlyMinter {
#0 - c4-judge
2023-02-16T07:04:00Z
kirk-baird marked the issue as grade-a
#1 - c4-judge
2023-02-21T06:59:41Z
kirk-baird marked the issue as selected for report