Foundation contest - TerrierLover's results

1. Inconsistent style of inheritance at NFTmarketOffer.sol

The inheritance of NFTmarketOffer.sol is not aligned with other contracts, and it is put in single line even though it exceeds more than 80 characters.

abstract contract NFTMarketOffer is FoundationTreasuryNode, NFTMarketCore, ReentrancyGuardUpgradeable, NFTMarketFees {

The inheritance of NFTMarketCreators.sol is not set at single line even though it only inherits 2 contracts.

abstract contract NFTMarketCreators is
  Constants,
  ReentrancyGuardUpgradeable // Adding this unused mixin to help with linearization
{

Hence, it is reasonable that the inheritance goes to next line for consistency.

https://github.com/code-423n4/2022-02-foundation/blob/a03a7e198c1dfffb1021c0e8ec91ba4194b8aa12/contracts/mixins/NFTMarketOffer.sol#L29

abstract contract NFTMarketOffer is 
  FoundationTreasuryNode, 
  NFTMarketCore, 
  ReentrancyGuardUpgradeable,
  NFTMarketFees 
{

2. NFTMarketPrivateSale.sol uses assemby code which is not needed

Assembly code chainid() can be replaced with block.chainid at NFTMarketPrivateSale.sol.

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketPrivateSale.sol#L70-L84

uint256 chainId;
// solhint-disable-next-line no-inline-assembly
assembly {
    chainId := chainid()
}
DOMAIN_SEPARATOR = keccak256(
  abi.encode(
    keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
    keccak256(bytes(NAME)),
    // Incrementing the version can be used to invalidate previously signed messages.
    keccak256(bytes("1")),
    block.chainid,
    marketProxyAddress
  )
);

Here is the updated code which simply uses block.chainid.

DOMAIN_SEPARATOR = keccak256(
  abi.encode(
    keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
    keccak256(bytes(NAME)),
    // Incrementing the version can be used to invalidate previously signed messages.
    keccak256(bytes("1")),
    block.chainid,
    marketProxyAddress
  )
);

Item1

Gas cost reduction by logic change at _getMinIncrement function at NFTMarketCore.sol

Vulnerability details

Impact

_getMinIncrement function at NFTMarketCore.sol seems to have an unnecessary logic which ends up adding more gas cost.

Proof of Concept

minIncrement can be gotten by currentAmount * MIN_PERCENT_INCREMENT_IN_BASIS_POINTS / BASIS_POINTS as well. Hence, using MIN_PERCENT_INCREMENT_IN_BASIS_POINTS and BASIS_POINTS seems unnecessary. The current implementation is readable, but unnecessary which would increase gas cost.

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCore.sol#L119-L131

  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;
  }

Tools Used

yarn test

Recommended Mitigation Steps

After rewriting the logic as follows, it has same logic while being able to reduce the gas.

  function _getMinIncrement(uint256 currentAmount) internal pure returns (uint256) {
    uint256 minIncrement = currentAmount;
    unchecked {
      minIncrement /= MIN_PERCENT_INCREMENT; // <--- COMMENT: MIN_PERCENT_INCREMENT=10
      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;
  }

Before the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5236255 Max=5236291 Avg=5236287

After the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5232850 Max=5232886 Avg=5232882

As a result, the above change can reduce 3405 gas cost on average.

---

Item2

Missing uncheck directory can reduce gas fee at NFTMarketCreators.sol

Vulnerability details

Impact

NFTMarketCreators.sol uses unchecked directory at where it can be applied, but following part is not wrapped by unchecked directory.

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCreators.sol#L155-L162

for (uint256 i = 0; i < _recipients.length; ++i) {
  if (_recipients[i] != address(0)) {
    hasRecipient = true;
    if (_recipients[i] == seller) {
      return (_recipients, recipientBasisPoints, true);
    }
  }
}

Proof of Concept

unchecked directory would reduce the gas fee

Tools Used

$ yarn test

Recommended Mitigation Steps

By wrapping by unchecked directory, it can reduce the gas fee.

unchecked {
  for (uint256 i = 0; i < _recipients.length; ++i) {
    if (_recipients[i] != address(0)) {
      hasRecipient = true;
      if (_recipients[i] == seller) {
        return (_recipients, recipientBasisPoints, true);
      }
    }
  }
}

Before the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5236255 Max=5236291 Avg=5236287

After the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5234530 Max=5234566 Avg=5234562

As a result, the above change can reduce 1725 gas cost on average.

---

Item3

Duplicates condition check recipients.length == 0 increases gas fee at NFTMarketCreators.sol

Vulnerability details

Impact

NFTMarketCreators.sol has duplicate condition check recipients.length == 0 at multiple places which increase gas fee.

Proof of Concept

NFTMarketCreators.sol has up to 7th priority. 2th, 3rd, 4th, and 5th logic does recipients.length == 0 condition check, which can be removed by simply wrapping 2th, 3rd, 4th, and 5th logic by if (recipients.length == 0) and remove the duplicate checks.

Tools Used

$ yarn test

Recommended Mitigation Steps

This is an example of the modified code where the gas reduction was confirmed. https://gist.github.com/TerrierLover/53bb75cdb57e8237e2339682e2083298

Before the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5236255 Max=5236291 Avg=5236287

After the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5229802 Max=5229838 Avg=5229834

As a result, the above change can reduce 6453 gas cost on average.

---

Item4

Using != 0 instead of > 0 can reduce gas fee at NFTMarketCreators.sol

Vulnerability details

Impact

NFTMarketCreators.sol uses _recipients.length > 0 at multiple places, and replacing this with _recipients.length != 0 can decrease the gas fee.

Proof of Concept

Following codes use _recipients.length > 0 which can be replaced by != 0. https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCreators.sol#L90 https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCreators.sol#L153 https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketCreators.sol#L185

_recipients is an array of address, so it will never be less than 0. Hence, just checking _recipients.length != 0 is suffice.

Tools Used

$ yarn test

Recommended Mitigation Steps

Before the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5236255 Max=5236291 Avg=5236287

After removing _recipients.length > 0 and using _recipients.length != 0 instead at NFTMarketCreators.sol, $ yarn test returns gas cost for FNDNFTMarket: Min=5235907 Max=5235943 Avg=5235939

As a result, the above change can reduce 348 gas cost on average.

---

Item5

Improvement of _toAsciiString and _char function at AccountMigrationLibrary.sol

Vulnerability details

Impact

There are some unnecessary converstion, initialization, and calculation at _toAsciiString and _char function AccountMigrationLibrary.sol.

Proof of Concept

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/libraries/AccountMigrationLibrary.sol#L49-L73

Tools Used

$ yarn test

Recommended Mitigation Steps

Here is an optimization of _toAsciiString and _char function to reduce the gas usage.

  function _toAsciiString(address x) private pure returns (string memory) {
    bytes memory s = new bytes(42);
    s[0] = "0";
    s[1] = "x";
    uint256 inAddress = uint256(uint160(x));
    unchecked {
      for (uint256 i = 2; i < 42; i += 2) {
          uint8 b = uint8(inAddress >> (4 * (40 - i)));
          s[i] = _char(b >> 4);
          s[i + 1] = _char(b & 0x0f);
      }
    }
    return string(s);
  }

  function _char(uint8 b) private pure returns (bytes1 c) {
    unchecked {
      if (b < 10) return bytes1(b + 0x30);
      else return bytes1(b + 0x57);
    }
  }  

Before the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5236255 Max=5236291 Avg=5236287

After the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5219693 Max=5219729 Avg=5219725

As a result, the above change can reduce 16562 gas cost on average.

---

Item6

_getMinIncrement function at is NFTMarketReserveAuction.sol called twice

Vulnerability details

Impact

_getMinIncrement function at is NFTMarketReserveAuction.sol called twice but calling twice seems not necessary. _getMinIncrement function can be heavy, and just calling it once by changing the logic is beneficial.

Proof of Concept

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketReserveAuction.sol#L430-L439

if (auction.endTime < block.timestamp) {
  // The auction has already ended.
  revert NFTMarketReserveAuction_Cannot_Bid_On_Ended_Auction(auction.endTime);
} else if (auction.bidder == msg.sender) {
  // We currently do not allow a bidder to increase their bid unless another user has outbid them first.
  revert NFTMarketReserveAuction_Cannot_Rebid_Over_Outstanding_Bid();
} 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));
}

At the third else if check, _getMinIncrement function is called twice.

Tools Used

$ yarn test

Recommended Mitigation Steps

Here is the example of the optimization.

if (auction.endTime < block.timestamp) {
  // The auction has already ended.
  revert NFTMarketReserveAuction_Cannot_Bid_On_Ended_Auction(auction.endTime);
} else if (auction.bidder == msg.sender) {
  // We currently do not allow a bidder to increase their bid unless another user has outbid them first.
  revert NFTMarketReserveAuction_Cannot_Rebid_Over_Outstanding_Bid();
} 

uint256 minIncrement = _getMinIncrement(auction.amount);
if (amount < minIncrement) {
  // 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(minIncrement);
}

Before the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5236255 Max=5236291 Avg=5236287

After the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5234770 Max=5234806 Avg=5234802

As a result, the above change can reduce 1485 gas cost on average.

---

Item7

placeBidOf function has unnecessary variable and logic at NFTMarketReserveAuction.sol

Vulnerability details

Impact

placeBidOf function has unnecessary variable uint256 delta and else if (amount > msg.value) at NFTMarketReserveAuction.sol which can increase the gas cost.

Proof of Concept

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketReserveAuction.sol#L387-L400

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);
}

delta variable is not needed to be defined.

Tools Used

$ yarn test

Recommended Mitigation Steps

Here is the example of the optimization.

unchecked {
  // The if above ensures delta will not underflow.
  // Withdraw ETH from the buyer's account in the FETH token contract.
  feth.marketWithdrawFrom(msg.sender, amount - msg.value);
}

Before the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5236255 Max=5236291 Avg=5236287

After the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5235403 Max=5235439 Avg=5235435

As a result, the above change can reduce 852 gas cost on average.

---

Item8

Duplicate logic to store the bid details at NFTMarketReserveAuction.sol

Vulnerability details

Impact

There is duplicate logic to store the bid details at NFTMarketReserveAuction.sol.

Proof of Concept

Following codes are duplicated at placeBidOf funciton at NFTMarketReserveAuction.sol.

auction.amount = amount;
auction.bidder = payable(msg.sender);

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketReserveAuction.sol#L420-L422 https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketReserveAuction.sol#L444-L445

Tools Used

$ yarn test

Recommended Mitigation Steps

if (auction.endTime == 0) {
  ... // Remove duplicate logic
} else {
  ... // Remove duplicate logic
}

auction.amount = amount;              // duplicate logic is moved here
auction.bidder = payable(msg.sender); // 

Before the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5236255 Max=5236291 Avg=5236287

After the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5233078 Max=5233090 Avg=5233086

As a result, the above change can reduce 3201 gas cost on average.

---

Item9

Can use local variable for the addition block.timestamp + auction.extensionDuration at NFTMarketReserveAuction.sol.

Vulnerability details

Impact

Proof of Concept

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketReserveAuction.sol#L447-L454

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;
  }
}

Subtraction and addition happen here, but can combine into one addition.

Tools Used

yarn test

Recommended Mitigation Steps

Here is an example modification.

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.
  uint256 endTime = block.timestamp + auction.extensionDuration;
  if (auction.endTime < endTime) {
    // Current time plus extension duration (always 15 mins) cannot overflow.
    auction.endTime = endTime;
  }
}

Before the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5236255 Max=5236291 Avg=5236287

After the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5236123 Max=5236135 Avg=5236131

As a result, the above change can reduce 156 gas cost on average.

---

Item10

placeBidOf function has unnecessary variable and logic at NFTMarketReserveAuction.sol

Vulnerability details

Impact

placeBidOf function has unnecessary variable uint256 delta and else if (amount > msg.value) at NFTMarketReserveAuction.sol which can increase the gas cost.

Proof of Concept

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketPrivateSale.sol#L143-L154

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;
    feth.marketWithdrawFrom(msg.sender, delta);
  }
} else if (amount < msg.value) {
  // The terms of the sale cannot change, so if too much ETH is sent then something went wrong.
  revert NFTMarketPrivateSale_Too_Much_Value_Provided();
}

delta variable is not needed to be defined.

Tools Used

$ yarn test

Recommended Mitigation Steps

Here is the example of the optimization.

if (amount > msg.value) {
  // Withdraw additional ETH required from their available FETH balance.

  unchecked {
    // The if above ensures delta will not underflow
    feth.marketWithdrawFrom(msg.sender, amount - msg.value);
  }
} else if (amount < msg.value) {
  // The terms of the sale cannot change, so if too much ETH is sent then something went wrong.
  revert NFTMarketPrivateSale_Too_Much_Value_Provided();
}

Before the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5236255 Max=5236291 Avg=5236287

After the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5235427 Max=5235439 Avg=5235435

As a result, the above change can reduce 852 gas cost on average.

---

Item11

LockBalance.sol contains some duplicated logic which increase gas cost

Vulnerability details

Impact

LockBalance.sol contains some duplicated logic which increase gas cost. Namely, index /= 2 or others.

Proof of Concept

Here are target functions which can reduce gas cost by refactoring.

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/libraries/LockedBalance.sol#L34-L44

function del(Lockups storage lockups, uint256 index) internal {
  unchecked {
    if (index % 2 == 0) {
      index /= 2;
      lockups.lockups[index] = (lockups.lockups[index] & last128BitsMask);
    } else {
      index /= 2;
      lockups.lockups[index] = (lockups.lockups[index] & first128BitsMask);
    }
  }
}

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/libraries/LockedBalance.sol#L49-L67

function set(
  Lockups storage lockups,
  uint256 index,
  uint256 expiration,
  uint256 totalAmount
) internal {
  unchecked {
    uint256 lockedBalanceBits = totalAmount | (expiration << 96);
    if (index % 2 == 0) {
      // set first 128 bits.
      index /= 2;
      lockups.lockups[index] = (lockups.lockups[index] & last128BitsMask) | (lockedBalanceBits << 128);
    } else {
      // set last 128 bits.
      index /= 2;
      lockups.lockups[index] = (lockups.lockups[index] & first128BitsMask) | lockedBalanceBits;
    }
  }
}

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/libraries/LockedBalance.sol#L72-L86

function setTotalAmount(
  Lockups storage lockups,
  uint256 index,
  uint256 totalAmount
) internal {
  unchecked {
    if (index % 2 == 0) {
      index /= 2;
      lockups.lockups[index] = (lockups.lockups[index] & firstAmountBitsMask) | (totalAmount << 128);
    } else {
      index /= 2;
      lockups.lockups[index] = (lockups.lockups[index] & secondAmountBitsMask) | totalAmount;
    }
  }
}

Tools Used

yarn test

Recommended Mitigation Steps

Here are example changes:

function del(Lockups storage lockups, uint256 index) internal {
  unchecked {
    bool isEven = index % 2 == 0;
    index /= 2;
    lockups.lockups[index] = (lockups.lockups[index] & (isEven ? last128BitsMask: first128BitsMask));
  }
}

function set(
  Lockups storage lockups,
  uint256 index,
  uint256 expiration,
  uint256 totalAmount
) internal {
  unchecked {
    uint256 lockedBalanceBits = totalAmount | (expiration << 96);
    bool isEven = index % 2 == 0;
    index /= 2;
    if (isEven) {
      // set first 128 bits.
      lockups.lockups[index] = (lockups.lockups[index] & last128BitsMask) | (lockedBalanceBits << 128);
    } else {
      // set last 128 bits.
      lockups.lockups[index] = (lockups.lockups[index] & first128BitsMask) | lockedBalanceBits;
    }
  }
}

function setTotalAmount(
  Lockups storage lockups,
  uint256 index,
  uint256 totalAmount
) internal {
  unchecked {
    bool isEven = index % 2 == 0;
    index /= 2;
    if (isEven) {
      lockups.lockups[index] = (lockups.lockups[index] & firstAmountBitsMask) | (totalAmount << 128);
    } else {
      lockups.lockups[index] = (lockups.lockups[index] & secondAmountBitsMask) | totalAmount;
    }
  }
}

Before the change, $ yarn test returns gas cost for FETH: Min=1545162 Max=1545174 Avg=1545174

After the change, $ yarn test returns gas cost for FETH: Min=1540631 Max=1540643 Avg=1540643

As a result, the above change can reduce 4531 gas cost on average.

---

Item12

constructor of FETH.sol has some logic which can be wrapped by unchecked directory.

Vulnerability details

Impact

constructor of FETH.sol has some logic which can be wrapped by unchecked directory. By doing so, it can reduce gas cost.

Proof of Concept

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/FETH.sol#L191-L194

lockupInterval = _lockupDuration / 24;
if (lockupInterval * 24 != _lockupDuration || _lockupDuration == 0) {
  revert FETH_Invalid_Lockup_Duration();
}

Tools Used

yarn test

Recommended Mitigation Steps

Here is an example modifiction.

constructor(address payable _foundationMarket, uint256 _lockupDuration) {
  if (!_foundationMarket.isContract()) {
    revert FETH_Market_Must_Be_A_Contract();
  }
  foundationMarket = _foundationMarket;
  lockupDuration = _lockupDuration;
  unchecked {
    lockupInterval = _lockupDuration / 24;
    if (lockupInterval * 24 != _lockupDuration || _lockupDuration == 0) {
      revert FETH_Invalid_Lockup_Duration();
    }
  }
}

_lockupDuration / 24 will not be underflown since _lockupDuration is uint256. lockupInterval * 24 is equivalent with (_lockupDuration / 24) * 24 so this will not be overflown.

Before the change, $ yarn test returns gas cost for FETH: Min=1545162 Max=1545174 Avg=1545174

After the change, $ yarn test returns gas cost for FETH: Min=1543566 Max=1543578 Avg=1543578

As a result, the above change can reduce 1596 gas cost on average.

---

Item13

Missing unchecked directory at _removeFromLockedBalance function in FETH.sol

Vulnerability details

Impact

Potential gas reduction.

Proof of Concept

escrowIndex + 1 at below code can be wrapped by unchecked directory. According to the comment The number of escrow buckets will never overflow 32 bits, it is safe to assume that escrowIndex which is accountInfo.lockupStartIndex will not overflown.

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/FETH.sol#L611-L616

if (accountInfo.lockups.get(escrowIndex + 1).expiration != 0) {
  // The number of escrow buckets will never overflow 32 bits.
  unchecked {
    ++accountInfo.lockupStartIndex;
  }
}

Tools Used

yarn test

Recommended Mitigation Steps

Simply wrap escrowIndex + 1 by unchecked directory.

// Bump the escrow start index unless it's the last one
unchecked {
  // The number of escrow buckets will never overflow 32 bits.
  if (accountInfo.lockups.get(escrowIndex + 1).expiration != 0) {
    ++accountInfo.lockupStartIndex;
  }
}

Before the change, $ yarn test returns gas cost for FETH: Min=1545162 Max=1545174 Avg=1545174

After the change, $ yarn test returns gas cost for FETH: Min=1543410 Max=1543422 Avg=1543422

As a result, the above change can reduce 1752 gas cost on average.

---

Item14

Refactoring _getFees function at NFTMarketFees.sol can reduce gas cost

Vulnerability details

Impact

Gas reduction

Proof of Concept

Following codes can be wrapped by unchecked directory since they will not be underflown based on the values of constants (e.g., CREATOR_ROYALTY_BASIS_POINTS or others). https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketFees.sol#L199

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketFees.sol#L204

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketFees.sol#L209

Tools Used

yarn test

Recommended Mitigation Steps

Here is an example code which can reduce gas cost.

foundationFee = (price * fee) / BASIS_POINTS;

if (creatorRecipients.length > 0) {
  if (isCreator) {
    // When sold by the creator, all revenue is split if applicable.
    unchecked {
      creatorRev = price - foundationFee;
    }
  } else {
    // Rounding favors the owner first, then creator, and foundation last.
    creatorRev = (price * CREATOR_ROYALTY_BASIS_POINTS) / BASIS_POINTS;
    ownerRevTo = seller;
    unchecked {
      ownerRev = price - foundationFee - creatorRev;
    }
  }
} else {
  // No royalty recipients found.
  ownerRevTo = seller;
  unchecked {
    ownerRev = price - foundationFee;
  }
}

Before the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5236255 Max=5236291 Avg=5236287

After the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5229802 Max=5229838 Avg=5229834

As a result, the above change can reduce 6453 gas cost on average.

---

Item15

Remove unnecessary call of _sendValueWithFallbackWithdraw from _distributeFunds function in NFTMarketFees.sol

Vulnerability details

Impact

Gas reducation.

Proof of Concept

Following code does not need to be called when creatorFee - totalDistributed does not fullfill the condition. https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketFees.sol#L107-L112

// Send the remainder to the 1st creator, rounding in their favor
_sendValueWithFallbackWithdraw(
  creatorRecipients[0],
  creatorFee - totalDistributed,
  SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS
);

Tools Used

yarn test

Recommended Mitigation Steps

Here is an example of the modified code:

// Send the remainder to the 1st creator, rounding in their favor
uint256 reminderAmount = creatorFee - totalDistributed;
if (reminderAmount != 0) {
  _sendValueWithFallbackWithdraw(
    creatorRecipients[0],
    reminderAmount,
    SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS
  );
}

Before the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5236255 Max=5236291 Avg=5236287

After the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5234566 Max=5234578 Avg=5234574

As a result, the above change can reduce 1713 gas cost on average.

Item16

Wrapping ++i with unchecked directory can reduce gas cost at _distributeFunds function in NFTMarketFees.sol

Vulnerability details

Impact

gas cost reduction

Proof of Concept

++i in the following code is not wrapped by unchecked.

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketFees.sol#L101

for (uint256 i = 1; i <= maxCreatorIndex; ++i) {
  uint256 share = (creatorFee * creatorShares[i]) / totalShares;
  totalDistributed += share;
  _sendValueWithFallbackWithdraw(creatorRecipients[i], share, SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS);
}

Tools Used

yarn test

Recommended Mitigation Steps

Here is an example of modified code:

// Send payouts to each additional recipient if more than 1 was defined
uint256 totalDistributed;
for (uint256 i = 1; i <= maxCreatorIndex; ) {
  uint256 share = (creatorFee * creatorShares[i]) / totalShares;
  totalDistributed += share;
  _sendValueWithFallbackWithdraw(creatorRecipients[i], share, SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS);
  unchecked {
    ++i;
  }
}

Before the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5236255 Max=5236291 Avg=5236287

After the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5234578 Max=5234590 Avg=5234586

As a result, the above change can reduce 1701 gas cost on average.

Item17

Change the order of acceptOffer function in NFTMarketOffer.sol can reduce gas fee.

Vulnerability details

Impact

Gas cost reduction

Proof of Concept

Currently, offer.buyer != offerFrom if check is done after offer.expiration < block.timestamp or offer.amount < minAmount.

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/NFTMarketOffer.sol#L127-L136

// Validate offer expiry and amount
if (offer.expiration < block.timestamp) {
  revert NFTMarketOffer_Offer_Expired(offer.expiration);
} else if (offer.amount < minAmount) {
  revert NFTMarketOffer_Offer_Below_Min_Amount(offer.amount);
}
// Validate the buyer
if (offer.buyer != offerFrom) {
  revert NFTMarketOffer_Offer_From_Does_Not_Match(offer.buyer);
}

Tools Used

yarn test

Recommended Mitigation Steps

Simply changing the order may reduce the gas cost. From the code, if offer.buyer is different from offerFrom, it looks OK to change the order.

// Validate the buyer
if (offer.buyer != offerFrom) {
  revert NFTMarketOffer_Offer_From_Does_Not_Match(offer.buyer);
}
// Validate offer expiry and amount
if (offer.expiration < block.timestamp) {
  revert NFTMarketOffer_Offer_Expired(offer.expiration);
} else if (offer.amount < minAmount) {
  revert NFTMarketOffer_Offer_Below_Min_Amount(offer.amount);
}

Before the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5236255 Max=5236291 Avg=5236287

After the change, $ yarn test returns gas cost for FNDNFTMarket: Min=5235226 Max=5235262 Avg=5235258

As a result, the above change can reduce 1029 gas cost on average.

---