Infinity NFT Marketplace contest - PPrieditis's results

Title: Wrong naming convention for variables

Impact

Existing style for naming variables is misleading and decreases code maintainability. Only constant variable names should be in all capital letters. A lot of projects use all capital letters not only for constant but also for immutable variables.

Official Solidity language documentation recommendations should be followed for variable naming convention: https://docs.soliditylang.org/en/latest/style-guide.html?highlight=variable#local-and-state-variable-names

Recommended Mitigation Steps

Do not use the same naming convention for state variables and constants. Below is a example with wrong naming convention: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L54-L63 /// @dev WETH address of a chain; set at deploy time to the WETH address of the chain that this contract is deployed to address public immutable WETH; /// @dev Used in order signing with EIP-712 bytes32 public immutable DOMAIN_SEPARATOR; /// @dev This is the adress that is used to send auto sniped orders for execution on chain address public MATCH_EXECUTOR; /// @dev Gas cost for auto sniped orders are paid by the buyers and refunded to this contract in the form of WETH uint32 public WETH_TRANSFER_GAS_UNITS = 50000; /// @notice Exchange fee in basis points (250 bps = 2.5%) uint16 public PROTOCOL_FEE_BPS = 250;

To: Local and state variables should use mixedCase. Examples: totalSupply, remainingSupply, balancesOf, creatorAddress, isPreSale, tokenExchangeRate. Constants should be named with all capital letters with underscores separating words. Examples: MAX_BLOCKS, TOKEN_NAME, TOKEN_TICKER, CONTRACT_VERSION. Additionally I would like to recommend to use Solidity language naming convention as base and extend it with prefixes:

• “i_” prefix for immutable variables
• “s_” prefix for storage variables And don’t use prefix “_” for private variables because all data on the blockchain is public and the only difference is automatically generated getter function. Below are naming convention examples: address public immutable i_weth; address public s_matchExecutor; EnumerableSet.AddressSet private s_complications;

---

Title: Events aren't indexed

Impact

Events are not indexed, so their filtering is disabled, which makes it harder to programmatically use the system

Recommended Mitigation Steps

Add indexes for events:
https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L80-L102

---

Title: Remove fallback() and receive()

Impact

Empty fallback() and receive() do not serve any purpose and unnecessary increases amount of code. It is always possible to send ether from a self-destruct function so empty fallback, receive are not necessary.

Recommended Mitigation Steps

Remove fallback() and receive(): https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L119-L121 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L54-L57

---

Title: Create onlyExecutor modifier

Impact

Three functions has the same same duplicate code to check for access control. Best practice is to have access control in a modifier and not to copy the logic to each function.

Recommended Mitigation Steps

Remove code - require(msg.sender == MATCH_EXECUTOR, 'OME');
From: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L138 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L183 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L263

Create a new modifier and add it to above mentioned functions: modifier onlyExecutor() { require(msg.sender == MATCH_EXECUTOR, 'OME'); _; }

---

Title: Remove MakerOrder.execParams[]

Impact

MakerOrder.execParams[] makes code hard to read and it stores only two variables: MakerOrder.execParams[0] - complication address MakerOrder.execParams[1] - currency

Both .execParams[] are checked at the main contract so keeping them as a array doesn’t give any benefit and only complicates code: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L442-L443

Recommended Mitigation Steps

Change: MakerOrder.execParams[0] MakerOrder.execParams[1] To: MakerOrder.compAddress MakerOrder.currency

---

Title: Constant variables should be declared as constants and not memory variables

Impact

Only constant variables should use the appropriate constant naming convention.

Recommended Mitigation Steps

Change: uint256 PRECISION = 104; // precision for division; similar to bps https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityOrderBookComplication.sol#L338 To: uint256 constant PRECISION = 104; // precision for division; similar to bps

---

Title: Magic numbers should be constants

Impact

There are many instances of the value 10000. Currently this integer value is used directly without a clear indication that this value relates to the decimals value, which could lead to one of these values being modified but not the other (perhaps by a typo), which is the basis for many past hacks. Coding best practices suggests using a constant integer to store this value in a way that clearly explains the purpose of this value to prevent confusion.

Recommended Mitigation Steps

Create a constant: Uint256 const BASIS_POINTS = 10000:

And replace magic number 10000 with the new constant: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L725 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L775 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L819 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L873 https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L1135

Replace 10**18: https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L237

Title: Use Solidity v0.8.4 because of Solidity Custom Errors

Impact

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Custom errors decrease both deploy and runtime gas costs. Source: https://blog.soliditylang.org/2021/04/21/custom-errors/

OpenZeppelin is also planing to use custom errors starting with next major release 5.0. Source release v5.0: https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2961 Source OZ custom error issue: https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2839

Recommended Mitigation Steps

Refactor code and use Solidity custom errors.

---

Title: Useless initializations to default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Recommended Mitigation Steps

Remove explicit initialization for default values.

1 - InfinityExchange.sol#L148 for (uint256 i = 0; i < numMakerOrders; ) {

2 - InfinityExchange.sol#L200 for (uint256 i = 0; i < ordersLength; ) {

3 - InfinityExchange.sol#L219 for (uint256 i = 0; i < ordersLength; ) {

4 - InfinityExchange.sol#L272 for (uint256 i = 0; i < numSells; ) {

5 - InfinityExchange.sol#L308 for (uint256 i = 0; i < numMakerOrders; ) {

6 - InfinityExchange.sol#L349 for (uint256 i = 0; i < ordersLength; ) {

7 - InfinityExchange.sol#L393 for (uint256 i = 0; i < numNonces; ) {

8 - InfinityExchange.sol#L1048 for (uint256 i = 0; i < numNfts; ) {

9 - InfinityExchange.sol#L1086 for (uint256 i = 0; i < numNfts; ) {

10 - InfinityExchange.sol#L1190 for (uint256 i = 0; i < numNfts; ) {

11 - InfinityExchange.sol#L1206 for (uint256 i = 0; i < numTokens; ) {

1 - InfinityOrderBookComplication.sol#L42 bool _isPriceValid = false;

2 - https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityOrderBookComplication.sol#L73-L94 The code is: bool isOrdersTimeValid = true; bool itemsIntersect = true; uint256 ordersLength = manyMakerOrders.length; for (uint256 i = 0; i < ordersLength; ) { if (!isOrdersTimeValid || !itemsIntersect) { return false; // short circuit }

  uint256 nftsLength = manyMakerOrders[i].nfts.length;
  for (uint256 j = 0; j < nftsLength; ) {
    numItems += manyMakerOrders[i].nfts[j].tokens.length;
    unchecked {
      ++j;
    }
  }

  isOrdersTimeValid =
    isOrdersTimeValid &&
    manyMakerOrders[i].constraints[3] <= block.timestamp &&
    manyMakerOrders[i].constraints[4] >= block.timestamp;

  itemsIntersect = itemsIntersect && doItemsIntersect(makerOrder.nfts, manyMakerOrders[i].nfts);

But it should be: bool isOrdersTimeValid; // <———————— removed initial true value bool itemsIntersect; // <———————— removed initial true value uint256 ordersLength = manyMakerOrders.length; for (uint256 i = 0; i < ordersLength; ) { uint256 nftsLength = manyMakerOrders[i].nfts.length; for (uint256 j = 0; j < nftsLength; ) { numItems += manyMakerOrders[i].nfts[j].tokens.length; unchecked { ++j; } }

  isOrdersTimeValid =
    isOrdersTimeValid &&
    manyMakerOrders[i].constraints[3] <= block.timestamp &&
    manyMakerOrders[i].constraints[4] >= block.timestamp;

  itemsIntersect = itemsIntersect && doItemsIntersect(makerOrder.nfts, manyMakerOrders[i].nfts);

  if (!isOrdersTimeValid || !itemsIntersect) { // <——— changed position for short circuit
    return false; // short circuit
  }

3 - InfinityOrderBookComplication.sol#L108 bool _isPriceValid = false;

4 - InfinityOrderBookComplication.sol#L197 uint256 numConstructedItems = 0;

5 - InfinityOrderBookComplication.sol#L199 for (uint256 i = 0; i < nftsLength; ) {

6 - InfinityOrderBookComplication.sol#L214 uint256 numTakerItems = 0;

7 - InfinityOrderBookComplication.sol#L216 for (uint256 i = 0; i < nftsLength; ) {

8 - InfinityOrderBookComplication.sol#L244-L247 uint256 numCollsMatched = 0; // check if taker has all items in maker for (uint256 i = 0; i < order2NftsLength; ) { for (uint256 j = 0; j < order1NftsLength; ) {

9 - InfinityOrderBookComplication.sol#L289-L291 uint256 numTokenIdsPerCollMatched = 0; for (uint256 k = 0; k < item2TokensLength; ) { for (uint256 l = 0; l < item1TokensLength; ) {

10 - InfinityOrderBookComplication.sol#L318 uint256 sum = 0;

11 - InfinityOrderBookComplication.sol#L320 for (uint256 i = 0; i < ordersLength; ) {