QA Report

[code423n4]

Typo

AddressLibrary.sol: L13

 * @title A library for address helpers not already covered by the OZ library library.

Remove repeated word library

---

---

Long single line comments

In theory, comments over 79 characters should wrap using multi-line comment syntax. Even if somewhat longer comments are acceptable, there are cases where very long comments interfere with readability. Below are several instances of long comments that could be improved by wrapping, as shown:

---

MarketFees.sol: L148-151

      unchecked {
        // Add the referrer fee back into the total fees so that all 3 return fields sum to the total price for events
        totalFees += buyReferrerFee;
      }

Recommendation:

      unchecked {
        // Add the referrer fee back into the total fees so that all 3 return fields
        //  sum to the total price for events.
        totalFees += buyReferrerFee;
      }

Similarly for the following:

NFTCollection.sol: L174-177

---

NFTCollectionFactory.sol: L35-36

  /**
   * @notice Create a new collection contract.
   * @dev The nonce must be unique for the msg.sender + implementation version, otherwise this call will revert.
   * @param name The collection's `name`.
   * @param symbol The collection's `symbol`.
   * @param nonce An arbitrary value used to allow a creator to mint multiple collections with a counterfactual address.
   * @return collection The address of the newly created collection contract.
   */

Suggestion:

  /**
   * @notice Create a new collection contract.
   * @dev The nonce must be unique for the msg.sender + implementation version,
   *   otherwise this call will revert.
   * @param name The collection's `name`.
   * @param symbol The collection's `symbol`.
   * @param nonce An arbitrary value used to allow a creator to mint
   *   multiple collections with a counterfactual address.
   * @return collection The address of the newly created collection contract.
   */

Similarly for the following:

NFTCollectionFactory.sol: L272-285

NFTCollectionFactory.sol: L308-323

NFTCollectionFactory.sol: L347-362

NFTCollectionFactory.sol: L425-431

NFTCollectionFactory.sol: L436-443

NFTCollection.sol: L71-77

---

NFTCollectionFactory.sol: L57-61

  /**
 * @title A factory to create NFT collections.
 * @notice Call this factory to create NFT collections.
 * @dev This creates and initializes an ERC-1165 minimal proxy pointing to a NFT collection contract implementation.
 */

Suggestion:

  /**
 * @title A factory to create NFT collections.
 * @notice Call this factory to create NFT collections.
 * @dev This creates and initializes an ERC-1165 minimal proxy pointing to a
 *   NFT collection contract implementation.
 */

---

---

Comments concerning unfinished work or open items

Comments that contain TODOs or other language indicating open items should be addressed and modified or removed. Below are two instances:

---

MarketFees.sol: L193

      // TODO add referral info

---

MarketFees.sol: L284-285

    /* Overrides must support ERC-165 when registered, except for overrides defined by the registry owner.
       If that results in an override w/o 165 we may need to upgrade the market to support or ignore that override. */

---

---

Missing @param statement

---

MarketFees.sol: L79-93

  /**
   * @notice Configures the registry allowing for royalty overrides to be defined.
   * @param _royaltyRegistry The registry to use for royalty overrides.
   */
  constructor(address _royaltyRegistry, bool _assumePrimarySale) {
    if (!_royaltyRegistry.supportsInterface(type(IRoyaltyRegistry).interfaceId)) {
      revert NFTMarketFees_Address_Does_Not_Support_IRoyaltyRegistry();
    }
    royaltyRegistry = IRoyaltyRegistry(_royaltyRegistry);

    assumePrimarySale = _assumePrimarySale;

    // In the constructor, `this` refers to the implementation address. Everywhere else it'll be the proxy.
    implementationAddress = this;
  }

@param statement is missing for _assumePrimarySale

---

---

[HardlyDifficult]

Typo

Agree, will fix.

Maximum line length exceeded

Fair however we use a limit of 120 instead. I believe this is more readable since it prevents excessive wrapping, particularly in comment. This is enforced by our linter.

Unresolved TODO comments

Agree, will fix.

Missing param

Agree, will fix.