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: 33/108
Findings: 3
Award: $75.82
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 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
41.7141 USDC - $41.71
Severity: Low
Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.
Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those (details credit to: code-423n4/2021-09-sushimiso-findings#64)
function initialize(
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L105
function initialize(uint32 _versionNFTCollection) external initializer {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L192
function initialize(
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L120
function initialize() external initializer {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropMarket.sol#L100
function _initializeSequentialMintCollection(address payable _creator, uint32 _maxTokenId) internal onlyInitializing {
function _initializeAdminRole(address admin) internal onlyInitializing {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/AdminRole.sol#L13
function _initializeMinterRole(address minter) internal onlyInitializing {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/MinterRole.sol#L26
function _initializeAdminRole(address admin) internal onlyInitializing {
Ensure atomic calls to init functions along with deployment via robust deployment scripts or factory contracts. Emit explicit events for initializations of contracts. Enforce prevention of re-initializations via explicit setting/checking of boolean initialized variables
Severity: Low
According to openzepplin's ERC721, the use of _mint is discouraged, use _safeMint whenever possible. https://docs.openzeppelin.com/contracts/3.x/api/token/erc721#ERC721-_mint-address-uint256-
tokenId = _mint(tokenCID);
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L130
tokenId = _mint(tokenCID);
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L143
tokenId = _mint(tokenCID);
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L159
_mint(msg.sender, tokenId);
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L271
_mint(msg.sender, tokenId);
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L271
_mint(to, i);
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L182
Use _safeMint whenever possible instead of _mint
Severity: Low
function adminUpdateNFTCollectionImplementation(address _implementation) external onlyAdmin {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L202
function adminUpdateNFTDropCollectionImplementation(address _implementation) external onlyAdmin {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L226
function _initializeAdminRole(address admin) internal onlyInitializing {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/AdminRole.sol#L13
function grantAdmin(address account) external {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/AdminRole.sol#L28
function revokeAdmin(address account) external {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/AdminRole.sol#L37
function _initializeMinterRole(address minter) internal onlyInitializing {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/MinterRole.sol#L26
function grantMinter(address account) external {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/MinterRole.sol#L36
function revokeMinter(address account) external {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/MinterRole.sol#L45
function _initializeAdminRole(address admin) internal onlyInitializing {
function grantAdmin(address account) external {
function revokeAdmin(address account) external {
Consider adding zero-address checks
Severity: Low
Users can potentially call external public functions that can lead to malicious activity
function createNFTCollection(
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L257
function createNFTDropCollection(
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L286
function createNFTDropCollectionWithPaymentAddress(
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L324
function createNFTDropCollectionWithPaymentFactory(
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L363
function _createNFTDropCollection(
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L386
function createFixedPriceSale(
Consider adding a mapping of who can call these create functions to avoid malicious use
Severity: Non-Critical
Found usage of floating pragmas ^0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L3
Found usage of floating pragmas ^0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L42
Found usage of floating pragmas ^0.8.12 of Solidit https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/AddressLibrary.sol#L3
Found usage of floating pragmas ^0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/ArrayLibrary.sol#L3
Found usage of floating pragmas ^0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/BytesLibrary.sol#L3
Found usage of floating pragmas ^0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/collections/CollectionRoyalties.sol#L3
Found usage of floating pragmas ^0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/collections/SequentialMintCollection.sol#L3
Found usage of floating pragmas ^0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/nftDropMarket/NFTDropMarketCore.sol#L3
Found usage of floating pragmas ^0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L3
Found usage of floating pragmas ^0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/AdminRole.sol#L3
Found usage of floating pragmas ^0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/MinterRole.sol#L3
Found usage of floating pragmas ^0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/Constants.sol#L3
Found usage of floating pragmas ^0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/ContractFactory.sol#L3
Found usage of floating pragmas ^0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/FETHNode.sol#L3
Found usage of floating pragmas ^0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/FoundationTreasuryNode.sol#L3
Found usage of floating pragmas ^0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/Gap10000.sol#L3
Found usage of floating pragmas ^0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/MarketFees.sol#L3
Found usage of floating pragmas ^0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/MarketSharedCore.sol#L3
Found usage of floating pragmas ^0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/treasury/AdminRole.sol#L3
Severity: Non-Critical
if (creatorRecipients.length > 1) {
Severity: Non-Critical
Each event should use three indexed fields if there are three or more fields.
event URIUpdated(string baseURI, bytes32 postRevealBaseURIHash);
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L85
event CreateFixedPriceSale( address indexed nftContract, address indexed seller, uint256 price, uint256 limitPerAccount );
event BuyReferralPaid( address indexed nftContract, uint256 indexed tokenId, address buyReferrer, uint256 buyReferrerFee, uint256 buyReferrerSellerFee );
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
Severity: Non-Critical
contract NFTCollection is INFTCollectionInitializer, IGetRoyalties, IGetFees, IRoyaltyInfo, ITokenCreator, ContractFactory, Initializable, ERC165Upgradeable, ERC721Upgradeable, ERC721BurnableUpgradeable, SequentialMintCollection, CollectionRoyalties
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L28
contract NFTCollectionFactory is ICollectionFactory, Initializable, Gap10000
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L62
contract NFTDropCollection is INFTDropCollectionInitializer, INFTDropCollectionMint, IGetRoyalties, IGetFees, IRoyaltyInfo, ITokenCreator, ContractFactory, Initializable, ContextUpgradeable, ERC165Upgradeable, AccessControlUpgradeable, AdminRole, MinterRole, ERC721Upgradeable, ERC721BurnableUpgradeable, SequentialMintCollection, CollectionRoyalties
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L28
contract NFTDropMarket is Initializable, FoundationTreasuryNode, FETHNode, MarketSharedCore, NFTDropMarketCore, ReentrancyGuardUpgradeable, SendValueWithFallbackWithdraw, MarketFees, Gap10000, NFTDropMarketFixedPriceSale
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropMarket.sol#L63
Severity: Non-Critical
Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L3
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L42
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropMarket.sol#L42
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/AddressLibrary.sol#L3
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/ArrayLibrary.sol#L3
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/BytesLibrary.sol#L3
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/collections/CollectionRoyalties.sol#L3
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/collections/SequentialMintCollection.sol#L3
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/nftDropMarket/NFTDropMarketCore.sol#L3
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L3
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/AdminRole.sol#L3
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/MinterRole.sol#L3
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/Constants.sol#L3
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/ContractFactory.sol#L3
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/FETHNode.sol#L3
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/FoundationTreasuryNode.sol#L3
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/Gap10000.sol#L3
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/MarketFees.sol#L3
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/MarketSharedCore.sol#L3
Found old version 0.8.12 of Solidity https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/treasury/AdminRole.sol#L3
Consider updating to a more recent solidity version.
Severity: Non-Critical
Use (e.g. 1e6) rather than decimal literals (e.g. 100000), for better code readability.
uint256 constant BASIS_POINTS = 10000;
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/Constants.sol#L10
Severity: Non-Critical
Severity: Non-Critical
function _burn(uint256 tokenId) internal override(ERC721Upgradeable, SequentialMintCollection) {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L255
function _mint(string calldata tokenCID) private onlyCreator returns (uint256 tokenId) {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L262
function _createNFTDropCollection(
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L386
function _getSalt(address creator, uint256 nonce) private pure returns (bytes32) {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L448
function _burn(uint256 tokenId) internal override(ERC721Upgradeable, SequentialMintCollection) {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L245
#0 - HardlyDifficult
2022-08-18T21:18:37Z
(1) Init functions are susceptible to front-running
Invalid: our deploy scripts will initialize when the proxy is first set to this implementation address. e.g. see the logs for our Goerli proxy deployment: https://goerli.etherscan.io/tx/0xe0cea1c4cc1a9d1db9686b2b7c4154cab72f94a6b0823b7d4b9966d1e03cd8e8#eventlog
And the collections are initialized by the factory when they are created.
Use safeMint
Agree will fix - for context see our response here.
Missing Checks for Address(0
Invalid for the contract references since .isContract will revert on address(0).
For the role references, we choose to be consistent with the OZ AccessControl grantRole function which will also be publicly exposed.
(4) Users can call public funcions that create
Invalid - these are meant to be called by any user.
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.
(6) Constants Should Be Defined Rather Than Using Magic Numbers
I don't agree that 1 is a magic number here..
BuyReferralPaid event should index buyReferrer
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 constructor to initialize templates
Agree this is a good best practice to add. Will fix.
(9) Use a more recent version of Solidity
We are using the latest, 0.8.16 already
(10) Large multiples of ten should use scientific notation
Fair feedback, we'll consider a change.
Missing natspec comments
Fair feedback -- for natspec we aim for complete coverage of the public interfaces but for internal/private/libraries we have some gaps in order to reduce redundancy, for those we aim to include comments where things are unclear or not obvious from the function and variable names.
🌟 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
20.9449 USDC - $20.94
Severity: Gas Optimizations
The overheads outlined below are PER LOOP, excluding the first loop
storage arrays incur a Gwarmaccess (100 gas) memory arrays use MLOAD (3 gas) calldata arrays use 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
for (uint256 i = 0; i < creatorRecipients.length; ++i) {
for (uint256 i = 0; i < creatorShares.length; ++i) {
for (uint256 i = 0; i < creatorRecipients.length; ++i) {
for (uint256 i = 1; i < creatorRecipients.length; ) {
for (uint256 i = 0; i < creatorRecipients.length; ++i) {
for (uint256 i = 1; i < creatorRecipients.length; ) {
Severity: Gas Optimizations
When 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. Structs have the same overhead as an array of length one
function capLength(address payable[] memory data, uint256 maxLength) internal pure {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/ArrayLibrary.sol#L13
function getFeeRecipients(uint256 tokenId) external view returns (address payable[] memory recipients) {
function getFeeBps(
function getRoyalties(uint256 tokenId)
function getFeesAndRecipients(
function internalGetImmutableRoyalties(address nftContract, uint256 tokenId)
function internalGetMutableRoyalties(
function _getFees(
Severity: Gas Optimizations
for (uint256 i = 0; i < 20; ++i) {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/BytesLibrary.sol#L25
for (uint256 i = 0; i < 4; ++i) {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/BytesLibrary.sol#L44
for (uint256 i = 0; i < creatorRecipients.length; ++i) {
for (uint256 i = 0; i < creatorShares.length; ++i) {
for (uint256 i = 0; i < creatorRecipients.length; ++i) {
Severity: Gas Optimizations
This change saves 6 gas per instance
require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L130
require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L131
Severity: Gas Optimizations
creatorRev += creatorShares[i];
totalFees += buyReferrerFee;
creatorRev += creatorShares[i];
totalShares += creatorShares[i];
totalRoyaltiesDistributed += royalty;
totalFees -= buyReferrerFee;
Severity: Gas Optimizations
If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table
bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/MinterRole.sol#L19
Set variable to private.
Severity: Gas Optimizations
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 burn(uint256 tokenId) public override onlyCreator {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L119
function mintWithCreatorPaymentFactory(
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L192
function tokenURI(uint256 tokenId) public view override returns (string memory uri) {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L326
function burn(uint256 tokenId) public override onlyAdmin {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L159
function getTokenCreatorPaymentAddress(
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L252
function tokenURI(uint256 tokenId) public view override returns (string memory uri) {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L300
function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool interfaceSupported) {
function totalSupply() public view returns (uint256 supply) {
function isAdmin(address account) public view returns (bool approved) {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/AdminRole.sol#L47
function isMinter(address account) public view returns (bool approved) {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/MinterRole.sol#L54
function getFoundationTreasury() public view returns (address payable treasuryAddress) {
Severity: Gas Optimizations
require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L158
require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required");
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L263
require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L264
require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L268
require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L327
require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L203
require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L227
require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L262
require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L130
require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L131
require(count != 0, "NFTDropCollection: `count` must be greater than 0");
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L172
require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L179
require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L238
require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");
require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");
require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");
require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");
require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");
require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");
Severity: Gas Optimizations
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.
delete cidToMinted[_tokenCIDs[tokenId]];
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L256
delete tokenIdToCreatorPaymentAddress[tokenId];
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L257
delete _tokenCIDs[tokenId];
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L258
require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L264
hasBeenMinted = cidToMinted[tokenCID];
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L292
creatorPaymentAddress = tokenIdToCreatorPaymentAddress[tokenId];
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L304
uri = string.concat(_baseURI(), _tokenCIDs[tokenId]);
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L329
if (data[dataLocation] != expectedData[i]) {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/BytesLibrary.sol#L27
data[dataLocation] = newData[i];
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/BytesLibrary.sol#L30
FixedPriceSaleConfig storage saleConfig = nftContractToFixedPriceSaleConfig[nftContract];
FixedPriceSaleConfig memory saleConfig = nftContractToFixedPriceSaleConfig[nftContract];
FixedPriceSaleConfig memory saleConfig = nftContractToFixedPriceSaleConfig[nftContract];
Severity: Gas Optimizations
Lower than uint256 size storage variables are less gas efficient. Using uint64 does not give any efficiency, actually, it is the opposite as EVM operates on default of 256-bit values so uint64 is more expensive in this case as it needs a conversion. It only gives improvements in cases where you can pack variables together, e.g. structs.
uint32 public versionNFTCollection;
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L80
uint32 public versionNFTDropCollection;
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L95
uint32 public latestTokenId;
uint32 public maxTokenId;
uint32 private burnCounter;
uint80 price;
uint16 limitPerAccount;
Severity: Gas Optimizations
The 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
for (uint256 i = firstTokenId; i <= latestTokenId; ) {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L181
for (uint256 i = 0; i < 20; ++i) {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/BytesLibrary.sol#L25
for (uint256 i = 0; i < 4; ++i) {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/BytesLibrary.sol#L44
for (uint256 i = 0; i < creatorRecipients.length; ++i) {
for (uint256 i = 0; i < creatorShares.length; ++i) {
for (uint256 i = 0; i < creatorRecipients.length; ++i) {
for (uint256 i = 1; i < creatorRecipients.length; ) {
Severity: Gas Optimizations
These functions are missing address zero checks. Saves 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 adminUpdateNFTCollectionImplementation(address _implementation) external onlyAdmin {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L202
function adminUpdateNFTDropCollectionImplementation(address _implementation) external onlyAdmin {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L226
function _initializeAdminRole(address admin) internal onlyInitializing {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/AdminRole.sol#L13
function grantAdmin(address account) external {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/AdminRole.sol#L28
function revokeAdmin(address account) external {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/AdminRole.sol#L37
function _initializeMinterRole(address minter) internal onlyInitializing {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/MinterRole.sol#L26
function grantMinter(address account) external {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/MinterRole.sol#L36
function revokeMinter(address account) external {
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/MinterRole.sol#L45
function _initializeAdminRole(address admin) internal onlyInitializing {
function grantAdmin(address account) external {
function revokeAdmin(address account) external {
Severity: Gas Optimizations
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.
mapping(string => bool) private cidToMinted;
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L53
bool private immutable assumePrimarySale;
#0 - HardlyDifficult
2022-08-17T14:44:58Z
(1) .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.
(2) Using Calldata Instead Of Memory For Read-only Arguments In External Functions Saves Gas
Invalid. Yes this can help and does for createNFTCollection and createNFTDropCollectionWithPaymentFactory but those examples were not included here. The ones listed are either using internally sourced data or are return values, neither which can change to calldata.
(3) It Costs More Gas To Initialize Variables To Zero Than To Let The Default Of Zero Be Applied
Invalid. This optimization technique is no longer applicable with the current version of Solidity.
(4) Using > 0 Costs More Gas Than != 0 When Used On A Uint In A Require() Statement
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
5 += COSTS MORE GAS THAN = + FOR STATE VARIABLES
No impact. We made the recommended change and ran gas-reporter for our entire test suite. 0 impact detected. Even if there were a small benefit, we feel this hurts readability and would not be worth trivial savings.
6 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.
(7) Public Functions To External
Invalid examples listed.
burn overrides the OZ implementation and cannot be changed to externalmintWithCreatorPaymentFactory is used by another function so must be public or the code refactored into helpers, keeping as is to keep the code simple.(8) require()/revert() Strings Longer Than 32 Bytes Cost Extra Gas
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.
9 Help The Optimizer By Saving A Storage Variable’s Reference Instead Of Repeatedly Fetching It
Invalid. The examples listed don't support the rec. e.g. in this listed example https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol#L304 we are doing exactly what you seem to be suggesting.
(10) non-uint256 state variable is less efficient than uint256
Potentially valid but won't fix. This is uint32 to allow packing storage for a significant gas savings. Accepting the input as uint256 would incur additional overhead in checking for overflow before saving these values. Other inputs are logically limited to the size exposed, huge values cannot be used. Accepting the input as uint256 would incur additional overhead in checking for overflow before using these values.
(11) ++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
6 of the 7 examples are already unchecked -- invalid.
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 assembly to check for address(0)
Won't fix. While this may save a tiny bit of gas, we reserve the use of assembly to areas where it would provide significant benefit or accomplish something that's otherwise not possible with Solidity alone.
(13) 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.