Foundation contest - CertoraInc's results

Lows and non-critical

mistake in comment of cancelReserveAuction function in the NFTMarketReserveAuction contract

Nothing very critical here, just wanted you to know that you had a mistake in the comment (the has should be removed):

@dev The NFT is transferred back to the owner unless there is still has a buy price set.

There's another way to move ETH into the contract (self-destruct)

FETH contract that has an ether balance that is not related to any user is not a wanted behavior. We can see that it is not a wanted behavior for example in the transferFrom, where the sender isn't allowed to transfer FETH to the FETH contract.

Although, we can use the depositFor, that allows a user to deposit ether for every account, and even for the FETH contract. But also if that will be fixed, There is another way to do it.

As you probably know, contracts can be deleted from the blockchain by calling selfdestruct. selfdestruct sends all remaining Ether stored in the contract to a designated address. A malicious contract can use selfdestruct to force sending Ether to any contract. This can mess up the totalSupply function, because nobody will be able to use the sent ether, but they will still be considered as supplied FETH, because of the totalSupply function returns the ether balance of the FETH contract.

Gas Optimizations

Loops optimization

Loops can be optimized in several ways. Let's take for example the loop in the adminAccountMigration function of the NFTMarketReserveAuction contract.

for (uint256 i = 0; i < listedAuctionIds.length; ++i) {
  uint256 auctionId = listedAuctionIds[i];
  ReserveAuction storage auction = auctionIdToAuction[auctionId];
  if (auction.seller != address(0)) {
    // Only if the auction was found and not finalized before this transaction.
    if (auction.seller != originalAddress) {
      // Confirm that the signature approval was the correct owner of this auction.
      revert NFTMarketReserveAuction_Cannot_Migrate_Non_Matching_Seller(auction.seller);
    }
    // Update the auction's seller address.
    auction.seller = newAddress;
    emit ReserveAuctionSellerMigrated(auctionId, originalAddress, newAddress);
  }
}

To optimize this loop and make it consume less gas, we can do the foloowing things:

1. Save the listedAuctionIds array length in a local variable instead of accessing it in every iteration.
2. Save auction.seller in a local variable instead of accessing it 3 times in every iteration. This will save accssing the struct's field 2/3 times in every iteration.
3. There's no need to initialize i to its default value, it will be done automatically and it will consume more gas if it will be done (I know, sounds stupid, but trust me - it works). So after applying all these changes, the loop will look something like this:

uint length = listedAuctionIds.length;
address seller_i;
for (uint256 i; i < length; ++i) {
  uint256 auctionId = listedAuctionIds[i];
  ReserveAuction storage auction = auctionIdToAuction[auctionId];
  seller_i = auction.seller;
  if (seller_i != address(0)) {
    // Only if the auction was found and not finalized before this transaction.
    if (seller_i != originalAddress) {
      // Confirm that the signature approval was the correct owner of this auction.
      revert NFTMarketReserveAuction_Cannot_Migrate_Non_Matching_Seller(seller_i);
    }
    // Update the auction's seller address.
    auction.seller = newAddress;
    emit ReserveAuctionSellerMigrated(auctionId, originalAddress, newAddress);
  }
}

Another example is the loop in the adminCancelOffers function of the NFTMarketOffer contract:

for (uint256 i = 0; i < nftContracts.length; ++i) {
  Offer memory offer = nftContractToIdToOffer[nftContracts[i]][tokenIds[i]];
  delete nftContractToIdToOffer[nftContracts[i]][tokenIds[i]];
  if (offer.expiration >= block.timestamp) {
    // Unlock from escrow and emit an event only if the offer is still active
    feth.marketUnlockFor(offer.buyer, offer.expiration, offer.amount);
    emit OfferCanceledByAdmin(nftContracts[i], tokenIds[i], reason);
  }
  // Else continue on so the rest of the batch transaction can process successfully
}

Here we can also save the nftContracts[i] and the tokenIds[i] variables instead of accessing them 3 times. The code will look something like this:

uint length = nftContracts.length;
address nft_contract, token_id;
for (uint256 i; i < length; ++i) {
  nft_contract = nftContracts[i];
  token_id = tokenIds[i];
  Offer memory offer = nftContractToIdToOffer[nft_contract][token_id];
  delete nftContractToIdToOffer[nft_contract][token_id];
  if (offer.expiration >= block.timestamp) {
    // Unlock from escrow and emit an event only if the offer is still active
    feth.marketUnlockFor(offer.buyer, offer.expiration, offer.amount);
    emit OfferCanceledByAdmin(nft_contract, token_id, reason);
  }
  // Else continue on so the rest of the batch transaction can process successfully
}

unnecessary check in _transferFromEscrowIfAvailable function

In the cancelReserveAuction function, we delete nftContractToTokenIdToAuctionId[auction.nftContract][auction.tokenId] and then calling the _transferFromEscrowIfAvailable function which checks if nftContractToTokenIdToAuctionId[auction.nftContract][auction.tokenId] == 0, and calls the super function if so. We can call the super function directly instead of calling the _transferFromEscrowIfAvailable function.

implement placeBid without placeBidOf (also in NFTMarketPrivateSale)

The placeBid function calls the placeBidOf with amount = msg.value. Implementing the function without calling the placeBidOf will save gas spending on 2 if statements:

if (amount < msg.value) {
  // The amount is specified by the bidder, so if too much ETH is sent then something went wrong.
  revert NFTMarketReserveAuction_Too_Much_Value_Provided();
} else if (amount > msg.value) {
  // Withdraw additional ETH required from their available FETH balance.
  unchecked {
    // The if above ensures delta will not underflow.
    uint256 delta = amount - msg.value;
    // Withdraw ETH from the buyer's account in the FETH token contract.
    feth.marketWithdrawFrom(msg.sender, delta);
  }
}
...

save value of _getMinIncrement in the placeBidOf

Instead of calling the `` function twice, its return value can be saved in order to save gas in case the statement inside the if it true.

code before:

... else if (amount < _getMinIncrement(auction.amount)) {
  // If this bid outbids another, it must be at least 10% greater than the last bid.
  revert NFTMarketReserveAuction_Bid_Must_Be_At_Least_Min_Amount(_getMinIncrement(auction.amount));

code after:

uint min_inc = _getMinIncrement(auction.amount);
... else if (amount < min_inc) {
  // If this bid outbids another, it must be at least 10% greater than the last bid.
  revert NFTMarketReserveAuction_Bid_Must_Be_At_Least_Min_Amount(min_inc);

implement transfer without using transferFrom in FETH contract

The transfer function calls the transferFrom function with from = msg.sender. Implementing it without the call to transferFrom will save an if statement:

if (from != msg.sender) {
  _deductAllowanceFrom(fromAccountInfo, amount);
}

because we know for sure that from == msg.sender.

Saving a struct mapping element instead of accessing it multiple times _deductAllowanceFrom

In the _deductAllowanceFrom function in the FETH contract, a variable can be saved in order to prevent calling it multiple times. The accountInfo.allowance[msg.sender] is accessed multiple times, and can be saved as a local variable to prevent all these accesses. code before:

function _deductAllowanceFrom(AccountInfo storage accountInfo, uint256 amount) private {
  if (accountInfo.allowance[msg.sender] != type(uint256).max) {
    if (accountInfo.allowance[msg.sender] < amount) {
      revert FETH_Insufficient_Allowance(accountInfo.allowance[msg.sender]);
    }
    // The check above ensures allowance cannot underflow.
    unchecked {
      accountInfo.allowance[msg.sender] -= amount;
    }
  }
}

code after:

function _deductAllowanceFrom(AccountInfo storage accountInfo, uint256 amount) private {
  uint allowence = accountInfo.allowance[msg.sender];
  if (allowence != type(uint256).max) {
    if (allowance < amount) {
      revert FETH_Insufficient_Allowance(allowence);
    }
    // The check above ensures allowance cannot underflow.
    unchecked {
      accountInfo.allowance[msg.sender] = allowance - amount;
    }
  }
}

optimize _getMinIncrement in NFTMarketCore

Instead of calculating 10% of the amount (minIncrement) and then adding it to the current amount, we can calculate the sum by calculating 100 + increment % (in our example 110%), and adding one if it is equal to the amount before. code before:

function _getMinIncrement(uint256 currentAmount) internal pure returns (uint256) {
  uint256 minIncrement = currentAmount * MIN_PERCENT_INCREMENT_IN_BASIS_POINTS;
  unchecked {
    minIncrement /= BASIS_POINTS;
    if (minIncrement == 0) {
      // Since minIncrement reduces from the currentAmount, this cannot overflow.
      // The next amount must be at least 1 wei greater than the current.
      return currentAmount + 1;
    }
  }
  return minIncrement + currentAmount;
}

code after:

function _getMinIncrement(uint256 currentAmount) internal pure returns (uint256) {
  uint256 amountAfter = currentAmount * MIN_PERCENT_INCREMENT_IN_BASIS_POINTS; // this will be 110% instead of 10%
  unchecked {
    amountAfter /= BASIS_POINTS;
    if (amountAfter == currentAmount) {
      // Since minIncrement reduces from the currentAmount, this cannot overflow.
      // The next amount must be at least 1 wei greater than the current.
      return currentAmount + 1;
    }
  }
  return amountAfter;
}

don't calculate things twice

In the placeBidOf function in NFTMarketReserveAuction, a calculation can be saved - that's becuase of these 2 statements are equivilent: auction.endTime - block.timestamp < auction.extensionDuration auction.endTime < block.timestamp + auction.extensionDuration

So the code can be changed from this:

...
unchecked {
  // When a bid outbids another, check to see if a time extension should apply.
  // We confirmed that the auction has not ended, so endTime is always >= the current timestamp.
  if (auction.endTime - block.timestamp < auction.extensionDuration) {
    // Current time plus extension duration (always 15 mins) cannot overflow.
    auction.endTime = block.timestamp + auction.extensionDuration;
  }
}

To this:

...
unchecked {
  // When a bid outbids another, check to see if a time extension should apply.
  // We confirmed that the auction has not ended, so endTime is always >= the current timestamp.
  uint timeWithExtension = block.timestamp + auction.extensionDuration;
  if (auction.endTime < timeWithExtension) {
    // Current time plus extension duration (always 15 mins) cannot overflow.
    auction.endTime = timeWithExtension;
  }
}