Infinity NFT Marketplace contest - unforgiven's results

Lines of code

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

Vulnerability details

Impact

function matchOneToManyOrders() suppose to match one order to many orders. the transferred tokens by this function most be valid based on what seller and buyer signed in their orders. but due to no check for many.constraints[0] (which shows home many tokens user wants to sell) in canExecMatchOneToMany() contract allow transferring more NFT from seller(when many orders are sell) to buyer. when many orders are sell, because seller signed order.constraints[0] (which shows hoe many tokens seller wants to sell), so contract most never allow that anything other than what user signed happens, the out of chain logic may have some bugs or malicious actor can take control of it.

Proof of Concept

This is canExecMatchOneToMany() code:

  /**
   * @notice Checks whether one to matches matches can be executed
   * @dev This function is called by the main exchange to check whether one to many matches can be executed.
          It checks whether orders have the right constraints - i.e they have the right number of items, whether time is still valid,
          prices are valid and whether the nfts intersect
   * @param makerOrder the one makerOrder
   * @param manyMakerOrders many maker orders
   * @return returns whether the order can be executed
   */
  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;
  }

As you can see there is no check for manyMakerOrders[i].constraints[0] and contract only checks that total number of tokens user's specified in many orders are equal to number of tokens one order wants to buy. but there are cases where seller wants to sell 1 token from list of tokens and this logic won't consider that. there is no other checks in matchOneToManyOrders() execution to check this many.constraints[0] and make sure in matching contract don't transfer more than what user has been signed. This example shows for what data vulnerability happens: (for simplicity let's assume collection, price, timestamp are valid and matching)

1. user1 signs this order: A:(user1 SELL 1 of IDs[1,2,3])
2. user2 signs this order: B:(user2 SELL 1 of IDS[4,5,6])
3. user3 sign this order: C:(user3 BUY 6 of IDS[1,2,3,4,5,6])
4. matching enging send this orders to matchOneToManyOrders(), manyOrders = [A,B] , oneOrder=C
5. contract code would check and verify orders and their matching and perform the orders but sends all IDS[1,2,3,4,5,6] from user1 and user2 to userr3

so when many orders are sell, because of inefficient checks for many.constraints[0] which shows how many tokens seller wants to sell contract could transfer more tokens than what user specified and signed in order.

Tools Used

VIM

Recommended Mitigation Steps

add checks for manyMakerOrders[i].constraint[0] in canExecMatchOneToMany() and make sure contract don't transfer more tokens that what user specified.

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L271-L312 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L59-L116 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L245-L294 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L118-L143 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L330-L364 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L934-L951 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L145-L164 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L171-L243

Vulnerability details

Impact

Function matchOneToManyOrders() and takeOrders() and matchOrders() suppose to match sell order to buy order and should perform some checks to ensure that user specified parameters in orders which are signed are not violated when order matching happens. but There is no check in their execution flow to check that an order has different NFT token ids in each one of it's collections, so even so number of tokens could be valid in order to order transfer but the number of real transferred tokens and their IDs can be different than what user specified and signed. and user funds would be lost. (because of ERC1155 there can be more than one token for a tokenId, so it would be possible to transfer it)

Proof of Concept

This is _takeOrders() and and code:

  /**
   * @notice Internal helper function to take orders
   * @dev verifies whether order can be executed
   * @param makerOrder the maker order
   * @param takerItems nfts to be transferred
   * @param execPrice execution price
   */
  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);
  }

As you can see it uses canExecTakeOrder() to check that it is valid to perform matching. This is canExecTakeOrder() and areTakerNumItemsValid() and doTokenIdsIntersect() code which are used in execution flow to check orders and matching validity:

  /**
   * @notice Checks whether take orders with a higher order intent can be executed
   * @dev This function is called by the main exchange to check whether take orders with a higher order intent can be executed.
          It checks whether orders have the right constraints - i.e they have the right number of items, whether time is still valid
          and whether the nfts intersect
   * @param makerOrder the maker order
   * @param takerItems the taker items specified by the taker
   * @return returns whether order can be executed
   */
  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));
  }

  /// @dev sanity check to make sure that a taker is specifying the right number of items
  function areTakerNumItemsValid(OrderTypes.MakerOrder calldata makerOrder, OrderTypes.OrderItem[] calldata takerItems)
    public
    pure
    returns (bool)
  {
    uint256 numTakerItems = 0;
    uint256 nftsLength = takerItems.length;
    for (uint256 i = 0; i < nftsLength; ) {
      unchecked {
        numTakerItems += takerItems[i].tokens.length;
        ++i;
      }
    }
    return makerOrder.constraints[0] == numTakerItems;
  }

  /**
   * @notice Checks whether tokenIds intersect
   * @dev This function checks whether there are intersecting tokenIds between two order items
   * @param item1 first item
   * @param item2 second item
   * @return returns whether tokenIds intersect
   */
  function doTokenIdsIntersect(OrderTypes.OrderItem calldata item1, OrderTypes.OrderItem calldata item2)
    public
    pure
    returns (bool)
  {
    uint256 item1TokensLength = item1.tokens.length;
    uint256 item2TokensLength = item2.tokens.length;
    // case where maker/taker didn't specify any tokenIds for this collection
    if (item1TokensLength == 0 || item2TokensLength == 0) {
      return true;
    }
    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
        ) {
          // increment numTokenIdsPerCollMatched
          unchecked {
            ++numTokenIdsPerCollMatched;
          }
          // short circuit
          break;
        }
        unchecked {
          ++l;
        }
      }
      unchecked {
        ++k;
      }
    }

    return numTokenIdsPerCollMatched == item2TokensLength;
  }

As you can see there is no logic to check that token IDs in one collection of order are different and code only checks that total number of tokens in one order matches the number of tokens specified and the ids in one order exists in other list defined. function doTokenIdsIntersect() checks to see that tokens ids in one collection can match list of specified tokens. because of this check lacking there are some scenarios that can cause fund lose for ERC1155 tokens (normal ERC721 requires more strange conditions). here is first example:

1. for simplicity let's assume collection and timestamp are valid and match for orders and token is ERC1155
2. user1 has signed this order: A:(user1 BUY 3 NFT IDs[(1,1),(2,1),(3,1)] at 15 ETH) (buy 1 token of each id=1,2,3)
3. NFT ID[1] fair price is 1 ETH, NFT ID[2] fair price is 2 ETH, NFT ID[3] fair price is 12 ETH
4. attacker who has 3 of NFT ID[1] create this list: B:(NFT IDs[(1,1), (1,1), (1,1)] ) (list to trade 1token of id=1 for 3 times)
5. attacker call takeOrders() with this parameters: makerOrder: A , takerNfts: B
6. contract logic would check all the conditions and validate and verify orders and their matching (they intersect and total number of token to sell is equal to total number of tokens to buy and all of the B list is inside A list) and perform the transaction.
7. attacker would receive 15 ETH for his 3 token of NFT ID[1] and steal user1 funds. user1 would receive 3 of NFT ID[1] and pays 15 ETH and even so his order A has been executed he doesn't receive NFT IDs[(2,1),(3,1)] and contract would violates his signed parameters.

This examples shows that in verifying one to many order code should verify that one order's one collection's token ids are not duplicates. (the function doTokenIdsIntersect() doesn't check for this).

This scenario is performable to matchOneToManyOrders() and matchOrders() and but exists in their code (related check logics) too. more important things about this scenario is that it doesn't require off-chain maching engine to make mistake or malicious act, anyone can call takeOrders() if NFT tokens are ERC1155. for other NFT tokens to perform this attack it requires that seller==buyer or some other strange cases (like auto selling when receiving in one contract).

Tools Used

VIM

Recommended Mitigation Steps

add checks to ensure order's one collection's token ids are not duplicate in doTokenIdsIntersect()

Lines of code

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

Vulnerability details

Impact

function rescueETH() is supposed to transfer fee pain in ETH to destination address but its logic is wrong and can't transfer ETH balance of contract and ETH fees would stuck in contract address forever.

Proof of Concept

This is rescueETH() code:

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

As you can see code only transfers msg.value amount of ETH to destination address (msg.value is amount of ETH that caller sent to contract). so it only transfers what ETH amount it received from owner and not from ETH balance of contract which is ETH fees. so ETH fees would stuck in contract address and it would be lost forever.

Tools Used

VIM

Recommended Mitigation Steps

add amount as argument and send amount of ETH to destination address.

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L296-L328 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L330-L364 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1152-L1165

Vulnerability details

Impact

Order's currency can be ETH and buyer need to pay ETH to get NFT tokens. price of orders an be changed by time, so buyer can't know the exact amount of ETH to buy NFT tokens and he needs to send extra ETH to takeMultipleOneOrders() and takeOrders() so transaction won't fail and take those orders. but contract code don't return the extra amount of ETH to the buyer.

Proof of Concept

This is takeOrders() code:

  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);
      unchecked {
        ++i;
      }
    }
    // check to ensure that for ETH orders, enough ETH is sent
    // for non ETH orders, IERC20 safeTransferFrom will throw error if insufficient amount is sent
    if (isMakerSeller && currency == address(0)) {
      require(msg.value >= totalPrice, 'invalid total price');
    }
  }

As you can see in the last lines contract checks that msg.value >= totalPrice when sender tries to buy and currency is ETH. but buyer needs to send extra ETH when he is signing the transaction because the exact price of orders are changing over time(for Dutch auction orders) and if buyer send low amount transaction could fail (there is some time difference between transaction signing and transaction execution in blockchain). so buyer would send extra ETH to take those orders but contract don't send back extra ETH that buyer sent and every time someone buys NFT tokens with ETH he is paying more ETH than he is supposed to.

The takeMultipleOneOrders() is similar.

This is _getCurrentPrice() code which shows that price of orders change based on timestamp and there is no way for buyer to know exact amount of ETH he needs to send to contract: (he is forced to send extra amount)

  /// @dev Gets current order price for orders that vary in price over time (dutch and reverse dutch auctions)
  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;
  }

Tools Used

VIM

Recommended Mitigation Steps

return the extra amount of ETH

Lines of code

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

Vulnerability details

Impact

function matchOrders() suppose to match seller and buyer orders based on NFTs defined in constructs tokens. the defined tokens most be valid based on what seller and buyer signed in their orders. but due to wrong check for sell.constraints[0] (which shows home many tokens seller wants to sell) contract allow transferring more NFT from seller to buyer if constructedNfts is have set wrongly. because seller signed sell.constraints[0], so contract most never allow that anything other than what user signed happens, the out of chain logic may have some bugs or malicious actor can take control of it.

Proof of Concept

This is areNumItemsValid() code in InfinityOrderBookComplication contract:

  /// @dev sanity check to make sure the constructed nfts conform to the user signed constraints
  function areNumItemsValid(
    OrderTypes.MakerOrder calldata sell,
    OrderTypes.MakerOrder calldata buy,
    OrderTypes.OrderItem[] calldata constructedNfts
  ) public pure returns (bool) {
    uint256 numConstructedItems = 0;
    uint256 nftsLength = constructedNfts.length;
    for (uint256 i = 0; i < nftsLength; ) {
      unchecked {
        numConstructedItems += constructedNfts[i].tokens.length;
        ++i;
      }
    }
    return numConstructedItems >= buy.constraints[0] && buy.constraints[0] <= sell.constraints[0];
  }

As you can see in the last line it checks that numConstructedItems >= buy.constraints[0] but it don't check that numConstructedItems <= sell.constraints[0] As numConstructedItems is the number of items that are going to transferred from seller to buyer so numConstructedItems can be very higher than what seller defined to sell (checking buy.constraints[0] <= sell.constraints[0] is inefficient).

Function matchOrders() is for matching orders one to one where no specific NFTs are specified. and constructs shows what NFT ids should be transferred from seller to buyer. when matchOrders() get called the execution follow reach areNumItemsValid() to check that number of NFTs transferred from seller to buyer is valid based on what seller and buyer defined for their sell amount or buy amount (which is defined in constraints[0]). because areNumItemValid()'s logic is wrong it's possible match orders and transfer NFTs from seller more than what seller defined. this the detailed steps: 0. for simplicity let's assume NFT collection for all orders are same and price and timestamp of orders are valid and match together.

1. seller wants to sell from IDs[1,2,3,4,5,6,7,8,9,10] only 3 (sell.constraints[0] == 3) of them.
2. buyer wants to buy 2 of IDS [1,2,3,4,5] (buy.constraints[0] == 2).
3. constructedNfts is set to IDs[1,2,3,4] by matching engine(let's assume price and timestamp and collection of orders are matched correctly).
4. off-chain matching engine calls matchOrders() with above data.
5. all the checks in matchOrders()'s execution would be passed (canExecMatchOrder() would return true because all intersect check are valid and areNumItemsValid() would return true too)
6. contract would match orders and transfer 4 NFT IDS[1,2,3,4] from seller to buyer even so seller was signed 3 NFT id to be sold.

This bug is critical because contract don't fully check that all the signed values by seller is valid when matching transactions and users are not sure that what is going to happen when they are signing their orders. if there are some logics in off-chain codes, it should only be able to chose what order matches other order and it shouldn't be able to perform matching in a way that it violates what users signed in their orders, and there should be full checks in code that to make sure orders would match in a way that they don't violate what signers signed.

Tools Used

VIM

Recommended Mitigation Steps

change the return line in areNumItemsValid() to this:

return numConstructedItems >= buy.constraints[0] && numConstructedItems  <= sell.constraints[0]

Lines of code

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

Vulnerability details

Impact

Functions matchOneToOneOrders(), matchOneToManyOrders(), matchOrders(), takeMultipleOneOrders(), takeOrders() are for order matching and order execution and they validate different things about orders but there is no check for that seller != buyer, which can cause wrong order matching resulting in fund lose or fund theft or griefing. (it can be combined with other vulns to perform more damaging attacks)

Proof of Concept

We only give proof of concept for matchOneToManyOrders() and other order execution/matching functions has similar bugs which root cause is not checking seller != buyer. This is matchOneToManyOrders() code:

  /**
   @notice Matches one  order to many orders. Example: A buy order with 5 specific NFTs with 5 sell orders with those specific NFTs.
   @dev Can only be called by the match executor. Refunds gas cost incurred by the
        match executor to this contract. Checks whether the given complication can execute the match.
   @param makerOrder The one order to match
   @param manyMakerOrders Array of multiple orders to match the one order against
  */
  function matchOneToManyOrders(
    OrderTypes.MakerOrder calldata makerOrder,
    OrderTypes.MakerOrder[] calldata manyMakerOrders
  ) external {
    uint256 startGas = gasleft();
    require(msg.sender == MATCH_EXECUTOR, 'OME');
    require(_complications.contains(makerOrder.execParams[0]), 'invalid complication');
    require(
      IComplication(makerOrder.execParams[0]).canExecMatchOneToMany(makerOrder, manyMakerOrders),
      'cannot execute'
    );
    bytes32 makerOrderHash = _hash(makerOrder);
    require(isOrderValid(makerOrder, makerOrderHash), 'invalid maker order');
    uint256 ordersLength = manyMakerOrders.length;
    // 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;
    if (makerOrder.isSellOrder) {
      for (uint256 i = 0; i < ordersLength; ) {
        // 20000 for the SSTORE op that updates maker nonce status from zero to a non zero status
        uint256 startGasPerOrder = gasleft() + ((startGas + 20000 - gasleft()) / ordersLength);
        _matchOneMakerSellToManyMakerBuys(
          makerOrderHash,
          makerOrder,
          manyMakerOrders[i],
          startGasPerOrder,
          protocolFeeBps,
          wethTransferGasUnits,
          weth
        );
        unchecked {
          ++i;
        }
      }
      isUserOrderNonceExecutedOrCancelled[makerOrder.signer][makerOrder.constraints[5]] = true;
    } else {
      uint256 protocolFee;
      for (uint256 i = 0; i < ordersLength; ) {
        protocolFee += _matchOneMakerBuyToManyMakerSells(
          makerOrderHash,
          manyMakerOrders[i],
          makerOrder,
          protocolFeeBps
        );
        unchecked {
          ++i;
        }
      }
      isUserOrderNonceExecutedOrCancelled[makerOrder.signer][makerOrder.constraints[5]] = true;
      uint256 gasCost = (startGas - gasleft() + WETH_TRANSFER_GAS_UNITS) * 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
      // since the buyer is common across many sell orders, this part can be executed outside the above for loop
      // in contrast to the case where if the one order is a sell order, we need to do this in each for loop
      if (makerOrder.execParams[1] == weth) {
        IERC20(weth).safeTransferFrom(makerOrder.signer, address(this), protocolFee + gasCost);
      } else {
        IERC20(makerOrder.execParams[1]).safeTransferFrom(makerOrder.signer, address(this), protocolFee);
        IERC20(weth).safeTransferFrom(makerOrder.signer, address(this), gasCost);
      }
    }
  }

in its executions it calls InfinityOrderBookComplication.canExecMatchOneToMany(), verifyMatchOneToManyOrders(), isOrderValid() to see that if orders are valid and one order matched to all other orders but there is no check for seller != buyer in any of those functions. and also ERC721 and ERC20 allows funds and assets to be transferred from address to itself. So it's possible for matchOneToManyOrders() to match one user sell orders to its buy orders which can cause fund theft or griefing. This is the scenario for fund lose in matchOneToManyOrders():

1. let's assume orders NFT ids are for one collection for simplicity.
2. NFT ID[1] fair price is 8 ETH and NFT ID[2] fair price is 2 ETH.
3. user1 wants to buy NFT IDs[1,2] at 10 ETH (both of them) so he create one buy order and signs it.
4. user1 wants to sell NFT ID[1] at 2.5 ETH and sell NFT ID[2] at 8.5 ETH. and he wants to sell them immediately after buying them so he create this two sell orders and sign them.
5. attacker who has NFT ID[1] creates an sell order for it at 7.5 ETH and signs it.
6. off-chain machining engine sends this orders to matchOneToManyOrders(): many orders = [(attacker sell ID[1] at 7.5 ETH) , (user1 sell ID[1] at 2.5 ETH)] , one order = (user1 buy IDs[1,2] at 10ETH)
7. function matchOneToManyOrders() logic will check orders and their matching and all the checks would be passed for matching one order to many order(becase tokens lists intersects and numTokens are valids too (1+1=2))
8. function matchOneToManyOrders() would execute order and transfer funds and tokens which would result in: (transferring 7.5 ETH from user 1 to attacker) (transferring 2.5 ETH from user1 to user1) (transferring NFT ID[1] from attacker to user1) (transferring NFT ID[1] from user1 to user1)
9. so in the end contract executed user1 buy order (user1 buy IDs[1,2] at 10ETH) but user only received NFT ID[1] and didn't received NFT ID[2] so contract code perform operation contradiction to what user1 has been signed.

Of course for this attack to work for matchOneToManyOrders() off-chain matching engine need to send wrong data but checks on the contract are not enough.

There are other scenarios for other functions that can cause griefing, for example for function matchOrders(): a user can have multiple order to buy some tokens in list of ids. it's possible to match these old orders:

1. user1 has this order: A:(user1 BUY 1 of IDs[1,2,3]) and B:(user1 BUY 1 of IDs[1,4,5])
2. then the order B get executed for ID[1] and user1 become the owner of ID[1]
3. user1 wants to sell some of his tokens so he signs this order: C::(user1 SELL 1 of IDs[1,6,7])
4. matching engine would send order A and C with constructedNfts=ID[1] to matchOrders().
5. matchOrders() would check conditions and would see that conditions are met and perform the transaction.
6. user1 would pay some unnecessary order fee and it would become like griefing and DOS attack for him.

There may be other scenarios for this vuln to be harmful for users.

Tools Used

VIM

Recommended Mitigation Steps

add some checks to ensure that seller != buyer

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L287-L325

Vulnerability details

Impact

function _updateUserStakedAmounts() is supposed to update user staked amounts and it sets userstakedAmounts[user][].timestamp=0 when userstakedAmounts[user][].amount is 0x0. but there are some cases that code logic don't handle them and amount become 0x0 and code don't set timestamp to 0x0. so any logic that is depended on timestamp==0 when amount==0 could fail, to my understanding setting timestamp is for gas efficiency.

Proof of Concept

This is _updateUserStakedAmounts() code:

  /** @notice Update user staked amounts for different duration on unstake
    * @dev A more elegant recursive function is possible but this is more gas efficient
   */
  function _updateUserStakedAmounts(
    address user,
    uint256 amount,
    uint256 noVesting,
    uint256 vestedThreeMonths,
    uint256 vestedSixMonths,
    uint256 vestedTwelveMonths
  ) internal {
    if (amount > noVesting) {
      userstakedAmounts[user][Duration.NONE].amount = 0;
      userstakedAmounts[user][Duration.NONE].timestamp = 0;
      amount = amount - noVesting;
      if (amount > vestedThreeMonths) {
        userstakedAmounts[user][Duration.THREE_MONTHS].amount = 0;
        userstakedAmounts[user][Duration.THREE_MONTHS].timestamp = 0;
        amount = amount - vestedThreeMonths;
        if (amount > vestedSixMonths) {
          userstakedAmounts[user][Duration.SIX_MONTHS].amount = 0;
          userstakedAmounts[user][Duration.SIX_MONTHS].timestamp = 0;
          amount = amount - vestedSixMonths;
          if (amount > vestedTwelveMonths) {
            userstakedAmounts[user][Duration.TWELVE_MONTHS].amount = 0;
            userstakedAmounts[user][Duration.TWELVE_MONTHS].timestamp = 0;
          } else {
            userstakedAmounts[user][Duration.TWELVE_MONTHS].amount -= amount;
          }
        } else {
          userstakedAmounts[user][Duration.SIX_MONTHS].amount -= amount;
        }
      } else {
        userstakedAmounts[user][Duration.THREE_MONTHS].amount -= amount;
      }
    } else {
      userstakedAmounts[user][Duration.NONE].amount -= amount;
    }
  }

for example if amount=noVesting then code would execute line: userstakedAmounts[user][Duration.NONE].amount -= amount; which sets the userstakedAmounts[user][Duration.NONE].amount to 0x0 but userstakedAmounts[user][Duration.NONE].timestamp won't change. As as in all other logics when amount is 0x0 code set timestamp to 0x0 too but here that logic is not happening for this cases (amount equal to noVesting or noVesting + vestedThreeMonths or ...).

Tools Used

VIM

Recommended Mitigation Steps

change if conditions from > to >=, so for equal case the code set timestamp to 0x0 too.