Foundation Drop contest - carlitox477's results

NFTCollectionFactory

[LOW] createNFTCollection

This method check that the symbol isn't null, however it does not check nothing about the symbol. This last check may be needed to be added.

[Non-critical] createNFTDropCollection; createNFTDropCollectionWithPaymentAddress; createNFTDropCollectionWithPaymentFactory unnecessary return variable collection.

In the three cases, collection is never set. Their signature should make the next change to their signatures: returns (address collection) to just returns (address)

NFTCollection

[Non-critical] Redundant inherit

NFTCollection inherit from ERC721BurnableUpgradeable already inherit ERC721Upgradeable and ERC165Upgradeable, meaning that ERC721Upgradeable and ERC165Upgradeable should be removed.

However, SequentialMintCollection already inherit ERC721BurnableUpgradeable, meaning that ERC721BurnableUpgradeable should be removed.

SequentialMintCollection already inherit Initializable and ITokenCreator , meaning that both should be removed.

CollectionRoyalties already inherit IGetRoyalties, IGetFees, IRoyaltyInfo, meaning that all of them should be removed.

In conclusion, NFTCollection should be:

• INFTCollectionInitializer,
• ContractFactory
• SequentialMintCollection,
• CollectionRoyalties

[Non-critical]: cidToMinted does not respect private naming convention

Should change it name to _cidToMinted

[Non-critical]: tokenIdToCreatorPaymentAddress does not respect private naming convention

Should change it name to _tokenIdToCreatorPaymentAddress

NFTDropCollection

[Non-critical] Redundant inherit

NFTDropCollection inherit from ERC721BurnableUpgradeable already inherit ERC721Upgradeable and ERC165Upgradeable, meaning that ERC721Upgradeable and ERC165Upgradeable should be removed.

However, SequentialMintCollection already inherit ERC721BurnableUpgradeable, meaning that ERC721BurnableUpgradeable should be removed.

SequentialMintCollection already inherit Initializable and ITokenCreator , meaning that both should be removed.

CollectionRoyalties already inherit IGetRoyalties, IGetFees, IRoyaltyInfo, meaning that all of them should be removed.

AdminRole already inherit AccessControlUpgradeable. meaning it can be removed

In conclusion, NFTDropCollection should be:

• INFTDropCollectionInitializer,
• INFTDropCollectionMint,
• ContractFactory,
• AdminRole,
• MinterRole,
• SequentialMintCollection,
• CollectionRoyalties

[Non-critical] _baseURI shadowing in multiple functions

By defining _baseURI as a parameter in multiple function, this shadows a storage variable with the same name, inherited from ERC721Upgradable contract, making the code more difficult to understand.

Suggestion: change _baseURI to __baseURI in functions: validBaseURI; initialize; reveal; updatePreRevealContent

NFTCollectionFactory

adminUpdateNFTCollectionImplementation: multiple storage access can be avoided

Storage variable versionNFTCollection is used multiple times. It is better to use a a memory variable to save gas in multiple storage calls (4), and only at the end write the storage variable. New function to save gas

function adminUpdateNFTCollectionImplementation(address _implementation) external onlyAdmin {
    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
    implementationNFTCollection = _implementation;
    uint32 _versionNFTCollection;
    unchecked {
      // Version cannot overflow 256 bits.
      _versionNFTCollection=versionNFTCollection+1;
    }

    // The implementation is initialized when assigned so that others may not claim it as their own.
    INFTCollectionInitializer(_implementation).initialize(
      payable(address(rolesContract)),
      string.concat("NFT Collection Implementation v", _versionNFTCollection.toString()),
      string.concat("NFTv", _versionNFTCollection.toString())
    );

    emit ImplementationNFTCollectionUpdated(_implementation, _versionNFTCollection);
    versionNFTCollection=_versionNFTCollection;
  }

adminUpdateNFTCollectionImplementation: possible wrong initialization can spend more gas than require

Giving that implementationNFTCollection = _implementation; is executed before trying to initialize the contract in _implementation, it would be better to move this line to the end of the function.

adminUpdateNFTDropCollectionImplementation: possible wrong initialization can spend more gas than require

Giving that implementationNFTDropCollection = _implementation; is executed before trying to initialize the contract in _implementation, it would be better to move this line to the end of the function.

adminUpdateNFTDropCollectionImplementation: multiple storage access can be avoided

Storage variable versionNFTCollection is used multiple times. It is better to use a a memory variable to save gas in multiple storage calls (4), and only at the end write the storage variable. New function to save gas

function adminUpdateNFTDropCollectionImplementation(address _implementation) external onlyAdmin {
    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
    implementationNFTDropCollection = _implementation;
    uint32 _versionNFTDropCollection;
    unchecked {
      // Version cannot overflow 256 bits.
      _versionNFTDropCollection=versionNFTDropCollection+1;
    }

    emit ImplementationNFTDropCollectionUpdated(_implementation, _versionNFTDropCollection);

    // The implementation is initialized when assigned so that others may not claim it as their own.
    INFTDropCollectionInitializer(_implementation).initialize(
      payable(address(this)),
      string.concat("NFT Drop Collection Implementation v", _versionNFTDropCollection.toString()),
      string.concat("NFTDropV", _versionNFTDropCollection.toString()),
      "ipfs://bafybeibvxnuaqtvaxu26gdgly2rm4g2piu7b2tqlx2dsz6wwhqbey2gddy/",
      0x1337000000000000000000000000000000000000000000000000000000001337,
      1,
      address(0),
      payable(0)
    );
    versionNFTDropCollection=_versionNFTDropCollection;
  }

NFTDropCollection

mintCountTo: multiple storage access

latestTokenId is accessed 5 times + loop access, it can be reduced to 2

function mintCountTo(uint16 count, address to) external onlyMinterOrAdmin returns (uint256 firstTokenId) {
    require(count != 0, "NFTDropCollection: `count` must be greater than 0");

    uint32 _latestTokenId=latestTokenId;
    unchecked {
      // If +1 overflows then +count would also overflow, unless count==0 in which case the loop would exceed gas limits
      firstTokenId = _latestTokenId + 1;
    }
    _latestTokenId = _latestTokenId + count;
    require(_latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");

    for (uint256 i = firstTokenId; i <= _latestTokenId; ) {
      _mint(to, i);
      unchecked {
        ++i;
      }
    }
    latestTokenId=_latestTokenId
  }

NFTCollection

Minted event has redundant information.

Minted event is emitted in function _mint, which includes tokenCID twice, once as indexedTokenCID and once as tokenCID. Meaning that tokenCID will always be indexedTokenCID. It would be better to eliminate one parameter, leaving Mint event as: event Minted(address indexed creator, uint256 indexed tokenId, string indexed tokenCID);