Gas Optimizations

[code423n4]

Title: Change transferFrom to transfer
Severity: GAS

'transferFrom(address(this), , **)' could be replaced by the following more gas efficient 'transfer(, **)'
This replacement is more gas efficient and improves the code quality.

    NFTMarketCore.sol, 86 : IERC721(nftContract).transferFrom(address(this), recipient, tokenId);
    NFTMarketCore.sol, 97 : IERC721(nftContract).transferFrom(address(this), recipient, tokenId);

Title: Unnecessary cast
Severity: Gas

    address NFTMarketBuyPrice.sol._buy - unnecessary casting address(nftContract)

Title: Unused state variables
Severity: GAS

Unused state variables are gas consuming at deployment (since they are located in storage) and are
a bad code practice. Removing those variables will decrease deployment gas cost and improve code quality.
This is a full list of all the unused storage variables we found in your code base.

    SendValueWithFallbackWithdraw.sol, __gap
    Constants.sol, READ_ONLY_GAS_LIMIT
    Constants.sol, MAX_ROYALTY_RECIPIENTS_INDEX
    Constants.sol, SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS
    AccessControlUpgradeable.sol, __gap
    FoundationTreasuryNode.sol, __gap_was_treasury
    NFTMarketCore.sol, __gap
    NFTMarketPrivateSale.sol, __gap_was_DOMAIN_SEPARATOR
    NFTMarketAuction.sol, __gap
    NFTMarketReserveAuction.sol, __gap_was_config
    CollateralManagement.sol, __gap
    Constants.sol, SEND_VALUE_GAS_LIMIT_SINGLE_RECIPIENT
    NFTMarketFees.sol, __gap_was_fees
    AdminRole.sol, __gap
    Constants.sol, MIN_PERCENT_INCREMENT_IN_BASIS_POINTS
    NFTMarketBuyPrice.sol, __gap

Title: Unused declared local variables
Severity: GAS

Unused local variables are gas consuming, since the initial value assignment costs gas. And are
a bad code practice. Removing those variables will decrease the gas cost and improve code quality.
This is a full list of all the unused storage variables we found in your code base.

    ERC165Checker.sol, supportsERC165Interface, encodedParams

Title: Unused imports
Severity: GAS

In the following files there are contract imports that aren't used
Import of unnecessary files costs deployment gas (and is a bad coding practice that is important to ignore)

    NFTMarketBuyPrice.sol, line 11, import "@openzeppelin/contracts/token/ERC721/IERC721.sol";
    NFTMarketFees.sol, line 12, import "@openzeppelin/contracts/token/ERC721/IERC721.sol";
    NFTMarketBuyPrice.sol, line 5, import "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
    NFTMarketBuyPrice.sol, line 7, import "./Constants.sol";
    NFTMarketReserveAuction.sol, line 14, import "@openzeppelin/contracts/token/ERC721/IERC721.sol";

Title: Use != 0 instead of > 0
Severity: GAS

Using != 0 is slightly cheaper than > 0. (see code-423n4/2021-12-maple-findings#75 for similar issue)

    LockedBalance.sol, 113: change 'balance > 0' to 'balance != 0'

Title: Public functions to external
Severity: GAS

The following functions could be set external to save gas and improve code quality.
External call cost is less expensive than of public functions.

    FoundationTreasuryNode.sol, getFoundationTreasury

Title: Use bytes32 instead of string to save gas whenever possible
Severity: GAS

Use bytes32 instead of string to save gas whenever possible.
String is a dynamic data structure and therefore is more gas consuming then bytes32.


    FETH.sol (L120), string public constant name = "Foundation Wrapped Ether"; 
    NFTMarketPrivateSale.sol (L38), string private constant NAME = "FNDNFTMarket"; 
    FETH.sol (L125), string public constant symbol = "FETH"; 

Title: Internal functions to private
Severity: GAS

The following functions could be set private to save gas and improve code quality:

    AccessControlUpgradeable.sol, grantRole
    AccessControlUpgradeable.sol, getRoleMemberCount
    AccessControlUpgradeable.sol, __AccessControl_init_unchained
    ERC165Checker.sol, supportsERC165
    ERC165Checker.sol, supportsERC165Interface
    LockedBalance.sol, setTotalAmount

Severity: GAS

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)


    AccessControlUpgradeable.sol, getRoleAdmin, { return _roles[role].adminRole; }
    AccessControlUpgradeable.sol, getRoleMemberCount, { return _roles[role].members.length(); }
    AccessControlUpgradeable.sol, getRoleMember, { return _roles[role].members.at(index); }
    AccessControlUpgradeable.sol, hasRole, { return _roles[role].members.contains(account); }

Title: Use calldata instead of memory
Severity: GAS

Use calldata instead of memory for function parameters
In some cases, having function arguments in calldata instead of
memory is more optimal.

    AccountMigrationLibrary.requireAuthorizedAccountMigration (signature)
    NFTMarketReserveAuction.adminAccountMigration (signature)

Title: Unnecessary constructor
Severity: GAS

The following constructors are empty.
(A similar issue code-423n4/2021-11-fei-findings#12)

    FNDNFTMarket.sol.constructor

Title: Use unchecked to save gas for certain additive calculations that cannot overflow
Severity: GAS

You can use unchecked in the following calculations since there is no risk to overflow:

    FETH.sol (L#528) - expiration = lockupDuration + block.timestamp.ceilDiv(lockupInterval) * lockupInterval;
    NFTMarketPrivateSale.sol (L#135) - } else if (deadline > block.timestamp + 2 days) { 
    NFTMarketReserveAuction.sol (L#427) - auction.endTime = block.timestamp + auction.duration;
    FETH.sol (L#718) - if (escrow.expiration >= block.timestamp && escrow.totalAmount > 0) { ++lockedCount;
    NFTMarketReserveAuction.sol (L#452) - auction.endTime = block.timestamp + auction.extensionDuration;

Title: Inline one time use functions
Severity: GAS

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

    FNDNFTMarket.sol, _afterAuctionStarted
    AccessControlUpgradeable.sol, __AccessControl_init_unchained
    FNDNFTMarket.sol, _getSellerFor
    AccountMigrationLibrary.sol, _toAsciiString
    NFTMarketBuyPrice.sol, _afterAuctionStarted
    FNDNFTMarket.sol, _transferToEscrow
    NFTMarketBuyPrice.sol, _getSellerFor
    FNDNFTMarket.sol, _transferFromEscrowIfAvailable
    FNDNFTMarket.sol, _transferFromEscrow
    NFTMarketReserveAuction.sol, _getSellerFor

Title: Unused inheritance
Severity: GAS

Some of your contract inherent contracts but aren't use them at all.
We recommend not to inherent those contracts.

    FNDNFTMarket.sol; the inherited contracts Constants, SendValueWithFallbackWithdraw, NFTMarketFees, NFTMarketReserveAuction, NFTMarketPrivateSale not used
    WithdrawFromEscrow.sol; the inherited contracts AdminRole not used
    FoundationTreasury.sol; the inherited contracts OperatorRole, CollateralManagement, WithdrawFromEscrow not used
    NFTMarketReserveAuction.sol; the inherited contracts FoundationTreasuryNode not used
    CollateralManagement.sol; the inherited contracts AdminRole not used

[HardlyDifficult]

5 changes have been made based on this feedback:

• Remove unnecessary casts: improves readability.
• Removed unused imports: improves readability.
• Use != instead of > 1: improves readability; +/- 20 gas (approx neutral); saved 0.001 KB contract size.
• Use calldata instead of memory: saved ~25 gas on admin calls; saved 0.047 KB contract size.
• Add unchecked: saved ~62 gas on private sales; 0.007 KB contract size.

In total this had a very small impact on gas costs, and ~0.055 KB contract size savings.

The others were reviewed and do not apply and/or do not require changes, either because it does not seem to help or it hurts readability.

[alcueca]

Given that only 5 out of 15 findings were correct, and of those 5 only 3 belonged in the gas report, and the gas savings were minimal, I'm giving a score of zero. The time of the sponsor should be respected.