Infinity NFT Marketplace contest - zer0dot's results

Lines of code

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

Vulnerability details

Impact

The rescueETH() function seemingly incorrectly uses the msg.value transferred along with the function as the amount to send to the destination address. This means that the actual contract's balance can never be retrieved.

Proof of Concept

See the concerned code here.

Note that the function is payable when it doesn't need to be, and uses the msg.value transaction property as the amount to withdraw.

Tools Used

Manual review.

Recommended Mitigation Steps

Replace the msg.value parameter with address(this).balance and optionally remove the payable modifier.

Lines of code

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

Vulnerability details

Impact

Similarly to another finding I submitted involving a compromised owner being able to frontrun a transaction and steal the entire sale price by setting the protocol fee to 10000 bps, a compromised owner can also set the fee greater than 10000 bps.

This would cause every (variable) - protocolFee calculation to revert, as well as in the _transferFees() function called within taker functions.

Proof of Concept

See the concerned code here.

An example of where this would break functionality would be here.

This same issue exists for all order matching or taking functions.

Tools Used

Manual review.

Recommended Mitigation Steps

Just like the other issue with frontrunning, include an upper limit to the protocol fee.

The optimizations are applied additively, so every change is applied on top of the last!

1. Optimize line 583 of the InfinityExchange Contract by combining the ternary operators into a single if statement.

   matchOneToOneOrders() average gas 221896 => 221773, also saves 27 bytes of contract code size.

InfinityExchange._matchOneToOneOrders():

...
    OrderTypes.MakerOrder calldata sell;
    OrderTypes.MakerOrder calldata buy;
    if (makerOrder1.isSellOrder) {
      sell = makerOrder1;
      buy = makerOrder2;
    } else {
      sell = makerOrder2;
      buy = makerOrder1;
    }
...

2. Remove line 523 of the InfinityExchange contract since it's redundant.

   The order.signer parameter is checked against address(0), but this check is already done in the SignatureChecker's recover() function, which is called from the SignatureChecker's verify() function prior to the aforementioned zero address check. More specifically, if the signer returned from the ecrecover call is the zero address, an explicit revert is thrown. Otherwise, if the order.signer is zero, then it forcibly cannot equal the recovered non-zero address, reverting due to an invalid sig.

   This should affect all calls to isOrderValid() but for reference, the matchOneToOneOrders() average gas cost went from 221773 (with the prior change) to 221531. Also saves 41 more bytes of contract code size.

InfinityExchange.isOrderValid():

...
    if (
      orderExpired ||
      !sigValid ||
      !_complications.contains(order.execParams[0]) ||
      !_currencies.contains(order.execParams[1])
    ) {
      return false;
    }
    return true;
...

3. Revert in the verify() function instead of returning a boolean at line 50 of the signatureChecker contract. verify returns whether the signature is indeed valid (though reverting on zero address signers), then the result is compared in the isOrderValid() function. Reverting directly in the verify() function saves a bit of gas at the cost of increased code size.

   This should affect all calls to isOrderValid() but for reference, the matchOneToOneOrders() average gas cost went from 221531 to 221480. However, the code size increased by 105 bytes, which brings the net code size increase to 37 bytes, I think it's a good tradeoff!

InfinityExchange.isOrderValid():

...
    if (
      orderExpired ||
      !_complications.contains(order.execParams[0]) ||
      !_currencies.contains(order.execParams[1])
    ) {
      return false;
    }
    return true;
...

SignatureChecker.verify():

...
    if (Address.isContract(signer)) {
      // 0x1626ba7e is the interfaceId for signature contracts (see IERC1271)
      require(IERC1271(signer).isValidSignature(digest, abi.encodePacked(r, s, v)) == 0x1626ba7e, "Invalid sig");
    } else {
      require(recover(digest, r, s, v) == signer, "Invalid sig");
    }
...