Infinity NFT Marketplace contest - antonttc's results

Lines of code

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

Vulnerability details

Impact

User funds might be lost due to malicious orders.

Proof of Concept

in _transferNFTs, the function doesn’t take care of the case if item.collection is not an NFT. It will still be treated like a “valid order”, but it won’t actually be exchanged when the trade is triggered.

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);
    }
		// if item.collection is not an NFT, nothing happen and the trade proceeeds
  }

If a malicious user creates a non-standard ERC721 that returns false on supportsInterface, the exchange will skip the ERC721 transfer, but the buyer still pay the currency.

This opens up wide range of phishing attack opportunities, a malicious attacker can put in an collectionId that either returns false, or have a fallback function that won’t revert on supportsInterface and trick people into signing an order.

Recommended Mitigation Steps

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);
    } else {
      revert("not an NFT!!!");
    }
  }

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L132

Vulnerability details

Impact

Buyer cannot stop the matcher from executing their order with high gas price (or during the period when the gas price is high) and forcing a high fee.

Proof of Concept

in all the matching functions, the buyer are forced to pay the gas cost of execution, and there is no way for a buyer to prevent himself from being charged too much except constantly maintaining its weth allowance to the contract, which is not a good UX.

When the gas price is higher, buyer can easily be charge hundreds of dollars because of an outstanding order.

Recommended Mitigation Steps

Consider adding gasPrice protection as one field in the constraints, and revert when tx.gasPrice is higher than this value.

Lines of code

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

Vulnerability details

Impact

Gas calculation is wrong in matchOneToOneOrders and matchOneToManyOrders, and can leads to buyers paying enormous weth as fees

Proof of Concept

The current code calculates startGasPerOrder at the “start of each iteration” with the following formula:

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

the pseudo code for the current calculation.

// matchOneToOneOrders
uint256 startGas = gasleft();
...
for (uint256 i = 0; i < numMakerOrders; ) {
      uint256 startGasPerOrder = gasleft() + ((startGas - gasleft()) / numMakerOrders);

	// will charge startGas - gasLeft() from buyer
	_matchOneToOneOrders(... ,startGasPerOrder,...);
}

The idea for using ((startGas - gasleft()) / numMakerOrders) is to share the cost among all buyers for the gas used in the shared execution logic.

The problem with this being used in the loop, is that “shared cost” for all orders changes with each iteration. If the first order is very expensive to execute, it will cost the difference between startGas and gasLeft() to be huge, therefore forcing the second buyer to pay a lot in fees.

Or even if all orders are normal, the later it is for the order to be in the loop, the more the buyer pays, because startGas - gasLeft() will only get bigger with iterations going on.

Recommended Mitigation Steps

The shared cost should be calculated separated before the loop, and added to each startGasPerOrder as follow:

// matchOneToOneOrders
uint256 startGas = gasleft();
...
uint256 sharedCost = (startGas - gasleft()) / numMakerOrders;
for (uint256 i = 0; i < numMakerOrders; ) {
      uint256 startGasPerOrder = gasleft() + sharedCost;

	_matchOneToOneOrders(... ,startGas,...);
}

QA Report

1. Owner settable variable protocol fee and wethTrasnferGasare not capped

these governable variables should have upper bounds. For free it should be something like 10%, and trasnferGas should probably be 60000 just to make sure even owner can’t potentially use these variables to steal from users. This would reduce the trust assumption users need to make to interact with the protocol.

2. No emit of event for fee paid makes it hard for the team to know how much weth is paid for the matching

the contract currently pulls the weth into its own contract, along with protocol fee.

There is no event emitted when protocol fee is paid and buyer fee is paid for auto matching, making it hard to separate how much should be consider “gas compensation” vs “protocol revenue”. This might make it hard to do analysis, or do protocol fee distribution via governance in the future.

I’d suggest just transfer the “match fee” to matcher (msg.sender) and consider all weth in the contract to be protocol revenue. Using an event for one of the fee charge event can also help for offchain calculation, but won’t make it easier for future on-chain governance operations.

Gas optimization

1. Optimize _getCurrentPric can save few hundred gas on average

A few optimisation can be applied to this function:

• consider return early for fix price trades, if it is considered the main usecase.
• add uncheck blocks on pre-checked calculation
• refactor the function to remove duplicated comparison (startPrice vs endPrice)

Example Implementation:

// assuming PRECISION is declared as constant

function _getCurrentPrice(OrderTypes.MakerOrder calldata order) internal view returns (uint256) {
    (uint256 startPrice, uint256 endPrice) = (order.constraints[1], order.constraints[2]);
    
    // return early to optimize fixed price trades
    if (startPrice == endPrice) return startPrice;
    
    uint256 duration = order.constraints[4] - order.constraints[3];
    if (duration == 0) return startPrice;

    uint256 elapsedTime = block.timestamp - order.constraints[3];

    unchecked {
      // elapsedTime * PRECISION will not overflow
      uint256 portionBps = elapsedTime > duration ? PRECISION : ((elapsedTime * PRECISION) / duration);
      if (startPrice > endPrice) {

        // startPrice - endPrice will not underflow
        uint256 priceDiff = (startPrice - endPrice) * portionBps / PRECISION; 
        return startPrice - priceDiff;
      } else {
        // endPrice - startPrice will not underflow
        uint256 priceDiff = (endPrice - startPrice) * portionBps / PRECISION;  
        return startPrice + priceDiff;
      }  
    }
  }

This saves average of 500 gas for takeOrders , and 1500 gas for matchOneToManyOrders

more gas can be solved if we update the same function in OrderBookComplication too.

2. Remove time check on OrderBookComplication

All orders are already checked if constraints[3] < now && constraint[4] > now in InfinityExchange, so relevent checks can be removed from the Complication layer to save gas.

Gas saving: 2000 ~ 50000. (benchmark on passing tests)

• matchOneToManyOrders: 352545 → 294291
• matchOneToOneOrders: 816451 → 810531
• matchOrders: 424195 → 422034
• takeOrders : 210136 → 205584

3. remove safeTransfe on WETH

safeERC20 library are designed to be used on non-standard ERC20 tokens. Removing the use of the library from well tested weth token shouldn’t be consider risky, and save you some gas.

4. Pack variables in InfinityToken

packing the following 2 variables into 1 storage slot can save gas when trying to read and write these 2 variables:currentEpoch previousEpochTimestamp.

Gas saving

• advanceEpoch: 118230 ⇒ 109376

5. currentEpochTimestamp should be used as immutable

the variable currentEpochTimestamp is not updated outside of the constructor, defining it as immutable can save gas on SLOAD.

Gas saving

• advanceEpoch: 109376 ⇒ 107273