Infinity NFT Marketplace contest - hyh's results

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.

Lines of code

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

Vulnerability details

ETH fees accumulated from takeOrders() and takeMultipleOneOrders() operations are permanently frozen within the contract as there is only one way designed to retrieve them, a rescueETH() function, and it will work as intended, not being able to access ETH balance of the contract.

Setting the severity as high as the case is a violation of system's core logic and a permanent freeze of ETH revenue of the project.

Proof of Concept

Fees are accrued in user-facing takeOrders() and takeMultipleOneOrders() via the following call sequences:

takeOrders -> _takeOrders -> _execTakeOrders -> _transferNFTsAndFees -> _transferFees
takeMultipleOneOrders -> _execTakeOneOrder -> _transferNFTsAndFees -> _transferFees

While token fees are transferred right away, ETH fees are kept with the InfinityExchange contract:

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

  /**
   * @notice Transfer fees. Fees are always transferred from buyer to the seller and the exchange although seller is 
            the one that actually 'pays' the fees
   * @dev if the currency ETH, no additional transfer is needed to pay exchange fees since the contract is 'payable'
   * @param seller the seller
   * @param buyer the buyer
   * @param amount amount to transfer
   * @param currency currency of the transfer
   */
  function _transferFees(
    address seller,
    address buyer,
    uint256 amount,
    address currency
  ) internal {
    // protocol fee
    uint256 protocolFee = (PROTOCOL_FEE_BPS * amount) / 10000;
    uint256 remainingAmount = amount - protocolFee;
    // ETH
    if (currency == address(0)) {
      // transfer amount to seller
      (bool sent, ) = seller.call{value: remainingAmount}('');
      require(sent, 'failed to send ether to seller');

I.e. when currency is ETH the fee part of the amount, protocolFee, is left with the InfinityExchange contract.

The only way to retrieve ETH from the contract is rescueETH() function:

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

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

However, it cannot reach ETH on the contract balance as msg.value is used as the amount to be sent over. I.e. only ETH attached to the rescueETH() call is transferred from owner to destination. ETH funds that InfinityExchange contract holds remain inaccessible.

Recommended Mitigation Steps

Consider adding contract balance to the funds transferred:

  /// @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}('');
+   (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#L1056-L1073

Vulnerability details

Malicious maker can list an NFT that conforms to ERC-165, but reports that it's neither ERC721, nor ERC1155, i.e. both supportsInterface(0x80ac58cd) and supportsInterface(0xd9b67a26) are false. In all other regards it can be fully valid NFT, for example having well-know NFT, say Crypto punk, in a transparent custody, and having no other issues.

When a taker attempts to buy it, however, the InfinityExchange simply will not transfer it to him, while transferring his bid to the maker, who steals from the taker this way. As this specifics of the NFT isn't malicious per se, i.e. it doesn't steal simply by reporting the wrong state of interface support, it will not be flagged as or known to be faulty.

Proof of Concept

_transferNFTs performs a noop when NFT doesn't conform to both standards:

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

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

However, at this point all other obligations are considered to be met and maker's bid for the NFT is transferred to taker, who can steal it this way by listing such an NFT that simply do not support both interfaces.

Recommended Mitigation Steps

Consider requiring conformity and performing unconditional transfer, so there can be no case of unmet obligations.

1. Rage quit require do not control anything (low)

Proof of Concept

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

    require(totalStaked >= 0, 'nothing staked to rage quit');

Recommended Mitigation Steps

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

    require(totalStaked > 0, 'nothing staked to rage quit');

2. InfinityExchange and InfinityStaker fallback functions aren't needed (low)

The case when contract has received ether and there is no data is covered by receive(), i.e. fallback() isn't needed.

Proof of Concept

InfinityExchange and InfinityStaker has both payable fallback() and receive():

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

  fallback() external payable {}

  receive() external payable {}

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

  // Fallback
  fallback() external payable {}

  receive() external payable {}

Recommended Mitigation Steps

Consider leaving only receive() in both cases. Also, consider removing both for InfinityStaker as there is no need to receive ether there.

3. There is no pausing in InfinityExchange's user facing functions (low)

Proof of Concept

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

  /**
   @notice Batch buys or sells orders where maker orders can have unspecified NFTs. Transaction initiated by an end user.
   @param makerOrders The orders to fulfill
   @param takerNfts The specific NFTs that the taker is willing to take that intersect with the higher order intent of the maker
   Example: If a makerOrder is 'buy any one of these 2 specific NFTs', then the takerNfts would be 'this one specific NFT'.
  */
  function takeOrders(OrderTypes.MakerOrder[] calldata makerOrders, OrderTypes.OrderItem[][] calldata takerNfts)
    external
    payable
    nonReentrant
  {

Pausing functionality is already introduced in Staking:

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

  /**
   * @notice Untake tokens
   * @dev Storage updates are done for each stake level. See _updateUserStakedAmounts for more details
   * @param amount Amount of tokens to unstake
   */
  function unstake(uint256 amount) external override nonReentrant whenNotPaused {

Recommended Mitigation Steps

Consider expanding pausing to cover all user facing functions of InfinityExchange: takeOrders(), takeMultipleOneOrders(), transferMultipleNFTs(). For example, by introducing of the whenNotPaused modifier.

4. InfinityExchange events aren't indexed (non-critical)

Proof of Concept

InfinityExchange has core event not indexed:

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

  event CancelAllOrders(address user, uint256 newMinNonce);
  event CancelMultipleOrders(address user, uint256[] orderNonces);
  event NewWethTransferGasUnits(uint32 wethTransferGasUnits);
  event NewProtocolFee(uint16 protocolFee);

InfinityToken also:

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/token/InfinityToken.sol#L35

  event EpochAdvanced(uint256 currentEpoch, uint256 supplyMinted);

Recommended Mitigation Steps

Consider adding indexes to ids and addresses in the all important events to improve their usability

5. There is no limit for new PROTOCOL_FEE_BPS (non-critical)

Owner can set PROTOCOL_FEE_BPS higher than 100%, as an operational mistake or with a malicious intent.

Proof of Concept

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

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

Recommended Mitigation Steps

Require PROTOCOL_FEE_BPS to be within a predefined range, say in [0, 10%]

6. Basis points precision is hard coded (non-critical)

Proof of Concept

There are several instances across the code where 10000 is used:

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

    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;

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

    uint256 protocolFee = (PROTOCOL_FEE_BPS * amount) / 10000;

Recommended Mitigation Steps

Introduce the constant to reduce operational errors on future system development.

The cycle in InfinityExchange performs expensive enough checks more times than it's needed:

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

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

Recommended Mitigation Steps

The cycle can be rewritten this way:

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

    for (uint256 i = 0; ; ) {
      uint256 execPrice = _getCurrentPrice(makerOrders[i]);
      totalPrice += execPrice;
      _takeOrders(makerOrders[i], takerNfts[i], execPrice);
      unchecked {
		if (++i >= ordersLength) break;
		require(currency == makerOrders[i].execParams[1], 'cannot mix currencies');
		require(isMakerSeller == makerOrders[i].isSellOrder, 'cannot mix order sides');
      }
    }