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: 21/173
Findings: 2
Award: $230.95
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 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 | Contexts | |
|---|---|---|
| LOW‑1 | Add to blacklist function | 17 |
| LOW‑2 | Use _safeMint instead of _mint | 1 |
| LOW‑3 | Missing parameter validation in constructor | 7 |
| LOW‑4 | Contracts are not using their OZ Upgradeable counterparts | 9 |
| LOW‑5 | Protect your NFT from copying in POW forks | 1 |
| LOW‑6 | Admin privilege - A single point of failure can allow a hacked or malicious owner use critical functions in the project | 21 |
| LOW‑7 | TransferOwnership Should Be Two Step | 4 |
| LOW‑8 | No Storage Gap For Upgradeable Contracts | 3 |
| LOW‑9 | Upgrade OpenZeppelin Contract Dependency | 2 |
| LOW‑10 | Use safeTransferOwnership instead of transferOwnership function | 4 |
Total: 69 contexts over 10 issues
| Issue | Contexts | |
|---|---|---|
| NC‑1 | Add a timelock to critical functions | 3 |
| NC‑2 | Avoid Floating Pragmas: The Version Should Be Locked | 10 |
| NC‑3 | No need for == true or == false checks | 2 |
| NC‑4 | Constants Should Be Defined Rather Than Using Magic Numbers | 1 |
| NC‑5 | Expressions for constant values such as a call to keccak256(), should use immutable rather than constant | 1 |
| NC‑6 | Critical Changes Should Use Two-step Procedure | 12 |
| NC‑7 | Function writing that does not comply with the Solidity Style Guide | All in-scope contracts |
| NC‑8 | Imports can be grouped together | 12 |
| NC‑9 | NatSpec return parameters should be included in contracts | All in-scope contracts |
| NC‑10 | No need to initialize uints to zero | 3 |
| NC‑11 | Lines are too long | 7 |
| NC‑12 | Missing event for critical parameter change | 11 |
| NC‑13 | Implementation contract may not be initialized | 5 |
| NC‑14 | NatSpec comments should be increased in contracts | All in-scope contracts |
| NC‑15 | Non-usage of specific imports | 4 |
| NC‑16 | Use a more recent version of Solidity | 10 |
| NC‑17 | Open TODOs | 1 |
| NC‑18 | Public Functions Not Called By The Contract Should Be Declared External Instead | 13 |
| NC‑19 | Use bytes.concat() | 4 |
| NC‑20 | Use of Block.Timestamp | 4 |
| NC‑21 | Interchangeable usage of uint and uint256 | 10 |
Total: 143 contexts over 21 issues
blacklist functionNFT thefts have increased recently, so with the addition of hacked NFTs to the platform, NFTs can be converted into rewards via RabbitHoleReceipt and RabbitHoleTickets. To prevent this, I recommend adding the blacklist function.
Marketplaces such as Opensea have a blacklist feature that will not list NFTs that have been reported theft, NFT projects such as Manifold have blacklist functions in their smart contracts.
Here is the project example; Manifold
Manifold Contract https://etherscan.io/address/0xe4e4003afe3765aca8149a82fc064c0b125b9e5a#code
modifier nonBlacklistRequired(address extension) { require(!_blacklistedExtensions.contains(extension), "Extension blacklisted"); _; }
Add to Blacklist function and modifier.
constructorSome parameters of constructors are not checked for invalid values.
14: address rewardTokenAddress_ 20: address receiptContractAddress_
18: address rewardTokenAddress_ 24: address receiptContractAddress_ 26: address protocolFeeRecipient_
27: address rewardTokenAddress_ 33: address receiptContractAddress_
Validate the parameters.
The non-upgradeable standard version of OpenZeppelin’s library are inherited / used by the contracts. It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour.
4: import {IERC1155} from '@openzeppelin/contracts/token/ERC1155/ERC1155.sol'; 5: import {ERC1155Holder} from '@openzeppelin/contracts/token/ERC1155/utils/ERC1155Holder.sol';
4: import {IERC20, SafeERC20} from '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol';
4: import {Ownable} from '@openzeppelin/contracts/access/Ownable.sol'; 7: import {ECDSA} from '@openzeppelin/contracts/utils/cryptography/ECDSA.sol';
4: import '@openzeppelin/contracts/utils/Base64.sol'; 5: import '@openzeppelin/contracts/utils/Strings.sol';
4: import '@openzeppelin/contracts/utils/Base64.sol'; 5: import '@openzeppelin/contracts/utils/Strings.sol';
Where applicable, use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts.
See https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/tree/master/contracts for list of available upgradeable contracts
Ethereum has performed the long-awaited "merge" that will dramatically reduce the environmental impact of the network
There may be forked versions of Ethereum, which could cause confusion and lead to scams as duplicated NFT assets enter the market.
If the Ethereum Merge, which took place in September 2022, results in the Blockchain splitting into two Blockchains due to the 'THE DAO' attack in 2016, this could result in duplication of immutable tokens (NFTs).
In any case, duplicate NFTs will exist due to the ETH proof-of-work chain and other potential forks, and there’s likely to be some level of confusion around which assets are 'official' or 'authentic.'
Even so, there could be a frenzy for these copies, as NFT owners attempt to flip the proof-of-work versions of their valuable tokens.
As ETHPOW and any other forks spin off of the Ethereum mainnet, they will yield duplicate versions of Ethereum’s NFTs. An NFT is simply a blockchain token, and it can work as a deed of ownership to digital items like artwork and collectibles. A forked Ethereum chain will thus have duplicated deeds that point to the same tokenURI
About Merge Replay Attack: https://twitter.com/elerium115/status/1558471934924431363?s=20&t=RRheaYJwo-GmSnePwofgag
158: function tokenURI( uint tokenId_ ) public view virtual override(ERC721Upgradeable, ERC721URIStorageUpgradeable) returns (string memory) {
Add the following check:
if(block.chainid != 1) { revert(); }
The owner role has a single point of failure and onlyOwner can use critical a few functions.
owner role in the project:
Owner is not behind a multisig and changes are not behind a timelock.
Even if protocol admins/developers are not malicious there is still a chance for Owner keys to be stolen. In such a case, the attacker can cause serious damage to the project due to important functions. In such a case, users who have invested in project will suffer high financial losses.
Hacked owner or malicious owner can immediately use critical functions in the project.
54: function withdrawRemainingTokens(address to_) public override onlyOwner {
81: function withdrawRemainingTokens(address to_) public override onlyOwner {
50: function start() public virtual onlyOwner {
150: function withdrawRemainingTokens(address to_) public virtual onlyOwner onlyAdminWithdrawAfterEnd {
142: function changeCreateQuestRole(address account_, bool canCreateQuest_) public onlyOwner {
159: function setClaimSignerAddress(address claimSignerAddress_) public onlyOwner {
165: function setProtocolFeeRecipient(address protocolFeeRecipient_) public onlyOwner {
172: function setRabbitHoleReceiptContract(address rabbitholeReceiptContract_) public onlyOwner {
179: function setRewardAllowlistAddress(address rewardAddress_, bool allowed_) public onlyOwner {
186: function setQuestFee(uint256 questFee_) public onlyOwner {
65: function setReceiptRenderer(address receiptRenderer_) public onlyOwner {
71: function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner {
77: function setQuestFactory(address questFactory_) public onlyOwner {
83: function setMinterAddress(address minterAddress_) public onlyOwner {
90: function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner {
98: function mint(address to_, string memory questId_) public onlyMinter {
54: function setTicketRenderer(address ticketRenderer_) public onlyOwner {
60: function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner {
66: function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner {
73: function setMinterAddress(address minterAddress_) public onlyOwner {
83: function mint(address to_, uint256 id_, uint256 amount_, bytes memory data_) public onlyMinter {
Add a time lock to critical functions. Admin-only functions that change critical parameters should emit events and have timelocks. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that 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.
Allow only multi-signature wallets to call the function to reduce the likelihood of an attack.
https://twitter.com/danielvf/status/1572963475101556738?s=20&t=V1kvzfJlsx-D2hfnG0OmuQ
The status quo regarding significant centralization vectors has always been to award M severity but I have lowered it to QA as it is a common finding this is also in order to at least warn users of the protocol of this category of risks. See <a href="https://gist.github.com/GalloDaSballo/881e7a45ac14481519fb88f34fdb8837">here</a> for list of centralization issues previously judged.
Recommend considering implementing a two step process where the owner or admin nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.
100: function createQuest: newQuest.transferOwnership(msg.sender);
131: function createQuest: newQuest.transferOwnership(msg.sender);
Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.
For upgradeable contracts, there must be storage gap to "allow developers to freely add new state variables in the future without compromising the storage compatibility with existing deployments". Otherwise it may be very difficult to write new implementation code. Without storage gap, the variable in child contract might be overwritten by the upgraded base contract if new variables are added to the base contract. This could have unintended and very serious consequences to the child contracts.
Refer to the bottom part of this article: https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable
However, the contract doesn't contain a storage gap. The storage gap is essential for upgradeable contract because "It allows us to freely add new state variables in the future without compromising the storage compatibility with existing deployments". See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps 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.
contract QuestFactory is Initializable, OwnableUpgradeable, AccessControlUpgradeable, IQuestFactory
contract RabbitHoleReceipt is Initializable, ERC721Upgradeable, ERC721EnumerableUpgradeable, ERC721URIStorageUpgradeable, OwnableUpgradeable, IERC2981Upgradeable
contract RabbitHoleTickets is Initializable, ERC1155Upgradeable, OwnableUpgradeable, ERC1155BurnableUpgradeable, IERC2981Upgradeable
Recommend adding appropriate storage gap at the end of upgradeable contracts such as the below. Please reference OpenZeppelin upgradeable contract templates.
uint256[50] private __gap;
An outdated OZ version is used (which has known vulnerabilities, see: https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories).
Require dependencies to be at least version of 4.8.1 to include critical vulnerability patches.
"@openzeppelin/contracts": "4.8.0"
"@openzeppelin/contracts-upgradeable": "4.8.0"
Update OpenZeppelin Contracts Usage in package.json and require at least version of 4.8.1 via >=4.8.1
safeTransferOwnership instead of transferOwnership functionUse safeTransferOwnership which is safer. Use it as it is more secure due to 2-stage ownership transfer.
100: function createQuest: newQuest.transferOwnership(msg.sender);
131: function createQuest: newQuest.transferOwnership(msg.sender);
Use <a href="https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable2Step.sol">Ownable2Step.sol</a>
function acceptOwnership() external { address sender = _msgSender(); require(pendingOwner() == sender, "Ownable2Step: caller is not the new owner"); _transferOwnership(sender); } }
It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user). Consider adding a timelock to the following functions:
186: function setQuestFee(uint256 questFee_) public onlyOwner {
90: function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner {
66: function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner {
Avoid floating pragmas for non-library contracts.
While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.
A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.
It is recommended to pin to a concrete compiler version.
Found usage of floating pragmas ^0.8.15 of Solidity in [Erc1155Quest.sol]
Found usage of floating pragmas ^0.8.15 of Solidity in [Erc20Quest.sol]
Found usage of floating pragmas ^0.8.15 of Solidity in [Quest.sol]
Found usage of floating pragmas ^0.8.15 of Solidity in [QuestFactory.sol]
Found usage of floating pragmas ^0.8.15 of Solidity in [RabbitHoleReceipt.sol]
Found usage of floating pragmas ^0.8.15 of Solidity in [RabbitHoleTickets.sol]
Found usage of floating pragmas ^0.8.15 of Solidity in [ReceiptRenderer.sol]
Found usage of floating pragmas ^0.8.15 of Solidity in [TicketRenderer.sol]
Found usage of floating pragmas ^0.8.15 of Solidity in [IQuest.sol]
Found usage of floating pragmas ^0.8.15 of Solidity in [IQuestFactory.sol]
== true or == false checksThere is no need to verify that == true or == false when the variable checked upon is a boolean as well.
73: if (rewardAllowlist[rewardTokenAddress_] == false) revert RewardNotAllowed();
221: if (quests[questId_].addressMinted[msg.sender] == true) revert AddressAlreadyMinted();
Instead simply check for variable or !variable
It will revert when questFee_ is above 10_000 but there's no indication anywhere that it can't be above 10_000. Should be defined so that when setting QuestFee will know that 10_000 is the limit.
187: if (questFee_ > 10_000) revert QuestFeeTooHigh();
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.
17: bytes32 public constant CREATE_QUEST_ROLE = keccak256('CREATE_QUEST_ROLE');
The critical procedures should be two step process.
See similar findings in previous Code4rena contests for reference: https://code4rena.com/reports/2022-06-illuminate/#2-critical-changes-should-use-two-step-procedure
142: function changeCreateQuestRole(address account_, bool canCreateQuest_) public onlyOwner {
159: function setClaimSignerAddress(address claimSignerAddress_) public onlyOwner {
165: function setProtocolFeeRecipient(address protocolFeeRecipient_) public onlyOwner {
172: function setRabbitHoleReceiptContract(address rabbitholeReceiptContract_) public onlyOwner {
179: function setRewardAllowlistAddress(address rewardAddress_, bool allowed_) public onlyOwner {
65: function setReceiptRenderer(address receiptRenderer_) public onlyOwner {
71: function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner {
77: function setQuestFactory(address questFactory_) public onlyOwner {
83: function setMinterAddress(address minterAddress_) public onlyOwner {
54: function setTicketRenderer(address ticketRenderer_) public onlyOwner {
60: function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner {
73: function setMinterAddress(address minterAddress_) public onlyOwner {
Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.
Order of Functions; ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.
https://docs.soliditylang.org/en/v0.8.17/style-guide.html
Functions should be grouped according to their visibility and ordered:
All in-scope contracts
Consider importing OZ first, then all interfaces, then all utils.
4: import {Ownable} from '@openzeppelin/contracts/access/Ownable.sol'; 5: import {IQuest} from './interfaces/IQuest.sol'; 6: import {RabbitHoleReceipt} from './RabbitHoleReceipt.sol'; 7: import {ECDSA} from '@openzeppelin/contracts/utils/cryptography/ECDSA.sol';
4: import '@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol'; 5: import {Erc20Quest} from './Erc20Quest.sol'; 6: import {IQuestFactory} from './interfaces/IQuestFactory.sol'; 7: import {Erc1155Quest} from './Erc1155Quest.sol'; 8: import {RabbitHoleReceipt} from './RabbitHoleReceipt.sol'; 9: import {OwnableUpgradeable} from '@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol'; 10: import '@openzeppelin/contracts-upgradeable/utils/cryptography/ECDSAUpgradeable.sol'; 11: import '@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol';
It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.
https://docs.soliditylang.org/en/v0.8.15/natspec-format.html
All in-scope contracts
Include return parameters in NatSpec comments
Recommendation Code Style: (from Uniswap3)
/// @notice Adds liquidity for the given recipient/tickLower/tickUpper position /// @dev The caller of this method receives a callback in the form of IUniswapV3MintCallback#uniswapV3MintCallback /// in which they must pay any token0 or token1 owed for the liquidity. The amount of token0/token1 due depends /// on tickLower, tickUpper, the amount of liquidity, and the current price. /// @param recipient The address for which the liquidity will be created /// @param tickLower The lower tick of the position in which to add liquidity /// @param tickUpper The upper tick of the position in which to add liquidity /// @param amount The amount of liquidity to mint /// @param data Any data that should be passed through to the callback /// @return amount0 The amount of token0 that was paid to mint the given amount of liquidity. Matches the value in the callback /// @return amount1 The amount of token1 that was paid to mint the given amount of liquidity. Matches the value in the callback function mint( address recipient, int24 tickLower, int24 tickUpper, uint128 amount, bytes calldata data ) external returns (uint256 amount0, uint256 amount1);
There is no need to initialize uint variables to zero as their default value is 0
103: uint256 redeemableTokenCount = 0;
115: uint foundTokens = 0; 126: uint filterTokensIndexTracker = 0;
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 Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length
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
79: /// @dev Every receipt minted should still be able to claim rewards (and cannot be withdrawn). This function can only be called after the quest end time
56: /// @dev Only the owner of the Quest can call this function. Also requires that the Quest has started (not by date, but by calling the start function)
62: /// @dev Only the owner of the Quest can call this function. Also requires that the Quest has started (not by date, but by calling the start function)
When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.
142: function changeCreateQuestRole(address account_, bool canCreateQuest_) public onlyOwner {
159: function setClaimSignerAddress(address claimSignerAddress_) public onlyOwner {
165: function setProtocolFeeRecipient(address protocolFeeRecipient_) public onlyOwner {
172: function setRabbitHoleReceiptContract(address rabbitholeReceiptContract_) public onlyOwner {
179: function setRewardAllowlistAddress(address rewardAddress_, bool allowed_) public onlyOwner {
186: function setQuestFee(uint256 questFee_) public onlyOwner {
65: function setReceiptRenderer(address receiptRenderer_) public onlyOwner {
71: function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner {
77: function setQuestFactory(address questFactory_) public onlyOwner {
54: function setTicketRenderer(address ticketRenderer_) public onlyOwner {
60: function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner {
OpenZeppelin recommends that the initializer modifier be applied to constructors. Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits. https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/5
13: constructor( address rewardTokenAddress_, uint256 endTime_, uint256 startTime_, uint256 totalParticipants_, uint256 rewardAmountInWeiOrTokenId_, string memory questId_, address receiptContractAddress_ ) Quest( rewardTokenAddress_, endTime_, startTime_, totalParticipants_, rewardAmountInWeiOrTokenId_, questId_, receiptContractAddress_ )
17: constructor( address rewardTokenAddress_, uint256 endTime_, uint256 startTime_, uint256 totalParticipants_, uint256 rewardAmountInWeiOrTokenId_, string memory questId_, address receiptContractAddress_, uint256 questFee_, address protocolFeeRecipient_ ) Quest( rewardTokenAddress_, endTime_, startTime_, totalParticipants_, rewardAmountInWeiOrTokenId_, questId_, receiptContractAddress_ )
26: constructor( address rewardTokenAddress_, uint256 endTime_, uint256 startTime_, uint256 totalParticipants_, uint256 rewardAmountInWeiOrTokenId_, string memory questId_, address receiptContractAddress_ )
39: constructor()
28: constructor()
It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability. https://docs.soliditylang.org/en/v0.8.15/natspec-format.html
All in-scope contracts
NatSpec comments should be increased in contracts
The current form of relative path import is not recommended for use because it can unpredictably pollute the namespace. Instead, the Solidity docs recommend specifying imported symbols explicitly. https://docs.soliditylang.org/en/v0.8.15/layout-of-source-files.html#importing-other-source-files
A good example:
import {OwnableUpgradeable} from "openzeppelin-contracts-upgradeable/contracts/access/OwnableUpgradeable.sol"; import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol"; import {SafeCastLib} from "solmate/utils/SafeCastLib.sol"; import {ERC20} from "solmate/tokens/ERC20.sol"; import {IProducer} from "src/interfaces/IProducer.sol"; import {GlobalState, UserState} from "src/Common.sol";
11: import './ReceiptRenderer.sol'; 12: import './interfaces/IQuestFactory.sol'; 13: import './interfaces/IQuest.sol';
9: import './TicketRenderer.sol';
Use specific imports syntax per solidity docs recommendation.
<a href="https://blog.soliditylang.org/2021/04/21/solidity-0.8.4-release-announcement/">0.8.4</a>: bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>)
<a href="https://blog.soliditylang.org/2022/02/16/solidity-0.8.12-release-announcement/">0.8.12</a>: string.concat() instead of abi.encodePacked(<str>,<str>)
<a href="https://blog.soliditylang.org/2022/03/16/solidity-0.8.13-release-announcement/">0.8.13</a>: Ability to use using for with a list of free functions
<a href="https://blog.soliditylang.org/2022/05/18/solidity-0.8.14-release-announcement/">0.8.14</a>:
ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases. Override Checker: Allow changing data location for parameters only when overriding external functions.
<a href="https://blog.soliditylang.org/2022/06/15/solidity-0.8.15-release-announcement/">0.8.15</a>:
Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays. Yul Optimizer: Keep all memory side-effects of inline assembly blocks.
<a href="https://blog.soliditylang.org/2022/08/08/solidity-0.8.16-release-announcement/">0.8.16</a>:
Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.
<a href="https://blog.soliditylang.org/2022/09/08/solidity-0.8.17-release-announcement/">0.8.17</a>:
Yul Optimizer: Prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call.
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
Consider updating to a more recent solidity version.
An open TODO is present. It is recommended to avoid open TODOs as they may indicate programming errors that still need to be fixed.
4: // TODO clean this whole thing up
Contracts are allowed to override their parents’ functions and change the visibility from external to public.
function withdrawFee() public onlyAdminWithdrawAfterEnd {
function pause() public onlyOwner onlyStarted {
function unPause() public onlyOwner onlyStarted {
function claim() public virtual onlyQuestActive {
function createQuest( address rewardTokenAddress_, uint256 endTime_, uint256 startTime_, uint256 totalParticipants_, uint256 rewardAmountOrTokenId_, string memory contractType_, string memory questId_ ) public onlyRole(CREATE_QUEST_ROLE) returns (address) {
function changeCreateQuestRole(address account_, bool canCreateQuest_) public onlyOwner {
function setClaimSignerAddress(address claimSignerAddress_) public onlyOwner {
function setRabbitHoleReceiptContract(address rabbitholeReceiptContract_) public onlyOwner {
function setRewardAllowlistAddress(address rewardAddress_, bool allowed_) public onlyOwner {
function mintReceipt(string memory questId_, bytes32 hash_, bytes memory signature_) public {
function setReceiptRenderer(address receiptRenderer_) public onlyOwner {
function setQuestFactory(address questFactory_) public onlyOwner {
function setTicketRenderer(address ticketRenderer_) public onlyOwner {
bytes.concat()Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>))
72: if (keccak256(abi.encodePacked(contractType_)) == keccak256(abi.encodePacked('erc20') 105: if (keccak256(abi.encodePacked(contractType_)) == keccak256(abi.encodePacked('erc1155') 211: bytes32 messageDigest = keccak256(abi.encodePacked('/x19Ethereum Signed Message:/n32', hash_) 222: if (keccak256(abi.encodePacked(msg.sender, questId_)) != hash_) revert InvalidHash();
Use bytes.concat() and upgrade to at least Solidity version 0.8.4 if required.
Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts. References: SWC ID: 116
35: if (endTime_ <= block.timestamp) revert EndTimeInPast();
36: if (startTime_ <= block.timestamp) revert StartTimeInPast();
77: if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
90: if (block.timestamp < startTime) revert ClaimWindowNotStarted();
Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.
Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.
uint and uint256Some developers prefer to use uint256 because it is consistent with other uint data types, which also specify their size, and also because making the size of the data explicit reminds the developer and the reader how much data they've got to play with, which may help prevent or detect bugs.
For example, if doing bytes4(keccak('transfer(address, uint)’)), you'll get a different method sig ID than bytes4(keccak('transfer(address, uint256)’)) and smart contracts will only understand the latter when comparing method sig IDs
initialize( address receiptRenderer_, address royaltyRecipient_, address minterAddress_, uint royaltyFee_ ) public initializer {
tokenURI( uint tokenId_ ) public view virtual override(ERC721Upgradeable, ERC721URIStorageUpgradeable) returns (string memory) {
initialize( address ticketRenderer_, address royaltyRecipient_, address minterAddress_, uint royaltyFee_ ) public initializer {
generateTokenURI( uint tokenId_, string memory questId_, uint totalParticipants_, bool claimed_, uint rewardAmount_, address rewardAddress_ ) public view virtual returns (string memory) {
generateDataURI( uint tokenId_, string memory questId_, uint totalParticipants_, bool claimed_, uint rewardAmount_, address rewardAddress_ ) public view virtual returns (bytes memory) {
generateTokenURI( uint tokenId_ ) public pure returns (string memory) {
createQuest( address rewardTokenAddress_, uint256 endTime_, uint256 startTime_, uint256 totalParticipants_, uint256 rewardAmountOrTokenId_, string memory contractType_, string memory questId_ ) public onlyRole(CREATE_QUEST_ROLE) returns (address) {
_beforeTokenTransfer( address from_, address to_, uint256 tokenId_, uint256 batchSize_ ) internal override(ERC721Upgradeable, ERC721EnumerableUpgradeable) {
royaltyInfo( uint256 tokenId_, uint256 salePrice_ ) external view override returns (address receiver, uint256 royaltyAmount) {
mint(address to_, uint256 id_, uint256 amount_, bytes memory data_) public onlyMinter {
#0 - c4-sponsor
2023-02-08T05:42:46Z
jonathandiep marked the issue as sponsor confirmed
#1 - c4-judge
2023-02-16T07:23:15Z
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
111.3544 USDC - $111.35
| Issue | Contexts | Estimated Gas Saved | |
|---|---|---|---|
| GAS‑1 | Comparing to constant boolean | 2 | - |
| GAS‑2 | State variables only set in the constructor should be declared immutable | 1 | 2100 |
| GAS‑3 | Setting the constructor to payable | 6 | 78 |
| GAS‑4 | Empty Blocks Should Be Removed Or Emit Something | 1 | - |
| GAS‑5 | Using delete statement can save gas | 3 | - |
| GAS‑6 | Use hardcoded address instead address(this) | 6 | - |
| GAS‑7 | internal functions only called once can be inlined to save gas | 9 | 198 |
| GAS‑8 | Multiple Address Mappings Can Be Combined Into A Single Mapping Of An Address To A Struct, Where Appropriate | 2 | - |
| GAS‑9 | Optimize names to save gas | 6 | 132 |
| GAS‑10 | <x> += <y> Costs More Gas Than <x> = <x> + <y> For State Variables | 1 | - |
| GAS‑11 | Public Functions To External | 52 | - |
| GAS‑12 | Help The Optimizer By Saving A Storage Variable’s Reference Instead Of Repeatedly Fetching It | 1 | - |
| GAS‑13 | Superfluous event fields | 1 | - |
| GAS‑14 | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead | 3 | - |
| GAS‑15 | Use assembly to check for address(0) | 1 | - |
| GAS‑16 | Use of Custom Errors Instead of String | 3 | - |
| GAS‑17 | Use solidity version 0.8.17 to gain some gas boost | 10 | 880 |
| GAS‑18 | Using storage instead of memory saves gas | 1 | 350 |
Total: 109 contexts over 18 issues
There is no need to verify that == true or == false when the variable checked upon is a boolean as well.
73: if (rewardAllowlist[rewardTokenAddress_] == false) revert RewardNotAllowed();
221: if (quests[questId_].addressMinted[msg.sender] == true) revert AddressAlreadyMinted();
Instead simply check for variable or !variable
constructor should be declared immutableAvoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).
43: questId = questId_
constructor to payableSaves ~13 gas per instance
13: constructor( address rewardTokenAddress_, uint256 endTime_, uint256 startTime_, uint256 totalParticipants_, uint256 rewardAmountInWeiOrTokenId_, string memory questId_, address receiptContractAddress_ ) Quest( rewardTokenAddress_, endTime_, startTime_, totalParticipants_, rewardAmountInWeiOrTokenId_, questId_, receiptContractAddress_ )
17: constructor( address rewardTokenAddress_, uint256 endTime_, uint256 startTime_, uint256 totalParticipants_, uint256 rewardAmountInWeiOrTokenId_, string memory questId_, address receiptContractAddress_, uint256 questFee_, address protocolFeeRecipient_ ) Quest( rewardTokenAddress_, endTime_, startTime_, totalParticipants_, rewardAmountInWeiOrTokenId_, questId_, receiptContractAddress_ )
26: constructor( address rewardTokenAddress_, uint256 endTime_, uint256 startTime_, uint256 totalParticipants_, uint256 rewardAmountInWeiOrTokenId_, string memory questId_, address receiptContractAddress_ )
35: constructor() initializer
39: constructor()
28: constructor()
The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}})
150: function withdrawRemainingTokens(address to_) public virtual onlyOwner onlyAdminWithdrawAfterEnd {}
delete statement can save gas103: uint256 redeemableTokenCount = 0;
115: uint foundTokens = 0; 126: uint filterTokensIndexTracker = 0;
address(this)Instead of using address(this), it is more gas-efficient to pre-calculate and use the hardcoded address. Foundry's script.sol and solmate's LibRlp.sol contracts can help achieve this.
References: https://book.getfoundry.sh/reference/forge-std/compute-create-address
https://twitter.com/transmissions11/status/1518507047943245824
34: if (IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId) < totalParticipants)
42: IERC1155(rewardToken).safeTransferFrom(address(this), msg.sender, rewardAmountInWeiOrTokenId, amount_, '0x00');
57: address(this), 60: IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId),
59: if (IERC20(rewardToken).balanceOf(address(this)) < maxTotalRewards() + maxProtocolReward())
85: uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
Use hardcoded address
internal functions only called once can be inlined to save gas41: function _transferRewards
48: function _calculateRewards
66: function _transferRewards
74: function _calculateRewards
122: function _calculateRewards
129: function _transferRewards
152: function grantDefaultAdminAndCreateQuestRole
139: function _burn
148: function _beforeTokenTransfer
Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot.
20: mapping(address => bool) addressMinted;
30: mapping(address => bool) public rewardAllowlist;
Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions.
See more <a href="https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92">here</a>
All in-scope contracts
Find a lower method ID name for the most called functions for example Call() vs. Call1() is cheaper by 22 gas For example, the function IDs in the Gauge.sol contract will be the most used; A lower method ID may be given.
<x> += <y> Costs More Gas Than <x> = <x> + <y> For State Variables115: redeemedTokens += redeemableTokenCount;
The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.
function start() public override {
function withdrawRemainingTokens(address to_) public override onlyOwner {
function maxTotalRewards() public view returns (uint256) {
function maxProtocolReward() public view returns (uint256) {
function start() public override {
function withdrawRemainingTokens(address to_) public override onlyOwner {
function receiptRedeemers() public view returns (uint256) {
function protocolFee() public view returns (uint256) {
function withdrawFee() public onlyAdminWithdrawAfterEnd {
function start() public virtual onlyOwner {
function pause() public onlyOwner onlyStarted {
function unPause() public onlyOwner onlyStarted {
function claim() public virtual onlyQuestActive {
function isClaimed(uint256 tokenId_) public view returns (bool) {
function getRewardAmount() public view returns (uint256) {
function getRewardToken() public view returns (address) {
function withdrawRemainingTokens(address to_) public virtual onlyOwner onlyAdminWithdrawAfterEnd {
function initialize( address claimSignerAddress_, address rabbitholeReceiptContract_, address protocolFeeRecipient_ ) public initializer {
function createQuest( address rewardTokenAddress_, uint256 endTime_, uint256 startTime_, uint256 totalParticipants_, uint256 rewardAmountOrTokenId_, string memory contractType_, string memory questId_ ) public onlyRole(CREATE_QUEST_ROLE) returns (address) {
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 recoverSigner(bytes32 hash_, bytes memory signature_) public pure returns (address) {
function mintReceipt(string memory questId_, bytes32 hash_, bytes memory signature_) public {
function initialize( address receiptRenderer_, address royaltyRecipient_, address minterAddress_, uint royaltyFee_ ) public initializer {
function setReceiptRenderer(address receiptRenderer_) public onlyOwner {
function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner {
function setQuestFactory(address questFactory_) public onlyOwner {
function setMinterAddress(address minterAddress_) public onlyOwner {
function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner {
function mint(address to_, string memory questId_) public onlyMinter {
function getOwnedTokenIdsOfQuest( string memory questId_, address claimingAddress_ ) public view returns (uint[] memory) {
function tokenURI( uint tokenId_ ) public view virtual override(ERC721Upgradeable, ERC721URIStorageUpgradeable) returns (string memory) {
function supportsInterface( bytes4 interfaceId_ ) public view virtual override(ERC721Upgradeable, ERC721EnumerableUpgradeable, IERC165Upgradeable) returns (bool) {
function initialize( address ticketRenderer_, address royaltyRecipient_, address minterAddress_, uint royaltyFee_ ) public initializer {
function setTicketRenderer(address ticketRenderer_) public onlyOwner {
function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner {
function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner {
function setMinterAddress(address minterAddress_) public onlyOwner {
function mint(address to_, uint256 id_, uint256 amount_, bytes memory data_) public onlyMinter {
function mintBatch( address to_, uint256[] memory ids_, uint256[] memory amounts_, bytes memory data_ ) public onlyMinter {
function uri(uint tokenId_) public view virtual override(ERC1155Upgradeable) returns (string memory) {
function supportsInterface( bytes4 interfaceId_ ) public view virtual override(ERC1155Upgradeable, IERC165Upgradeable) returns (bool) {
function generateTokenURI( uint tokenId_, string memory questId_, uint totalParticipants_, bool claimed_, uint rewardAmount_, address rewardAddress_ ) public view virtual returns (string memory) {
function generateDataURI( uint tokenId_, string memory questId_, uint totalParticipants_, bool claimed_, uint rewardAmount_, address rewardAddress_ ) public view virtual returns (bytes memory) {
function generateAttribute(string memory key, string memory value) public pure returns (string memory) {
function generateSVG(uint tokenId_, string memory questId_) public pure returns (string memory) {
function generateTokenURI( uint tokenId_ ) public pure returns (string memory) {
function generateSVG(uint tokenId_) public pure returns (string memory) {
To help the optimizer, declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array. The effect can be quite significant. As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex] and use it.
119: if (keccak256(bytes(questIdForTokenId[tokenId])) == keccak256(bytes(questId_))) {
block.number and block.timestamp are added to the event information by default, so adding them manually will waste additional gas.
16: event QuestCreated(address indexed creator, address indexed contractAddress, string indexed questId, string contractType, address rewardTokenAddress, uint256 endTime, uint256 startTime, uint256 totalParticipants, uint256 rewardAmountOrTokenId);
uints/ints smaller than 32 bytes (256 bits) incurs overheadWhen using elements that are smaller than 32 bytes, your contract's gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html
Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed
99: uint[] memory tokens = rabbitHoleReceiptContract.getOwnedTokenIdsOfQuest(questId, msg.sender);
114: uint[] memory tokenIdsForQuest = new uint[](msgSenderBalance);
125: uint[] memory filteredTokens = new uint[](foundTokens);
address(0)Save 6 gas per instance if using assembly to check for address(0)
e.g.
assembly { if iszero(_addr) { mstore(0x00, "AddressZero") revert(0x00, 0x20) } }
function setProtocolFeeRecipient(address protocolFeeRecipient_) public onlyOwner {
To save some gas the use of custom errors leads to cheaper deploy time cost and run time cost. The run time cost is only relevant when the revert condition is met.
File: ./Projects/rabbithold2023-01/2023-01-rabbithole/quest-protocol/contracts/RabbitHoleReceipt.sol
Use Custom Errors instead of strings.
Upgrade to the latest solidity version 0.8.17 to get additional gas savings.
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
pragma solidity ^0.8.15;
storage instead of memory saves gasWhen fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct
164: string memory questId = questIdForTokenId[tokenId_];
uint256(1)/uint256(2) instead for true and false boolean statesIf you don't use boolean for storage you will avoid Gwarmaccess 100 gas. In addition, state changes of boolean from true to false can cost up to ~20000 gas rather than uint256(2) to uint256(1) that would cost significantly less.
24: mapping(uint256 => bool) private claimedList;
51: isPaused = false;
64: isPaused = false;
52: hasStarted = true;
58: isPaused = true;
71: claimedList[tokenIds_[i]] = true;
20: mapping(address => bool) addressMinted;
30: mapping(address => bool) public rewardAllowlist;
225: quests[questId_].addressMinted[msg.sender] = true;
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.
19: bool public hasStarted;
20: bool public isPaused;
24: mapping(uint256 => bool) private claimedList;
20: mapping(address => bool) addressMinted;
30: mapping(address => bool) public rewardAllowlist;
#0 - c4-judge
2023-02-16T07:22:55Z
kirk-baird marked the issue as grade-a