Infinity NFT Marketplace contest - 0xf15ers's results

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L1230 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L344-L348

Vulnerability details

Impact

The rescueETH function is implemented to collect any unexpected ETH transferred to the infinityExchange.sol contract, But this function will not work as expected. The function is supposed to return the eth from the contract to the specified destination address, but it transfers only the amount of ETH send by the caller as msg.value is used here.

Proof of Concept

In InfinityExchange.sol#L1230

  /// @dev used for rescuing exchange fees paid to the contract in ETH
  function rescueETH(address destination) external payable onlyOwner {
    (bool sent, ) = destination.call{value: msg.value}('');
    require(sent, 'failed');
  }

Also in InfinityStaker.sol#L344-L348

Recommended Mitigation Steps

use address(this).balance or custom amount specified by the caller for rescuing ETH.

  /// @dev used for rescuing exchange fees paid to the contract in ETH
  function rescueETH(address destination) external payable onlyOwner {
    (bool sent, ) = destination.call{value: address(this).balance}('');
    require(sent, 'failed');
  }

Or,

  /// @dev used for rescuing exchange fees paid to the contract in ETH
  function rescueETH(address destination, uint256 amount) external payable onlyOwner {
    (bool sent, ) = destination.call{value: amount}('');
    require(sent, 'failed');
  }

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L1062-L1072

Vulnerability details

Impact

The _transferNFTs function use ERC165 to check if the item(nft) supports ERC721 interface or ERC1155 interface and execute transfer accordingly. But if it doesn't supports either, it just exits the function(no revert).

Proof of Concept

in InfinityExchange.sol#L1062-L1072

  /**
   * @notice Transfer NFTs
   * @param from address of the sender
   * @param to address of the recipient
   * @param item item to transfer
   */
  function _transferNFTs(
    address from,
    address to,
    OrderTypes.OrderItem calldata item
  ) internal {
    if (IERC165(item.collection).supportsInterface(0x80ac58cd)) {
      _transferERC721s(from, to, item);
    } else if (IERC165(item.collection).supportsInterface(0xd9b67a26)) {
      _transferERC1155s(from, to, item);
    }
  }

Tools Used

Manual Analysis

Recommended Mitigation Steps

Revert the transaction if the item doesn't supports any NFT specification

  /**
   * @notice Transfer NFTs
   * @param from address of the sender
   * @param to address of the recipient
   * @param item item to transfer
   */
  function _transferNFTs(
    address from,
    address to,
    OrderTypes.OrderItem calldata item
  ) internal {
    if (IERC165(item.collection).supportsInterface(0x80ac58cd)) {
      _transferERC721s(from, to, item);
    } else if (IERC165(item.collection).supportsInterface(0xd9b67a26)) {
      _transferERC1155s(from, to, item);
    } else {
      require(false, "The item is not an NFT");
    }
  }

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L272-L273

Vulnerability details

Impact

While matching the orders the gas cost is calculated so that it can be refunded back to the contract. The calculation for this gas is incorrect.

The function keeps track of the gasleft at the beginning of the loop and adds additional amount of gas for pre loop calculation inside the loop which breaks the logic.

Proof of Concept

In InfinityExchange.sol#L272-L273

The same issue repeats in matchOneToOneOrders and matchOneToManyOrders functions on the same file too.

    ...
    for (uint256 i = 0; i < numSells; ) {
      uint256 startGasPerOrder = gasleft() + ((startGas - gasleft()) / numSells);
    ...

Tools Used

Manual analysis

Recommended Mitigation Steps

The amount of gas used in the pre loop part can be calculated outside the loop

    uint256 additionalGas = (startGas - gasleft()) / numSells;
    for (uint256 i = 0; i < numSells; ) {
      uint256 startGasPerOrder = gasleft() + additionalGas;

To make it more stricter, small amount gas can also be added to account for loop iteration calculation.

1.Match functions in complications will not check if the orders are buy/sell pair

for eg.

• In InfinityOrderBookComplication.sol#L26 The function canExecMatchOneToOne() will not check if the orders are a buy/sell pair. Rather this check is done later when executing the matches. This should be done early in the canExecMatchOneToOne function so it gives proper result.

1 Use unchecked Math for calculations which will never overflow or underflow

There are many calculations which will never over/under flow because of verifications done prior to those calculations. Those can be put inside unchecked block to save gas.

for eg.

• https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityOrderBookComplication.sol#L333
• https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L1156
• https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L1164

2 Read data from memory instead of storage when possible to save gas

Reading from storage uses SLOAD opcode which is more expensive than MLOAD which is used for memory read.

• https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L231 use wethTransferGasUnits(memory) instead of WETH_TRANSFER_GAS_UNITS(storage).

3. Use solidity custom errors to save gas

solidity 0.8.4 introduces custom errors which are cheaper than using revert strings in terms of gas. This can be used to save gas.

for eg.

  // Before
  require(condition, "Revert strings");

  // After
  error CustomError();
  if (!condition) {
    revert CustomError();
  }

more details can be found here

4. Constant expressions are expressions not constants

Constant expressions are not really constants but expressions, hence need to be evaluated every time they are used. Using actual constants can save gas.

• https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/token/InfinityToken.sol#L25-L28

5. canExecMatchOneToMany function can be optimized to save more gas

In InfinityOrderBookComplication.sol#L68

a. move this block to the end of the loop since first iteration will always evaluate to false || false

b. itemsIntersect does not needs to be checked in return statement since it is already checked inside the loop body (because of [a])

c. in InfinityOrderBookComplication.sol#L101-L103, isOrdersTimeValid can be ignored since it will always be true (checked inside the loop, because of [a])

d. return false immediately if _isTimeValid is false

e. return false immediately after the loop if numItems == makerOrder.constraints[0] is false

6. Long revert strings (> 32bytes) will cost extra gas

For eg. In InfinityExchange.sol#L395