Gas Optimizations

[code423n4]

[G-01] Upgrade pragma to 0.8.15 to save gas

Across the whole solution, the declared pragma is 0.8.12 Upgrading to 0.8.15 has many benefits, cheaper on gas is one of them. The following information is regarding 0.8.15 over 0.8.14. Assume that over 0.8.12 the save is higher!

According to the release note of 0.8.15: https://blog.soliditylang.org/2022/06/15/solidity-0.8.15-release-announcement/
The benchmark shows saving of 2.5-10% Bytecode size,
Saving 2-8% Deployment gas,
And saving up to 6.2% Runtime gas.

---

[G-02] An arrays length should be cached to save gas in for-loops

An array’s length should be cached to save gas in for-loops Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset). Caching the array length in the stack saves around 3 gas per iteration.

I've found 4 locations in 1 file:

contracts/mixins/shared/MarketFees.sol:
  125      unchecked {
  126:       for (uint256 i = 0; i < creatorRecipients.length; ++i) {
  127          _sendValueWithFallbackWithdraw(

  197      // Sum the total creator rev from shares
  198:     for (uint256 i = 0; i < creatorShares.length; ++i) {
  199        creatorRev += creatorShares[i];

  483          unchecked {
  484:           for (uint256 i = 0; i < creatorRecipients.length; ++i) {
  485              if (creatorShares[i] > BASIS_POINTS) {

  502        uint256 totalRoyaltiesDistributed;
  503:       for (uint256 i = 1; i < creatorRecipients.length; ) {
  504          uint256 royalty = (creatorRev * creatorShares[i]) / totalShares;

---

[G-03] Using default values is cheaper than assignment

If a variable is not set/initialized, it is assumed to have the default value 0 for uint, and false for boolean. Explicitly initializing it with its default value is an anti-pattern and wastes gas. For example: uint8 i = 0; should be replaced with uint8 i;

I've found 5 locations in 2 files:

contracts/libraries/BytesLibrary.sol:
  24        // An address is 20 bytes long
  25:       for (uint256 i = 0; i < 20; ++i) {
  26          uint256 dataLocation = startLocation + i;

  43      unchecked {
  44:       for (uint256 i = 0; i < 4; ++i) {
  45          if (callData[i] != functionSig[i]) {

contracts/mixins/shared/MarketFees.sol:
  125      unchecked {
  126:       for (uint256 i = 0; i < creatorRecipients.length; ++i) {
  127          _sendValueWithFallbackWithdraw(

  197      // Sum the total creator rev from shares
  198:     for (uint256 i = 0; i < creatorShares.length; ++i) {
  199        creatorRev += creatorShares[i];

  483          unchecked {
  484:           for (uint256 i = 0; i < creatorRecipients.length; ++i) {
  485              if (creatorShares[i] > BASIS_POINTS) {

---

[G-04] != 0 is cheaper than > 0

!= 0 costs less gas compared to > 0 for unsigned integers even when optimizer enabled All of the following findings are uint (E&OE) so >0 and != have exactly the same effect. saves 6 gas each

I've found 4 locations in 2 files:

contracts/NFTDropCollection.sol:
   87    modifier validBaseURI(string calldata _baseURI) {
   88:     require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
   89      _;

  129    ) external initializer onlyContractFactory validBaseURI(_baseURI) {
  130:     require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");
  131:     require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");
  132  

contracts/mixins/shared/MarketFees.sol:
  243          // - so ignore results when the amount is 0
  244:         if (royaltyAmount > 0) {
  245            recipients = new address payable[](1);

---

[G-05] Custom errors save gas

From solidity 0.8.4 and above, Custom errors from are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) Source: https://blog.soliditylang.org/2021/04/21/custom-errors/: Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

I've found 29 locations in 9 files:

contracts/NFTCollection.sol:
  157    {
  158:     require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");
  159      tokenId = _mint(tokenCID);

  262    function _mint(string calldata tokenCID) private onlyCreator returns (uint256 tokenId) {
  263:     require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required");
  264:     require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");
  265      unchecked {

  267        tokenId = ++latestTokenId;
  268:       require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");
  269        cidToMinted[tokenCID] = true;

  326    function tokenURI(uint256 tokenId) public view override returns (string memory uri) {
  327:     require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");
  328  

contracts/NFTCollectionFactory.sol:
  172    modifier onlyAdmin() {
  173:     require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");
  174      _;

  181    constructor(address _rolesContract) {
  182:     require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract");
  183  

  202    function adminUpdateNFTCollectionImplementation(address _implementation) external onlyAdmin {
  203:     require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
  204      implementationNFTCollection = _implementation;

  226    function adminUpdateNFTDropCollectionImplementation(address _implementation) external onlyAdmin {
  227:     require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
  228      implementationNFTDropCollection = _implementation;

  261    ) external returns (address collection) {
  262:     require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");
  263  

contracts/NFTDropCollection.sol:
   87    modifier validBaseURI(string calldata _baseURI) {
   88:     require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
   89      _;

   92    modifier onlyWhileUnrevealed() {
   93:     require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");
   94      _;

  129    ) external initializer onlyContractFactory validBaseURI(_baseURI) {
  130:     require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");
  131:     require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");
  132  

  171    function mintCountTo(uint16 count, address to) external onlyMinterOrAdmin returns (uint256 firstTokenId) {
  172:     require(count != 0, "NFTDropCollection: `count` must be greater than 0");
  173  

  178      latestTokenId = latestTokenId + count;
  179:     require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");
  180  

  237    {
  238:     require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");
  239  

contracts/libraries/AddressLibrary.sol:
  30      contractAddress = abi.decode(returnData, (address));
  31:     require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");
  32    }

contracts/mixins/collections/SequentialMintCollection.sol:
  57    modifier onlyCreator() {
  58:     require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");
  59      _;

  62    function _initializeSequentialMintCollection(address payable _creator, uint32 _maxTokenId) internal onlyInitializing {
  63:     require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");
  64  

  73    function _selfDestruct() internal {
  74:     require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");
  75  

  86    function _updateMaxTokenId(uint32 _maxTokenId) internal {
  87:     require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");
  88:     require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");
  89:     require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");
  90  

contracts/mixins/roles/AdminRole.sol:
  18    modifier onlyAdmin() {
  19:     require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");
  20      _;

contracts/mixins/roles/MinterRole.sol:
  21    modifier onlyMinterOrAdmin() {
  22:     require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");
  23      _;

contracts/mixins/shared/ContractFactory.sol:
  21    modifier onlyContractFactory() {
  22:     require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");
  23      _;

  30    constructor(address _contractFactory) {
  31:     require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");
  32      contractFactory = _contractFactory;

contracts/mixins/treasury/AdminRole.sol:
  19    modifier onlyAdmin() {
  20:     require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");
  21      _;

---

Note: Could not find any improvments for i++ or for immutables(constructor) - good job devs!

[HardlyDifficult]

[G-01] Upgrade pragma to 0.8.15 to save gas

Invalid - we are compiling with 0.8.16, see the hardhat config. We do use a floating pragma in code is all.

Cache Array Length Outside of Loop

May be theoretically valid, but won't fix. I tested this: gas-reporter and our gas-stories suite is reporting a small regression using this technique. It also hurts readability a bit so we wouldn't want to include it unless it was a clear win.

Don't initialize variables with default values.

Invalid. This optimization technique is no longer applicable with the current version of Solidity.

Use != 0 instead of > 0

Invalid. We tested the recommendation and got the following results:

createNFTDropCollection gas reporter results:
  using > 0 (current):
    - 319246  ·     319578  ·      319361
  using != 0 (recommendation):
    -  319252  ·     319584  ·      319367
  impact: +6 gas

Custom errors

Agree but won't fix at this time. We use these in the market but not in collections. Unfortunately custom errors are still not as good of an experience for users (e.g. on etherscan). We used them in the market originally because we were nearing the max contract size limit and this was a good way to reduce the bytecode. We'll consider this in the future as tooling continues to improve.