Lines of code

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

Vulnerability details

Maker bid for a bundle of ERC-1155 items can be tricked into successful execution by providing several instances of the cheapest item instead of the required bundle. This way a malicious taker can receive full maker's price, providing several instances of the least valuable NFT, in result stealing from the maker the difference between maker's bid and total value of the provided NFTs.

Say maker provides 10 tokens as a bid, aiming to buy 1st ERC-1155 item with quantity of 1 that has market price of 9 tokens, and 2nd item with quantity of 1 from the same collection, which have a price of 1 tokens, i.e. the bid is marked to the current market. Malicious taker can provide 2nd item with the quantity of 2, which worth 2 tokens in total, obtaining the full 10 token bid of the maker. Total amount stolen can be up to the whole maker's bid when the required bundle has items with near-zero market value. The required quantity of these items can be supplied by the taker to obtain the whole maker's bid.

Setting high severity as that's the violation of the core logic of the system, and the maximum loss of a maker is limited only by total exposure. Also, there are no preconditions for the attack, any ERC-1155 item bundle bid can be executed this way. I.e. when:

1) bid is a bundle of items from a collection, has more than one item, makerOrder.constraints[0] > 1 2) bid has different ids and those ids have different market value

the attack of providing the several items from the cheapest id instead of the bundle is possible.

Proof of Concept

The system checks for NFT validity via areTakerNumItemsValid() and doItemsIntersect() methods called in canExecTakeOrder(), whose success is then required.

areTakerNumItemsValid() completes successfully as makerOrder.constraints[0] is 2 (using the above example) as maker wants the costly and the cheap item, 2 in total, and sum of lengths in the offer is also 2 as taker provided two cheap items with the required quantity.

doItemsIntersect() is called with makerOrder.nfts as order1Nfts, takerItems as order2Nfts. For each items in order2Nfts, which is taker's [cheap, cheap] array, it runs a full search for collection in order1Nfts = makerOrder.nfts. When it founds the match, doTokenIdsIntersect() is called.

doTokenIdsIntersect() returns true as cheap NFT is indeed present in maker's list. But this happens two times with taker's 2 cheap items being matched with the very same maker's cheap item as full cycle is performed each time.

In more details the call sequence is:

After the unrelated checks user facing takeOrders() calls _takeOrders():

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

  function takeOrders(OrderTypes.MakerOrder[] calldata makerOrders, OrderTypes.OrderItem[][] calldata takerNfts)
    external
    payable
    nonReentrant
  {
    uint256 ordersLength = makerOrders.length;
    require(ordersLength == takerNfts.length, 'mismatched lengths');
    uint256 totalPrice;
    address currency = makerOrders[0].execParams[1];
    bool isMakerSeller = makerOrders[0].isSellOrder;
    if (!isMakerSeller) {
      require(currency != address(0), 'offers only in ERC20');
    }
    for (uint256 i = 0; i < ordersLength; ) {
      require(currency == makerOrders[i].execParams[1], 'cannot mix currencies');
      require(isMakerSeller == makerOrders[i].isSellOrder, 'cannot mix order sides');
      uint256 execPrice = _getCurrentPrice(makerOrders[i]);
      totalPrice += execPrice;
      _takeOrders(makerOrders[i], takerNfts[i], execPrice);

_takeOrders() calls IComplication(makerOrder.execParams[0]).canExecTakeOrder(makerOrder, takerItems):

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

  function _takeOrders(
    OrderTypes.MakerOrder calldata makerOrder,
    OrderTypes.OrderItem[] calldata takerItems,
    uint256 execPrice
  ) internal {
    bytes32 makerOrderHash = _hash(makerOrder);
    bool makerOrderValid = isOrderValid(makerOrder, makerOrderHash);
    bool executionValid = IComplication(makerOrder.execParams[0]).canExecTakeOrder(makerOrder, takerItems);
    require(makerOrderValid && executionValid, 'order not verified');
    _execTakeOrders(makerOrderHash, makerOrder, takerItems, makerOrder.isSellOrder, execPrice);

canExecTakeOrder() checks time, then areTakerNumItemsValid() and doItemsIntersect():

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

  function canExecTakeOrder(OrderTypes.MakerOrder calldata makerOrder, OrderTypes.OrderItem[] calldata takerItems)
    external
    view
    override
    returns (bool)
  {
    return (makerOrder.constraints[3] <= block.timestamp &&
      makerOrder.constraints[4] >= block.timestamp &&
      areTakerNumItemsValid(makerOrder, takerItems) &&
      doItemsIntersect(makerOrder.nfts, takerItems));
  }

doItemsIntersect() runs the external cycle twice for taker's [cheap, cheap] order2Nfts, that is matched with the same cheap item in the maker's list as internal order1Nfts cycle is full each time.

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

  function doItemsIntersect(OrderTypes.OrderItem[] calldata order1Nfts, OrderTypes.OrderItem[] calldata order2Nfts)
    public
    pure
    returns (bool)
  {
    uint256 order1NftsLength = order1Nfts.length;
    uint256 order2NftsLength = order2Nfts.length;
    // case where maker/taker didn't specify any items
    if (order1NftsLength == 0 || order2NftsLength == 0) {
      return true;
    }

    uint256 numCollsMatched = 0;
    // check if taker has all items in maker
    for (uint256 i = 0; i < order2NftsLength; ) {
      for (uint256 j = 0; j < order1NftsLength; ) {
        if (order1Nfts[j].collection == order2Nfts[i].collection) {
          // increment numCollsMatched
          unchecked {
            ++numCollsMatched;
          }
          // check if tokenIds intersect
          bool tokenIdsIntersect = doTokenIdsIntersect(order1Nfts[j], order2Nfts[i]);
          require(tokenIdsIntersect, 'tokenIds dont intersect');
          // short circuit
          break;
        }

Recommended Mitigation Steps

Consider introducing memory bool array with the same size as item1Tokens and marking the ids found in the maker's list, then ignoring previously found ones to prohibit double counting described.

For example:

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

    uint256 numTokenIdsPerCollMatched = 0;
    for (uint256 k = 0; k < item2TokensLength; ) {
      for (uint256 l = 0; l < item1TokensLength; ) {
        if (
-         item1.tokens[l].tokenId == item2.tokens[k].tokenId && item1.tokens[l].numTokens == item2.tokens[k].numTokens
+         item1.tokens[l].tokenId == item2.tokens[k].tokenId && item1.tokens[l].numTokens == item2.tokens[k].numTokens && !item1TokensMatched[l]
        ) {

This will prevent supplying same id with the required quantity many times to deliver the cheapest item repeatedly instead of the required bundle.

code-423n4 / 2022-06-infinity-findings

Taker can provide several instances of the cheapest ERC-1155 item in a multi item bundle #332

Lines of code

Vulnerability details

Proof of Concept

Recommended Mitigation Steps