Gas Optimizations #63

code423n4 · 2022-03-02T20:27:05Z

Gas Report

Table of Contents:

Gas Report

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.

The text was updated successfully, but these errors were encountered:

HardlyDifficult · 2022-03-10T13:08:29Z

This was a very detailed and helpful gas report! I love that you commented the code with the @Audit tags and detailed exactly how much savings could be made and why. This was very useful when evaluating the recommendations.

We have implemented many of the suggestions here. Some we choose not to change in order to preserve readability. Generally the savings is inline with the expected values you have stated here.

alcueca · 2022-03-17T15:34:12Z

Great report. Score: 4300 (including +20% from formatting)

code423n4 added bug Something isn't working G (Gas Optimization) labels Mar 2, 2022

code423n4 added a commit that referenced this issue Mar 2, 2022

Dravee issue #63

aba558b

IllIllI000 mentioned this issue Jun 4, 2022

How are Gas/QA reports ranked? code-423n4/org#21

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gas Optimizations #63

Gas Optimizations #63

code423n4 commented Mar 2, 2022

HardlyDifficult commented Mar 10, 2022

alcueca commented Mar 17, 2022

Gas Optimizations #63

Gas Optimizations #63

Comments

code423n4 commented Mar 2, 2022

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

HardlyDifficult commented Mar 10, 2022

alcueca commented Mar 17, 2022

Cache `accountInfo.allowance[msg.sender]`

Cache `accountInfo.freedBalance`

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

Cache `buyPrice.seller`

Cache `buyPrice.seller`

Cache `buyPrice.seller`

Cache `buyPrice.seller`

Cache `buyPrice.seller`

Cache `offer.expiration`

Cache `offer.amount`

Cache `offer.expiration`

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

Cache `auction.seller`

Cache `auction.endTime`

Cache `auction.extensionDuration`

Cache `auction.bidder`

Cache `auction.amount`