QA Report

[code423n4]

(1) Init functions are susceptible to front-running

Severity: Low

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those
(details credit to: code-423n4/2021-09-sushimiso-findings#64)

Proof Of Concept

function initialize(

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L105

function initialize(uint32 _versionNFTCollection) external initializer {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L192

function initialize(

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L120

function initialize() external initializer {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropMarket.sol#L100

function _initializeSequentialMintCollection(address payable _creator, uint32 _maxTokenId) internal onlyInitializing {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/collections/SequentialMintCollection.sol#L62

function _initializeAdminRole(address admin) internal onlyInitializing {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/AdminRole.sol#L13

function _initializeMinterRole(address minter) internal onlyInitializing {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/MinterRole.sol#L26

function _initializeAdminRole(address admin) internal onlyInitializing {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/treasury/AdminRole.sol#L14

Recommended Mitigation Steps

Ensure atomic calls to init functions along with deployment via robust deployment scripts or factory contracts. Emit explicit events for initializations of contracts. Enforce prevention of re-initializations via explicit setting/checking of boolean initialized variables

(2) Use _safeMint instead of _mint

Severity: Low

According to openzepplin's ERC721, the use of _mint is discouraged, use _safeMint whenever possible.
https://docs.openzeppelin.com/contracts/3.x/api/token/erc721#ERC721-_mint-address-uint256-

Proof Of Concept

tokenId = _mint(tokenCID);

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L130

tokenId = _mint(tokenCID);

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L143

tokenId = _mint(tokenCID);

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L159

_mint(msg.sender, tokenId);

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L271

_mint(msg.sender, tokenId);

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L271

_mint(to, i);

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L182

Recommended Mitigation Steps

Use _safeMint whenever possible instead of _mint

(3) Missing Checks for Address(0x0)

Severity: Low

Proof Of Concept

function adminUpdateNFTCollectionImplementation(address _implementation) external onlyAdmin {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L202

function adminUpdateNFTDropCollectionImplementation(address _implementation) external onlyAdmin {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L226

 function _initializeAdminRole(address admin) internal onlyInitializing {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/AdminRole.sol#L13

 function grantAdmin(address account) external {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/AdminRole.sol#L28

function revokeAdmin(address account) external {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/AdminRole.sol#L37

function _initializeMinterRole(address minter) internal onlyInitializing {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/MinterRole.sol#L26

function grantMinter(address account) external {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/MinterRole.sol#L36

function revokeMinter(address account) external {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/MinterRole.sol#L45

function _initializeAdminRole(address admin) internal onlyInitializing {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/treasury/AdminRole.sol#L14

 function grantAdmin(address account) external {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/treasury/AdminRole.sol#L29

function revokeAdmin(address account) external {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/treasury/AdminRole.sol#L38

Recommended Mitigation Steps

Consider adding zero-address checks

(4) Users can call public funcions that create

Severity: Low

Users can potentially call external public functions that can lead to malicious activity

Proof Of Concept

function createNFTCollection(

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L257

function createNFTDropCollection(

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L286

function createNFTDropCollectionWithPaymentAddress(

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L324

function createNFTDropCollectionWithPaymentFactory(

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L363

function _createNFTDropCollection(

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L386

function createFixedPriceSale(

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L118

Recommended Mitigation Steps

Consider adding a mapping of who can call these create functions to avoid malicious use

(5) Avoid Floating Pragmas: The Version Should Be Locked

Severity: Non-Critical

Proof Of Concept

Found usage of floating pragmas ^0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L3

Found usage of floating pragmas ^0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L42

Found usage of floating pragmas ^0.8.12 of Solidit
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/AddressLibrary.sol#L3

Found usage of floating pragmas ^0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/ArrayLibrary.sol#L3

Found usage of floating pragmas ^0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/BytesLibrary.sol#L3

Found usage of floating pragmas ^0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/collections/CollectionRoyalties.sol#L3

Found usage of floating pragmas ^0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/collections/SequentialMintCollection.sol#L3

Found usage of floating pragmas ^0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/nftDropMarket/NFTDropMarketCore.sol#L3

Found usage of floating pragmas ^0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L3

Found usage of floating pragmas ^0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/AdminRole.sol#L3

Found usage of floating pragmas ^0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/MinterRole.sol#L3

Found usage of floating pragmas ^0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/Constants.sol#L3

Found usage of floating pragmas ^0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/ContractFactory.sol#L3

Found usage of floating pragmas ^0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/FETHNode.sol#L3

Found usage of floating pragmas ^0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/FoundationTreasuryNode.sol#L3

Found usage of floating pragmas ^0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/Gap10000.sol#L3

Found usage of floating pragmas ^0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/MarketFees.sol#L3

Found usage of floating pragmas ^0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/MarketSharedCore.sol#L3

Found usage of floating pragmas ^0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/treasury/AdminRole.sol#L3

(6) Constants Should Be Defined Rather Than Using Magic Numbers

Severity: Non-Critical

Proof Of Concept

  if (creatorRecipients.length > 1) {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/MarketFees.sol#L482

(7) Event Is Missing Indexed Fields

Severity: Non-Critical

Each event should use three indexed fields if there are three or more fields.

Proof Of Concept

event URIUpdated(string baseURI, bytes32 postRevealBaseURIHash);

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L85

 event CreateFixedPriceSale(
address indexed nftContract,
address indexed seller,
uint256 price,
uint256 limitPerAccount
  );

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L79

 event BuyReferralPaid(
address indexed nftContract,
uint256 indexed tokenId,
address buyReferrer,
uint256 buyReferrerFee,
uint256 buyReferrerSellerFee
 );

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/MarketFees.sol#L71

(8) Implementation contract may not be initialized

OpenZeppelin recommends that the initializer modifier be applied to constructors.
Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits.
https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/5

Severity: Non-Critical

Proof Of Concept

contract NFTCollection is
  INFTCollectionInitializer,
  IGetRoyalties,
  IGetFees,
  IRoyaltyInfo,
  ITokenCreator,
  ContractFactory,
  Initializable,
  ERC165Upgradeable,
  ERC721Upgradeable,
  ERC721BurnableUpgradeable,
  SequentialMintCollection,
  CollectionRoyalties

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L28

contract NFTCollectionFactory is ICollectionFactory, Initializable, Gap10000 

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L62

contract NFTDropCollection is
  INFTDropCollectionInitializer,
  INFTDropCollectionMint,
  IGetRoyalties,
  IGetFees,
  IRoyaltyInfo,
  ITokenCreator,
  ContractFactory,
  Initializable,
  ContextUpgradeable,
  ERC165Upgradeable,
  AccessControlUpgradeable,
  AdminRole,
  MinterRole,
  ERC721Upgradeable,
  ERC721BurnableUpgradeable,
  SequentialMintCollection,
  CollectionRoyalties

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L28

contract NFTDropMarket is
  Initializable,
  FoundationTreasuryNode,
  FETHNode,
  MarketSharedCore,
  NFTDropMarketCore,
  ReentrancyGuardUpgradeable,
  SendValueWithFallbackWithdraw,
  MarketFees,
  Gap10000,
  NFTDropMarketFixedPriceSale

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropMarket.sol#L63

(9) Use a more recent version of Solidity

Severity: Non-Critical

Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

Proof Of Concept

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L3

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L42

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropMarket.sol#L42

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/AddressLibrary.sol#L3

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/ArrayLibrary.sol#L3

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/BytesLibrary.sol#L3

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/collections/CollectionRoyalties.sol#L3

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/collections/SequentialMintCollection.sol#L3

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/nftDropMarket/NFTDropMarketCore.sol#L3

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L3

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/AdminRole.sol#L3

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/roles/MinterRole.sol#L3

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/Constants.sol#L3

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/ContractFactory.sol#L3

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/FETHNode.sol#L3

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/FoundationTreasuryNode.sol#L3

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/Gap10000.sol#L3

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/MarketFees.sol#L3

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/MarketSharedCore.sol#L3

Found old version 0.8.12 of Solidity
https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/treasury/AdminRole.sol#L3

Recommended Mitigation Steps

Consider updating to a more recent solidity version.

(10) Large multiples of ten should use scientific notation

Severity: Non-Critical

Use (e.g. 1e6) rather than decimal literals (e.g. 100000), for better code readability.

Proof Of Concept

uint256 constant BASIS_POINTS = 10000;

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/Constants.sol#L10

(11) Incomplete NATSPEC documentation

Severity: Non-Critical

Proof Of Concept

• NFTCollection.sol
• NFTCollectionFactory.sol
• NFTDropCollection.sol

(12) Missing NATSPEC documentation

Severity: Non-Critical

Proof Of Concept

function _burn(uint256 tokenId) internal override(ERC721Upgradeable, SequentialMintCollection) {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L255

function _mint(string calldata tokenCID) private onlyCreator returns (uint256 tokenId) {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollection.sol#L262

function _createNFTDropCollection(

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L386

function _getSalt(address creator, uint256 nonce) private pure returns (bytes32) {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L448

function _burn(uint256 tokenId) internal override(ERC721Upgradeable, SequentialMintCollection) {

https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L245

[HardlyDifficult]

(1) Init functions are susceptible to front-running

Invalid: our deploy scripts will initialize when the proxy is first set to this implementation address. e.g. see the logs for our Goerli proxy deployment: https://goerli.etherscan.io/tx/0xe0cea1c4cc1a9d1db9686b2b7c4154cab72f94a6b0823b7d4b9966d1e03cd8e8#eventlog

And the collections are initialized by the factory when they are created.

Use safeMint

Agree will fix - for context see our response here.

Missing Checks for Address(0

Invalid for the contract references since .isContract will revert on address(0).
For the role references, we choose to be consistent with the OZ AccessControl grantRole function which will also be publicly exposed.

(4) Users can call public funcions that create

Invalid - these are meant to be called by any user.

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.

(6) Constants Should Be Defined Rather Than Using Magic Numbers

I don't agree that 1 is a magic number here..

BuyReferralPaid event should index buyReferrer

Agree but won't fix at this time. We have already started emitting this event on mainnet, to simplify integrations such as subgraph we are going to continue with this event as is. However it's a good catch and I agree that buyReferrer ideally would have been indexed here.

For the others I believe this is invalid. index should be reserved for params that are likely to be requested as a filter. In these examples those params are data not really filter candidates. And for the string specifically, indexed would prevent the full information from being communicated, requiring a second unindexed version which is a bit redundant and increases gas costs.

Use constructor to initialize templates

Agree this is a good best practice to add. Will fix.

(9) Use a more recent version of Solidity

We are using the latest, 0.8.16 already

(10) Large multiples of ten should use scientific notation

Fair feedback, we'll consider a change.

Missing natspec comments

Fair feedback -- for natspec we aim for complete coverage of the public interfaces but for internal/private/libraries we have some gaps in order to reduce redundancy, for those we aim to include comments where things are unclear or not obvious from the function and variable names.