Back to Bnke0x0's contests

Foundation Drop contest - Bnke0x0's results

Foundation is a web3 destination.

General Information

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

Foundation

Findings Distribution

Researcher Performance

Rank: 31/108

Findings: 3

Award: $80.29

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest PageGithub Contest RepositoryGithub Findings RepositoryFoundation

Findings Information

🌟 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

Awards

41.4024 USDC - $41.40

Labels

bug
QA (Quality Assurance)

External Links

Link to GitHub issue

Low Risk

[L-01] Insufficient input validation (Checking for length greater than one is useless because the caller can just pass a weighting of zero for the second asset in order to exclude it):-

  1. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 482):

    if (creatorRecipients.length > 1) {

[L-02] Missing checks for address(0x0) when assigning values to address state variables:-

  1. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 293):

    nftContract = overrideContract;

  2. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 193):

    versionNFTCollection = _versionNFTCollection;

  3. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 202):

    implementationNFTCollection = _implementation;

  4. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 228):

    implementationNFTDropCollection = _implementation;

  5. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 140):

    paymentAddress = _paymentAddress;

  6. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 200):

    baseURI = _baseURI;

  7. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 240-241):

    postRevealBaseURIHash = _postRevealBaseURIHash; baseURI = _baseURI;

  8. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 255):

    creatorPaymentAddress = paymentAddress;

  9. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 65-66):

    owner = _creator; maxTokenId = _maxTokenId;

  10. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 91):

    maxTokenId = _maxTokenId;

  11. File: 2022-08-foundation/contracts/NFTCollection.sol (line 130):

    tokenId = _mint(tokenCID);

  12. File: 2022-08-foundation/contracts/NFTCollection.sol (line 143):

    tokenId = _mint(tokenCID);

  13. File: 2022-08-foundation/contracts/NFTCollection.sol (line 239):

    baseURI_ = baseURIOverride;

  14. File: 2022-08-foundation/contracts/NFTCollection.sol (line 292):

    hasBeenMinted = cidToMinted[tokenCID];

  15. File: 2022-08-foundation/contracts/mixins/shared/FoundationTreasuryNode.sol (line 51):

    treasury = _treasury;

  16. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 89):

    assumePrimarySale = _assumePrimarySale;

  17. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 184):

    rolesContract = IRoles(_rolesContract);

  18. File: 2022-08-foundation/contracts/mixins/shared/ContractFactory.sol (line 32):

    contractFactory = _contractFactory;

[L-03] initialize functions can be front-run (See this finding from a prior badger-dao contest for details: https://github.com/code-423n4/2021-10-badgerdao-findings/issues/40 ):-

  1. File: 2022-08-foundation/contracts/NFTDropMarket.sol (line 100):

    function initialize() external initializer {

  2. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 192):

    function initialize(uint32 _versionNFTCollection) external initializer {

  3. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 120):

    function initialize(

  4. File: 2022-08-foundation/contracts/NFTCollection.sol (line 105):

    function initialize(

[L-04] Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions (See this [ https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps ] 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.):-

  1. File: 2022-08-foundation/contracts/NFTDropMarket.sol (line 63-74):

    contract NFTDropMarket is Initializable, FoundationTreasuryNode, FETHNode, MarketSharedCore, NFTDropMarketCore, ReentrancyGuardUpgradeable, SendValueWithFallbackWithdraw, MarketFees, Gap10000, NFTDropMarketFixedPriceSale {

  2. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 28-46):

    contract NFTDropCollection is INFTDropCollectionInitializer, INFTDropCollectionMint, IGetRoyalties, IGetFees, IRoyaltyInfo, ITokenCreator, ContractFactory, Initializable, ContextUpgradeable, ERC165Upgradeable, AccessControlUpgradeable, AdminRole, MinterRole, ERC721Upgradeable, ERC721BurnableUpgradeable, SequentialMintCollection, CollectionRoyalties {

  3. File: 2022-08-foundation/contracts/mixins/roles/AdminRole.sol (line 12):

    abstract contract AdminRole is Initializable, AccessControlUpgradeable {

  4. File: 2022-08-foundation/contracts/mixins/roles/MinterRole.sol (line 14):

    abstract contract MinterRole is Initializable, AccessControlUpgradeable, AdminRole {

  5. File: 022-08-foundation/contracts/mixins/collections/CollectionRoyalties.sol (line 17):

    abstract contract CollectionRoyalties is IGetRoyalties, IGetFees, IRoyaltyInfo, ITokenCreator, ERC165Upgradeable {

  6. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 13):

    abstract contract SequentialMintCollection is ITokenCreator, Initializable, ERC721BurnableUpgradeable {

  7. File: 2022-08-foundation/contracts/NFTCollection.sol (line 28-41):

    contract NFTCollection is INFTCollectionInitializer, IGetRoyalties, IGetFees, IRoyaltyInfo, ITokenCreator, ContractFactory, Initializable, ERC165Upgradeable, ERC721Upgradeable, ERC721BurnableUpgradeable, SequentialMintCollection, CollectionRoyalties {

[L-05] Open TODOs (Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment):-

  1. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 193):

    // TODO add referral info

[L-06] _safeMint() should be used rather than _mint() wherever possible (_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):-

  1. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 182):

    _mint(to, i);

Non-Critical Issues

[N-01] Adding a return statement when the function defines a named return variable, is redundant:-

  1. File: 2022-08-foundation/contracts/NFTDropMarket.sol (line 125):

    return super._getSellerOf(nftContract, tokenId);

  2. File: 2022-08-foundation/contracts/mixins/shared/MarketSharedCore.sol (line 21):

    return _getSellerOf(nftContract, tokenId);

[N-02] Event is missing indexed fields (Each event should use three indexed fields if there are three or more fields):-

  1. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 71-77):

    event BuyReferralPaid( address indexed nftContract, uint256 indexed tokenId, address buyReferrer, uint256 buyReferrerFee, uint256 buyReferrerSellerFee );

  2. File: 2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol (line 79-84):

    event CreateFixedPriceSale( address indexed nftContract, address indexed seller, uint256 price, uint256 limitPerAccount );

  3. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 85):

    event URIUpdated(string baseURI, bytes32 postRevealBaseURIHash);

  4. File: 2022-08-foundation/contracts/NFTCollection.sol (line 70):

    event BaseURIUpdated(string baseURI);

[N-03] Unused file:-

  1. File: 2022-08-foundation/contracts/NFTDropMarket.sol (line 40):

    // SPDX-License-Identifier: MIT OR Apache-2.0

  2. File: 2022-08-foundation/contracts/mixins/shared/Constants.sol (line 1):

    // SPDX-License-Identifier: MIT OR Apache-2.0

  3. File: 2022-08-foundation/contracts/mixins/shared/FETHNode.sol (line 1):

    // SPDX-License-Identifier: MIT OR Apache-2.0

  4. File: 2022-08-foundation/contracts/mixins/shared/FoundationTreasuryNode.sol (line 1):

    // SPDX-License-Identifier: MIT OR Apache-2.0

  5. File: 2022-08-foundation/contracts/mixins/shared/Gap10000.sol (line 1):

    // SPDX-License-Identifier: MIT OR Apache-2.0

  6. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 1):

    // SPDX-License-Identifier: MIT OR Apache-2.0

  7. File: 2022-08-foundation/contracts/libraries/BytesLibrary.sol (line 1):

    // SPDX-License-Identifier: MIT OR Apache-2.0

  8. File: 2022-08-foundation/contracts/libraries/ArrayLibrary.sol (line 1):

    // SPDX-License-Identifier: MIT OR Apache-2.0

  9. File: 2022-08-foundation/contracts/mixins/shared/MarketSharedCore.sol (line 1):

    // SPDX-License-Identifier: MIT OR Apache-2.0

  10. File: 2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketCore.sol (line 1):

    // SPDX-License-Identifier: MIT OR Apache-2.0

  11. File: 2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol (line 1):

    f// SPDX-License-Identifier: MIT OR Apache-2.0

  12. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 40):

    // SPDX-License-Identifier: MIT OR Apache-2.0

  13. File: 2022-08-foundation/contracts/libraries/AddressLibrary.sol (line 1):

    // SPDX-License-Identifier: MIT OR Apache-2.0

  14. File: 2022-08-foundation/contracts/NFTCollection.sol (line 1):

    2022-08-foundation/contracts/NFTDropCollection.sol

  15. File: 2022-08-foundation/contracts/mixins/shared/ContractFactory.sol(line 1):

    // SPDX-License-Identifier: MIT OR Apache-2.0

  16. File: 2022-08-foundation/contracts/mixins/roles/AdminRole.sol (line 1):

    // SPDX-License-Identifier: MIT OR Apache-2.0

  17. File: 2022-08-foundation/contracts/mixins/roles/MinterRole.sol (line 1):

    // SPDX-License-Identifier: MIT OR Apache-2.0

  18. File: 2022-08-foundation/contracts/mixins/collections/CollectionRoyalties.sol (line 1):

    // SPDX-License-Identifier: MIT OR Apache-2.0

  19. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 1):

    // SPDX-License-Identifier: MIT OR Apache-2.0

  20. File: 2022-08-foundation/contracts/NFTCollection.sol (line 1):

    // SPDX-License-Identifier: MIT OR Apache-2.0

[N-04] Constant redefined elsewhere (Consider defining in only one contract so that values cannot become out of sync when only one location is updated):-

  1. File: 2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol (line 70):

    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

  2. File: 2022-08-foundation/contracts/mixins/roles/MinterRole.sol (line 19):

    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

  3. File: 2022-08-foundation/contracts/mixins/shared/Constants.sol (line 10-): 53 `uint256 constant BASIS_POINTS = 10000;

/**

  • @dev The default admin role defined by OZ ACL modules. */ bytes32 constant DEFAULT_ADMIN_ROLE = 0x00;

/**

  • @dev Cap the number of royalty recipients.
  • A cap is required to ensure gas costs are not too high when a sale is settled. */ uint256 constant MAX_ROYALTY_RECIPIENTS = 5;

/**

  • @dev The minimum increase of 10% required when making an offer or placing a bid. */ uint256 constant MIN_PERCENT_INCREMENT_DENOMINATOR = BASIS_POINTS / 1000;

/**

  • @dev The gas limit used when making external read-only calls.
  • This helps to ensure that external calls does not prevent the market from executing. */ uint256 constant READ_ONLY_GAS_LIMIT = 40000;

/**

  • @dev Default royalty cut paid out on secondary sales.
  • Set to 10% of the secondary sale. */ uint96 constant ROYALTY_IN_BASIS_POINTS = 1000;

/**

  • @dev 10%, expressed as a denominator for more efficient calculations. */ uint256 constant ROYALTY_RATIO = BASIS_POINTS / ROYALTY_IN_BASIS_POINTS;

/**

  • @dev The gas limit to send ETH to multiple recipients, enough for a 5-way split. */ uint256 constant SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS = 210000;

/**

  • @dev The gas limit to send ETH to a single recipient, enough for a contract with a simple receiver. */ uint256 constant SEND_VALUE_GAS_LIMIT_SINGLE_RECIPIENT = 20000;`

  1. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 45-51):

    uint256 private constant CREATOR_ROYALTY_DENOMINATOR = BASIS_POINTS / 1000; // 10% /// @notice The fee collected by Foundation for sales facilitated by this market contract. uint256 private constant PROTOCOL_FEE_DENOMINATOR = BASIS_POINTS / 500; // 5% /// @notice The fee collected by the buy referrer for sales facilitated by this market contract. /// This fee is calculated from the total protocol fee. /// @dev 20% of protocol fee == 1% of total sale. uint256 private constant BUY_REFERRER_PROTOCOL_FEE_DENOMINATOR = 5;

[N-05] Non-library/interface files should use fixed compiler versions, not floating ones:-

  1. File: 2022-08-foundation/contracts/NFTDropMarket.sol (line 42):

    pragma solidity ^0.8.12;

  2. File: 2022-08-foundation/contracts/mixins/shared/Constants.sol (line 3):

    pragma solidity ^0.8.12;

  3. File: 2022-08-foundation/contracts/mixins/shared/FETHNode.sol (line 3):

    pragma solidity ^0.8.12;

  4. File: 2022-08-foundation/contracts/mixins/shared/FoundationTreasuryNode.sol (line 3):

    pragma solidity ^0.8.12;

  5. File: 2022-08-foundation/contracts/mixins/shared/Gap10000.sol (line 3):

    pragma solidity ^0.8.12;

  6. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 3):

    pragma solidity ^0.8.12;

  7. File: 2022-08-foundation/contracts/libraries/BytesLibrary.sol (line 3):

    pragma solidity ^0.8.12;

  8. File: 2022-08-foundation/contracts/libraries/ArrayLibrary.sol (line 3):

    pragma solidity ^0.8.12;

  9. File: 2022-08-foundation/contracts/mixins/shared/MarketSharedCore.sol (line 3):

    pragma solidity ^0.8.12;

  10. File: 2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketCore.sol (line 3):

    pragma solidity ^0.8.12;

  11. File: 2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol (line 3):

    fpragma solidity ^0.8.12;

  12. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 42):

    pragma solidity ^0.8.12;

  13. File: 2022-08-foundation/contracts/libraries/AddressLibrary.sol (line 3):

    pragma solidity ^0.8.12;

  14. File: 2022-08-foundation/contracts/NFTCollection.sol (line 3):

    2022-08-foundation/contracts/NFTDropCollection.sol

  15. File: 2022-08-foundation/contracts/mixins/shared/ContractFactory.sol(line 3):

    pragma solidity ^0.8.12;

  16. File: 2022-08-foundation/contracts/mixins/roles/AdminRole.sol (line 3):

    pragma solidity ^0.8.12;

  17. File: 2022-08-foundation/contracts/mixins/roles/MinterRole.sol (line 3):

    pragma solidity ^0.8.12;

  18. File: 2022-08-foundation/contracts/mixins/collections/CollectionRoyalties.sol (line 3):

    pragma solidity ^0.8.12;

  19. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 3):

    pragma solidity ^0.8.12;

  20. File: 2022-08-foundation/contracts/NFTCollection.sol (line 3):

    pragma solidity ^0.8.12;

[N-06] public functions not called by the contract should be declared external instead (Contracts are allowed to override their parents’ functions and change the visibility from external to public.):-

  1. File: 2022-08-foundation/contracts/mixins/shared/FoundationTreasuryNode.sol (line 59):

    function getFoundationTreasury() public view returns (address payable treasuryAddress) {

  2. File: 2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol (line 264-267):

    function getFixedPriceSale(address nftContract) public view returns (

  3. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 159):

    function burn(uint256 tokenId) public override onlyAdmin {

  4. File: 2022-08-foundation/contracts/mixins/roles/AdminRole.sol (line 47):

    function isAdmin(address account) public view returns (bool approved) {

  5. File: 2022-08-foundation/contracts/mixins/roles/MinterRole.sol (line 54):

    function isMinter(address account) public view returns (bool approved) {

  6. File: 2022-08-foundation/contracts/NFTCollection.sol (line 192-196):

    function mintWithCreatorPaymentFactory( string calldata tokenCID, address paymentAddressFactory, bytes calldata paymentAddressCallData ) public returns (uint256 tokenId) {

[N-07] States/flags should use Enums rather than separate constants:-

  1. File: 2022-08-foundation/contracts/mixins/shared/Constants.sol (line 10-53):

    `uint256 constant BASIS_POINTS = 10000;

/**

  • @dev The default admin role defined by OZ ACL modules. */ bytes32 constant DEFAULT_ADMIN_ROLE = 0x00;

/**

  • @dev Cap the number of royalty recipients.
  • A cap is required to ensure gas costs are not too high when a sale is settled. */ uint256 constant MAX_ROYALTY_RECIPIENTS = 5;

/**

  • @dev The minimum increase of 10% required when making an offer or placing a bid. */ uint256 constant MIN_PERCENT_INCREMENT_DENOMINATOR = BASIS_POINTS / 1000;

/**

  • @dev The gas limit used when making external read-only calls.
  • This helps to ensure that external calls does not prevent the market from executing. */ uint256 constant READ_ONLY_GAS_LIMIT = 40000;

/**

  • @dev Default royalty cut paid out on secondary sales.
  • Set to 10% of the secondary sale. */ uint96 constant ROYALTY_IN_BASIS_POINTS = 1000;

/**

  • @dev 10%, expressed as a denominator for more efficient calculations. */ uint256 constant ROYALTY_RATIO = BASIS_POINTS / ROYALTY_IN_BASIS_POINTS;

/**

  • @dev The gas limit to send ETH to multiple recipients, enough for a 5-way split. */ uint256 constant SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS = 210000;

/**

  • @dev The gas limit to send ETH to a single recipient, enough for a contract with a simple receiver. */ uint256 constant SEND_VALUE_GAS_LIMIT_SINGLE_RECIPIENT = 20000;`

[N-07] States/flags should use Enums rather than separate constants:-

  1. File: 2022-08-foundation/contracts/mixins/shared/Constants.sol (line 10-53):

    ccc

  2. File: 2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol (line 264-267):

    function getFixedPriceSale(address nftContract) public view returns (

  3. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 159):

    function burn(uint256 tokenId) public override onlyAdmin {

  4. File: 2022-08-foundation/contracts/mixins/roles/AdminRole.sol (line 47):

    function isAdmin(address account) public view returns (bool approved) {

  5. File: 2022-08-foundation/contracts/mixins/roles/MinterRole.sol (line 54):

    function isMinter(address account) public view returns (bool approved) {

  6. File: 2022-08-foundation/contracts/NFTCollection.sol (line 192-196):

    function mintWithCreatorPaymentFactory( string calldata tokenCID, address paymentAddressFactory, bytes calldata paymentAddressCallData ) public returns (uint256 tokenId) {

[N-08] Use a more recent version of solidity (Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions):-

  1. File: 2022-08-foundation/contracts/mixins/shared/FETHNode.sol (line 3):

    pragma solidity ^0.8.12;

  2. File: 2022-08-foundation/contracts/mixins/shared/FoundationTreasuryNode.sol (line 3):

    pragma solidity ^0.8.12;

  3. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 3):

    pragma solidity ^0.8.12;

  4. File: 2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol (line 3):

    pragma solidity ^0.8.12;

  5. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 42):

    pragma solidity ^0.8.12;

  6. File: 2022-08-foundation/contracts/libraries/AddressLibrary.sol (line 3):

    pragma solidity ^0.8.12;

  7. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 3):

    pragma solidity ^0.8.12;

  8. File: 2022-08-foundation/contracts/mixins/shared/ContractFactory.sol (line 3):

    pragma solidity ^0.8.12;

  9. File: 2022-08-foundation/contracts/NFTCollection.sol (line 3):

    pragma solidity ^0.8.12;

#0 - HardlyDifficult

2022-08-18T20:46:42Z

[L-01] Insufficient input validation

Invalid: the code will handle 0 shares already.

[L-02] Missing checks for address(0x0) when assigning values to address state variables:-

  1. Fair but this is already handled inside the RoyaltyRegistry itself.
  2. Invalid - 0 is an acceptable input here
  3. Invalid - .isContract reverts if address(0) is used
  4. Invalid - .isContract reverts if address(0) is used
  5. Invalid - that's inside a != address(0) check
  6. Invalid - checked via a modifier
  7. Invalid - checked via a modifier
  8. Invalid - this code is handling the address(0) scenario
  9. Invalid - this requirement is enforced from the factory where this call originates
  10. Invalid - 0 is acceptable here
  11. Invalid - checked in the helper
  12. Invalid - checked in the helper
  13. Invalid - empty is acceptable here
  14. Invalid - this is just a getter, reverting is not required here
  15. Invalid - .isContract reverts if address(0) is used
  16. Invalid - this is a bool, false is acceptable
  17. Invalid - .isContract reverts if address(0) is used
  18. Invalid - .isContract reverts if address(0) is used

L-03] initialize functions can be front-run

Invalid. See our comment here for context

[L-04] Upgradeable contract is missing a __gap[50]

  1. Invalid - top level contracts do not require a gap
  2. Invalid - collections are not upgradeable
  3. We choose not to include a gap since this is a stateless contract
  4. We choose not to include a gap since this is a stateless contract
  5. Invalid - collections are not upgradeable
  6. Invalid - collections are not upgradeable
  7. Invalid - collections are not upgradeable

Unresolved TODO comments

Agree, will fix.

Use safeMint

Agree will fix - for context see our response here.

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

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.

Missing indexed event parameters

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.

[N-03] Unused file:-

Invalid. The compile requires a license comment.

[N-04] Constant redefined elsewhere

For consistency with OZ we wanted to expose these publicly, for simplicity MINTER_ROLE is copied across 2 different contracts since we couldn't make them public and use our Constants.sol file but we may revisit this in the future.

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-06] public functions not called

  1. Invalid, used internally
  2. Invalid, used internally
  3. Invalid, declared by the OZ dependency
  4. Invalid, used internally
  5. Invalid, used internally
  6. Invalid, used internally

[N-07] States/flags should use Enums rather than separate constants

Disagree, I don't think enums for these makes things more clear.

[N-08] Use a more recent version of solidity

All contracts are compiled using 0.8.16

Findings Information

🌟 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

Awards

25.7188 USDC - $25.72

Labels

bug
G (Gas Optimization)

External Links

Link to GitHub issue

[G-01] State variables only set in the constructor should be declared immutable[Avoids a Gsset (20000 gas)]:-

  1. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 73):

    address public implementationNFTCollection;

  2. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 80):

    uint32 public versionNFTCollection;

  3. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 88):

    address public implementationNFTDropCollection;

  4. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 95):

    uint32 public versionNFTDropCollection;

  5. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 64):

    string public baseURI;

  6. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 74):

    bytes32 public postRevealBaseURIHash;

  7. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 54):

    address payable private paymentAddress;

  8. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 20):

    address payable public owner;

  9. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 27):

    uint32 public latestTokenId;

  10. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 34):

    uint32 public maxTokenId;

  11. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 40):

    uint32 private burnCounter;

  12. File: 2022-08-foundation/contracts/NFTCollection.sol (line 48):

    string private baseURI_;

[G-02] x = x + y is cheaper than x += y:-

  1. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 527):

    totalFees -= buyReferrerFee;

  2. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 134):

    creatorRev += creatorShares[i];

  3. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 150):

    totalFees += buyReferrerFee;

  4. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 199):

    creatorRev += creatorShares[i];

  5. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 490):

    totalShares += creatorShares[i];

  6. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 505):

    totalRoyaltiesDistributed += royalty;

[G-02] <array>.length should not be looked up in every loop of a for-loop (Even memory arrays incur the overhead of bit tests and bit shifts to calculate the array length. Storage array length checks incur an extra Gwarmaccess (100 gas) PER-LOOP):-

  1. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 126):

    for (uint256 i = 0; i < creatorRecipients.length; ++i) {

  2. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 198):

    for (uint256 i = 0; i < creatorShares.length; ++i) {

  3. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 484):

    for (uint256 i = 0; i < creatorRecipients.length; ++i) {

  4. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 503):

    for (uint256 i = 1; i < creatorRecipients.length; ) {

[G-03] ++i/i++ should be unchecked{++i}/unchecked{++i} when it is not possible for them to overflow, as is the case when used in forand whileloops:-

  1. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 126):

    for (uint256 i = 0; i < creatorRecipients.length; ++i) {

  2. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 198):

    for (uint256 i = 0; i < creatorShares.length; ++i) {

  3. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 484):

    for (uint256 i = 0; i < creatorRecipients.length; ++i) {

  4. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 503):

    for (uint256 i = 1; i < creatorRecipients.length; ) {

  5. File: 2022-08-foundation/contracts/libraries/BytesLibrary.sol (line 25):

    for (uint256 i = 0; i < 20; ++i) {

  6. File: 2022-08-foundation/contracts/libraries/BytesLibrary.sol (line 44):

    for (uint256 i = 0; i < 4; ++i) {

[G-04] require()/revert() strings longer than 32 bytes cost extra gas:-

  1. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 173):

    require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");

  2. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 182):

    require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract");

  3. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 203):

    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

  4. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 227):

    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

  5. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 262):

    require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");

  6. File: 2022-08-foundation/contracts/libraries/AddressLibrary.sol (line 31):

    require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");

  7. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 88):

    require(bytes(_baseURI).length > 0, "NFTDropCollection: _baseURI must be set");

  8. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 93):

    require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");

  9. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 130-131):

    require(bytes(_symbol).length > 0, "NFTDropCollection:_symbolmust be set"); require(_maxTokenId > 0, "NFTDropCollection:_maxTokenId must be set");

  10. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 172):

    require(count != 0, "NFTDropCollection: count must be greater than 0");

  11. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 179):

    require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");

  12. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 238):

    require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use reveal instead");

  13. File: 2022-08-foundation/contracts/mixins/shared/ContractFactory.sol (line 22):

    require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");

  14. File: 2022-08-foundation/contracts/mixins/shared/ContractFactory.sol (line 238):

    require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");

  15. File: 2022-08-foundation/contracts/mixins/roles/AdminRole.sol (line 19):

    require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");

  16. File: 2022-08-foundation/contracts/mixins/roles/MinterRole.sol (line 22):

    require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");

  17. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 58):

    require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");

  18. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 63):

    require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");

  19. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 74):

    require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");

  20. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 87-89):

    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");

  21. File: 2022-08-foundation/contracts/NFTCollection.sol (line 158):

    require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");

  22. File: 2022-08-foundation/contracts/NFTCollection.sol (line 263-264):

    require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required"); require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");

  23. File: 2022-08-foundation/contracts/NFTCollection.sol (line 268):

    require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");

  24. File: 2022-08-foundation/contracts/NFTCollection.sol (line 327):

    require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");

[G-05] Not using the named return variables when a function returns, wastes deployment gas:-

  1. File: 2022-08-foundation/contracts/NFTDropMarket.sol (line 125):

    return super._getSellerOf(nftContract, tokenId);

  2. File: 2022-08-foundation/contracts/mixins/shared/MarketSharedCore.sol (line 21):

    return _getSellerOf(nftContract, tokenId);

[G-06] Using > 0 costs more gas than != 0 when used on a uint in a require() statement:-

  1. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 88):

    require(bytes(_baseURI).length > 0, "NFTDropCollection: _baseURI must be set");

  2. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 93):

    require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");

  3. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 130-131):

    require(bytes(_symbol).length > 0, "NFTDropCollection:_symbolmust be set"); require(_maxTokenId > 0, "NFTDropCollection:_maxTokenId must be set");

[G-07] It costs more gas to initialize variables to zero than to let the default of zero be applied:-

  1. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 126):

    for (uint256 i = 0; i < creatorRecipients.length; ++i) {

  2. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 198):

    for (uint256 i = 0; i < creatorShares.length; ++i) {

  3. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 484):

    for (uint256 i = 0; i < creatorRecipients.length; ++i) {

  4. File: 2022-08-foundation/contracts/libraries/BytesLibrary.sol (line 25):

    for (uint256 i = 0; i < 20; ++i) {

  5. File: 2022-08-foundation/contracts/libraries/BytesLibrary.sol (line 44):

    for (uint256 i = 0; i < 4; ++i) {

[G-08] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead :-

(When 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).

  1. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 54):

    uint80 price;

  2. File: 2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol (line 58):

    uint16 limitPerAccount;

  3. File: 2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol (line 120-121):

    uint80 price, uint16 limitPerAccount

  4. File: 2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol (line 172):

    uint16 count,

  5. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 65):

    using Strings for uint32;

  6. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 80):

    uint32 public versionNFTCollection;

  7. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 95):

    uint32 public versionNFTDropCollection;

  8. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 192):

    function initialize(uint32 _versionNFTCollection) external initializer {

  9. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 291):

    uint32 maxTokenId,

  10. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 329):

    uint32 maxTokenId,

  11. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 370):

    uint32 maxTokenId,

  12. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 394):

    uint32 maxTokenId,

  13. File: 022-08-foundation/contracts/NFTDropCollection.sol (line 126):

    uint32 _maxTokenId,

  14. File: 022-08-foundation/contracts/NFTDropCollection.sol (line 220):

    function updateMaxTokenId(uint32 _maxTokenId) external onlyAdmin {

  15. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 27):

    uint32 public latestTokenId;

  16. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 34):

    uint32 public maxTokenId;

  17. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 40):

    uint32 private burnCounter;

  18. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 62):

    function _initializeSequentialMintCollection(address payable _creator, uint32 _maxTokenId) internal onlyInitializing {

  19. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 86):

    function _updateMaxTokenId(uint32 _maxTokenId) internal {

  20. File: 2022-08-foundation/contracts/NFTCollection.sol (line 251):

    function updateMaxTokenId(uint32 _maxTokenId) external onlyCreator {

[G-09] Expressions for constant values such as a call to keccak256(), should use immutable rather than constant (See this issue for a detail description of the issue https://github.com/ethereum/solidity/issues/9232 ) :-

  1. File: 2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol (line 70):

    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

  2. File: 2022-08-foundation/contracts/mixins/roles/MinterRole.sol (line 19):

    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

[G-10] Duplicated require()/revert() checks should be refactored to a modifier or function :-

  1. File: 2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol (line 227):

    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

[G-11] require() or revert() statements that check input arguments should be at the top of the function

(Checks that involve constants should come before checks that involve state variables):-

  1. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 173):

    require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");

  2. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 182):

    require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract");

  3. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 203):

    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

  4. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 227):

    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

  5. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 262):

    require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");

  6. File: 2022-08-foundation/contracts/libraries/AddressLibrary.sol (line 31):

    require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");

  7. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 88):

    require(bytes(_baseURI).length > 0, "NFTDropCollection: _baseURI must be set");

  8. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 93):

    require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");

  9. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 130-131):

    require(bytes(_symbol).length > 0, "NFTDropCollection:_symbolmust be set"); require(_maxTokenId > 0, "NFTDropCollection:_maxTokenId must be set");

  10. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 172):

    require(count != 0, "NFTDropCollection: count must be greater than 0");

  11. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 179):

    require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");

  12. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 238):

    require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use reveal instead");

  13. File: 2022-08-foundation/contracts/mixins/shared/ContractFactory.sol (line 22):

    require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");

  14. File: 2022-08-foundation/contracts/mixins/shared/ContractFactory.sol (line 238):

    require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");

  15. File: 2022-08-foundation/contracts/mixins/roles/AdminRole.sol (line 19):

    require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");

  16. File: 2022-08-foundation/contracts/mixins/roles/MinterRole.sol (line 22):

    require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");

  17. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 58):

    require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");

  18. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 63):

    require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");

  19. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 74):

    require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");

  20. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 87-89):

    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");

  21. File: 2022-08-foundation/contracts/NFTCollection.sol (line 158):

    require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");

  22. File: 2022-08-foundation/contracts/NFTCollection.sol (line 263-264):

    require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required"); require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");

  23. File: 2022-08-foundation/contracts/NFTCollection.sol (line 268):

    require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");

  24. File: 2022-08-foundation/contracts/NFTCollection.sol (line 327):

    require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");

[G-12] Use custom errors rather than revert()/require() strings to save deployment gas:-

  1. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 173):

    require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");

  2. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 182):

    require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract");

  3. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 203):

    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

  4. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 227):

    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

  5. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 262):

    require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");

  6. File: 2022-08-foundation/contracts/libraries/AddressLibrary.sol (line 31):

    require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");

  7. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 88):

    require(bytes(_baseURI).length > 0, "NFTDropCollection: _baseURI must be set");

  8. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 93):

    require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");

  9. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 130-131):

    require(bytes(_symbol).length > 0, "NFTDropCollection:_symbolmust be set"); require(_maxTokenId > 0, "NFTDropCollection:_maxTokenId must be set");

  10. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 172):

    require(count != 0, "NFTDropCollection: count must be greater than 0");

  11. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 179):

    require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");

  12. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 238):

    require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use reveal instead");

  13. File: 2022-08-foundation/contracts/mixins/shared/ContractFactory.sol (line 22):

    require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");

  14. File: 2022-08-foundation/contracts/mixins/shared/ContractFactory.sol (line 238):

    require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");

  15. File: 2022-08-foundation/contracts/mixins/roles/AdminRole.sol (line 19):

    require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");

  16. File: 2022-08-foundation/contracts/mixins/roles/MinterRole.sol (line 22):

    require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");

  17. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 58):

    require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");

  18. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 63):

    require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");

  19. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 74):

    require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");

  20. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 87-89):

    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");

  21. File: 2022-08-foundation/contracts/NFTCollection.sol (line 158):

    require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");

  22. File: 2022-08-foundation/contracts/NFTCollection.sol (line 263-264):

    require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required"); require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");

  23. File: 2022-08-foundation/contracts/NFTCollection.sol (line 268):

    require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");

  24. File: 2022-08-foundation/contracts/NFTCollection.sol (line 327):

    require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");

[G-13] Functions guaranteed to revert when called by normal users can be marked payable (If 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.):-

  1. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 202):

    function adminUpdateNFTCollectionImplementation(address _implementation) external onlyAdmin {

  2. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 226):

    function adminUpdateNFTDropCollectionImplementation(address _implementation) external onlyAdmin {

  3. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 120-129):

    function initialize( address payable _creator, string calldata _name, string calldata _symbol, string calldata _baseURI, bytes32 _postRevealBaseURIHash, uint32 _maxTokenId, address _approvedMinter, address payable _paymentAddress ) external initializer onlyContractFactory validBaseURI(_baseURI) {

  4. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 159):

    function burn(uint256 tokenId) public override onlyAdmin {

  5. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 171):

    function mintCountTo(uint16 count, address to) external onlyMinterOrAdmin returns (uint256 firstTokenId) {

  6. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 195):

    function reveal(string calldata _baseURI) external onlyAdmin validBaseURI(_baseURI) onlyWhileUnrevealed {

  7. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 209):

    function selfDestruct() external onlyAdmin {

  8. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 220):

    function updateMaxTokenId(uint32 _maxTokenId) external onlyAdmin {

  9. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 232-237):

    function updatePreRevealContent(string calldata _baseURI, bytes32 _postRevealBaseURIHash) external validBaseURI(_baseURI) onlyWhileUnrevealed onlyAdmin {

  10. File: 2022-08-foundation/contracts/mixins/roles/MinterRole.sol (line 26):

    function _initializeMinterRole(address minter) internal onlyInitializing {

  11. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 62):

    function _initializeSequentialMintCollection(address payable _creator, uint32 _maxTokenId) internal onlyInitializing {

  12. File: 2022-08-foundation/contracts/NFTCollection.sol (line 105-109):

    function initialize( address payable _creator, string memory _name, string memory _symbol ) external initializer onlyContractFactory {

  13. File: 2022-08-foundation/contracts/NFTCollection.sol (line 119):

    function burn(uint256 tokenId) public override onlyCreator {

  14. File: 2022-08-foundation/contracts/NFTCollection.sol (line 230):

    function selfDestruct() external onlyCreator {

  15. File: 2022-08-foundation/contracts/NFTCollection.sol (line 238):

    function updateBaseURI(string calldata baseURIOverride) external onlyCreator {

  16. File: 2022-08-foundation/contracts/NFTCollection.sol (line 251):

    function updateMaxTokenId(uint32 _maxTokenId) external onlyCreator {

  17. File: 2022-08-foundation/contracts/NFTCollection.sol (line 262):

    function _mint(string calldata tokenCID) private onlyCreator returns (uint256 tokenId) {

[G-14] Use a more recent version of solidity (Use a solidity version of at least 0.8.16 to have external calls skip contract existence checks if the external call has a return value):-

  1. File: 2022-08-foundation/contracts/NFTDropMarket.sol (line 42):

    pragma solidity ^0.8.12;

  2. File: 2022-08-foundation/contracts/mixins/shared/Constants.sol (line 3):

    pragma solidity ^0.8.12;

  3. File: 2022-08-foundation/contracts/mixins/shared/FETHNode.sol (line 3):

    pragma solidity ^0.8.12;

  4. File: 2022-08-foundation/contracts/mixins/shared/FoundationTreasuryNode.sol (line 3):

    pragma solidity ^0.8.12;

  5. File: 2022-08-foundation/contracts/mixins/shared/Gap10000.sol (line 3):

    pragma solidity ^0.8.12;

  6. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 3):

    pragma solidity ^0.8.12;

  7. File: 2022-08-foundation/contracts/libraries/BytesLibrary.sol (line 3):

    pragma solidity ^0.8.12;

  8. File: 2022-08-foundation/contracts/libraries/ArrayLibrary.sol (line 3):

    pragma solidity ^0.8.12;

  9. File: 2022-08-foundation/contracts/mixins/shared/MarketSharedCore.sol (line 3):

    pragma solidity ^0.8.12;

  10. File: 2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketCore.sol (line 3):

    pragma solidity ^0.8.12;

  11. File: 2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol (line 3):

    fpragma solidity ^0.8.12;

  12. File: 2022-08-foundation/contracts/NFTCollectionFactory.sol (line 42):

    pragma solidity ^0.8.12;

  13. File: 2022-08-foundation/contracts/libraries/AddressLibrary.sol (line 3):

    pragma solidity ^0.8.12;

  14. File: 2022-08-foundation/contracts/NFTCollection.sol (line 3):

    2022-08-foundation/contracts/NFTDropCollection.sol

  15. File: 2022-08-foundation/contracts/mixins/shared/ContractFactory.sol(line 3):

    pragma solidity ^0.8.12;

  16. File: 2022-08-foundation/contracts/mixins/roles/AdminRole.sol (line 3):

    pragma solidity ^0.8.12;

  17. File: 2022-08-foundation/contracts/mixins/roles/MinterRole.sol (line 3):

    pragma solidity ^0.8.12;

  18. File: 2022-08-foundation/contracts/mixins/collections/CollectionRoyalties.sol (line 3):

    pragma solidity ^0.8.12;

  19. File: 2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol (line 3):

    pragma solidity ^0.8.12;

  20. File: 2022-08-foundation/contracts/NFTCollection.sol (line 3):

    pragma solidity ^0.8.12;

[G-15] Use != 0 instead of > 0 (Using > 0 uses slightly more gas than using != 0. Use != 0 when comparing uint variables to zero, which cannot hold values below zero):-

  1. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 244):

    if (royaltyAmount > 0) {

  2. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 88):

    require(bytes(_baseURI).length > 0, "NFTDropCollection: _baseURI must be set");

  3. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 130-131):

    require(bytes(_symbol).length > 0, "NFTDropCollection:_symbolmust be set"); require(_maxTokenId > 0, "NFTDropCollection:_maxTokenId must be set");

Recommended Mitigation Steps:-

Replace > 0 with != 0 to save gas.

[G-16] Use simple comparison in if statement (The comparison operators >= and <= use more gas than >, <, or ==. Replacing the >= and ≤ operators with a comparison operator that has an opcode in the EVM saves gas.):-

  1. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 244):

    if (currentBalance >= limitPerAccount) {

Recommended Mitigation Steps:-

A simple comparison can be used for gas savings by reversing the logic:

  `if (currentBalance > limitPerAccount) {`

[G-17] Using calldata instead of memory for read-only arguments in external functions saves gas:-

  1. File: 2022-08-foundation/contracts/NFTCollection.sol (line 105-109):

    function initialize( address payable _creator, string memory _name, string memory _symbol ) external initializer onlyContractFactory {

[G-18] Using bools for storage incurs overhead:-

  1. File: 2022-08-foundation/contracts/NFTCollection.sol (line 53):

    mapping(string => bool) private cidToMinted;

  2. File: 2022-08-foundation/contracts/mixins/shared/MarketFees.sol (line 61):

    bool private immutable assumePrimarySale;

[G-19] Using private rather than public for constants, saves gas (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):-

  1. File: 2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol (line 70):

    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

  2. File: 2022-08-foundation/contracts/mixins/roles/MinterRole.sol (line 19):

    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

[G-20] Empty blocks should be removed or emit something:-

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{...}})

  1. File: 2022-08-foundation/contracts/NFTDropMarket.sol (line 94):

    {}

  2. File: 2022-08-foundation/contracts/NFTDropCollection.sol (line 103):

    {}

  3. File: 2022-08-foundation/contracts/NFTCollection.sol (line 97):

    {}

#0 - HardlyDifficult

2022-08-17T15:15:49Z

[G-01] State variables only set in the constructor should be declared immutable[Avoids a Gsset (20000 gas)]:-

Invalid. All of the examples listed are mutable variables.

[G-02] x = x + y is cheaper than x += y:-

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.

G-02] <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.

G-03] ++i/i++ should be unchecked{++i}/unchecked{++i} when it is not possible for them to overflow, as is the case when used in forand whileloops:-

5 of the 6 examples listed 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.

[G-04] 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.

[G-05] Not using the named return variables when a function returns, wastes deployment gas:-

Agree for code consistency with other parts of our code. Saves 0.004 bytes on the bytecode size.

[G-06] Using > 0 costs more gas than != 0 when used on a uint in a require() stateme

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

[G-07] 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.

[G-08] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead :-

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.

[G-09] Expressions for constant values such as a call to keccak256(), should use immutable rather than constant (See this issue for a detail description of the issue

While it seems this should help, changing to immutable shows a regression in our test suite. Gas costs go up by ~50 gas.

[G-10] Duplicated require()/revert() checks should be refactored to a modifier or function

Agree, but just one redundant instance found.

[G-11] require() or revert() statements that check input arguments should be at the top of the function

Invalid. The examples listed already do what's recommended.

[G-12] Use custom errors rather than revert()/require() strings to save deployment gas:-

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.

[G-13] 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.

G-14] Use a more recent version of solidity (Use a solidity version of at least 0.8.16

Invalid. We are using the latest version -- see hardhat.config

[G-15] 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

G-16] Use simple comparison in if statement

Invalid -- the recommendation there changes the logic.

[G-17] Using calldata instead of memory for read-only arguments in external functions saves gas:-

Valid & will fix. This saves ~50 gas on createNFTCollection.

[G-18] 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.

[G-19] Using private rather than public for constants, saves gas

Agree but won't fix. For ease of use and consistency we will continue to expose some constants publicly.

[G-20] Empty blocks should be removed or emit something:

Invalid -- these examples cannot be made abstract due to the inheritance patterns used

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter