Infinity NFT Marketplace contest - WatchPug's results

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L68-L116

Vulnerability details

The call stack: matchOneToManyOrders() -> _matchOneMakerSellToManyMakerBuys() -> _execMatchOneMakerSellToManyMakerBuys() -> _execMatchOneToManyOrders() -> _transferMultipleNFTs()

Based on the context, a maker buy order can set OrderItem.tokens as an empty array to indicate that they can accept any tokenId in this collection, in that case, InfinityOrderBookComplication.doTokenIdsIntersect() will always return true.

However, when the system matching a sell order with many buy orders, the InfinityOrderBookComplication contract only ensures that the specified tokenIds intersect with the sell order, and the total count of specified tokenIds equals the sell order's quantity (makerOrder.constraints[0]).

This allows any maker buy order with same collection and empty tokenIds to be added to manyMakerOrders as long as there is another maker buy order with specified tokenIds that matched the sell order's tokenIds.

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L68-L116

function canExecMatchOneToMany(
    OrderTypes.MakerOrder calldata makerOrder,
    OrderTypes.MakerOrder[] calldata manyMakerOrders
  ) external view override returns (bool) {
    uint256 numItems;
    bool isOrdersTimeValid = true;
    bool itemsIntersect = true;
    uint256 ordersLength = manyMakerOrders.length;
    for (uint256 i = 0; i < ordersLength; ) {
      if (!isOrdersTimeValid || !itemsIntersect) {
        return false; // short circuit
      }

      uint256 nftsLength = manyMakerOrders[i].nfts.length;
      for (uint256 j = 0; j < nftsLength; ) {
        numItems += manyMakerOrders[i].nfts[j].tokens.length;
        unchecked {
          ++j;
        }
      }

      isOrdersTimeValid =
        isOrdersTimeValid &&
        manyMakerOrders[i].constraints[3] <= block.timestamp &&
        manyMakerOrders[i].constraints[4] >= block.timestamp;

      itemsIntersect = itemsIntersect && doItemsIntersect(makerOrder.nfts, manyMakerOrders[i].nfts);

      unchecked {
        ++i;
      }
    }

    bool _isTimeValid = isOrdersTimeValid &&
      makerOrder.constraints[3] <= block.timestamp &&
      makerOrder.constraints[4] >= block.timestamp;

    uint256 currentMakerOrderPrice = _getCurrentPrice(makerOrder);
    uint256 sumCurrentOrderPrices = _sumCurrentPrices(manyMakerOrders);

    bool _isPriceValid = false;
    if (makerOrder.isSellOrder) {
      _isPriceValid = sumCurrentOrderPrices >= currentMakerOrderPrice;
    } else {
      _isPriceValid = sumCurrentOrderPrices <= currentMakerOrderPrice;
    }

    return (numItems == makerOrder.constraints[0]) && _isTimeValid && itemsIntersect && _isPriceValid;
  }

However, because buy.nfts is used as OrderItem to transfer the nfts from seller to buyer, and there are no tokenIds specified in the matched maker buy order, the buyer wont receive any nft (_transferERC721s does nothing, 0 transfers) despite the buyer paid full in price.

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L763-L786

function _execMatchOneMakerSellToManyMakerBuys(
    bytes32 sellOrderHash,
    bytes32 buyOrderHash,
    OrderTypes.MakerOrder calldata sell,
    OrderTypes.MakerOrder calldata buy,
    uint256 startGasPerOrder,
    uint256 execPrice,
    uint16 protocolFeeBps,
    uint32 wethTransferGasUnits,
    address weth
  ) internal {
    isUserOrderNonceExecutedOrCancelled[buy.signer][buy.constraints[5]] = true;
    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
    uint256 remainingAmount = execPrice - protocolFee;
    _execMatchOneToManyOrders(sell.signer, buy.signer, buy.nfts, buy.execParams[1], remainingAmount);
    _emitMatchEvent(
      sellOrderHash,
      buyOrderHash,
      sell.signer,
      buy.signer,
      buy.execParams[0],
      buy.execParams[1],
      execPrice
    );

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1080-L1092

function _transferERC721s(
    address from,
    address to,
    OrderTypes.OrderItem calldata item
  ) internal {
    uint256 numTokens = item.tokens.length;
    for (uint256 i = 0; i < numTokens; ) {
      IERC721(item.collection).safeTransferFrom(from, to, item.tokens[i].tokenId);
      unchecked {
        ++i;
      }
    }
  }

PoC

1. Alice signed and submitted a maker buy order #1, to buy 2 Punk with 2 WETH and specified tokenIds = 1,2
2. Bob signed and submitted a maker buy order #2, to buy 1 Punk with 1 WETH and with no specified tokenIds.
3. Charlie signed and submitted a maker sell order #3, ask for 3 WETH for 2 Punk and specified tokenIds = 1,2
4. The match executor called matchOneToManyOrders() match Charlie's sell order #3 with buy order #1 and #2, Alice received 2 Punk, Charlie received 3 WETH, Bob paid 1 WETH and get nothing in return.

Recommendation

Change to:

function canExecMatchOneToMany(
    OrderTypes.MakerOrder calldata makerOrder,
    OrderTypes.MakerOrder[] calldata manyMakerOrders
  ) external view override returns (bool) {
    uint256 numItems;
    uint256 numConstructedItems;
    bool isOrdersTimeValid = true;
    bool itemsIntersect = true;
    uint256 ordersLength = manyMakerOrders.length;
    for (uint256 i = 0; i < ordersLength; ) {
      if (!isOrdersTimeValid || !itemsIntersect) {
        return false; // short circuit
      }

      numConstructedItems += manyMakerOrders[i].constraints[0];

      uint256 nftsLength = manyMakerOrders[i].nfts.length;
      for (uint256 j = 0; j < nftsLength; ) {
        numItems += manyMakerOrders[i].nfts[j].tokens.length;
        unchecked {
          ++j;
        }
      }

      isOrdersTimeValid =
        isOrdersTimeValid &&
        manyMakerOrders[i].constraints[3] <= block.timestamp &&
        manyMakerOrders[i].constraints[4] >= block.timestamp;

      itemsIntersect = itemsIntersect && doItemsIntersect(makerOrder.nfts, manyMakerOrders[i].nfts);

      unchecked {
        ++i;
      }
    }

    bool _isTimeValid = isOrdersTimeValid &&
      makerOrder.constraints[3] <= block.timestamp &&
      makerOrder.constraints[4] >= block.timestamp;

    uint256 currentMakerOrderPrice = _getCurrentPrice(makerOrder);
    uint256 sumCurrentOrderPrices = _sumCurrentPrices(manyMakerOrders);

    bool _isPriceValid = false;
    if (makerOrder.isSellOrder) {
      _isPriceValid = sumCurrentOrderPrices >= currentMakerOrderPrice;
    } else {
      _isPriceValid = sumCurrentOrderPrices <= currentMakerOrderPrice;
    }

    return (numItems == makerOrder.constraints[0]) && (numConstructedItems == numItems) && _isTimeValid && itemsIntersect && _isPriceValid;
  }

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1228-L1232

Vulnerability details

/// @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');
  }

Recommendation

Change to:

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

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L739

Vulnerability details

uint256 gasCost = (startGasPerOrder - gasleft() + wethTransferGasUnits) * tx.gasprice;

When the orders are matched by the MATCH_EXECUTOR, the gas cost of each order is paid by the buyer in WETH, the amount gasCost is calculated based on startGasPerOrder, gasleft() and gasprice.

And in the current implementation, startGasPerOrder is calculated based on numMakerOrders:

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L132-L149

function matchOneToOneOrders(
  OrderTypes.MakerOrder[] calldata makerOrders1,
  OrderTypes.MakerOrder[] calldata makerOrders2
) external {
  uint256 startGas = gasleft();
  uint256 numMakerOrders = makerOrders1.length;
  require(msg.sender == MATCH_EXECUTOR, 'OME');
  require(numMakerOrders == makerOrders2.length, 'mismatched lengths');

  // the below 3 variables are copied to memory once to save on gas
  // an SLOAD costs minimum 100 gas where an MLOAD only costs minimum 3 gas
  // since these values won't change during function execution, we can save on gas by copying them to memory once
  // instead of SLOADing once for each loop iteration
  uint16 protocolFeeBps = PROTOCOL_FEE_BPS;
  uint32 wethTransferGasUnits = WETH_TRANSFER_GAS_UNITS;
  address weth = WETH;
  for (uint256 i = 0; i < numMakerOrders; ) {
    uint256 startGasPerOrder = gasleft() + ((startGas - gasleft()) / numMakerOrders);

Because numMakerOrders is fixed, and startGas - gasleft() is nearly 0 for the first order, and will get larger by each order, at last, reach the highest value for the last order.

While the actual gas cost for the order is the gasleft() at L739 minus the gasleft() at L149, the last buyer of the last order will pay much more than the first user.

Actually, while the buyer of the first order pays only about the actual gas cost, the last user pays about 2x the actual gas cost.

PoC

Given:

• startGas = 1M + 300
• numMakerOrders = 10
• the gas cost from L136 to L148 is 300
• the gas cost for each order from L149 to L739 is 35k
• the gas cost for each order from L739 to L746 is 5k

For the 1st order:

• gasleft() at L149 is: 1M + 300 - 300 == 1M
• startGas - gasleft() is: 300
• startGasPerOrder is: 1M + (300 / 10)
• gasleft() at L739 is: 1M - 35k

gasCost for the first order: (1M + 30) - (1M - 35k) = 35k + 30.

When n > 1, for the Nth order:

• gasleft() at L149 is: 1M - (35k + 5k) * (n - 1)
• startGas - gasleft() is: (1M + 300) - (1M - (35k + 5k) * (n - 1))
• gasleft() at L739 is: 1M - (35k + 5k) * (n - 1) - (35k + 5k)

Then, gasCost for for the Nth order:

2. 44030
3. 48030
4. 52030
...
9. 72030
10. 76030 

The gasCost of the last (10th) order is 76k, which is about 2x the gasCost of the first order.

Recommendation

startGasPerOrder can be changed to gasleft().

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L893-L901

Vulnerability details

uint256 gasCost = (startGasPerOrder - gasleft() + wethTransferGasUnits) * tx.gasprice;
// if the execution currency is weth, we can send the protocol fee and gas cost in one transfer to save gas
// else we need to send the protocol fee separately in the execution currency
if (buy.execParams[1] == weth) {
  IERC20(weth).safeTransferFrom(buy.signer, address(this), protocolFee + gasCost);
} else {
  IERC20(buy.execParams[1]).safeTransferFrom(buy.signer, address(this), protocolFee);
  IERC20(weth).safeTransferFrom(buy.signer, address(this), gasCost);
}

In the current design/implementation, while the order is executed by the MATCH_EXECUTOR, the gas cost will always be paid by the maker order's buyer.

While the buyer did agreed to pay a certain price for certain NFTs, the actual cost for that maker buy order is unpredictable: because the MATCH_EXECUTOR can choose to execute the order while the network is extremly busy and the gas price is skyhigh.

As the gas cost at whatever gas price will be reimbursed by the buyer, the executor has no incentive to optimize and choose to execute the order at a lower gas cost.

The result is the buyer can sometimes end up paying much higher total price (incl. gas cost) for the items they bought.

Impact

While this is more of a design issue than a wrong implementation, the impact can be very severe for some users, and can cause defacto fund loss to the users who have they maker buy orders matched at a high gas price transactions.

Recommendation

Consider adding a new paramer to maker buy orders, maxGasCost to allow the buyer to limit the max gas they agreed to pay.

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L132-L146

Vulnerability details

function matchOneToOneOrders(
    OrderTypes.MakerOrder[] calldata makerOrders1,
    OrderTypes.MakerOrder[] calldata makerOrders2
  ) external {
    uint256 startGas = gasleft();
    uint256 numMakerOrders = makerOrders1.length;
    require(msg.sender == MATCH_EXECUTOR, 'OME');
    require(numMakerOrders == makerOrders2.length, 'mismatched lengths');

    // the below 3 variables are copied to memory once to save on gas
    // an SLOAD costs minimum 100 gas where an MLOAD only costs minimum 3 gas
    // since these values won't change during function execution, we can save on gas by copying them to memory once
    // instead of SLOADing once for each loop iteration
    uint16 protocolFeeBps = PROTOCOL_FEE_BPS;
    uint32 wethTransferGasUnits = WETH_TRANSFER_GAS_UNITS;

Per the comment:

Transfer fees. Fees are always transferred from buyer to the seller and the exchange although seller is the one that actually 'pays' the fees

And the code:

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L725-L729

    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
    uint256 remainingAmount = execPrice - protocolFee;
    _transferMultipleNFTs(sell.signer, buy.signer, sell.nfts);
    // transfer final amount (post-fees) to seller
    IERC20(buy.execParams[1]).safeTransferFrom(buy.signer, sell.signer, remainingAmount);

In the current design/implementation, the protocol fee is paid from the buyer's wallet, regardless of whether the buyer is the taker or the maker. And the protocol fee will be deducted from the execPrice, only the remainingAmount will be sent to the seller.

This is unconventional as if the buyer placed a limit order, say to sell 1 Punk for 100 ETH, it means that the seller is expected to receive 100 ETH. And now the seller must consider the fee rate and if they expect 100 ETH, the price must be set to 101 ETH.

While this is unconventional and a little inconvenience, it's still acceptable IF the protocol fee rate is fixed, OR the seller is the taker so that they can do the math and agrees to the protocol fee when they choose to fulfill the counterparty's maker order.

However, that's not always the case with the current implementation: the protocol can be changed, effective immediately, and applies to all existing orders.

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1265-L1269

    /// @dev updates exchange fees
  function setProtocolFee(uint16 _protocolFeeBps) external onlyOwner {
    PROTOCOL_FEE_BPS = _protocolFeeBps;
    emit NewProtocolFee(_protocolFeeBps);
  }

Plus, when the protocol fee rate updated to a higher rate, say from 5% to 50%, an maker order placed before this fee rate update can be fulfilled by a buyer, while the buyer still pays the same amount, the seller (maker) will receive 45% less than the initial sell order amount.

Recommendation

1. Consider making the protocol fee rate a constant, ie, can not be changed;
2. Or, consider changing to the protocol fee always be paid by the taker; while matching the maker buy and maker sell orders, the protocol fee must be paid from the price difference between the buy price and sell price;
3. Or, consider changing to the new protocol fee only applies to the orders created after the rate updated.

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L330-L342

Vulnerability details

  function _getCurrentPrice(OrderTypes.MakerOrder calldata order) internal view returns (uint256) {
    (uint256 startPrice, uint256 endPrice) = (order.constraints[1], order.constraints[2]);
    uint256 duration = order.constraints[4] - order.constraints[3];
    uint256 priceDiff = startPrice > endPrice ? startPrice - endPrice : endPrice - startPrice;
    if (priceDiff == 0 || duration == 0) {
      return startPrice;
    }
    uint256 elapsedTime = block.timestamp - order.constraints[3];
    uint256 PRECISION = 10**4; // precision for division; similar to bps
    uint256 portionBps = elapsedTime > duration ? PRECISION : ((elapsedTime * PRECISION) / duration);
    priceDiff = (priceDiff * portionBps) / PRECISION;
    return startPrice > endPrice ? startPrice - priceDiff : startPrice + priceDiff;
  }

For certain items, the diff of the start and end price of their auction can be small, eg 1% or 100 bps, and in the current implementation, the price will only change per bps due to precision loss.

For example, a ducth auction to sell a Punk for a price from $10M to 0 in 7 days.

When there is a counterparty maker buy order existing to buy a Punk at $69,690, the sell order can only be matched when it drops to $69,000, because the step of price drop is $10M / PRECISION == $1000.

As a result, the order can only be matched later at a lower price, the seller will lose $690.

Recommendation

Change to:

uint256 elapsedTime = block.timestamp - order.constraints[3];
if (elapsedTime < duration) {
    priceDiff = elapsedTime * priceDiff / duration;
}
return startPrice > endPrice ? startPrice - priceDiff : startPrice + priceDiff;