Gas Optimizations

[code423n4]

Creating a foundation auction is unnecessarily gas intensive in the current implementation, largely due to inefficient usage of storage slots with the Auction struct.

Currently, each field for address is mixed into a uint256 field requiring each a storage slot for each field.

3 Addresses in the struct can be grouped together and the field-specific initialization syntax can be used to make ordering more logical when constructing the struct.

|  createReserveAuction      ·     202570  ·     263853  ·      260788  ·           20  ·          -  │
|  createReserveAuction      ·     157142  ·     218425  ·      215020  ·           18  ·          -  │

Average gas savings of 45428 when making all time-based variables uint32 and re-ordering the struct from largest to smallest slots.

Code before:

contracts/mixins/NFTMarketReserveAuction.sol:62:

  /// @notice Stores the auction configuration for a specific NFT.
  struct ReserveAuction {
    /// @notice The address of the NFT contract.
    address nftContract;
    /// @notice The id of the NFT.
    uint256 tokenId;
    /// @notice The owner of the NFT which listed it in auction.
    address payable seller;
    /// @notice The duration for this auction.
    uint256 duration;
    /// @notice The extension window for this auction.
    uint256 extensionDuration;
    /// @notice The time at which this auction will not accept any new bids.
    /// @dev This is `0` until the first bid is placed.
    uint256 endTime;
    /// @notice The current highest bidder in this auction.
    /// @dev This is `address(0)` until the first bid is placed.
    address payable bidder;
    /// @notice The latest price of the NFT in this auction.
    /// @dev This is set to the reserve price, and then to the highest bid once the auction has started.
    uint256 amount;
  }

contracts/mixins/NFTMarketReserveAuction.sol:337:

    auctionIdToAuction[auctionId] = ReserveAuction(
      nftContract,
      tokenId,
      payable(msg.sender),
      DURATION,
      EXTENSION_DURATION,
      0, // endTime is only known once the reserve price is met
      payable(0), // bidder is only known once a bid has been placed
      reservePrice
    );

Code after:

contracts/mixins/NFTMarketReserveAuction.sol:61:

  /// @notice Stores the auction configuration for a specific NFT.
  struct ReserveAuction {
    /// @notice The id of the NFT.
    uint256 tokenId;
    /// @notice The latest price of the NFT in this auction.
    /// @dev This is set to the reserve price, and then to the highest bid once the auction has started.
    uint256 amount;
    /// @notice The address of the NFT contract.
    address nftContract;
    /// @notice The current highest bidder in this auction.
    /// @dev This is `address(0)` until the first bid is placed.
    address payable bidder;
    /// @notice The owner of the NFT which listed it in auction.
    address payable seller;
    /// @notice The extension window for this auction.
    uint32 extensionDuration;
    /// @notice The time at which this auction will not accept any new bids.
    /// @dev This is `0` until the first bid is placed.
    uint32 endTime;
    /// @notice The duration for this auction.
    uint32 duration;
  }

contracts/mixins/NFTMarketReserveAuction.sol:337:

    auctionIdToAuction[auctionId] = ReserveAuction({
      tokenId: tokenId,
      nftContract: nftContract,
      amount: reservePrice,
      bidder: payable(0),
      duration: DURATION,
      extensionDuration: EXTENSION_DURATION,
      seller: payable(msg.sender),
      endTime: 0
    });

Gas optimizations for the escrow behavior:
The _transferFromEscrow override pattern checks memory multiple times in a super constructor tree. Given the class construction of these modules, it becomes difficult to optimize or check behavior across multiple classes and states. A more gas-optimized way to approach this could be to have a separate memory struct that references which subclasses to check for any outstanding auction/escrow/buy price in global state (flag for auction set or buy price set at amount etc) then manually execute the needed to code pre-escrow to facilitate the transfer.

currentNFTMarketState[contract][id] = State(struct{type(enum): AUCTION, value: {auctionId, price});

This enum can be updated by added new fields for new market types and incorporating new fields in value to use in look up classes. This reduces reads for base case transfers by 3x by preventing a lookup for each market type and gives a clearer global state of the NFT within the FND market.

[HardlyDifficult]

Great suggestions!

RE Auction optimizations: we had called this out as a known issue and out of scope for the contest. We are planning on making changes similar to what you recommended, but it's a bit non-trivial as we need to preserve upgrade compatibility so not to lose or scramble information about any currently listed NFTs. There is a plan in place and we will tackle this after the upcoming launch.

RE escrow behavior: This is a very interesting suggestion. We do find it compelling to centralize escrow management. This may simplify some of the patterns used, and could potentially save gas as well. We do not plan on implementing this change though as it's a fairly significant change throughout.

[alcueca]

Score: 500 for the thorough and clear argumentation