search

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

4 stars 0 forks source link

`matchOneToManyOrders` doesn't consider `numItems` of the orders #314

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L178

Vulnerability details

Impact

An order can specify a numItems in MakerOrder.constraints[0]. This number is the min/max number of items the order wants to buy/sell. For example a buy order can provide a list of nfts and say that wants to buy only 3 of them from that list. The function matchOneToManyOrders has an issue where it doesn't check the numItems of the "many" orders. This leads to orders selling more items than specified by the owner.

Proof of Concept

As an example, let's consider two orders: 1) (Order1) buy 3 items from [Punk1, Punk2, Punk3] 2) (Order2) sell 1 item from [Punk1, Punk2, Punk3]

We can call matchOneToManyOrders(makerOrder, manyMakerOrders) with makerOrder = Order1 and manyMakerOrders = [Order2]. The result is that all 3 items get transferred, contrary to the will of Order2's signer.

I've proved this example in a hardhat test, link to gist.

Recommended Mitigation Steps

Check somewhere that manyMakerOrders[i].constraints[0] is valid during the execution of matchOneToManyOrders.

ghost commented 2 years ago

numItems is checked inside doTokenIdsIntersect on line 293.

nneverlander commented 2 years ago

Check this: https://github.com/infinitydotxyz/exchange-contracts-v2/commit/7f0e195d52165853281b971b8610b27140da6e41

nneverlander commented 2 years ago

https://github.com/code-423n4/2022-06-infinity-findings/issues/131

HardlyDifficult commented 2 years ago

The hardhat test included here makes it easy to understand and confirm this issue. Thank you! Because of that detail I'm inclined to make this report the primary.

This is a High risk issue. Effectively the system supports 2 relevant criteria on orders here - which tokens and how many, but the latter is not checked in this flow. This allows a buyer to purchase more NFTs from the seller than they intended to sell.

254 is very similar but flips the impact to explore how a buyer's offer could be attacked and how empty token lists are relevant. Duping into that issue.