Platform: Code4rena
Start Date: 11/08/2022
Pot Size: $40,000 USDC
Total HM: 8
Participants: 108
Period: 4 days
Judge: hickuphh3
Total Solo HM: 2
Id: 152
League: ETH
Rank: 14/108
Findings: 3
Award: $171.72
š Selected for report: 0
š Solo Findings: 0
š Selected for report: rbserver
Also found by: 0xc0ffEE, CodingNameKiki, Deivitto, Diraco, IllIllI, KIntern_NA, Lambda, Noah3o6, Treasure-Seeker, ignacio, oyc_109, zeesaw
13.1705 USDC - $13.17
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L271 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L182
_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both OpenZeppelin and solmate have versions of this function
If an NFTCollection creator uses a contract to automate the minting of NFTs defined by their metadata path, and mistakenly does not provide interfaces for getting back out the NFTs, the NFTs may be lost. Separately, if an NFTDropCollection mints NFTs to arbitrary addresses, and those addresses are wallets that cannot support NFTs, those NFTs may be lost as well.
There are two cases where _mint() has been used instead of _safeMint():
File: contracts/NFTCollection.sol 271: _mint(msg.sender, tokenId);
File: contracts/NFTDropCollection.sol 182: _mint(to, i);
Code inspection
Use _safeMint() instead of _mint()
#0 - 0xlgtm
2022-08-17T03:54:21Z
#1 - HardlyDifficult
2022-08-18T12:18:15Z
š Selected for report: Saw-mon_and_Natalie
Also found by: 0x1f8b, 0x52, 0xDjango, 0xNazgul, 0xSmartContract, 0xSolus, 0xackermann, 0xmatt, 0xsolstars, Aymen0909, Bnke0x0, Chom, Deivitto, DevABDee, Dravee, ElKu, IllIllI, JC, Kumpa, Lambda, LeoS, MiloTruck, PwnedNoMore, ReyAdmirado, Rohan16, Rolezn, Ruhum, Sm4rty, TomJ, Treasure-Seeker, Vexjon, Waze, Yiko, __141345__, apostle0x01, auditor0517, berndartmueller, bin2chen, bobirichman, brgltd, bulej93, c3phas, carlitox477, cccz, cryptphi, csanuragjain, d3e4, danb, delfin454000, durianSausage, erictee, fatherOfBlocks, gogo, iamwhitelights, joestakey, jonatascm, ladboy233, mics, oyc_109, rbserver, ret2basic, robee, rokinot, rvierdiiev, shenwilly, sikorico, simon135, thank_you, wagmi, yash90, zeesaw, zkhorse
109.8795 USDC - $109.88
| Issue | Instances | |
|---|---|---|
| [Lā01] | Upgradeable contract not initialized | 5 |
| [Lā02] | Open TODOs | 1 |
| [Lā03] | Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions | 3 |
Total: 9 instances over 3 issues
| Issue | Instances | |
|---|---|---|
| [Nā01] | Missing initializer modifier on constructor | 4 |
| [Nā02] | constants should be defined rather than using magic numbers | 5 |
| [Nā03] | Use a more recent version of solidity | 9 |
| [Nā04] | Expressions for constant values such as a call to keccak256(), should use immutable rather than constant | 2 |
| [Nā05] | Constant redefined elsewhere | 1 |
| [Nā06] | Non-library/interface files should use fixed compiler versions, not floating ones | 4 |
| [Nā07] | NatSpec is incomplete | 1 |
| [Nā08] | Event is missing indexed fields | 4 |
| [Nā09] | Not using the named return variables anywhere in the function is confusing | 11 |
| [Nā10] | Duplicated require()/revert() checks should be refactored to a modifier or function | 1 |
Total: 42 instances over 10 issues
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 are 5 instances of this issue:
File: contracts/NFTCollection.sol /// @audit missing __ERC165_init() 28 contract NFTCollection is 29 INFTCollectionInitializer, 30 IGetRoyalties, 31 IGetFees, 32 IRoyaltyInfo, 33 ITokenCreator, 34 ContractFactory, 35 Initializable, 36 ERC165Upgradeable, 37 ERC721Upgradeable, 38 ERC721BurnableUpgradeable, 39 SequentialMintCollection, 40: CollectionRoyalties
File: contracts/NFTDropCollection.sol /// @audit missing __Context_init() /// @audit missing __ERC165_init() /// @audit missing __AccessControl_init() 28 contract NFTDropCollection is 29 INFTDropCollectionInitializer, 30 INFTDropCollectionMint, 31 IGetRoyalties, 32 IGetFees, 33 IRoyaltyInfo, 34 ITokenCreator, 35 ContractFactory, 36 Initializable, 37 ContextUpgradeable, 38 ERC165Upgradeable, 39 AccessControlUpgradeable, 40 AdminRole, 41 MinterRole, 42 ERC721Upgradeable, 43 ERC721BurnableUpgradeable, 44 SequentialMintCollection, 45: CollectionRoyalties
File: contracts/NFTDropMarket.sol /// @audit missing __ReentrancyGuard_init() 63 contract NFTDropMarket is 64 Initializable, 65 FoundationTreasuryNode, 66 FETHNode, 67 MarketSharedCore, 68 NFTDropMarketCore, 69 ReentrancyGuardUpgradeable, 70 SendValueWithFallbackWithdraw, 71 MarketFees, 72 Gap10000, 73: NFTDropMarketFixedPriceSale
Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment
There is 1 instance of this issue:
File: contracts/mixins/shared/MarketFees.sol 193: // TODO add referral info
__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/NFTCollection.sol 28 contract NFTCollection is 29 INFTCollectionInitializer, 30 IGetRoyalties, 31 IGetFees, 32 IRoyaltyInfo, 33 ITokenCreator, 34 ContractFactory, 35 Initializable, 36 ERC165Upgradeable, 37 ERC721Upgradeable, 38 ERC721BurnableUpgradeable, 39 SequentialMintCollection, 40: CollectionRoyalties
File: contracts/NFTDropCollection.sol 28 contract NFTDropCollection is 29 INFTDropCollectionInitializer, 30 INFTDropCollectionMint, 31 IGetRoyalties, 32 IGetFees, 33 IRoyaltyInfo, 34 ITokenCreator, 35 ContractFactory, 36 Initializable, 37 ContextUpgradeable, 38 ERC165Upgradeable, 39 AccessControlUpgradeable, 40 AdminRole, 41 MinterRole, 42 ERC721Upgradeable, 43 ERC721BurnableUpgradeable, 44 SequentialMintCollection, 45: CollectionRoyalties
File: contracts/NFTDropMarket.sol 63 contract NFTDropMarket is 64 Initializable, 65 FoundationTreasuryNode, 66 FETHNode, 67 MarketSharedCore, 68 NFTDropMarketCore, 69 ReentrancyGuardUpgradeable, 70 SendValueWithFallbackWithdraw, 71 MarketFees, 72 Gap10000, 73: NFTDropMarketFixedPriceSale
initializer modifier on constructorOpenZeppelin recommends that the initializer modifier be applied to constructors in order to avoid potential griefs, social engineering, or exploits. Ensure that the modifier is applied to the implementation contract. If the default constructor is currently being used, it should be changed to be an explicit one with the modifier applied.
There are 4 instances of this issue:
File: contracts/NFTCollectionFactory.sol 181: constructor(address _rolesContract) {
File: contracts/NFTCollection.sol 95 constructor(address _contractFactory) 96: ContractFactory(_contractFactory) // solhint-disable-next-line no-empty-blocks
File: contracts/NFTDropCollection.sol 101 constructor(address _contractFactory) 102: ContractFactory(_contractFactory) // solhint-disable-next-line no-empty-blocks
File: contracts/NFTDropMarket.sol 82 constructor( 83 address payable _treasury, 84 address _feth, 85 address _royaltyRegistry 86 ) 87 FoundationTreasuryNode(_treasury) 88 FETHNode(_feth) 89 MarketFees( 90 _royaltyRegistry, 91 /*assumePrimarySale=*/ 92 true 93: ) // solhint-disable-next-line no-empty-blocks
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 5 instances of this issue:
File: contracts/libraries/BytesLibrary.sol /// @audit 20 25: for (uint256 i = 0; i < 20; ++i) { /// @audit 4 40: if (callData.length < 4) { /// @audit 4 44: for (uint256 i = 0; i < 4; ++i) {
File: contracts/mixins/shared/Constants.sol /// @audit 1000 26: uint256 constant MIN_PERCENT_INCREMENT_DENOMINATOR = BASIS_POINTS / 1000;
File: contracts/NFTCollectionFactory.sol /// @audit 0x1337000000000000000000000000000000000000000000000000000000001337 242: 0x1337000000000000000000000000000000000000000000000000000000001337,
Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions
There are 9 instances of this issue:
File: contracts/libraries/AddressLibrary.sol 3: pragma solidity ^0.8.12;
File: contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol 3: pragma solidity ^0.8.12;
File: contracts/mixins/shared/ContractFactory.sol 3: pragma solidity ^0.8.12;
File: contracts/mixins/shared/FETHNode.sol 3: pragma solidity ^0.8.12;
File: contracts/mixins/shared/FoundationTreasuryNode.sol 3: pragma solidity ^0.8.12;
File: contracts/mixins/shared/MarketFees.sol 3: pragma solidity ^0.8.12;
File: contracts/NFTCollectionFactory.sol 42: pragma solidity ^0.8.12;
File: contracts/NFTCollection.sol 3: pragma solidity ^0.8.12;
File: contracts/NFTDropCollection.sol 3: pragma solidity ^0.8.12;
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 are 2 instances of this issue:
File: contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol 70: bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
File: contracts/mixins/roles/MinterRole.sol 19: bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
Consider defining in only one contract so that values cannot become out of sync when only one location is updated. A cheap way to store constants in a single location is to create an internal constant in a library. If the variable is a local cache of another contract's value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don't get out of sync.
There is 1 instance of this issue:
File: contracts/mixins/roles/MinterRole.sol /// @audit seen in contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol 19: bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
There are 4 instances of this issue:
File: contracts/NFTCollectionFactory.sol 42: pragma solidity ^0.8.12;
File: contracts/NFTCollection.sol 3: pragma solidity ^0.8.12;
File: contracts/NFTDropCollection.sol 3: pragma solidity ^0.8.12;
File: contracts/NFTDropMarket.sol 42: pragma solidity ^0.8.12;
There is 1 instance of this issue:
File: contracts/mixins/shared/MarketFees.sol /// @audit Missing: '@param _assumePrimarySale' 79 /** 80 * @notice Configures the registry allowing for royalty overrides to be defined. 81 * @param _royaltyRegistry The registry to use for royalty overrides. 82 */ 83: constructor(address _royaltyRegistry, bool _assumePrimarySale) {
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 are 4 instances of this issue:
File: contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol 79 event CreateFixedPriceSale( 80 address indexed nftContract, 81 address indexed seller, 82 uint256 price, 83 uint256 limitPerAccount 84: );
File: contracts/mixins/shared/MarketFees.sol 71 event BuyReferralPaid( 72 address indexed nftContract, 73 uint256 indexed tokenId, 74 address buyReferrer, 75 uint256 buyReferrerFee, 76 uint256 buyReferrerSellerFee 77: );
File: contracts/NFTCollection.sol 70: event BaseURIUpdated(string baseURI);
File: contracts/NFTDropCollection.sol 85: event URIUpdated(string baseURI, bytes32 postRevealBaseURIHash);
Consider changing the variable to be an unnamed one
There are 11 instances of this issue:
File: contracts/libraries/AddressLibrary.sol /// @audit contractAddress 34 function callAndReturnContractAddress(CallWithoutValue memory call) 35 internal 36: returns (address payable contractAddress)
File: contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol /// @audit numberThatCanBeMinted 227 function getAvailableCountFromFixedPriceSale(address nftContract, address user) 228 external 229 view 230: returns (uint256 numberThatCanBeMinted)
File: contracts/mixins/shared/FoundationTreasuryNode.sol /// @audit treasuryAddress 59: function getFoundationTreasury() public view returns (address payable treasuryAddress) {
File: contracts/mixins/shared/MarketFees.sol /// @audit registry 208: function getRoyaltyRegistry() external view returns (address registry) {
File: contracts/mixins/shared/MarketSharedCore.sol /// @audit seller 20: function getSellerOf(address nftContract, uint256 tokenId) external view returns (address payable seller) {
File: contracts/NFTCollectionFactory.sol /// @audit collection 286 function createNFTDropCollection( 287 string calldata name, 288 string calldata symbol, 289 string calldata baseURI, 290 bytes32 postRevealBaseURIHash, 291 uint32 maxTokenId, 292 address approvedMinter, 293 uint256 nonce 294: ) external returns (address collection) { /// @audit collection 324 function createNFTDropCollectionWithPaymentAddress( 325 string calldata name, 326 string calldata symbol, 327 string calldata baseURI, 328 bytes32 postRevealBaseURIHash, 329 uint32 maxTokenId, 330 address approvedMinter, 331 uint256 nonce, 332 address payable paymentAddress 333: ) external returns (address collection) { /// @audit collection 363 function createNFTDropCollectionWithPaymentFactory( 364 string calldata name, 365 string calldata symbol, 366 string calldata baseURI, 367 bytes32 postRevealBaseURIHash, 368 uint32 maxTokenId, 369 address approvedMinter, 370 uint256 nonce, 371 CallWithoutValue memory paymentAddressFactoryCall 372: ) external returns (address collection) {
File: contracts/NFTDropCollection.sol /// @audit uri 300: function tokenURI(uint256 tokenId) public view override returns (string memory uri) {
File: contracts/NFTDropMarket.sol /// @audit seller 108 function _getSellerOf(address nftContract, uint256 tokenId) 109 internal 110 view 111 override(MarketSharedCore, NFTDropMarketFixedPriceSale) 112: returns (address payable seller) /// @audit sellerOrOwner 132 function _getSellerOrOwnerOf(address nftContract, uint256 tokenId) 133 internal 134 view 135 override 136: returns (address payable sellerOrOwner)
require()/revert() checks should be refactored to a modifier or functionThe compiler will inline the function, which will avoid JUMP instructions usually associated with functions
There is 1 instance of this issue:
File: contracts/NFTCollectionFactory.sol 227: require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
#0 - HardlyDifficult
2022-08-18T17:33:53Z
Very detailed report - thank you.
Upgradeable contract not initialized
Fair feedback but I don't believe changes are required. In the first two contracts the examples listed are no-ops, but it could be argued calling them is good practice anyways. For the last one, the suggested call is already included.
Unresolved TODO comments
Agree, will fix.
[Lā03] Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions
Invalid. The instances here are top level contracts, which do not require an explicit gap since their slots come after all inherited contracts. Also 2 of them, the collections, are proxies but not upgradable so gaps are not required at all (but some were included to encourage code reuse in our repo).
Use constructor to initialize templates
Agree this is a good best practice to add. Will fix.
Use of magic numbers is confusing
Valid NC feedback, will fix. The first two examples did have a comment explaining the number but a constant may be more clear. For MIN_PERCENT_INCREMENT_DENOMINATOR I think the existing comment is sufficient there. The last one is a special case where it really is just a magic / arbitrary value.
[Nā03] Use a more recent version of solidity
Invalid - this is a good consideration though. I confirmed our contracts compile with 0.8.12. The using for change in 0.8.13 was about global usings which do not apply to this repo.
[Nā04] Expressions for constant values such as a call to
Interesting point. I think we'll leave this as is since it does not save gas as you noted, and the OZ deployment helper throws an error with this change -- it may be nice to not suppress that just in case it provides valid feedback for a different issue later on.
[Nā05] Constant redefined elsewhere
Fair feedback but don't think we'll change here at this time. We inherit from OZ which has a public variable for DEFAULT_ADMIN_ROLE. For consistency we want MINTER_ROLE to be a getter as well, which would be lost if we use a library as suggested. An alt may be to have a shared constant and then include an explicit external getter separately..
Use fixed pragma
Disagree. We intentionally use a floating pragma in order to make integrating with contracts easier. Other contract developers are looking to interact with our contracts and they may be on a different version than we use. The pragma selected for our contracts is the minimum required in order to correctly compile and function. This way integration is easier if they lag a few versions behind, or if they use the latest but we don't bump our packages frequently enough, and when we do upgrade versions unless there was a breaking solidity change -- it should just swap in by incrementing our npm package version.
[Nā07] NatSpec is incomplete
Agree, fixed.
[Nā08] Event is missing indexed fields
BuyReferrerPaid: Agree but won't fix at this time. We have already started emitting this event on mainnet, to simplify integrations such as subgraph we are going to continue with this event as is. However it's a good catch and I agree that buyReferrer ideally would have been indexed here.
For the others I believe this is invalid. index should be reserved for params that are likely to be requested as a filter. In these examples those params are data not really filter candidates. And for the string specifically, indexed would prevent the full information from being communicated, requiring a second unindexed version which is a bit redundant and increases gas costs.
Use named returns consistently
Agree, we have opted to use the named returns instead of return ... This is more consistent with other code in our repo and saves a bit of on the contract size. We also like named returns as a way of improving natspec, and typechain (particularly when a tuple is returned).
Use modifier for duplicated require()/revert()
Agree, will fix.
š Selected for report: Dravee
Also found by: 0x040, 0x1f8b, 0xDjango, 0xHarry, 0xNazgul, 0xSmartContract, 0xbepresent, 0xkatana, Amithuddar, Aymen0909, Bnke0x0, Chom, CodingNameKiki, Deivitto, DevABDee, Diraco, ElKu, Fitraldys, Funen, IllIllI, JC, LeoS, Metatron, MiloTruck, Noah3o6, ReyAdmirado, Rohan16, Rolezn, Saw-mon_and_Natalie, Sm4rty, SpaceCake, TomJ, Tomio, Trabajo_de_mates, Waze, Yiko, __141345__, ajtra, apostle0x01, bobirichman, brgltd, bulej93, c3phas, cRat1st0s, carlitox477, d3e4, durianSausage, erictee, fatherOfBlocks, gerdusx, gogo, hakerbaya, ignacio, jag, joestakey, ladboy233, medikko, mics, newfork01, oyc_109, pfapostol, robee, rvierdiiev, sach1r0, saian, samruna, sikorico, simon135, wagmi, zeesaw, zkhorse, zuhaibmohd
48.6721 USDC - $48.67
| Issue | Instances | |
|---|---|---|
| [Gā01] | Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate | 1 |
| [Gā02] | Using calldata instead of memory for read-only arguments in external functions saves gas | 3 |
| [Gā03] | State variables should be cached in stack variables rather than re-reading them from storage | 8 |
| [Gā04] | The result of function calls should be cached rather than re-calling the function | 2 |
| [Gā05] | <array>.length should not be looked up in every loop of a for-loop | 4 |
| [Gā06] | ++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 | 1 |
| [Gā07] | require()/revert() strings longer than 32 bytes cost extra gas | 28 |
| [Gā08] | Optimize names to save gas | 13 |
| [Gā09] | Using bools for storage incurs overhead | 2 |
| [Gā10] | Using > 0 costs more gas than != 0 when used on a uint in a require() statement | 1 |
| [Gā11] | ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) | 2 |
| [Gā12] | Using private rather than public for constants, saves gas | 2 |
| [Gā13] | Use custom errors rather than revert()/require() strings to save gas | 28 |
| [Gā14] | Functions guaranteed to revert when called by normal users can be marked payable | 18 |
Total: 113 instances over 14 issues
address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriateSaves 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. Finally, if both fields are accessed in the same function, can save ~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.
There is 1 instance of this issue:
File: contracts/NFTCollection.sol 59 mapping(uint256 => address payable) private tokenIdToCreatorPaymentAddress; 60 61 /** 62 * @dev Stores a CID for each NFT. 63 */ 64: mapping(uint256 => string) private _tokenCIDs;
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 3 instances of this issue:
File: contracts/NFTCollectionFactory.sol /// @audit paymentAddressFactoryCall 363 function createNFTDropCollectionWithPaymentFactory( 364 string calldata name, 365 string calldata symbol, 366 string calldata baseURI, 367 bytes32 postRevealBaseURIHash, 368 uint32 maxTokenId, 369 address approvedMinter, 370 uint256 nonce, 371 CallWithoutValue memory paymentAddressFactoryCall 372: ) external returns (address collection) {
File: contracts/NFTCollection.sol /// @audit _name /// @audit _symbol 105 function initialize( 106 address payable _creator, 107 string memory _name, 108 string memory _symbol 109: ) external initializer onlyContractFactory {
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replace each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
There are 8 instances of this issue:
File: contracts/NFTCollectionFactory.sol /// @audit versionNFTCollection on line 207 213: string.concat("NFT Collection Implementation v", versionNFTCollection.toString()), /// @audit versionNFTCollection on line 213 214: string.concat("NFTv", versionNFTCollection.toString()) /// @audit versionNFTCollection on line 214 217: emit ImplementationNFTCollectionUpdated(_implementation, versionNFTCollection); /// @audit versionNFTDropCollection on line 231 234: emit ImplementationNFTDropCollectionUpdated(_implementation, versionNFTDropCollection); /// @audit versionNFTDropCollection on line 234 239: string.concat("NFT Drop Collection Implementation v", versionNFTDropCollection.toString()), /// @audit versionNFTDropCollection on line 239 240: string.concat("NFTDropV", versionNFTDropCollection.toString()),
File: contracts/NFTCollection.sol /// @audit baseURI_ on line 333 334: return baseURI_;
File: contracts/NFTDropCollection.sol /// @audit latestTokenId on line 179 181: for (uint256 i = firstTokenId; i <= latestTokenId; ) {
The instances below point to the second+ call of the function within a single function
There are 2 instances of this issue:
File: contracts/NFTCollectionFactory.sol /// @audit versionNFTCollection.toString() on line 213 214: string.concat("NFTv", versionNFTCollection.toString()) /// @audit versionNFTDropCollection.toString() on line 239 240: string.concat("NFTDropV", versionNFTDropCollection.toString()),
<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 4 instances of this issue:
File: contracts/mixins/shared/MarketFees.sol 126: for (uint256 i = 0; i < creatorRecipients.length; ++i) { 198: for (uint256 i = 0; i < creatorShares.length; ++i) { 484: for (uint256 i = 0; i < creatorRecipients.length; ++i) { 503: for (uint256 i = 1; i < creatorRecipients.length; ) {
++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 is 1 instance of this issue:
File: contracts/mixins/shared/MarketFees.sol 198: for (uint256 i = 0; i < creatorShares.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 are 28 instances of this issue:
File: contracts/libraries/AddressLibrary.sol 31: require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");
File: contracts/mixins/collections/SequentialMintCollection.sol 58: require(msg.sender == owner, "SequentialMintCollection: Caller is not creator"); 63: require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address"); 74: require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first"); 87: require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared"); 88: require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase"); 89: require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");
File: contracts/mixins/roles/AdminRole.sol 19: require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");
File: contracts/mixins/roles/MinterRole.sol 22: require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");
File: contracts/mixins/shared/ContractFactory.sol 22: require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory"); 31: require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");
File: contracts/NFTCollectionFactory.sol 173: require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role"); 182: require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract"); 203: require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract"); 227: require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract"); 262: require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");
File: contracts/NFTCollection.sol 158: require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required"); 263: require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required"); 264: require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted"); 268: require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted"); 327: require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");
File: contracts/NFTDropCollection.sol 88: require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set"); 93: require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed"); 130: require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set"); 131: require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set"); 172: require(count != 0, "NFTDropCollection: `count` must be greater than 0"); 179: require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId"); 238: require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");
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 13 instances of this issue:
File: contracts/mixins/collections/CollectionRoyalties.sol /// @audit getFeeRecipients(), getFeeBps(), getRoyalties(), getTokenCreatorPaymentAddress(), royaltyInfo() 17: abstract contract CollectionRoyalties is IGetRoyalties, IGetFees, IRoyaltyInfo, ITokenCreator, ERC165Upgradeable {
File: contracts/mixins/collections/SequentialMintCollection.sol /// @audit tokenCreator() 13: abstract contract SequentialMintCollection is ITokenCreator, Initializable, ERC721BurnableUpgradeable {
File: contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol /// @audit createFixedPriceSale(), mintFromFixedPriceSale(), getAvailableCountFromFixedPriceSale(), getFixedPriceSale() 36: abstract contract NFTDropMarketFixedPriceSale is MarketFees {
File: contracts/mixins/roles/AdminRole.sol /// @audit grantAdmin(), revokeAdmin(), isAdmin() 12: abstract contract AdminRole is Initializable, AccessControlUpgradeable {
File: contracts/mixins/roles/MinterRole.sol /// @audit grantMinter(), revokeMinter(), isMinter() 14: abstract contract MinterRole is Initializable, AccessControlUpgradeable, AdminRole {
File: contracts/mixins/shared/FETHNode.sol /// @audit getFethAddress() 15: abstract contract FETHNode {
File: contracts/mixins/shared/FoundationTreasuryNode.sol /// @audit getFoundationTreasury() 18: abstract contract FoundationTreasuryNode {
File: contracts/mixins/shared/MarketFees.sol /// @audit getFeesAndRecipients(), getRoyaltyRegistry(), internalGetTokenCreator(), internalGetImmutableRoyalties(), internalGetMutableRoyalties() 28: abstract contract MarketFees is FoundationTreasuryNode, MarketSharedCore, SendValueWithFallbackWithdraw {
File: contracts/mixins/shared/MarketSharedCore.sol /// @audit getSellerOf() 13: abstract contract MarketSharedCore is FETHNode {
File: contracts/NFTCollectionFactory.sol /// @audit initialize(), adminUpdateNFTCollectionImplementation(), adminUpdateNFTDropCollectionImplementation(), createNFTCollection(), createNFTDropCollection(), createNFTDropCollectionWithPaymentAddress(), createNFTDropCollectionWithPaymentFactory(), predictNFTCollectionAddress(), predictNFTDropCollectionAddress() 62: contract NFTCollectionFactory is ICollectionFactory, Initializable, Gap10000 {
File: contracts/NFTCollection.sol /// @audit initialize(), mint(), mintAndApprove(), mintWithCreatorPaymentAddress(), mintWithCreatorPaymentAddressAndApprove(), mintWithCreatorPaymentFactory(), mintWithCreatorPaymentFactoryAndApprove(), selfDestruct(), updateBaseURI(), updateMaxTokenId(), baseURI(), getHasMintedCID() 28: contract NFTCollection is
File: contracts/NFTDropCollection.sol /// @audit initialize(), mintCountTo(), reveal(), selfDestruct(), updateMaxTokenId(), updatePreRevealContent(), isRevealed(), numberOfTokensAvailableToMint() 28: contract NFTDropCollection is
File: contracts/NFTDropMarket.sol /// @audit initialize() 63: contract NFTDropMarket is
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 2 instances of this issue:
File: contracts/mixins/shared/MarketFees.sol 61: bool private immutable assumePrimarySale;
File: contracts/NFTCollection.sol 53: mapping(string => bool) private cidToMinted;
> 0 costs more gas than != 0 when used on a uint in a require() statementThis change saves 6 gas per instance. The optimization works until solidity version 0.8.13 where there is a regression in gas costs.
There is 1 instance of this issue:
File: contracts/NFTDropCollection.sol 131: require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");
++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)Saves 5 gas per loop
There are 2 instances of this issue:
File: contracts/NFTCollectionFactory.sol 207: versionNFTCollection++; 231: versionNFTDropCollection++;
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 2 instances of this issue:
File: contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol 70: bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
File: contracts/mixins/roles/MinterRole.sol 19: bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
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 28 instances of this issue:
File: contracts/libraries/AddressLibrary.sol 31: require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");
File: contracts/mixins/collections/SequentialMintCollection.sol 58: require(msg.sender == owner, "SequentialMintCollection: Caller is not creator"); 63: require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address"); 74: require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first"); 87: require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared"); 88: require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase"); 89: require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");
File: contracts/mixins/roles/AdminRole.sol 19: require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");
File: contracts/mixins/roles/MinterRole.sol 22: require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");
File: contracts/mixins/shared/ContractFactory.sol 22: require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory"); 31: require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");
File: contracts/NFTCollectionFactory.sol 173: require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role"); 182: require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract"); 203: require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract"); 227: require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract"); 262: require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");
File: contracts/NFTCollection.sol 158: require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required"); 263: require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required"); 264: require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted"); 268: require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted"); 327: require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");
File: contracts/NFTDropCollection.sol 88: require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set"); 93: require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed"); 130: require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set"); 131: require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set"); 172: require(count != 0, "NFTDropCollection: `count` must be greater than 0"); 179: require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId"); 238: require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");
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 18 instances of this issue:
File: contracts/mixins/collections/SequentialMintCollection.sol 62: function _initializeSequentialMintCollection(address payable _creator, uint32 _maxTokenId) internal onlyInitializing {
File: contracts/mixins/roles/AdminRole.sol 13: function _initializeAdminRole(address admin) internal onlyInitializing {
File: contracts/mixins/roles/MinterRole.sol 26: function _initializeMinterRole(address minter) internal onlyInitializing {
File: contracts/NFTCollectionFactory.sol 202: function adminUpdateNFTCollectionImplementation(address _implementation) external onlyAdmin { 226: function adminUpdateNFTDropCollectionImplementation(address _implementation) external onlyAdmin {
File: contracts/NFTCollection.sol 105 function initialize( 106 address payable _creator, 107 string memory _name, 108 string memory _symbol 109: ) external initializer onlyContractFactory { 119: function burn(uint256 tokenId) public override onlyCreator { 230: function selfDestruct() external onlyCreator { 238: function updateBaseURI(string calldata baseURIOverride) external onlyCreator { 251: function updateMaxTokenId(uint32 _maxTokenId) external onlyCreator { 262: function _mint(string calldata tokenCID) private onlyCreator returns (uint256 tokenId) {
File: contracts/NFTDropCollection.sol 120 function initialize( 121 address payable _creator, 122 string calldata _name, 123 string calldata _symbol, 124 string calldata _baseURI, 125 bytes32 _postRevealBaseURIHash, 126 uint32 _maxTokenId, 127 address _approvedMinter, 128 address payable _paymentAddress 129: ) external initializer onlyContractFactory validBaseURI(_baseURI) { 159: function burn(uint256 tokenId) public override onlyAdmin { 171: function mintCountTo(uint16 count, address to) external onlyMinterOrAdmin returns (uint256 firstTokenId) { 195: function reveal(string calldata _baseURI) external onlyAdmin validBaseURI(_baseURI) onlyWhileUnrevealed { 209: function selfDestruct() external onlyAdmin { 220: function updateMaxTokenId(uint32 _maxTokenId) external onlyAdmin { 232 function updatePreRevealContent(string calldata _baseURI, bytes32 _postRevealBaseURIHash) 233 external 234 validBaseURI(_baseURI) 235 onlyWhileUnrevealed 236: onlyAdmin
#0 - HardlyDifficult
2022-08-19T16:00:58Z
Gā01] Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate
Potentially valid, but since these are read in different use cases would need to do some testing to be sure.
calldata
Valid & will fix. This saves ~50 gas on createNFTCollection and ~60 gas on createNFTDropCollectionWithPaymentFactory
[Gā03] State variables should be cached in stack variables rather than re-reading them from storage
[Gā04] The result of function calls should be cached rather than re-calling the function
Agree, but it's not a goal to optimize admin-only functions - keeping as-is for simplicity.
[Gā05] <array>.length should not be looked up in every loop of a for-loop
May be theoretically valid, but won't fix. I tested this: gas-reporter and our gas-stories suite is reporting a small regression using this technique. It also hurts readability a bit so we wouldn't want to include it unless it was a clear win.
unchecked loop in
getFeesAndRecipients
getFeesAndRecipients is a read only function not intended to be used on-chain, but as a best practice we will add unchecked there as well.
Use short error messages
Agree but won't fix. We use up to 64 bytes, aiming to respect the incremental cost but 32 bytes is a bit too short to provide descriptive error messages for our users.
[Gā08] Optimize names to save gas
Disagree - this hurts the user experience and saves little gas.
Using bools for storage incurs overhead.
Valid for cidToMinted, saving ~200 gas. Not seeing any benefit for assumePrimarySale, potentially because it's an immutable variable.
Use != 0 instead of > 0
Invalid. We tested the recommendation and got the following results:
createNFTDropCollection gas reporter results: using > 0 (current): - 319246 Ā· 319578 Ā· 319361 using != 0 (recommendation): - 319252 Ā· 319584 Ā· 319367 impact: +6 gas
++i costs less than i++
Agree and will fix.
Using private rather than public for constants to saves gas.
Agree but won't fix. For ease of use and consistency we will continue to expose some constants publicly.
Custom errors
Agree but won't fix at this time. We use these in the market but not in collections. Unfortunately custom errors are still not as good of an experience for users (e.g. on etherscan). We used them in the market originally because we were nearing the max contract size limit and this was a good way to reduce the bytecode. We'll consider this in the future as tooling continues to improve.
Functions guaranteed to revert when called by normal users can be marked payable
Disagree. This could save gas but 1) this seems like a poor practice to encourage and could be error prone. and 2) we don't care about optimizing admin-only actions.