Infinity NFT Marketplace contest - 0xsanson's results

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.

Lines of code

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

Vulnerability details

Impact

Let's consider an example. Alice makes an order for an ERC1155, where she wants to buy 10 items with id=1 and 10 with id=2. This order can be matched using matchOneToManyOrders with two orders that sell both 10 items with id=1. Basically Alice gets 20 id1 instead of the expected 10 id1 + 10 id2. If id2 is a more expensive item this is definitely a problem.

Proof of Concept

I've made an hardhat test to prove the concept. Link to gist

Recommended Mitigation Steps

The functions in InfinityOrderBookComplication that check if items intersect should be reworked a little to consider possible overlapping (when dealing with multiple-to-one matching).

Lines of code

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

Vulnerability details

Impact

_transferNFTs checks if an item is ERC721 or ERC1155 by using IERC165(item.collection).supportsInterface(...).

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

The issue is that if an item is neither of that, the function doesn't revert. Basically the execution continues as if an item was transferred. If an user mistakenly used a wrong address, or if the NFT is something weird, someone will end up paying for buying nothing.

Proof of Concept

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

Recommended Mitigation Steps

Add a else with a revert statement.

Lines of code

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

Vulnerability details

Impact

During a match, there's some accounting on how much gas we're spending, so that the executor can be reimbursed. The gas cost is split between multiple orders, computing the difference between the gas at the start and at the end. The gas at the start is calculated as this (inside a loop):

uint256 startGasPerOrder = gasleft() + ((startGas - gasleft()) / numMakerOrders);

The second part should mean that a "heading" gas is to be split between all numMakerOrders. However this is true only in the first iteration of the loop, indeed increases in next iterations.

The impact is that when dealing with multiple orders, the last ones pay more than the due. It can be proven by switching orders in the array.

Proof of Concept

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

Recommended Mitigation Steps

The heading part should be calculated outside the loop, or in the first iteration, and saved in a variable commonGas. Then startGasPerOrder becomes

uint256 startGasPerOrder = gasleft() + commonGas;

Lines of code

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

Vulnerability details

Impact

Gas cost when matching orders is payed by the buyer. Since buyers don't have control on order execution, they may spend more gas than what they are willing to. Examples: periods of high gasPrice, or if NFTs for some reason consume a extra amount of gas.

Proof of Concept

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L739 and other similar functions...

Recommended Mitigation Steps

Consider adding a parameter to a MakerOrder where the user may specify the max amount of gas they're willing to spend.

Lines of code

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

Vulnerability details

Impact

In takeOrders, msg.sender can pay the items in ether (or other native coins).

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

However the extra payment isn't returned to the caller. This can be a risk considering that they may send extra for items with "moving" price. Also they may mistakenly send ether to the contract when it's not needed. Indeed the function doesn't revert in that case.

Proof of Concept

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

Recommended Mitigation Steps

Add a else with a require(msg.value==0). Also consider returning any extra value to msg.sender.