QA Report

[code423n4]

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

_mint() is discouraged in favour of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. 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.

Instances

contracts/NFTCollection.sol:271 contracts/NFTCollection.sol:182

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

Recommendations:

Use _safeMint() instead of _mint().

---

2. USE OF FLOATING PRAGMA

Recommend using fixed solidity version

Instances

All contracts in scope contains floating pragma: https://github.com/code-423n4/2022-08-foundation#sizes

contracts/NFTDropCollection.sol:3:pragma solidity ^0.8.12;
contracts/FoundationTreasury.sol:48:pragma solidity ^0.8.12;
contracts/mixins/roles/AdminRole.sol:3:pragma solidity ^0.8.12;
contracts/mixins/roles/MinterRole.sol:3:pragma solidity ^0.8.12;
contracts/mixins/shared/Gap10000.sol:3:pragma solidity ^0.8.12;
contracts/mixins/shared/MarketFees.sol:3:pragma solidity ^0.8.12;
contracts/mixins/shared/FETHNode.sol:3:pragma solidity ^0.8.12;
contracts/mixins/shared/Constants.sol:3:pragma solidity ^0.8.12;
contracts/mixins/shared/FoundationTreasuryNode.sol:3:pragma solidity ^0.8.12;
contracts/mixins/shared/MarketSharedCore.sol:3:pragma solidity ^0.8.12;
contracts/mixins/shared/ContractFactory.sol:3:pragma solidity ^0.8.12;
contracts/mixins/nftDropMarket/NFTDropMarketCore.sol:3:pragma solidity ^0.8.12;
contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol:3:pragma solidity ^0.8.12;
contracts/mixins/collections/SequentialMintCollection.sol:3:pragma solidity ^0.8.12;
contracts/mixins/collections/CollectionRoyalties.sol:3:pragma solidity ^0.8.12;
contracts/NFTCollectionFactory.sol:42:pragma solidity ^0.8.12;
contracts/NFTCollection.sol:3:pragma solidity ^0.8.12;
contracts/libraries/ArrayLibrary.sol:3:pragma solidity ^0.8.12;
contracts/libraries/BytesLibrary.sol:3:pragma solidity ^0.8.12;
contracts/libraries/AddressLibrary.sol:3:pragma solidity ^0.8.12;
contracts/NFTDropMarket.sol:42:pragma solidity ^0.8.12;

---

3. Open TODOs

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

Instances:

contracts/mixins/shared/MarketFees.sol:193

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

Recommendations:

Remove the commented code

---

4. Remove commented function or code of no use.

contracts/mixins/shared/MarketFees.sol

35:  /**
36:    * @dev Removing old unused variables in an upgrade safe way. Was:
37:    * uint256 private _primaryFoundationFeeBasisPoints;
38:    * uint256 private _secondaryFoundationFeeBasisPoints;
39:    * uint256 private _secondaryCreatorFeeBasisPoints;
40:    * mapping(address => mapping(uint256 => bool)) private _nftContractToTokenIdToFirstSaleCompleted;
41:     */

References:

https://code4rena.com/reports/2022-05-sturdy#n-12-remove-commented-out-code

[HardlyDifficult]

Use safeMint

Agree will fix - for context see our response here.

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.

Unresolved TODO comments

Agree, will fix.

Remove commented out code

Disagree - this comment is detailing how that awkwardly placed gap was previously used so future me can understand why it's important for upgrade compatibility that we don't leverage that space again (since those slots are dirty with data).