Foundation contest - Dravee's results

QA Report

Table of Contents:

• QA Report
  • Foreword
  • Global remarks
  • File: FoundationTreasury.sol
    • function initialize(address admin)
      • Front-runnable initialize
  • File: NFTMarketCore.sol
    • function _isInActiveAuction()
      • Missing comment @param nftContract
  • File: NFTMarketPrivateSale.sol
    • event PrivateSaleFinalized()
      • Missing comment @param deadline
  • File: NFTMarketReserveAuction.sol
    • function _finalizeReserveAuction()
      • Missing comment @param auctionId
  • Unused named returns

Foreword

• @audit tags

The code is annotated at multiple places with //@audit comments to pinpoint the issues. Please, pay attention to them for more details.

Global remarks

1. FoundationTreasury.sol:initialize(address admin) is front-runnable. Details below.
2. The solution uses custom errors, but the pragma used across all of the solution is ^0.8.0. As custom errors were introduced in 0.8.4, I suggest replacing the pragmas with at least 0.8.4 (and fixing a version before deployment).
3. Several functions are declaring named returns but then are using return statements. I suggest choosing only one for readability reasons. Details below.
4. In this solution, the @param and @return comments are extensively documented on events and functions. However, at some places, @param comments are missing. Details below.

File: FoundationTreasury.sol

function initialize(address admin)

File: FoundationTreasury.sol
55: /**
56:  * @title Manage revenue and roles for Foundation.
57:  * @notice All fees generated by the market are forwarded to this contract.
58:  * It also defines the Admin and Operator roles which are used in other contracts.
59:  */
60: contract FoundationTreasury is AdminRole, OperatorRole, CollateralManagement, WithdrawFromEscrow {
61:   function initialize(address admin) external initializer { //@audit front-runnable & L57 "All fees generated by the market are forwarded to this contract."
62:     AdminRole._initializeAdminRole(admin);
63:   }
64: }

Front-runnable initialize

While this initialized is protected against multiple calls, it doesn't implement any type of access-control. As it is mentioned L57 that "All fees generated by the market are forwarded to this contract.", I suggest adding some form of access-control (as an example: the deployer being the default/initial owner).

File: NFTMarketCore.sol

function _isInActiveAuction()

141:   /**
142:    * @notice Checks if an escrowed NFT is currently in active auction.
143:    * @return Returns false if the auction has ended, even if it has not yet been settled. //@audit missing @param nftContract and @param tokenId 
144:    */
145:   function _isInActiveAuction(address nftContract, uint256 tokenId) internal view virtual returns (bool);

Missing comment @param nftContract

File: NFTMarketPrivateSale.sol

event PrivateSaleFinalized()

40:   /**
41:    * @notice Emitted when an NFT is sold in a private sale.
42:    * @dev The total amount of this sale is `f8nFee` + `creatorFee` + `ownerRev`.
43:    * @param nftContract The address of the NFT contract.
44:    * @param tokenId The ID of the NFT.
45:    * @param seller The address of the seller.
46:    * @param buyer The address of the buyer.
47:    * @param f8nFee The amount of ETH that was sent to Foundation for this sale.
48:    * @param creatorFee The amount of ETH that was sent to the creator for this sale.
49:    * @param ownerRev The amount of ETH that was sent to the owner for this sale. //@audit missing @param deadline
50:    */
51:   event PrivateSaleFinalized(
52:     address indexed nftContract,
53:     uint256 indexed tokenId,
54:     address indexed seller,
55:     address buyer,
56:     uint256 f8nFee,
57:     uint256 creatorFee,
58:     uint256 ownerRev,
59:     uint256 deadline
60:   );

Missing comment @param deadline

File: NFTMarketReserveAuction.sol

function _finalizeReserveAuction()

483:   /**
484:    * @notice Settle an auction that has already ended.
485:    * This will send the NFT to the highest bidder and distribute revenue for this sale.
486:    * @param keepInEscrow If true, the NFT will be kept in escrow to save gas by avoiding //@audit missing @param auctionId
487:    * redundant transfers if the NFT should remain in escrow, such as when the new owner
488:    * sets a buy price or lists it in a new auction.
489:    */
490:   function _finalizeReserveAuction(uint256 auctionId, bool keepInEscrow) private {

Missing comment @param auctionId

Unused named returns

Several functions are declaring named returns but then are using return statements. I suggest choosing only one for readability reasons.

Places declaring a named returns but using a return statement:

contracts/FETH.sol:
  212:   function approve(address spender, uint256 amount) external returns (bool success) {
  215:     return true;

  266:   ) external payable onlyFoundationMarket returns (uint256 expiration) {
  268:     return _marketLockupFor(lockupFor, lockupAmount);

  284:     returns (uint256 expiration)
  286:     return _marketLockupFor(account, amount);

  356:   function transfer(address to, uint256 amount) external returns (bool success) {
  357:     return transferFrom(msg.sender, to, amount);
  
  371:   ) public returns (bool success) {
  391:     return true;

  470:   function _freeFromEscrow(address account) private returns (AccountInfo storage) {
  477:       return accountInfo;
  510:     return accountInfo;

  661:   function allowance(address account, address operator) external view returns (uint256 amount) {
  663:     return accountInfo.allowance[operator];

  693:   function getFoundationMarket() external view returns (address market) {
  694:     return foundationMarket;

  774:   function totalSupply() external view returns (uint256 supply) {
  777:     return address(this).balance;

contracts/FNDNFTMarket.sol:
  164:     returns (address payable seller)
  166:     return super._getSellerFor(nftContract, tokenId);

contracts/libraries/AccountMigrationLibrary.sol:
  68:   function _char(bytes1 b) private pure returns (bytes1 c) {
  70:       if (uint8(b) < 10) return bytes1(uint8(b) + 0x30);
  71:       else return bytes1(uint8(b) + 0x57);

contracts/mixins/FoundationTreasuryNode.sol:
  60:   function getFoundationTreasury() public view returns (address payable treasuryAddress) {
  61:     return treasury;

contracts/mixins/NFTMarketBuyPrice.sol:
  337:   function getBuyPrice(address nftContract, uint256 tokenId) external view returns (address seller, uint256 price) {
  340:       return (address(0), type(uint256).max);
  342:     return (buyPrice.seller, buyPrice.price);

contracts/mixins/NFTMarketCore.sol:
  112:   function getFethAddress() external view returns (address fethAddress) {
  113:     return address(feth);

contracts/mixins/NFTMarketCreators.sol:

  257:   function getRoyaltyRegistry() public view returns (address registry) {
  258:     return address(royaltyRegistry);

contracts/mixins/NFTMarketOffer.sol:
  358:   function getMinOfferAmount(address nftContract, uint256 tokenId) external view returns (uint256 minimum) {
  361:       return _getMinIncrement(offer.amount);
  364:     return 1;

  382:     returns (
  383:      address buyer,
  384:      uint256 expiration,
  385:      uint256 amount
  391:       return (address(0), 0, 0);
  395:     return (offer.buyer, offer.expiration, offer.amount);

contracts/mixins/NFTMarketReserveAuction.sol:
  617:   function getMinBidAmount(uint256 auctionId) external view returns (uint256 minimum) {
  620:       return auction.amount;
  622:     return _getMinIncrement(auction.amount);
  
  630:   function getReserveAuction(uint256 auctionId) external view returns (ReserveAuction memory auction) {
  631:     return auctionIdToAuction[auctionId];
  
  641:   function getReserveAuctionIdFor(address nftContract, uint256 tokenId) external view returns (uint256 auctionId) {
  642:     return nftContractToTokenIdToAuctionId[nftContract][tokenId];

contracts/mixins/SendValueWithFallbackWithdraw.sol:
  84:   function getPendingWithdrawal(address user) external view returns (uint256 balance) {
  85:     return pendingWithdrawals[user];

Gas Report

Table of Contents:

• Gas Report
  • Foreword
  • File: FETH.sol
    • function _deductAllowanceFrom()
      • Cache accountInfo.allowance[msg.sender]
    • function _deductBalanceFrom()
      • Cache accountInfo.freedBalance
    • function _marketLockupFor()
      • Cache accountInfo.freedBalance or call _deductBalanceFrom to save gas while saving some size
  • File: NFTMarketBuyPrice.sol
    • function cancelBuyPrice()
      • Cache buyPrice.seller
    • function setBuyPrice()
      • Cache buyPrice.seller
    • function _transferFromEscrow()
      • Cache buyPrice.seller
    • function _transferToEscrow()
      • Cache buyPrice.seller
    • function getBuyPrice()
      • Cache buyPrice.seller
  • File: NFTMarketFees.sol
    • function _distributeFunds()
      • Uncheck line L76
  • File: NFTMarketOffer.sol
    • function makeOffer()
      • Cache offer.expiration
      • Cache offer.amount
    • function getOffer()
      • Cache offer.expiration
  • File: NFTMarketReserveAuction.sol
    • function adminAccountMigration()
    • Use calldata instead of memory for external functions where the function argument is read-only
      • Cache auction.seller
    • function placeBidOf()
      • Cache auction.endTime
      • Cache auction.extensionDuration
      • Cache auction.bidder
      • Cache auction.amount
  • General recommendations
    • For-Loops
      • An array's length should be cached to save gas in for-loops
      • Increments can be unchecked

Foreword

• Storage-reading optimizations

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas). In the paragraphs below, please see the @audit-issue tags in the pieces of code's comments for more information about SLOADs that could be saved by caching the mentioned storage variables in memory variables.

• Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation, or the operation doesn't depend on user input), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

• @audit tags

The code is annotated at multiple places with //@audit comments to pinpoint the issues. Please, pay attention to them for more details.

File: FETH.sol

function _deductAllowanceFrom()

File: FETH.sol
441:   function _deductAllowanceFrom(AccountInfo storage accountInfo, uint256 amount) private {
442:     if (accountInfo.allowance[msg.sender] != type(uint256).max) { //@audit accountInfo.allowance[msg.sender] SLOAD 1
443:       if (accountInfo.allowance[msg.sender] < amount) { //@audit accountInfo.allowance[msg.sender] SLOAD 2
444:         revert FETH_Insufficient_Allowance(accountInfo.allowance[msg.sender]); //@audit accountInfo.allowance[msg.sender] SLOAD 3
445:       }
446:       // The check above ensures allowance cannot underflow.
447:       unchecked {
448:         accountInfo.allowance[msg.sender] -= amount; //@audit accountInfo.allowance[msg.sender] SLOAD 3
449:       }
450:     }
451:   }

Cache accountInfo.allowance[msg.sender]

Caching this in memory can save around 2 SLOADs (around 200 gas). This is due to the fact that both conditions will get evaluated before L448, which is using -= (therefore making a SLOAD + SSTORE) I recommend caching this value and using it as such:

  function _deductAllowanceFrom(AccountInfo storage accountInfo, uint256 amount) private {
    uint256 _allowance = accountInfo.allowance[msg.sender]; //@audit 1 MSTORE + 1 SLOAD
    if (_allowance != type(uint256).max) { //@audit 1 MLOAD spent
      if (_allowance < amount) { //@audit 1 SLOAD saved, 1 MLOAD spent
        revert FETH_Insufficient_Allowance(_allowance); //@audit 1 SLOAD saved, 1 MLOAD spent
      }
      // The check above ensures allowance cannot underflow.
      unchecked {
        accountInfo.allowance[msg.sender] = _allowance - amount; //@audit 1 SLOAD saved, 1 MLOAD spent
      }
    }
  }

function _deductBalanceFrom()

File: FETH.sol
456:   function _deductBalanceFrom(AccountInfo storage accountInfo, uint256 amount) private {
457:     // Free from escrow in order to consider any expired escrow balance
458:     if (accountInfo.freedBalance < amount) { //@audit accountInfo.freedBalance SLOAD 1
459:       revert FETH_Insufficient_Available_Funds(accountInfo.freedBalance); //@audit accountInfo.freedBalance SLOAD 2
460:     }
461:     // The check above ensures balance cannot underflow.
462:     unchecked {
463:       accountInfo.freedBalance -= uint96(amount); //@audit accountInfo.freedBalance SLOAD 2
464:     }
465:   }

Cache accountInfo.freedBalance

Caching this in memory can save around 1 SLOAD (around 100 gas). I recommend caching this value and using it as such:

  function _deductBalanceFrom(AccountInfo storage accountInfo, uint256 amount) private {
    uint256 _freedBalance = accountInfo.freedBalance; //@audit 1 MSTORE + 1 SLOAD
    // Free from escrow in order to consider any expired escrow balance
    if (_freedBalance < amount) { ///@audit 1 MLOAD spent
      revert FETH_Insufficient_Available_Funds(_freedBalance); //@audit 1 SLOAD saved, 1 MLOAD spent
    }
    // The check above ensures balance cannot underflow.
    unchecked {
      accountInfo.freedBalance = _freedBalance - uint96(amount); //@audit 1 SLOAD saved, 1 MLOAD spent
    }
  }

function _marketLockupFor()

Cache accountInfo.freedBalance or call _deductBalanceFrom to save gas while saving some size

Impacted code:

File: FETH.sol
535:       // The check above prevents underflow with delta.
536:       unchecked {
537:         uint256 delta = amount - msg.value; //@audit just use / should call private _deductBalanceFrom(accountInfo, amount - msg.value); : 6.833 vs 6.894
538:         if (accountInfo.freedBalance < delta) { //@audit accountInfo.freedBalance SLOAD 1
539:           revert FETH_Insufficient_Available_Funds(accountInfo.freedBalance); //@audit accountInfo.freedBalance SLOAD 2
540:         }
541:         // The check above prevents underflow of freed balance.
542:         accountInfo.freedBalance -= uint96(delta); //@audit accountInfo.freedBalance SLOAD 2
543:       }

The optimization by caching accountInfo.freedBalance would be exactly the same as above, which would save around 100 gas. However, here, it'd be better to actually call the optimized _deductBalanceFrom to benefit from the previous gas-gains and reduce the contract's size (0.061KB saved).

The code should become:

File: FETH.sol
534:     if (msg.value < amount) {
535:       _deductBalanceFrom(accountInfo, amount - msg.value);
536:     } else if (msg.value != amount) {
537:       // There's no reason to send msg.value more than the amount being locked up
538:       revert FETH_Too_Much_ETH_Provided();
539:     }

File: NFTMarketBuyPrice.sol

function cancelBuyPrice()

File: NFTMarketBuyPrice.sol
125:   function cancelBuyPrice(address nftContract, uint256 tokenId) external nonReentrant {
126:     BuyPrice storage buyPrice = nftContractToTokenIdToBuyPrice[nftContract][tokenId];
127:     if (buyPrice.seller == address(0)) { //@audit buyPrice.seller SLOAD 1
128:       // This check is redundant with the next one, but done in order to provide a more clear error message.
129:       revert NFTMarketBuyPrice_Cannot_Cancel_Unset_Price();
130:     } else if (buyPrice.seller != msg.sender) { //@audit buyPrice.seller SLOAD 2 (evaluated even if false)
131:       revert NFTMarketBuyPrice_Only_Owner_Can_Cancel_Price(buyPrice.seller); //@audit buyPrice.seller SLOAD 3
132:     }
...
141:   }

Cache buyPrice.seller

Caching this in memory can save around 2 SLOADs (around 200 gas).

function setBuyPrice()

File: NFTMarketBuyPrice.sol
150:   function setBuyPrice(
...
165:     BuyPrice storage buyPrice = nftContractToTokenIdToBuyPrice[nftContract][tokenId];
...
170:     if (buyPrice.seller == address(0)) { //@audit buyPrice.seller SLOAD 1
171:       // Transfer the NFT into escrow, if it's already in escrow confirm the `msg.sender` is the owner.
172:       _transferToEscrow(nftContract, tokenId);
173: 
174:       // The price was not previously set for this NFT, store the seller.
175:       buyPrice.seller = payable(msg.sender); 
176:     } else if (buyPrice.seller != msg.sender) { //@audit buyPrice.seller SLOAD 2 (evaluated even if false)
177:       // Buy price was previously set by a different user
178:       revert NFTMarketBuyPrice_Only_Owner_Can_Set_Price(buyPrice.seller); //@audit buyPrice.seller SLOAD 3
179:     }
...
182:   }

Cache buyPrice.seller

Caching this in memory can save around 2 SLOADs (around 200 gas).

function _transferFromEscrow()

File: NFTMarketBuyPrice.sol
276:   function _transferFromEscrow(
...
282:     BuyPrice storage buyPrice = nftContractToTokenIdToBuyPrice[nftContract][tokenId];
283:     if (buyPrice.seller != address(0)) { //@audit buyPrice.seller SLOAD 1
284:       // A buy price was set for this NFT.
285:       if (buyPrice.seller != seller) { //@audit buyPrice.seller SLOAD 2
286:         // When there is a buy price set, the `buyPrice.seller` is the owner of the NFT.
287:         revert NFTMarketBuyPrice_Seller_Mismatch(buyPrice.seller); //@audit buyPrice.seller SLOAD 3
288:       }
...
294:   }

Cache buyPrice.seller

Caching this in memory can save around 2 SLOADs (around 200 gas).

function _transferToEscrow()

File: NFTMarketBuyPrice.sol
316:   function _transferToEscrow(address nftContract, uint256 tokenId) internal virtual override {
317:     BuyPrice storage buyPrice = nftContractToTokenIdToBuyPrice[nftContract][tokenId];
318:     if (buyPrice.seller == address(0)) { //@audit buyPrice.seller SLOAD 1
319:       // The NFT is not in escrow for buy now.
320:       super._transferToEscrow(nftContract, tokenId); 
321:     } else if (buyPrice.seller != msg.sender) { //@audit buyPrice.seller SLOAD 2
322:       // When there is a buy price set, the `buyPrice.seller` is the owner of the NFT.
323:       revert NFTMarketBuyPrice_Seller_Mismatch(buyPrice.seller); //@audit buyPrice.seller SLOAD 3
324:     }
325:   }

Cache buyPrice.seller

Caching this in memory can save around 2 SLOADs (around 200 gas).

function getBuyPrice()

File: NFTMarketBuyPrice.sol
337:   function getBuyPrice(address nftContract, uint256 tokenId) external view returns (address seller, uint256 price) {
338:     BuyPrice storage buyPrice = nftContractToTokenIdToBuyPrice[nftContract][tokenId];
339:     if (buyPrice.seller == address(0)) { //@audit buyPrice.seller SLOAD 1
340:       return (address(0), type(uint256).max);
341:     }
342:     return (buyPrice.seller, buyPrice.price); //@audit buyPrice.seller SLOAD 2
343:   }

Cache buyPrice.seller

Caching this in memory can save around 1 SLOAD (around 100 gas).

File: NFTMarketFees.sol

function _distributeFunds()

File: NFTMarketFees.sol
48:   function _distributeFunds(
...
74:     if (creatorFee > 0) {
75:       if (creatorRecipients.length > 1) {
76:         uint256 maxCreatorIndex = creatorRecipients.length - 1; //@audit should be unchecked too (see L75)
77:         if (maxCreatorIndex > MAX_ROYALTY_RECIPIENTS_INDEX) {
78:           maxCreatorIndex = MAX_ROYALTY_RECIPIENTS_INDEX;
79:         }
80: 
81:         // Determine the total shares defined so it can be leveraged to distribute below
82:         uint256 totalShares;
83:         unchecked {
84:           // The array length cannot overflow 256 bits.
85:           for (uint256 i; i <= maxCreatorIndex; ++i) {
86:             if (creatorShares[i] > BASIS_POINTS) {
87:               // If the numbers are >100% we ignore the fee recipients and pay just the first instead
88:               maxCreatorIndex = 0;
89:               break;
90:             }
91:             // The check above ensures totalShares wont overflow.
92:             totalShares += creatorShares[i];
93:           }
94:         }

Uncheck line L76

This line can't underflow due to L75. Therefore, it should be wrapped in an unchecked block. I'd suggest starting the unchecked block L83 at line 76.

File: NFTMarketOffer.sol

function makeOffer()

File: NFTMarketOffer.sol
189:   function makeOffer(
...
204:     Offer storage offer = nftContractToIdToOffer[nftContract][tokenId];
205: 
206:     if (offer.expiration < block.timestamp) { //@audit offer.expiration SLOAD 1
...
211:     } else {
...
214:       if (amount < _getMinIncrement(offer.amount)) { //@audit offer.amount SLOAD 1
215:         // A non-trivial increase in price is required to avoid sniping
216:         revert NFTMarketOffer_Offer_Must_Be_At_Least_Min_Amount(_getMinIncrement(offer.amount)); //@audit offer.amount SLOAD 2
217:       }
...
221:       expiration = feth.marketChangeLockup{ value: msg.value }(
222:         offer.buyer,
223:         offer.expiration, //@audit offer.expiration SLOAD 2
224:         offer.amount, //@audit offer.amount SLOAD 2
225:         msg.sender,
226:         amount
227:       );
228:     }

Cache offer.expiration

Caching this in memory can save around 1 SLOAD (around 100 gas).

Cache offer.amount

Caching this in memory can save around 1 SLOAD (around 100 gas).

function getOffer()

File: NFTMarketOffer.sol
379:   function getOffer(address nftContract, uint256 tokenId)
...
388:     Offer storage offer = nftContractToIdToOffer[nftContract][tokenId];
389:     if (offer.expiration < block.timestamp) { //@audit offer.expiration SLOAD 1
390:       // Offer not found or has expired
391:       return (address(0), 0, 0);
392:     }
393: 
394:     // An offer was found and it has not yet expired.
395:     return (offer.buyer, offer.expiration, offer.amount); //@audit offer.expiration SLOAD 2
396:   }

Cache offer.expiration

Caching this in memory can save around 1 SLOAD (around 100 gas).

File: NFTMarketReserveAuction.sol

function adminAccountMigration()

File: NFTMarketReserveAuction.sol
263:   function adminAccountMigration(
264:     uint256[] calldata listedAuctionIds,
265:     address originalAddress,
266:     address payable newAddress,
267:     bytes memory signature //@audit gas should be calldata
268:   ) external onlyFoundationOperator { 
269:     // Validate the owner of the original account has approved this change.
270:     originalAddress.requireAuthorizedAccountMigration(newAddress, signature);
271: 
272:     unchecked {
273:       // The array length cannot overflow 256 bits.
274:       for (uint256 i; i < listedAuctionIds.length; ++i) {
275:         uint256 auctionId = listedAuctionIds[i];
276:         ReserveAuction storage auction = auctionIdToAuction[auctionId];
277:         if (auction.seller != address(0)) { //@audit auction.seller SLOAD 1
278:           // Only if the auction was found and not finalized before this transaction.
279: 
280:           if (auction.seller != originalAddress) { //@audit auction.seller SLOAD 2
281:             // Confirm that the signature approval was the correct owner of this auction.
282:             revert NFTMarketReserveAuction_Cannot_Migrate_Non_Matching_Seller(auction.seller); //@audit auction.seller SLOAD 3
283:           }
284: 
285:           // Update the auction's seller address.
286:           auction.seller = newAddress;
287: 
288:           emit ReserveAuctionSellerMigrated(auctionId, originalAddress, newAddress);
289:         }
290:       }
291:     }
292:   }

Use calldata instead of memory for external functions where the function argument is read-only

Here, bytes memory signature should be bytes calldata signature

Cache auction.seller

Caching this in memory can save around 2 SLOADs (around 200 gas).

function placeBidOf()

File: NFTMarketReserveAuction.sol
386:   function placeBidOf(uint256 auctionId, uint256 amount) public payable nonReentrant {
...
402:     ReserveAuction storage auction = auctionIdToAuction[auctionId];
403: 
404:     if (auction.amount == 0) { //@audit auction.amount SLOAD 1
405:       // No auction found
406:       revert NFTMarketReserveAuction_Cannot_Bid_On_Nonexistent_Auction();
407:     }
408: 
409:     if (auction.endTime == 0) { //@audit auction.endTime SLOAD 1
410:       // This is the first bid, kicking off the auction.
411: 
412:       if (auction.amount > amount) { //@audit auction.amount SLOAD 2
413:         // The bid must be >= the reserve price.
414:         revert NFTMarketReserveAuction_Cannot_Bid_Lower_Than_Reserve_Price(auction.amount); //@audit auction.amount SLOAD 3
415:       }
...
429:     } else {
430:       if (auction.endTime < block.timestamp) { //@audit auction.endTime SLOAD 2
431:         // The auction has already ended.
432:         revert NFTMarketReserveAuction_Cannot_Bid_On_Ended_Auction(auction.endTime); //@audit auction.endTime SLOAD 3
433:       } else if (auction.bidder == msg.sender) { //@audit auction.bidder SLOAD 1
434:         // We currently do not allow a bidder to increase their bid unless another user has outbid them first.
435:         revert NFTMarketReserveAuction_Cannot_Rebid_Over_Outstanding_Bid();
436:       } else if (amount < _getMinIncrement(auction.amount)) { //@audit auction.amount SLOAD 2
437:         // If this bid outbids another, it must be at least 10% greater than the last bid.
438:         revert NFTMarketReserveAuction_Bid_Must_Be_At_Least_Min_Amount(_getMinIncrement(auction.amount)); //@audit auction.amount SLOAD 3
439:       }
440: 
441:       // Cache and update bidder state
442:       uint256 originalAmount = auction.amount; //@audit auction.amount SLOAD 3
443:       address payable originalBidder = auction.bidder; //@audit auction.bidder SLOAD 2
444:       auction.amount = amount;
445:       auction.bidder = payable(msg.sender);
446: 
447:       unchecked {
448:         // When a bid outbids another, check to see if a time extension should apply.
449:         // We confirmed that the auction has not ended, so endTime is always >= the current timestamp.
450:         if (auction.endTime - block.timestamp < auction.extensionDuration) { //@audit auction.endTime SLOAD 3 //@audit auction.extensionDuration SLOAD 1
451:           // Current time plus extension duration (always 15 mins) cannot overflow.
452:           auction.endTime = block.timestamp + auction.extensionDuration; //@audit auction.extensionDuration SLOAD 2
453:         }
454:       }
455: 
456:       // Refund the previous bidder
457:       _sendValueWithFallbackWithdraw(originalBidder, originalAmount, SEND_VALUE_GAS_LIMIT_SINGLE_RECIPIENT);
458:     }
459: 
460:     emit ReserveAuctionBidPlaced(auctionId, msg.sender, amount, auction.endTime); //@audit auction.endTime SLOAD 4
461:   }

Cache auction.endTime

Caching this in memory can save around 3 SLOADs (around 300 gas).

Cache auction.extensionDuration

Caching this in memory can save around 1 SLOAD (around 100 gas).

Cache auction.bidder

Caching this in memory can save around 1 SLOAD (around 100 gas).

Cache auction.amount

Caching this in memory can save around 2 SLOADs (around 200 gas).

General recommendations

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) in the stack.
Caching the array length in the stack saves around 3 gas per iteration. Therefore, it's possible to save a significant amount of gas (>= 102 after 34 iterations).

Here, I suggest storing the array's length in a variable before the for-loop, and use it instead:

mixins/OZ/ERC165Checker.sol:64:        for (uint256 i = 0; i < interfaceIds.length; ++i) {
mixins/OZ/ERC165Checker.sol:90:      for (uint256 i = 0; i < interfaceIds.length; ++i) {
mixins/NFTMarketCreators.sol:94:            for (uint256 i = 0; i < _recipients.length; ++i) {
mixins/NFTMarketCreators.sol:155:                for (uint256 i = 0; i < _recipients.length; ++i) {
mixins/NFTMarketCreators.sol:193:                for (uint256 i = 0; i < _recipients.length; ++i) {
mixins/NFTMarketOffer.sol:161:      for (uint256 i = 0; i < nftContracts.length; ++i) {
mixins/NFTMarketReserveAuction.sol:274:      for (uint256 i = 0; i < listedAuctionIds.length; ++i) {

Increments can be unchecked

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save a significant amount of gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Instances include:

mixins/OZ/ERC165Checker.sol:64:        for (uint256 i = 0; i < interfaceIds.length; ++i) {
mixins/OZ/ERC165Checker.sol:90:      for (uint256 i = 0; i < interfaceIds.length; ++i) {
mixins/NFTMarketCreators.sol:94:            for (uint256 i = 0; i < _recipients.length; ++i) {
mixins/NFTMarketCreators.sol:155:                for (uint256 i = 0; i < _recipients.length; ++i) {
mixins/NFTMarketCreators.sol:193:                for (uint256 i = 0; i < _recipients.length; ++i) {
mixins/NFTMarketOffer.sol:161:      for (uint256 i = 0; i < nftContracts.length; ++i) {
mixins/NFTMarketReserveAuction.sol:274:      for (uint256 i = 0; i < listedAuctionIds.length; ++i) {
FETH.sol:552:      for (uint256 escrowIndex = accountInfo.lockupStartIndex; ; ++escrowIndex) {
FETH.sol:679:      for (uint256 escrowIndex = accountInfo.lockupStartIndex; ; ++escrowIndex) {
FETH.sol:713:      for (uint256 escrowIndex = accountInfo.lockupStartIndex; ; ++escrowIndex) {
FETH.sol:733:      for (uint256 escrowIndex = accountInfo.lockupStartIndex; ; ++escrowIndex) {
FETH.sol:760:      for (uint256 escrowIndex = accountInfo.lockupStartIndex; ; ++escrowIndex) {

The code would go from:

for (uint256 i; i < numIterations; ++i) {  
 // ...  
}  

to:

for (uint256 i; i < numIterations;) {  
 // ...  
 unchecked { ++i; }  
}  

The risk of overflow is inexistant for a uint256 here.