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

4 stars 0 forks source link

MATCH_EXECUTOR can submit wrong constructs in matchOrders #226

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#L274 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityOrderBookComplication.sol#L137 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityOrderBookComplication.sol#L205

Vulnerability details

MATCH_EXECUTOR can transfer more tokens than the user signed for. This is due to insufficient validation in canExecMatchOrder. The user could have signed an order to sell 2 ERC1155 tokens of id #1, and the match executor can successfully submit a matching that will transfer 4 such tokens.

Impact

User funds could be lost. I've labeled the severity as high because although this requires a faulty MATCH_EXECUTOR, I think this is a relatively severe logic bug. It breaks the basic trust assumption of users: that they can't be "billed" from the smart contract more tokens than they signed for.

Proof of Concept

In short: the MATCH_EXECUTOR can submit in matchOrders's parameter constructs the same OrderItem twice. There's no validation for this.

In detail: I will walk through such execution and show that there is no validation.

Let's say Alice signs a buy order for 5 ERC1155 of ID#1, And Bob signs a sell order for 5 ERC1155 of ID#1. The OrderItem would be [collectionAddress, [1, 5]]. Let's say the MATCH_EXECUTOR submits a tx to matchOrders with Alice and Bob's orders, and constructs that is [[collectionAddress, [1, 5]],[collectionAddress, [1, 5]]].

matchOrders will verify that the tx is valid by calling canExecMatchOrder. canExecMatchOrder will verify using the signed orders that the price and timings are right, and then will call areNumItemsValid(sell, buy, constructedNfts). areNumItemsValid will verify that the number of OrderItems in constructs is >= buy.constraints[0] (2>=1), and that buy.constraints[0] <= sell.constraints[0] (1<=1).

Then canExecMatchOrder will call doItemsIntersect(sell.nfts, constructedNfts). This function will check that for every OrderItem in constructedNfts, there's a matching OrderItem in sell.nts. And there is. The function is not checking whether there are duplicates.

Then canExecMatchOrder will similarly check that every element in constructedNfts is in buy.nfts and every element in buy.nfts is in sell.nfts.

All these checks will pass, and the contract would end up transferring constructedNfts which contains double the amount the seller intended to sell.

Recommended Mitigation Steps

I believe adding a sanity check to areNumItemsValid that the number of OrderItems in constructedNfts is <= sell.constraints[0] should be enough to fix this, but honestly I am not sure. Still investigating the contract and not much time left.

nneverlander commented 2 years ago

Fixed in https://github.com/infinitydotxyz/exchange-contracts-v2/commit/60ba1561e14f46e64844484a3931f6943c6fa038

We also removed support for ERC1155

nneverlander commented 2 years ago

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

HardlyDifficult commented 2 years ago

Dupe of https://github.com/code-423n4/2022-06-infinity-findings/issues/164