Infinity NFT Marketplace contest - kenzo's results

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L346

Vulnerability details

The rescue ETH function sends to the sender msg.value instead of address(this).balance.

Impact

Wrong implementation, ETH can't be rescued from the contract.

Proof of Concept

This is rescueETH function:

  function rescueETH(address destination) external payable onlyOwner {
    (bool sent, ) = destination.call{value: msg.value}('');
    require(sent, 'Failed to send Ether');
  }

Recommended Mitigation Steps

Send the contract's balance instead of msg.value.

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.

Lines of code

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

Vulnerability details

matchOneToManyOrders doesn't conform to Checks-Effects-Interactions pattern, and updates the maker order nonce only after the NFTs and payment have been sent. Using this, a malicious user can re-enter the contract and re-fulfill the order using takeOrders.

Impact

Orders can be executed twice. User funds would be lost.

Proof of Concept

matchOneToManyOrders will set the order nonce as used only after the tokens are being sent:

  function matchOneToManyOrders(OrderTypes.MakerOrder calldata makerOrder, OrderTypes.MakerOrder[] calldata manyMakerOrders) external {
    ...
    if (makerOrder.isSellOrder) {
      for (uint256 i = 0; i < ordersLength; ) {
        ...
        _matchOneMakerSellToManyMakerBuys(...); // @audit will transfer tokens in here
        ...
      }
      //@audit setting nonce to be used only here
      isUserOrderNonceExecutedOrCancelled[makerOrder.signer][makerOrder.constraints[5]] = true;
    } else {
      for (uint256 i = 0; i < ordersLength; ) {
        protocolFee += _matchOneMakerBuyToManyMakerSells(...); // @audit will transfer tokens in here
        ...
      }
      //@audit setting nonce to be used only here
      isUserOrderNonceExecutedOrCancelled[makerOrder.signer][makerOrder.constraints[5]] = true;
      ...
  }

So we can see that tokens are being transferred before nonce is being set to executed.

Therefore, POC for an attack - Alice wants to buy 2 unspecified WolfNFT, and she will pay via AMP, an ERC-777 token. Malicious user Bob will set up an offer to sell 2 WolfNFT. The MATCH_EXECUTOR will match the offers. Bob will set up a contract such that upon receiving of AMP, it will call takeOrders with Alice's order, and 2 other WolfNFTs. (Note that although takeOrders is nonReentrant, matchOneToManyOrders is not, and so the reentrancy will succeed.)

So in takeOrders, the contract will match Alice's order with Bob's NFTs, and then set Alice's order's nonce to true, then matchOneToManyOrders execution will resume, and again will set Alice's order's nonce to true.

Alice ended up buying 4 WolfNFTs although she only signed an order for 2. Tough luck, Alice.

(Note: a similar attack can be constructed via ERC721's onERC721Received.)

Recommended Mitigation Steps

Conform to CEI and set the nonce to true before executing external calls.

Lines of code

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#L202 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L739 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L273

Vulnerability details

When the MATCH_EXECUTOR executes orders, it transfers for itself from the buyer a refund for the gas cost. The calculation for this gas cost is wrong.

Impact

Users will pay MATCH_EXECUTOR more than they should, therefore, user funds will be lost.

Proof of Concept

In short: the startGasPerOrder is supposed to divide the "shared gas cost" evenly amongst all maker orders, but it accidently accumulates every loop more gas than needed, and each further order in the orders array will pay extra gas. The problem is in the line:

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

((startGas - gasleft()) / numMakerOrders) is probably supposed to be the shared gas cost of the initial common calculations, but it ends up including the gas cost of all previous orders, so order #100 will also pay gas cost of (order #1 + order #2 + ... + order #99)/100.

In detail:

Background: we'll look at matchOneToOneOrders. This is how the function starts:

  function matchOneToOneOrders(OrderTypes.MakerOrder[] calldata makerOrders1, OrderTypes.MakerOrder[] calldata makerOrders2) external {
    uint256 startGas = gasleft();
    ...some calculations...
    for (uint256 i = 0; i < numMakerOrders; ) {
      uint256 startGasPerOrder = gasleft() + ((startGas - gasleft()) / numMakerOrders);

After forwarding startGasPerOrder to few inner functions, _execMatchOneToOneOrders will calculate the gas refund as follows:

    uint256 gasCost = (startGasPerOrder - gasleft() + wethTransferGasUnits) * tx.gasprice;

POC: Let's assume matchOneToOneOrders will get called with 2 maker orders.

The problem is in the line:

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

For the first order, the startGasPerOrder will be gasleft1stOrder + (originalStartingGas - gasleft1stOrder)/2. This is accurate and is because the "shared initial lines" gas cost of matchOneToOneOrders function will be divided amongst the gas refund of all the orders.

For the second order, the startGasPerOrder will be gasleft2ndOrder + (originalStartingGas - gasleft2ndOrder)/2. As gasleft2ndOrder = gasleft1stOrder - gasSpent1stOrder, this means startGasPerOrder = gasleft2ndOrder + (originalStartingGas - gasleft1stOrder + gasSpent1stOrder)/2.

Note the last part: (originalStartingGas - gasleft1stOrder + gasSpent1stOrder)/2; this means the second order will pay more gas then needed - it will pay gasSpent1stOrder/2 extra. The second order shouldn't pay for gas related to the first order - plus, the first order already paid for it's gas cost.

This wrong calculation will continue to accumulate for all further orders.

This same issue is also present in matchOneToManyOrders and matchOrders.

Recommended Mitigation Steps

I believe the shared cost needs to be saved in advance, so for example in matchOneToOneOrders, change it to:

  function matchOneToOneOrders(OrderTypes.MakerOrder[] calldata makerOrders1, OrderTypes.MakerOrder[] calldata makerOrders2) external {
    uint256 startGas = gasleft();
    ...some calculations...
    uint256 sharedGasCost = (startGas - gasleft()) / numMakerOrders;
    for (uint256 i = 0; i < numMakerOrders; ) {
      uint256 startGasPerOrder = gasleft() + sharedGasCost;

(Note that you can insert sharedGasCost into the loop and initialize it only once to also count the initialization of the loop, but this will come with a gas cost.)