search

code-423n4 / 2022-08-foundation-findings

0 stars 0 forks source link

Gas Optimizations #52

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

[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:_baseURImust 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:_maxTokenIdmust be set");

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

    require(count != 0, "NFTDropCollection:countmust 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: userevealinstead");

  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:_baseURImust 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:_maxTokenIdmust 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:_baseURImust 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:_maxTokenIdmust be set");

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

    require(count != 0, "NFTDropCollection:countmust 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: userevealinstead");

  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:_baseURImust 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:_maxTokenIdmust be set");

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

    require(count != 0, "NFTDropCollection:countmust 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: userevealinstead");

  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:_baseURImust 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:_maxTokenIdmust 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):

    {}

HardlyDifficult commented 2 years ago

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