QA Report

[code423n4]

Overview

Risk Rating	Number of issues
Low Risk	6
Non-Critical Risk	7

Table of Contents

• 1. Low Risk Issues
  • 1.1. _safeMint() should be used rather than _mint() wherever possible
  • 1.2. Uninitialized Upgradeable contract
  • 1.3. Unclear Code Comment
  • 1.4. Incorrect Code Comment
  • 1.5. Inconsistent use of __gap[...]
  • 1.6. Inconsistent use of Symbol is required check
• 2. Non-Critical Issues
  • 2.1. It's better to emit after all processing is done
  • 2.2. A magic value should be documented and explained. Use a constant instead
  • 2.3. Open TODOS
  • 2.4. Default Visibility
  • 2.5. Use bytes.concat()
  • 2.6. Adding a return statement when the function defines a named return variable, is redundant
  • 2.7. Non-library/interface files should use fixed compiler versions, not floating ones

1. Low Risk Issues

1.1. _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, see the WARNING L278:

File: ERC721Upgradeable.sol
275:     /**
276:      * @dev Mints `tokenId` and transfers it to `to`.
277:      *
278:      * WARNING: Usage of this method is discouraged, use {_safeMint} whenever possible
...
286:      */
287:     function _mint(address to, uint256 tokenId) internal virtual {

Both open OpenZeppelin and solmate have versions of this function so that NFTs aren't lost if they're minted to contracts that cannot transfer them back out.

Consider replacing with _safeMint() here:

NFTCollection.sol:271:      _mint(msg.sender, tokenId);
NFTDropCollection.sol:182:      _mint(to, i); 

Be careful however to respect the CEI pattern or add a re-entrancy guard as _safeMint adds a callback-check (_checkOnERC721Received) and a malicious onERC721Received could be exploited if not careful.

As an example, here, the CEIP isn't respected:

File: NFTCollection.sol
154:   function mintWithCreatorPaymentAddress(string calldata tokenCID, address payable tokenCreatorPaymentAddress)
155:     public
156:     returns (uint256 tokenId)
157:   {
158:     require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");
159:     tokenId = _mint(tokenCID); //@audit CEIP not respected (if we assume this will be replaced with safeMint, it's important)
160:     tokenIdToCreatorPaymentAddress[tokenId] = tokenCreatorPaymentAddress;
161:     emit TokenCreatorPaymentAddressSet(address(0), tokenCreatorPaymentAddress, tokenId);
162:   }

As tokenId is needed L160, the lines L159 and L160 can't be swapped. The only mitigation here is to use a nonReentrant modifier on state-updating functions.

Reading material:

• https://blocksecteam.medium.com/when-safemint-becomes-unsafe-lessons-from-the-hypebears-security-incident-2965209bda2a
• https://samczsun.com/the-dangers-of-surprising-code/
• https://github.com/KadenZipfel/smart-contract-attack-vectors/blob/master/vulnerabilities/unprotected-callback.md

1.2. Uninitialized Upgradeable contract

Similar issue in the past: here

Upgradeable dependencies should be initialized.
While not causing any harm at the moment, suppose OZ someday decide to upgrade this contract and the sponsor uses the new version: it is possible that the contract will not work anymore.
Consider calling the init() here:

• NFTDropCollection.sol:

File: NFTDropCollection.sol
028: contract NFTDropCollection is
...
037:   ContextUpgradeable,
038:   ERC165Upgradeable,
039:   AccessControlUpgradeable,
...
042:   ERC721Upgradeable,
043:   ERC721BurnableUpgradeable,
...
120:   function initialize(
...
129:   ) external initializer onlyContractFactory validBaseURI(_baseURI) {
...
134:     __ERC721_init(_name, _symbol);
+ 135:     __Context_init();
+ 135:     __AccessControl_init();
+ 135:     __ERC165_init();
+ 135:     __ERC721Burnable_init();

• NFTCollection.sol:

File: NFTCollection.sol
028: contract NFTCollection is
...
036:   ERC165Upgradeable,
037:   ERC721Upgradeable,
038:   ERC721BurnableUpgradeable,
...
105:   function initialize(
...
109:   ) external initializer onlyContractFactory {
110:     __ERC721_init(_name, _symbol); //@audit missing __ERC165_init(); and __ERC721Burnable_init;
+ 110:     __ERC165_init();
+ 110:     __ERC721Burnable_init();
...
112:   }

1.3. Unclear Code Comment

Issue with comment

The following deserves to be clarified:

File: NFTDropCollection.sol
120:   function initialize(
121:     address payable _creator,
122:     string calldata _name,
123:     string calldata _symbol,
124:     string calldata _baseURI,
125:     bytes32 _postRevealBaseURIHash,
126:     uint32 _maxTokenId,
127:     address _approvedMinter,
128:     address payable _paymentAddress
129:   ) external initializer onlyContractFactory validBaseURI(_baseURI) {
...
137:     // Initialize royalties
138:     if (_paymentAddress != address(0)) {
139:       // If no payment address was defined, use the creator's address. //@audit should clarify better, here we're thinking that something is missing
140:       paymentAddress = _paymentAddress;
141:     }

Here, the comment can make us believe that the state variable paymentAddress should be set to the _creator argument if the initialize function didn't pass a _paymentAddress argument. We could even think that a else statement is missing. However, what it means here is that if paymentAddress == address(0) is true in the solution, it will let the owner variable (which corresponds to the creator) take precedence, as seen in getTokenCreatorPaymentAddress():

File: NFTDropCollection.sol
252:   function getTokenCreatorPaymentAddress(
253:     uint256 /* tokenId */
254:   ) public view override returns (address payable creatorPaymentAddress) {
255:     creatorPaymentAddress = paymentAddress;
256:     if (creatorPaymentAddress == address(0)) {
257:       creatorPaymentAddress = owner;
258:     }
259:   }

The comment can be clearer, maybe like this: If no payment address was defined, the owner (aka the creator's address) will be returned in getters later on

1.4. Incorrect Code Comment

Issue with comment

// SafeMath is not required when dividing by a non-zero constant.

Here, while what is meant is understandable, the solidity version used is 0.8.12 and the SafeMath lib isn't imported:

contracts/mixins/shared/MarketFees.sol:
  3: pragma solidity ^0.8.12;

  409      unchecked {
  410:       // SafeMath is not required when dividing by a non-zero constant. 
  411        totalFees = price / PROTOCOL_FEE_DENOMINATOR;
  412      }

  469          unchecked {
  470:           // SafeMath is not required when dividing by a non-zero constant. 
  471            creatorRev = price / CREATOR_ROYALTY_DENOMINATOR;
  472          }

Consider replacing the comment with something along the line of Can't overflow when dividing by a non-zero constant

1.5. Inconsistent use of __gap[...]

While some contracts inherit from Gap10000.sol, others declare their own private __gap :

contracts/NFTCollectionFactory.sol:
  62: contract NFTCollectionFactory is ICollectionFactory, Initializable, Gap10000 {

contracts/NFTDropMarket.sol:
  63  contract NFTDropMarket is
....
  72:   Gap10000,

contracts/mixins/nftDropMarket/NFTDropMarketCore.sol:
  17:   uint256[1000] private __gap;

contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol:
  313:   uint256[1000] private __gap;

contracts/mixins/roles/AdminRole.sol:
  56:   uint256[1000] private __gap;

contracts/mixins/shared/FoundationTreasuryNode.sol:
  68:   uint256[2000] private __gap;

contracts/mixins/shared/MarketFees.sol:
  537:   uint256[1000] private __gap;

contracts/mixins/shared/MarketSharedCore.sol:
  43:   uint256[500] private __gap;

contracts/mixins/shared/SendValueWithFallbackWithdraw.sol:
  57:   uint256[999] private __gap;

Furthermore, NFTDropCollection and NFTCollection don't have their own private __gap and don't inherit from Gap10000, while they are upgradable.

Consider correcting these inconsistencies.

1.6. Inconsistent use of Symbol is required check

For NFTCollection, the check is made in NFTCollectionFactory.createNFTCollection().

• NFTCollectionFactory.sol#L262

File: NFTCollectionFactory.sol
257:   function createNFTCollection(
...
261:   ) external returns (address collection) {
262:     require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");
263: 
264:     // This reverts if the NFT was previously created using this implementation version + msg.sender + nonce
265:     collection = implementationNFTCollection.cloneDeterministic(_getSalt(msg.sender, nonce));
266: 
267:     INFTCollectionInitializer(collection).initialize(payable(msg.sender), name, symbol);
268: 
269:     emit NFTCollectionCreated(collection, msg.sender, versionNFTCollection, name, symbol, nonce);
270:   }

However, for NFTDropCollection, the check is made way further, after even the contract's creation (during the initialization):

• NFTDropCollection.sol#L130

File: NFTDropCollection.sol
120:   function initialize(
...
129:   ) external initializer onlyContractFactory validBaseURI(_baseURI) {
130:     require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");

Consider moving the check in NFTCollectionFactory._createNFTDropCollection() for consistency:

• NFTCollectionFactory.sol#L396

File: NFTCollectionFactory.sol
386:   function _createNFTDropCollection(
...
395:   ) private returns (address collection) {
396:     // This reverts if the NFT was previously created using this implementation version + msg.sender + nonce
+ 396:     require(bytes(symbol).length ! 0, "NFTDropCollection: `symbol` must be set");
397:     collection = implementationNFTDropCollection.cloneDeterministic(_getSalt(msg.sender, nonce));
398: 
399:     INFTDropCollectionInitializer(collection).initialize(
400:       payable(msg.sender),
401:       name,
402:       symbol,
403:       baseURI,
404:       postRevealBaseURIHash,
405:       maxTokenId,
406:       approvedMinter,
407:       paymentAddress
408:     );

While more consistent, this would also save gas in case of revert.

2. Non-Critical Issues

2.1. It's better to emit after all processing is done

• contracts/NFTCollectionFactory.sol (inconsistent with L217 where the event is indeed emitted after the initialization, as it should):

  234:     emit ImplementationNFTDropCollectionUpdated(_implementation, versionNFTDropCollection); //@audit-issue low: do like L217 and emit after the following .initialize() call
  235  
  236      // The implementation is initialized when assigned so that others may not claim it as their own.
  237      INFTDropCollectionInitializer(_implementation).initialize(
  238        payable(address(this)),
  239        string.concat("NFT Drop Collection Implementation v", versionNFTDropCollection.toString()),
  240        string.concat("NFTDropV", versionNFTDropCollection.toString()),
  241        "ipfs://bafybeibvxnuaqtvaxu26gdgly2rm4g2piu7b2tqlx2dsz6wwhqbey2gddy/",
  242        0x1337000000000000000000000000000000000000000000000000000000001337,
  243        1,
  244        address(0), 
  245        payable(0)
  246      );
  247    }

• contracts/mixins/shared/MarketFees.sol:

  144      // Pay the buy referrer fee
  145      if (buyReferrerFee != 0) {
  146        _sendValueWithFallbackWithdraw(buyReferrer, buyReferrerFee, SEND_VALUE_GAS_LIMIT_SINGLE_RECIPIENT);
  147:       emit BuyReferralPaid(nftContract, tokenId, buyReferrer, buyReferrerFee, 0);
  148        unchecked {
  149          // Add the referrer fee back into the total fees so that all 3 return fields sum to the total price for events
  150          totalFees += buyReferrerFee;
  151        }
  152      }
  153    }

2.2. A magic value should be documented and explained. Use a constant instead

contracts/NFTCollectionFactory.sol:
  241:       "ipfs://bafybeibvxnuaqtvaxu26gdgly2rm4g2piu7b2tqlx2dsz6wwhqbey2gddy/",
  242:       0x1337000000000000000000000000000000000000000000000000000000001337,

Consider using constant variables as this would make the code more maintainable and readable while costing nothing gas-wise (constants are replaced by their value at compile-time).

2.3. Open TODOS

The codebase still contains TODO statements. Consider resolving and removing the TODOs before deploying.

mixins/shared/MarketFees.sol:193:      // TODO add referral info

2.4. Default Visibility

The following constants are using the default visibility:

mixins/shared/Constants.sol:10:uint256 constant BASIS_POINTS = 10000;
mixins/shared/Constants.sol:15:bytes32 constant DEFAULT_ADMIN_ROLE = 0x00;
mixins/shared/Constants.sol:21:uint256 constant MAX_ROYALTY_RECIPIENTS = 5;
mixins/shared/Constants.sol:26:uint256 constant MIN_PERCENT_INCREMENT_DENOMINATOR = BASIS_POINTS / 1000;
mixins/shared/Constants.sol:32:uint256 constant READ_ONLY_GAS_LIMIT = 40000;
mixins/shared/Constants.sol:38:uint96 constant ROYALTY_IN_BASIS_POINTS = 1000;
mixins/shared/Constants.sol:43:uint256 constant ROYALTY_RATIO = BASIS_POINTS / ROYALTY_IN_BASIS_POINTS;
mixins/shared/Constants.sol:48:uint256 constant SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS = 210000;
mixins/shared/Constants.sol:53:uint256 constant SEND_VALUE_GAS_LIMIT_SINGLE_RECIPIENT = 20000;

For readability, consider explicitly declaring the visibility.

2.5. Use bytes.concat()

Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>))

Consider using it here:

NFTCollectionFactory.sol:42:pragma solidity ^0.8.12;
NFTCollectionFactory.sol:449:    return keccak256(abi.encodePacked(creator, nonce));

Please notice that Solidity version 0.8.12 introduces string.concat() (vs abi.encodePacked(<str>,<str>)) and that it's extensively used in the solution already, which is a good thing:

contracts/NFTCollection.sol:
  329:     uri = string.concat(_baseURI(), _tokenCIDs[tokenId]);

contracts/NFTCollectionFactory.sol:
  213:       string.concat("NFT Collection Implementation v", versionNFTCollection.toString()),
  214:       string.concat("NFTv", versionNFTCollection.toString())
  239:       string.concat("NFT Drop Collection Implementation v", versionNFTDropCollection.toString()),
  240:       string.concat("NFTDropV", versionNFTDropCollection.toString()),

contracts/NFTDropCollection.sol:
  303:     return string.concat(baseURI, tokenId.toString(), ".json");

2.6. Adding a return statement when the function defines a named return variable, is redundant

While not consuming more gas with the Optimizer enabled: using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity.

Affected code (using both):

• contracts/NFTCollectionFactory.sol:

  294:   ) external returns (address collection) {
  333:   ) external returns (address collection) {
  372:   ) external returns (address collection) {

• contracts/NFTDropCollection.sol:

  300:   function tokenURI(uint256 tokenId) public view override returns (string memory uri) { 

• contracts/NFTDropMarket.sol:

  112:     returns (address payable seller)
  115:     try IERC721(nftContract).ownerOf(tokenId) returns (address owner) {
  136:     returns (address payable sellerOrOwner)
  139:     try IERC721(nftContract).ownerOf(tokenId) returns (address owner) {

• contracts/libraries/AddressLibrary.sol:

  36:     returns (address payable contractAddress)

• contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol:

  230:     returns (uint256 numberThatCanBeMinted)

• contracts/mixins/shared/FoundationTreasuryNode.sol:

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

• contracts/mixins/shared/MarketSharedCore.sol:

  20:   function getSellerOf(address nftContract, uint256 tokenId) external view returns (address payable seller) {
  27:   function _getSellerOf(address nftContract, uint256 tokenId) internal view virtual returns (address payable seller);

2.7. Non-library/interface files should use fixed compiler versions, not floating ones

mixins/collections/CollectionRoyalties.sol:3:pragma solidity ^0.8.12;
mixins/collections/SequentialMintCollection.sol:3:pragma solidity ^0.8.12;
mixins/nftDropMarket/NFTDropMarketCore.sol:3:pragma solidity ^0.8.12;
mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol:3:pragma solidity ^0.8.12;
mixins/roles/AdminRole.sol:3:pragma solidity ^0.8.12;
mixins/roles/MinterRole.sol:3:pragma solidity ^0.8.12;
mixins/shared/Constants.sol:3:pragma solidity ^0.8.12;
mixins/shared/ContractFactory.sol:3:pragma solidity ^0.8.12;
mixins/shared/FETHNode.sol:3:pragma solidity ^0.8.12;
mixins/shared/FoundationTreasuryNode.sol:3:pragma solidity ^0.8.12;
mixins/shared/Gap10000.sol:3:pragma solidity ^0.8.12;
mixins/shared/MarketFees.sol:3:pragma solidity ^0.8.12;
mixins/shared/MarketSharedCore.sol:3:pragma solidity ^0.8.12;
mixins/shared/OZERC165Checker.sol:3:pragma solidity ^0.8.12;
mixins/shared/SendValueWithFallbackWithdraw.sol:3:pragma solidity ^0.8.12;
NFTCollection.sol:3:pragma solidity ^0.8.12;
NFTCollectionFactory.sol:42:pragma solidity ^0.8.12;
NFTDropCollection.sol:3:pragma solidity ^0.8.12;
NFTDropMarket.sol:42:pragma solidity ^0.8.12;

[HardlyDifficult]

Good feedback, thanks.

1.1. _safeMint() should be used rather than _mint() wherever possible

Agree will fix - for context see our response here.

1.2. Uninitialized Upgradeable contract

Yes fair feedback, but as noted they are no-ops at the moment.

1.3. Unclear Code Comment

Good feedback, will fix.

1.4. Incorrect Code Comment

Agree, will fix.

1.5. Inconsistent use of __gap[...]

This is intentional. Top-level contracts do not require gaps since all inheritance is slotted before those files. The Gap10000 is a special case gap leaving room for new mixins to be leveraged in the future vs other gaps which are intended to allow new storage slots within a specific mixin; those can be used for new mixins as well but we wanted Gap10000 for a huge amount of space where new mixins are likely to be required.

1.6. Inconsistent use of Symbol is required check

Agree that we were inconsistent with these checks. We have moved the NFTDropCollection requirement into the factory so that both collection types are implemented in a similar fashion, and we went with the factory instead of the collection init in order to follow the best practice of fail fast.

2.1. It's better to emit after all processing is done

Fixed the ImplementationNFTDropCollectionUpdated

For BuyReferralPaid it's logically grouped, where 146 & 147 are the complete payment task, and the following line is supporting the containing function.

2.2. A magic value should be documented and explained. Use a constant instead

Disagree here - this is a special case where those values are magic / arbitrary values.

2.3. Open TODOS

Agree, will fix.

2.4. Default Visibility

Invalid. Visibility is not supported in Constants.sol and attempting to add one will cause a compile error.

2.5. Use bytes.concat()

Since the params there are not bytes, this would require additional casting. I think encodePacked is better readability here.

2.6. Adding a return statement when the function defines a named return variable, is redundant

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

2.7. Non-library/interface files should use fixed compiler versions, not floating ones

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.