code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityOrderBookComplication.sol#L237-L242

Vulnerability details

Impact

The protocol has a feature that an order can specify any token ids by default (OrderItem is empty), leading to users misunderstanding and getting unexpected behavior.

Proof of Concept

In doItemsIntersect, it doesn't specify any items by default if OrderItem is empty: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityOrderBookComplication.sol#L237-L242

    uint256 order1NftsLength = order1Nfts.length;
    uint256 order2NftsLength = order2Nfts.length;
    // case where maker/taker didn't specify any items
    if (order1NftsLength == 0 || order2NftsLength == 0) {
      return true;
    }

If Alice wants to sell id 10 of collection A with 1 ETH, and want to buy any ids of collection A with 2 ETH, she creates two orders in same time:

order1: collection A, id 10, 1 ETH, isSellOrder = true
order2: collection A, any ids, 2 ETH, isSellOrder = false

An attacker can first fulfill order1 and get id 10 of collection A, then sell the same id 10 token to Alice again by order2. Alice will lose 1 ETH.

Tools Used

None

Recommended Mitigation Steps

Consider adding excludeTokens for excluding token ids:

  struct OrderItem {
    address collection;
    TokenInfo[] tokens;
    TokenInfo[] excludeTokens;
  }

nneverlander commented 2 years ago

This is very interesting indeed. Will implement in a future updgrade. For now will implement this check offchain in our matching engine

HardlyDifficult commented 2 years ago

Adding excludeTokens may be a nice improvement to make. Lowering risk and merging with the warden's QA report #232

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