Infinity NFT Marketplace contest - reassor's results

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1266-L1269 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L725-L726 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L775-L776 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L819-L820 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L873-L874 https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1135-L1136

Vulnerability details

Impact

Contract InfinityExchange.sol charges protocol fee through PROTOCOL_FEE_BPS. The issue is that owner of the contract is able to change protocol fee at any time without any restriction which puts him in a very privileged position and allows him to steal user's funds via front-running attack by increasing protocol fee to 100%.

Exploit Scenario:

1. Buyers and sellers orders are being matched through matchOneToOneOrders
2. Owner notices valuable transaction in the mempool and launches front-running attack by updating PROTOCOL_FEE_BPS to 100% through setProtocolFee.
3. Transaction setProtocolFee is being included before matchOneToOneOrders
4. The protocol fee is now 100%
5. Based on calculations in _execMatchOneToOneOrders the remainingAmount sent to seller is 0 and protocol takes all the funds in form of protocol fee.

    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
    uint256 remainingAmount = execPrice - protocolFee;

This issue is also relevant in case of owner not being a malicious actor. User accepts some kind of protocol fee for example 2.5%. Then owner just changes the fee to 5%. User didn't expect that change and effectively lost funds.

Proof of Concept

• Update of fee - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1266-L1269
• Protocol fee applied in _execMatchToOneOrders - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L725-L726
• Protocol fee applied in _execMatchOneMakerSellToManyMakerBuys - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L775-L776
• Protocol fee applied in _execMatchOneMakerBuyToManyMakerSells - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L819-L820
• Protocol fee applied in _execMatchOrders - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L873-L874
• Protocol fee applied in _transferFees - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1135-L1136

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add additional parameter to sell - accepted protocol fee (similar to a slippage on a dex) and revert transaction if the fee is higher than the one accepted by user. In addition it is recommended to add validation to setProtocolFee so passed _protocolFeeBps cannot exceed hardcoded value e.g. 5%. The final improvement is adding timelock for updating fee through setProtocolFee.

1. Incorrect check >= instead of >

Risk

Low

Impact

Function InfinityStaker.getRageQuitAmounts uses require statement to check if uint256 variable totalStaked is bigger or equal to 0.

193:    require(totalStaked >= 0, 'nothing staked to rage quit');

The check should be verifying if totalStaked is bigger than 0 - >.

Proof of Concept

staking/InfinityStaker.so:193     require(totalStaked >= 0, 'nothing staked to rage quit');

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to change the require check to:

193:    require(totalStaked > 0, 'nothing staked to rage quit');

2. Missing zero address checks

Risk

Low

Impact

Multiple contracts do not check for zero addresses which might lead to loss of funds, failed transactions and can break the protocol functionality.

Proof of Concept

InfinityStaker.sol:

• Missing check for _tokenAddress and _infinityTreasury - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L49
• Missing check for destination - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L345
• Missing check for _infinityTreasury - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L375

InfinityExchange.sol:

• Missing checks for _WETH, _matchExecutor - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L104
• Missing check for to - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L371
• Missing check for destination - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1221
• Missing check for destination - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1229
• Missing check for _currency - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1235
• Missing check for _complication - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1240
• Missing check for _matchExecutor - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1255

InfinityToken.sol:

• Missing check for admin - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/token/InfinityToken.sol#L38

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add zero address checks for listed parameters.

3. Missing events

Risk

Low

Impact

Multiple contracts are not implementing events for critical functions. Lack of events makes it difficult for off-chain applications to monitor the protocol.

Proof of Concept

InfinityExchange.sol:

• Missing rescueTokens function event - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1220-L1226
• Missing rescureETH function event - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1229-L1232
• Missing addCurrency function event - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1235-L1237
• Missing addComplication function event - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1240-L1242
• Missing removeCurrency function event - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1245-L1247
• Missing removeComplication function event - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1250-L1252
• Missing updateMatchExecutor function event - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1255-L1257

InfinityStaker.sol:

• Missing updateStakeLevelThreshold function event - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L351-L361
• Missing updatePenalities function event - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L364-L372
• Missing updateInfinityTreasury function event - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L375-L377

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add missing events to listed functions.

4. Critical address change

Risk

Low

Impact

Changing critical addresses such as ownership should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one. This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

Proof of Concept

InfinityStaker.sol

• Changing ownership via Ownable inheritance - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L15

InfinityExchange.sol

• Changing ownership via Ownable inheritance - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L50

InfinityOrderBookComplication.sol:

• Changing ownership via Ownable inheritance - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L14

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to implement two-step process for changing critical addresses.

5. Use scientific notation

Risk

Non-Critical

Impact

Defining big numbers that consists of many digits or performing additional math calculations might lead to accidential errors and mistakes.

Proof of Concept

core/InfinityExchange.sol:1161:    uint256 PRECISION = 10**4; // precision for division; similar to bps
core/InfinityOrderBookComplication.sol:338:    uint256 PRECISION = 10**4; // precision for division; similar to bp
staking/InfinityStaker.sol:237:        (userstakedAmounts[user][Duration.TWELVE_MONTHS].amount * 4)) / (10**18);

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to use scientific notation.

6. Missing indexing for events

Risk

Non-Critical

Impact

Events should index addresses which helps off-chain applications in monitoring the protocol.

Proof of Concept

core/InfinityExchange.sol:80:  event CancelAllOrders(address user, uint256 newMinNonce);
core/InfinityExchange.sol:81:  event CancelMultipleOrders(address user, uint256[] orderNonces);
core/InfinityExchange.sol:82:  event NewWethTransferGasUnits(uint32 wethTransferGasUnits);
core/InfinityExchange.sol:83:  event NewProtocolFee(uint16 protocolFee);
---
core/InfinityExchange.sol:85:  event MatchOrderFulfilled(
				     bytes32 sellOrderHash,
				     bytes32 buyOrderHash,
				     address seller,
				     address buyer,
				     address complication, // address of the complication that defines the execution
				     address currency, // token address of the transacting currency
				     uint256 amount // amount spent on the order
				   );
---
core/InfinityExchange.sol:95:  event TakeOrderFulfilled(
				     bytes32 orderHash,
				     address seller,
				     address buyer,
				     address complication, // address of the complication that defines the execution
				     address currency, // token address of the transacting currency
				     uint256 amount // amount spent on the order
				   );
---
staking/InfinityStaker.sol:44:  event Staked(address indexed user, uint256 amount, Duration duration);
staking/InfinityStaker.sol:45:  event DurationChanged(address indexed user, uint256 amount, Duration oldDuration, Duration newDuration);
staking/InfinityStaker.sol:46:  event UnStaked(address indexed user, uint256 amount);
staking/InfinityStaker.sol:47:  event RageQuit(address indexed user, uint256 totalToUser, uint256 penalty);
token/TimelockConfig.sol:34:  event ChangeRequested(bytes32 configId, uint256 value);
token/TimelockConfig.sol:35:  event ChangeConfirmed(bytes32 configId, uint256 value);
token/TimelockConfig.sol:36:  event ChangeCanceled(bytes32 configId, uint256 value);
token/InfinityToken.sol:35:  event EpochAdvanced(uint256 currentEpoch, uint256 supplyMinted);

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add indexing to address type parameters.

7. Missing/incomplete natspec comments

Risk

Non-Critical

Impact

Contracts are missing natspec comments which makes code more difficult to read and prone to errors.

Proof of Concept

InfinityExchange.sol:

• Missing @param _WETH and @param _matchExecutor - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L104
• Missing @return - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L533
• Missing @return - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L538
• Missing @return - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L543
• Missing @return - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L548
• Missing @param index and @return - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L553
• Missing @param currency and @return - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L558
• Missing @param order and @return - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1153
• Missing @param order and @return - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1168
• Missing natspec comment - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1185
• Missing natspec comment - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1201
• Missing @param destination, @param currencyand@param amount` - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1220
• Missing @param destination - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1229
• Missing @param _currency - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1235
• Missing @param _complication - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1240
• Missing @param _currency - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1245
• Missing @param _complication - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1250
• Missing @param _matchExecutor - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1255
• Missing @param _wethTransferGasUnits - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1260
• Missing @param _protocolFeeBps - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1266

InfinityOrderBookComplication.sol:

• Missing @param sell, @param buy, @return - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L168-L169
• Missing @param sell, @param buy and @return - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L182
• Missing @param sell, @param buy, @param constructedNftsand@return` - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L191-L192
• Missing @param makeOrder, @param takerItems and @return - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L208-L209
• Missing @param orders and @return - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L316-L317
• Missing @param order and @return - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityOrderBookComplication.sol#L329-L330

InfinityStaker.sol:

• Missing @param _tokenAddress and @param _infinityTreasury - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L49
• Missing natspec - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L275
• Missing @param user, @param amount, @param noVesting, @param vestedThreeMonths, @param vestedSixMonths, @param vestedTwelveMonths - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L287-L290
• Missing @param user - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L327-L328
• Missing @param destination - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L344-L345
• Missing @param stakeLevel, @param threshold - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L350-L351
• Missing @param threeMonthPenalty, @param sixMonthPenalty, @param twelveMonthPenalty - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L363-L364
• Missing @param _infinityTreasury - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L374-L375

InfinityToken.sol:

• Missing natspec comments for all functions - https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/token/InfinityToken.sol

TimelockConfig.sol:

• Missing natspec comments for all functions - https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/token/TimelockConfig.sol

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add missing natspec comments.

8. Typos

Risk

Non-Critical

Impact

Code and comments contain typos which makes code more difficult to read and prone to errors.

Proof of Concept

InfinityStaker.sol:

• Typo in comment Untake tokens - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L112
• Typo in variable name vestedsixMonth, expected camel case vestedSixMonths - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L120

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to correct listed typos.

8. Check return values

Risk

Non-Critical

Impact

Multiple contracts do not check for return values of executed functions. This might lead to accidents and errors since the created transaction is valid but the underlying code did not execute properly.

Proof of Concept

InfinityExchange.sol:

• Return value of _currencies.add - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1236
• Return value of _complications.add - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1241
• Return value of _currencies.remove - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1246
• Return value of _complications.remove - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1251

InfinityToken.sol:

• Return value of _configSet.add - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/token/TimelockConfig.sol#L55
• Return value of _configSet.add - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/token/TimelockConfig.sol#L67

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to check the return values of executed functions.

1. Obsolete checks

Impact

Function InfinityStaker.getRageQuitAmounts uses require statement to check if uint256 variable totalStaked is bigger or equal to 0. Variable of type uint256 is always bigger or equal to 0 which makes this check completely obsolete.

Proof of Concept

staking/InfinityStaker.so:193     require(totalStaked >= 0, 'nothing staked to rage quit');

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to remove obsolete check or change it to check if totalStaked value is bigger > than 0.

2. Use scientific notation

Impact

Multiple contracts are using math exponent calculation to express big numbers. This consumes additional gas and its better to use scienfic notation.

Proof of Concept

core/InfinityOrderBookComplication.sol:338:    uint256 PRECISION = 10**4; // precision for division; similar to bps
core/InfinityExchange.sol:1161:    uint256 PRECISION = 10**4; // precision for division; similar to bps
staking/InfinityStaker.sol:237:        (userstakedAmounts[user][Duration.TWELVE_MONTHS].amount * 4)) / (10**18);

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to use scientific notation, for example: 1e18.

3. Long revert error messages

Impact

Shortening revert error messages to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

Proof of Concept

core/InfinityExchange.sol:399:      require(!isUserOrderNonceExecutedOrCancelled[msg.sender][orderNonces[i]], 'nonce already executed or cancelled');
--
staking/InfinityStaker.sol:92:    require(
			    	      userstakedAmounts[msg.sender][oldDuration].amount >= amount,
			    	      'insufficient staked amount to change duration'
			    	    );
--
staking/InfinityStaker.sol:96:    require(newDuration > oldDuration, 'new duration must be greater than old duration');

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to decrease revert messages to maximum 32 bytes. Alternatively custom error types should be used.

4. Use custom errors instead of revert strings to save gas

Impact

Usage of custom errors reduces the gas cost.

Proof of Concept

Contracts that should be using custom errors:

• InfinityExchange.sol
• InfinityOrderBookComplication.sol
• InfinityStaker.sol
• InfinityToken.sol
• TimelockConfig.sol

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add custom errors to listed contracts.

5. No need to explicitly initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for addresses). Explicitly initializing it with its default value is an anti-pattern and waste of gas.

Proof of Concept

core/InfinityOrderBookComplication.sol:42:    bool _isPriceValid = false;
core/InfinityOrderBookComplication.sol:108:    bool _isPriceValid = false;
core/InfinityOrderBookComplication.sol:76:    for (uint256 i = 0; i < ordersLength; ) {
core/InfinityOrderBookComplication.sol:82:      for (uint256 j = 0; j < nftsLength; ) {
core/InfinityOrderBookComplication.sol:197:    uint256 numConstructedItems = 0;
core/InfinityOrderBookComplication.sol:199:    for (uint256 i = 0; i < nftsLength; ) {
core/InfinityOrderBookComplication.sol:214:    uint256 numTakerItems = 0;
core/InfinityOrderBookComplication.sol:216:    for (uint256 i = 0; i < nftsLength; ) {
core/InfinityOrderBookComplication.sol:244:    uint256 numCollsMatched = 0;
core/InfinityOrderBookComplication.sol:246:    for (uint256 i = 0; i < order2NftsLength; ) {
core/InfinityOrderBookComplication.sol:247:      for (uint256 j = 0; j < order1NftsLength; ) {
core/InfinityOrderBookComplication.sol:289:    uint256 numTokenIdsPerCollMatched = 0;
core/InfinityOrderBookComplication.sol:290:    for (uint256 k = 0; k < item2TokensLength; ) {
core/InfinityOrderBookComplication.sol:291:      for (uint256 l = 0; l < item1TokensLength; ) {
core/InfinityOrderBookComplication.sol:318:    uint256 sum = 0;
core/InfinityOrderBookComplication.sol:320:    for (uint256 i = 0; i < ordersLength; ) {
core/InfinityExchange.sol:148:    for (uint256 i = 0; i < numMakerOrders; ) {
core/InfinityExchange.sol:200:      for (uint256 i = 0; i < ordersLength; ) {
core/InfinityExchange.sol:219:      for (uint256 i = 0; i < ordersLength; ) {
core/InfinityExchange.sol:272:    for (uint256 i = 0; i < numSells; ) {
core/InfinityExchange.sol:308:    for (uint256 i = 0; i < numMakerOrders; ) {
core/InfinityExchange.sol:349:    for (uint256 i = 0; i < ordersLength; ) {
core/InfinityExchange.sol:393:    for (uint256 i = 0; i < numNonces; ) {
core/InfinityExchange.sol:1048:    for (uint256 i = 0; i < numNfts; ) {
core/InfinityExchange.sol:1086:    for (uint256 i = 0; i < numTokens; ) {
core/InfinityExchange.sol:1109:    for (uint256 i = 0; i < numNfts; ) {
core/InfinityExchange.sol:1190:    for (uint256 i = 0; i < numNfts; ) {
core/InfinityExchange.sol:1206:    for (uint256 i = 0; i < numTokens; ) {

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to remove explicit initializations with default values.

6. Use != 0 instead of > 0 for unsigned integer comparison

Impact

When dealing with unsigned integer types, comparisons with != 0 are cheaper than with > 0.

Proof of Concept

core/InfinityExchange.sol:392:    require(numNonces > 0, 'cannot be empty');

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to use != 0 instead of > 0.

7. Pack integer values

Impact

Packing integer variables into storage slots saves gas.

Proof of Concept

InfinityStaker.sol:

 17   struct StakeAmount {
 18     uint256 amount;
 19     uint256 timestamp;
 20   }

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

If amount is not expected to reach specific values it is recommended to pack it in 192 bits and use 64 bits for timestamp.

 17   struct StakeAmount {
 18     uint192 amount;
 19     uint64 timestamp;
 20   }

8. Declare as external

Impact

Functions declared as public that are never called internally within the contract should be marked as external in order save gas (especially in the case where the function takes arguments, since external functions can read arguments directly from calldata instead of allocating memory).

Proof of Concept

InfinityStaker.sol:

• getUserTotalStaked can be set as external - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L154
• getUserTotalVested can be set as external - https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L167

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to change all public functions that are not called internally to external visibility.