Infinity NFT Marketplace contest - robee's results

Add a timelock

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

Code instance:

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

Div by 0

Division by 0 can lead to accidentally revert, (An example of a similar issue - https://github.com/code-423n4/2021-10-defiprotocol-findings/issues/84)

Code instances:

    https://github.com/code-423n4/2022-06-infinity/tree/main/contracts/core/InfinityExchange.sol#L202 ordersLength might be 0
    https://github.com/code-423n4/2022-06-infinity/tree/main/contracts/core/InfinityExchange.sol#L273 numSells might be 0
    https://github.com/code-423n4/2022-06-infinity/tree/main/contracts/core/InfinityExchange.sol#L149 numMakerOrders might be 0
    https://github.com/code-423n4/2022-06-infinity/tree/main/contracts/core/InfinityExchange.sol#L1162 duration might be 0

Does not validate the input fee parameter

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

Code instances:

    InfinityExchange._execMatchOrders (protocolFeeBps)
    InfinityExchange._matchOrders (protocolFeeBps)
    InfinityExchange._matchOneMakerSellToManyMakerBuys (protocolFeeBps)
    InfinityExchange._matchOneToOneOrders (protocolFeeBps)
    InfinityExchange._execMatchOneMakerSellToManyMakerBuys (protocolFeeBps)
    InfinityExchange.setProtocolFee (_protocolFeeBps)
    InfinityExchange._execMatchOneMakerBuyToManyMakerSells (protocolFeeBps)
    InfinityExchange._execMatchOneToOneOrders (protocolFeeBps)
    InfinityExchange._matchOneMakerBuyToManyMakerSells (protocolFeeBps)

Require with not comprehensive message

The following requires has a non comprehensive messages. This is very important to add a comprehensive message for any require. Such that the user has enough information to know the reason of failure:

Code instances:

    Solidity file: TimelockConfig.sol, In line 52 with Require message: too early
    Solidity file: InfinityExchange.sol, In line 1231 with Require message: failed
    Solidity file: TimelockConfig.sol, In line 94 with Require message: not config
    Solidity file: InfinityExchange.sol, In line 381 with Require message: too many

Not verified input

external / public functions parameters should be validated to make sure the address is not 0. Otherwise if not given the right input it can mistakenly lead to loss of user funds.

Code instances:

    InfinityExchange.sol.removeCurrency _currency
    InfinityStaker.sol.rescueETH destination
    InfinityExchange.sol.addComplication _complication
    InfinityExchange.sol.updateMatchExecutor _matchExecutor
    InfinityExchange.sol.rescueETH destination
    InfinityExchange.sol.rescueTokens destination

Treasury may be address(0)

Make sure the treasury is not address(0).
    
    

Code instance:

    InfinityStaker.sol.updateInfinityTreasury _infinityTreasury

Named return issue

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.

Code instances:

    TimelockConfig.sol, isConfig
    TimelockConfig.sol, getPendingByIndex
    TimelockConfig.sol, getConfigByIndex
    TimelockConfig.sol, isPending
    InfinityToken.sol, getAdmin
    InfinityToken.sol, getCliff

Check transfer receiver is not 0 to avoid burned money

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Code instances:

    https://github.com/code-423n4/2022-06-infinity/tree/main/contracts/staking/InfinityStaker.sol#L140
    https://github.com/code-423n4/2022-06-infinity/tree/main/contracts/token/InfinityToken.sol#L91
    https://github.com/code-423n4/2022-06-infinity/tree/main/contracts/token/InfinityToken.sol#L100
    https://github.com/code-423n4/2022-06-infinity/tree/main/contracts/core/InfinityExchange.sol#L842
    https://github.com/code-423n4/2022-06-infinity/tree/main/contracts/core/InfinityExchange.sol#L1139
    https://github.com/code-423n4/2022-06-infinity/tree/main/contracts/staking/InfinityStaker.sol#L73
    https://github.com/code-423n4/2022-06-infinity/tree/main/contracts/staking/InfinityStaker.sol#L127
    https://github.com/code-423n4/2022-06-infinity/tree/main/contracts/core/InfinityExchange.sol#L1116
    https://github.com/code-423n4/2022-06-infinity/tree/main/contracts/core/InfinityExchange.sol#L1225
    https://github.com/code-423n4/2022-06-infinity/tree/main/contracts/core/InfinityExchange.sol#L893
    https://github.com/code-423n4/2022-06-infinity/tree/main/contracts/core/InfinityExchange.sol#L917
    https://github.com/code-423n4/2022-06-infinity/tree/main/contracts/core/InfinityExchange.sol#L1087
    https://github.com/code-423n4/2022-06-infinity/tree/main/contracts/core/InfinityExchange.sol#L787
    https://github.com/code-423n4/2022-06-infinity/tree/main/contracts/core/InfinityExchange.sol#L197
    https://github.com/code-423n4/2022-06-infinity/tree/main/contracts/core/InfinityExchange.sol#L727

Never used parameters

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

Code instances:

    InfinityStaker.sol: function rescueETH parameter destination isn't used. 
    InfinityExchange.sol: function rescueETH parameter destination isn't used.

State variables that could be set immutable

In the following files there are state variables that could be set immutable to save gas.

Code instance:

    INFINITY_TOKEN in InfinityStaker.sol

Unnecessary array boundaries check when loading an array element twice

There are places in the code (especially in for-each loops) that loads the same array element more than once. 
In such cases, only one array boundaries check should take place, and the rest are unnecessary.
Therefore, this array element should be cached in a local variable and then be loaded
again using this local variable, skipping the redundant second array boundaries check: 

Code instances:

    InfinityExchange.sol._nftsHash - double load of nfts[i]
    InfinityExchange.sol._tokensHash - double load of tokens[i]

Prefix increments are cheaper than postfix increments

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

Code instance:

    change to prefix increment and unchecked: MockERC721.sol, i, 11

Unnecessary index init

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

Code instances:

    InfinityExchange.sol, 308
    InfinityExchange.sol, 272
    InfinityExchange.sol, 1190
    InfinityOrderBookComplication.sol, 320
    InfinityExchange.sol, 1109
    InfinityOrderBookComplication.sol, 246
    InfinityExchange.sol, 1086
    InfinityOrderBookComplication.sol, 216
    InfinityExchange.sol, 1048
    InfinityOrderBookComplication.sol, 199
    InfinityExchange.sol, 393
    InfinityExchange.sol, 349
    InfinityOrderBookComplication.sol, 290
    MockERC721.sol, 11
    InfinityExchange.sol, 1206
    InfinityOrderBookComplication.sol, 76
    InfinityExchange.sol, 148

Rearrange state variables

You can change the order of the storage variables to decrease memory uses.

Code instance:

In InfinityStaker.sol,rearranging the storage fields can optimize to: 2 slots from: 3 slots. The new order of types (you choose the actual variables): 1. address 2. uint16 3. uint16 4. uint16 5. uint16 6. uint16 7. uint16 8. address 9. uint16

Short the following require messages

The following require messages are of length more than 32 and we think are short enough to short them into exactly 32 characters such that it will be placed in one slot of memory and the require function will cost less gas. The list:

Code instance:

    Solidity file: InfinityExchange.sol, In line 395, Require message length to shorten: 35, The message: nonce already executed or cancelled

Use != 0 instead of > 0

Using != 0 is slightly cheaper than > 0. (see https://github.com/code-423n4/2021-12-maple-findings/issues/75 for similar issue)

Code instances:

    InfinityStaker.sol, 69: change 'balance > 0' to 'balance != 0'
    InfinityStaker.sol, 123: change 'balance > 0' to 'balance != 0'

Use unchecked to save gas for certain additive calculations that cannot overflow

You can use unchecked in the following calculations since there is no risk to overflow:

Code instances:

    InfinityToken.sol (L#63) - require(block.timestamp >= previousEpochTimestamp + getEpochDuration(), 'not ready to advance'); 
    InfinityToken.sol (L#62) - require(block.timestamp >= currentEpochTimestamp + getCliff(), 'cliff not passed');
    TimelockConfig.sol (L#52) - require(block.timestamp >= _pending[configId].timestamp + _config[TIMELOCK], 'too early'); 

Inline one time use functions

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

Code instances:

    InfinityExchange.sol, _execMatchOneMakerSellToManyMakerBuys
    InfinityExchange.sol, _matchOneMakerBuyToManyMakerSells
    InfinityStaker.sol, _updateUserStakedAmounts