QA Report

[code423n4]

Low Risk Findings Overview

	Finding	Instances
[L-01]	Floating Pragma	1
[L-02]	abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()	1

Non-critical Findings Overview

	Finding	Instances
[N-01]	Remove TODOs	1
[N-02]	Function doesn’t currently use try/catch as spec suggests	1

QA overview per contract

Contract	Total Instances	Total Findings	Low Findings	Low Instances	NC Findings	NC Instances
MarketFees.sol	2	2	0	0	2	2
NFTCollection.sol	1	1	1	1	0	0
NFTCollectionFactory.sol	1	1	1	1	0	0

Low Risk Findings

[L-01] Floating Pragma

A floating pragma might result in contract being tested/deployed with different or inconsistent compiler versions possibly leading to unexpected behaviour.
1 instance of this issue has been found:

[L-01] NFTCollection.sol#L3

pragma solidity ^0.8.12;

[L-02] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.
1 instance of this issue has been found:

[L-02] NFTCollectionFactory.sol#L449-L450

    return keccak256(abi.encodePacked(creator, nonce));

Non-critical Findings

[N-01] Remove TODOs

Please remove TODOs as they harm readability.
1 instance of this issue has been found:

[N-01] MarketFees.sol#L193-L194

      // TODO add referral info

[N-02] Function doesn’t currently use try/catch as spec suggests

Function can be turned into internal.
1 instance of this issue has been found:

[N-02] MarketFees.sol#L212-L223

  /**
   * @notice **For internal use only.**
   * @dev This function is external to allow using try/catch but is not intended for external use.
   * This checks the token creator.
   */
  function internalGetTokenCreator(address nftContract, uint256 tokenId)
    external
    view
    returns (address payable creator)
  {
    creator = ITokenCreator(nftContract).tokenCreator{ gas: READ_ONLY_GAS_LIMIT }(tokenId);
  }

[HardlyDifficult]

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.

abi.encodePacked() should not be used

It's a gas savings to use encodePacked for the use cases we support here. There does not appear to be a compelling reason to change.

Unresolved TODO comments

Agree, will fix.

[N-02] Function doesn’t currently use try/catch as spec suggests

Invalid, it's used here