Infinity NFT Marketplace contest - joestakey's results

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L1267 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#L729

Vulnerability details

NO TIMELOCK ON setProtocolFee() CAN LEAD TO SELLERS LOSING THEIR NFTs

In InfinityExchange.sol, there is no timelock on setProtocolFee(). This is the fee that is applied in orders, and determines how much the Exchange receives in fee VS how much the seller receives. But:

• there is no maximum value limiting what fee can be set to
• there is no timelock, ie PROTOCOL_FEE_BPS is set to its new value when setProtocolFee() is created.

Users will be incited to use the exchange if the fee is low (PROTOCOL_FEE_BPS is initially 2.5%). A malicious owner could effectively wait for enough activity to happen in the exchange, then set a very high fee, which would result in all further orders going through the exchange transferring the sales revenue from the buyers to the Exchange, resulting in the sellers effectively losing their NFTs.

Impact

Medium

Proof Of Concept

The malicious owner calls setProtocolFee(10000), setting PROTOCOL_FEE_BPS as 100%.

• A user calls matchOneToOneOrders() for a specific NFT criteria.
• A match is found and the order is executed automatically.
• In the internal call to _execMatchOneToOneOrders(), protocolFee = (protocolFeeBps * execPrice) / 10000 = execPrice
• remainingAmount = execPrice - protocolFee = 0
• the contract transfers nothing to the seller, and execPrice to the exchange. The seller has effectively lost their NFT.

Tools Used

Manual Analysis

Recommended Mitigation Steps

Two things can be done:

• set a maximum possible fee rate in setProtocolFee()
• add a timelock in setProtocolFee().

QA Report

Table of Contents

• summary
• Comment Missing function parameter
• Commented Code
• Constants instead of magic numbers
• Events indexing
• Event should be emitted in setters
• Function missing comments
• Public functions can be external
• Related data should be grouped in struct
• Scientific notation
• Check zero denominator
• Immutable addresses lack zero-address check
• Payable functions when using ERC20
• Receive function
• Setters should check the input value
• Timelock or maximum amount on updatePenalties

summary

Few vulnerabilities were found examining the contracts. The main concerns are with:

Comment Missing function parameter

PROBLEM

Some of the function comments are missing function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

bool verifySellOrder
uint256 execPrice
address seller,address buyer,OrderTypes.OrderItem[] calldata constructedNfts,address currency,uint256 amount
address seller,address buyer,uint256 sellNonce,uint256 buyNonce,OrderTypes.OrderItem[] calldata constructedNfts,address currency,uint256 amount
OrderTypes.MakerOrder calldata order
OrderTypes.MakerOrder calldata order
address destination,address currency,uint256 amount
address destination

InfinityOrderBookComplication.sol

OrderTypes.MakerOrder calldata sell, OrderTypes.MakerOrder calldata buy
OrderTypes.MakerOrder calldata sell, OrderTypes.MakerOrder calldata buy
OrderTypes.MakerOrder calldata sell,OrderTypes.MakerOrder calldata buy,OrderTypes.OrderItem[] calldata constructedNfts
OrderTypes.MakerOrder calldata makerOrder, OrderTypes.OrderItem[] calldata takerItems
OrderTypes.MakerOrder[] calldata orders
OrderTypes.MakerOrder calldata order

InfinityStaker.sol

address user,uint256 amount,uint256 noVesting,uint256 vestedThreeMonths,uint256 vestedSixMonths,uint256 vestedTwelveMonths
address user
address destination
StakeLevel stakeLevel, uint16 threshold
uint16 threeMonthPenalty,uint16 sixMonthPenalty,uint16 twelveMonthPenalty
address _infinityTreasury

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for these parameters

Commented code

PROBLEM

There are portions of commented code in some files.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

line 1169
line1186
line1202

TOOLS USED

Manual Analysis

MITIGATION

Remove commented code

Constants instead of magic numbers

PROBLEM

It is best practice to use constant variables rather than literal values to make the code easier to understand and maintain.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

20000
1000000
10000
10000
10000
10000
10**4

InfinityOrderBookComplication.sol

10**4

InfinityStaker.sol

10**18

TOOLS USED

Manual Analysis

MITIGATION

Define constant variables for the literal values aforementioned.

Events indexing

PROBLEM

Events should use indexed fields

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

event CancelAllOrders(address user, uint256 newMinNonce)
event CancelMultipleOrders(address user, uint256[] orderNonces)
event NewWethTransferGasUnits(uint32 wethTransferGasUnits))
event NewProtocolFee(uint16 protocolFee)
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)
event TakeOrderFulfilled(bytes32 orderHash,address seller,address buyer,address complication, // address of the complication that defines the executionaddress currency, // token address of the transacting currencyuint256 amount // amount spent on the order)

InfinityStaker.sol

event Staked(address indexed user, uint256 amount, Duration duration)
event DurationChanged(address indexed user, uint256 amount, Duration oldDuration, Duration newDuration)
event UnStaked(address indexed user, uint256 amount)
event RageQuit(address indexed user, uint256 totalToUser, uint256 penalty)

InfinityToken.sol

event EpochAdvanced(uint256 currentEpoch, uint256 supplyMinted)

TOOLS USED

Manual Analysis

MITIGATION

Add indexed fields to these events so that they have the maximum number of indexed fields possible.

Event should be emitted in setters

PROBLEM

Setters should emit an event so that Dapps can detect important changes to storage

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

All the functions editing the currencies and complications of the exchange should emit an event
function addCurrency
function addComplication
function removeCurrency
function removeComplication

function updateMatchExecutor

InfinityStaker.sol

function updateStakeLevelThreshold
function updatePenalties
function updateInfinityTreasury

TOOLS USED

Manual Analysis

MITIGATION

Emit an event in all setters.

Function missing comments

PROBLEM

Some functions are missing comments.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

function _emitMatchEvent
function _emitTakerEvent
function _nftsHash
function _tokensHash

InfinityStaker.sol

function _getDurationInSeconds

InfinityToken.sol

function advanceEpoch()
function _beforeTokenTransfer()
function _afterTokenTransfer()
function _mint()
function _burn()
function getAdmin()
function getTimelock()
function getInflation()
function getCliff()
function getMaxEpochs()
function getEpochDuration()

TOOLS USED

Manual Analysis

MITIGATION

Add comments to these functions

Public functions can be external

PROBLEM

It is good practice to mark functions as external instead of public if they are not called by the contract where they are defined.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

InfinityStaker.sol

function getUserTotalStaked()
function getUserTotalVested()

TOOLS USED

Manual Analysis

MITIGATION

Declare these functions as external instead of public

Related data should be grouped in struct

PROBLEM

When there are mappings that use the same key value, having separate fields is error prone, for instance in case of deletion or with future new fields.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

mapping(address => uint256) public userMinOrderNonce;
mapping(address => mapping(uint256 => bool)) public isUserOrderNonceExecutedOrCancelled;

TOOLS USED

Manual Analysis

MITIGATION

Group the related data in a struct and use one mapping:

struct OrderNonce {
  uint256 userMin;
  mapping(uint256 => bool) isExecutedOrCancelled;
}

And it would be used as a state variable:

mapping(address =>  OrderNonce) orderNonces;

Scientific notation

PROBLEM

For readability, it is best to use scientific notation (e.g 10e5) rather than decimal literals(100000) or exponentiation(10**5)

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

uint32 public WETH_TRANSFER_GAS_UNITS = 50000
20000
1000000
10000
10000
10000
10000
10**4

InfinityOrderBookComplication.sol

10**4

InfinityStaker.sol

uint16 public BRONZE_STAKE_THRESHOLD = 1000
uint16 public SILVER_STAKE_THRESHOLD = 5000
uint16 public GOLD_STAKE_THRESHOLD = 10000
uint16 public PLATINUM_STAKE_THRESHOLD = 20000
10**18

TOOLS USED

Manual Analysis

MITIGATION

Replace the numbers aforementioned with their scientific notation

Check zero denominator

PROBLEM

When a division is computed, it must be ensured that the denominator is non-zero to prevent failure of the function call.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

InfinityStaker.sol

((threeMonthLock - threeMonthVested) / THREE_MONTH_PENALTY)
((sixMonthLock - sixMonthVested) / SIX_MONTH_PENALTY)
((twelveMonthLock - twelveMonthVested) / TWELVE_MONTH_PENALTY)

All these storage variables in the denominators are set by the owner in updatePenalties(), and can be 0 as there is no non-zero check.

TOOLS USED

Manual Analysis

MITIGATION

Before doing these computations, add a non-zero check to these variables. Or alternatively, add a non-zero check in updatePenalties().

Immutable addresses lack zero-address check

IMPACT

constructors should check the address written in an immutable address variable is not the zero address

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

WETH = _WETH

TOOLS USED

Manual Analysis

MITIGATION

Add a zero address check for _WETH.

Payable functions when using ERC20

PROBLEM

There should be a require(0 == msg.value) to ensure no Ether is being sent to the exchange when the currency used in an order is a ERC20 token.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

scope: takeMultipleOneOrders

• When !isMakerSeller, there is no check to see if msg.value == 0

scope: takeOrders

• When !isMakerSeller, there is no check to see if msg.value == 0

TOOLS USED

Manual Analysis

MITIGATION

Add require(0 == msg.value) in both condition blocks mentioned above.

Receive function

PROBLEM

InfinityStaker.sol is not supposed to receive ETH. Instead of using a rescue function, remove receive() and fallback() altogether.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

InfinityStaker.sol

fallback() external payable
receive() external payable
function rescueETH(address destination) external payable

TOOLS USED

Manual Analysis

MITIGATION

Remove these functions, or include a call to rescueETH in receive(), so that a user that mistakenly sends ETH to the Staker retrieves it immediately.

Setters should check the input value

PROBLEM

Setters should check the input value - ie make revert if it is the zero address or zero

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

function updateMatchExecutor

InfinityStaker.sol

function updateStakeLevelThreshold There should be a check that the new threshold does not break the following: BRONZE < SILVER < GOLD < PLATINUM
function updatePenalties
function updateInfinityTreasury

TOOLS USED

Manual Analysis

MITIGATION

Add non-zero checks - address or uint - to the setters aforementioned.

Timelock or maximum amount on updatePenalties

PROBLEM

updatePenalties() changes how much a user loses upon "ragequiting" - ie withdrawing their tokens from the staker without waiting for the vesting period. It currently does not have any timelock, or any maximum amount: the owner can set the penalties such that a user calling rageQuit() loses all their Infinity tokens (all would be transferred to INFINITY_TREASURY). Adding a timelock would provide more guarantees to users and reduces the level of trust required.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

InfinityStaker.sol

updatePenalties

TOOLS USED

Manual Analysis

MITIGATION

Either add a timelock to updatePenalties(), or add a maximum penalty check.

Gas Report

Table of Contents

• Caching storage variables in memory to save gas
• Comparison operators
• Constant expressions
• Constructor parameters should be avoided when possible
• Custom errors
• Default value initialization
• Event emitting of local variable
• Immutable variables save storage
• Mathematical optimizations
• Modifier instead of duplicate require
• Multiplication by one
• Require instead of AND
• Require tautologies
• Return statements
• Revert strings length
• Tight variable packing
• unchecked arithmetic

Caching storage variables in memory to save gas

IMPACT

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

scope: cancelAllOrders()

• userMinOrderNonce[msg.sender] is read twice

line 380
line 381

InfinityStaker.sol

scope: stake()

• INFINITY_TOKEN is read twice

line 69
line 74

scope: rageQuit()

It may not be gas efficient to cache the storage variables read in this function, depending on the likelihood of the function to be called if totalPower > GOLD_STAKE_THRESHOLD

• INFINITY_TOKEN is read twice

line 141
line 142

scope: getUserStakeLevel()

• BRONZE_STAKE_THRESHOLD is read twice

line 213
line 215

• SILVER_STAKE_THRESHOLD is read twice

line 215
line 217

• GOLD_STAKE_THRESHOLD is read twice

line 217
line 219

InfinityToken.sol

scope: advanceEpoch()

• currentEpoch is read twice

line 61
line 66

• previousEpochTimestamp is read twice

line 63
line 65

TOOLS USED

Manual Analysis

MITIGATION

cache these storage variables in memory

Comparison Operators

IMPACT

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.

Using strict comparison operators saves approximately 20 gas per comparison.

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

makerOrders[i].constraints[3] <= block.timestamp
makerOrders[i].constraints[4] >= block.timestamp
msg.value >= totalPrice
msg.value >= totalPrice
orderNonces[i] >= userMinOrderNonce[msg.sender]

InfinityOrderBookComplication.sol

makerOrder2.constraints[3] <= block.timestamp
makerOrder2.constraints[4] >= block.timestamp
makerOrder1.constraints[3] <= block.timestamp
makerOrder1.constraints[4] >= block.timestamp
makerOrder2Price >= makerOrder1Price
makerOrder1Price >= makerOrder2Price
manyMakerOrders[i].constraints[3] <= block.timestamp
manyMakerOrders[i].constraints[4] >= block.timestamp
makerOrder.constraints[3] <= block.timestamp
makerOrder.constraints[4] >= block.timestamp
makerOrder.constraints[4] >= block.timestamp
sumCurrentOrderPrices >= currentMakerOrderPrice
sumCurrentOrderPrices <= currentMakerOrderPrice
makerOrder.constraints[3] <= block.timestamp
makerOrder.constraints[4] >= block.timestamp
sell.constraints[3] <= block.timestamp
sell.constraints[4] >= block.timestamp
buy.constraints[3] <= block.timestamp
buy.constraints[4] >= block.timestamp
currentBuyPrice >= currentSellPrice
numConstructedItems >= buy.constraints[0]
buy.constraints[0] <= sell.constraints[0]

InfinityStaker.sol

IERC20(INFINITY_TOKEN).balanceOf(msg.sender) >= amount
userstakedAmounts[msg.sender][oldDuration].amount >= amount
totalVested >= amount
totalStaked >= 0
totalPower <= BRONZE_STAKE_THRESHOLD
totalPower <= SILVER_STAKE_THRESHOLD
totalPower <= GOLD_STAKE_THRESHOLD
totalPower <= PLATINUM_STAKE_THRESHOLD
secondsSinceStake >= durationInSeconds

InfinityToken.sol

block.timestamp >= currentEpochTimestamp
block.timestamp >= previousEpochTimestamp

TOOLS USED

Manual Analysis

MITIGATION

Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable

example:

-totalPower <= BRONZE_STAKE_THRESHOLD;
+totalPower < BRONZE_STAKE_THRESHOLD + 1;

However, if 1 is negligible compared to the value of the variable, we can omit the increment.

example:

-block.timestamp >= currentEpochTimestamp;
+block.timestamp > currentEpochTimestamp;

Constant expressions

IMPACT

Constant expressions are re-calculated each time it is in use, costing an extra 97 gas than a constant every time they are called.

PROOF OF CONCEPT

Instances include:

InfinityToken.sol

bytes32 public constant EPOCH_INFLATION = keccak256('Inflation')
bytes32 public constant EPOCH_DURATION = keccak256('EpochDuration')
bytes32 public constant EPOCH_CLIFF = keccak256('Cliff')
bytes32 public constant MAX_EPOCHS = keccak256('MaxEpochs')

TOOLS USED

Manual Analysis

MITIGATION

Mark these as immutable instead of constant

Constructor parameters should be avoided when possible

IMPACT

Constructor parameters are expensive. The contract deployment will be cheaper in gas if they are hard coded instead of using constructor parameters.

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

WETH = _WETH
MATCH_EXECUTOR = _matchExecutor

InfinityStaker.sol

INFINITY_TOKEN = _tokenAddress
INFINITY_TREASURY = _infinityTreasury

TOOLS USED

Manual Analysis

MITIGATION

Hardcode storage variables with their initial value instead of writing it during contract deployment with constructor parameters.

Custom Errors

IMPACT

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here

It not only saves gas upon deployment - ~5500 gas saved per custom error instead of a require statement, but it is also cheaper in a function call, 22 gas saved per require statement replaced with a custom error.

Custom errors are defined using the error statement

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

require(msg.sender == MATCH_EXECUTOR, 'OME')
require(numMakerOrders == makerOrders2.length, 'mismatched lengths')
require(_complications.contains(makerOrders1[i].execParams[0]), 'invalid complication')
require(canExec, 'cannot execute')
require(msg.sender == MATCH_EXECUTOR, 'OME')
require(_complications.contains(makerOrder.execParams[0]), 'invalid complication')
require(IComplication(makerOrder.execParams[0]).canExecMatchOneToMany(makerOrder, manyMakerOrders),'cannot execute')
require(isOrderValid(makerOrder, makerOrderHash), 'invalid maker order')
require(msg.sender == MATCH_EXECUTOR, 'OME')
require(numSells == buys.length && numSells == constructs.length, 'mismatched lengths')
require(executionValid, 'cannot execute')
require(currency != address(0), 'offers only in ERC20')
require(isOrderValid(makerOrders[i], makerOrderHash), 'invalid maker order')
require(isTimeValid, 'invalid time')
require(currency == makerOrders[i].execParams[1], 'cannot mix currencies')
require(isMakerSeller == makerOrders[i].isSellOrder, 'cannot mix order sides')
require(msg.value >= totalPrice, 'invalid total price')
require(ordersLength == takerNfts.length, 'mismatched lengths')
require(currency != address(0), 'offers only in ERC20')
require(currency == makerOrders[i].execParams[1], 'cannot mix currencies')
require(isMakerSeller == makerOrders[i].isSellOrder, 'cannot mix order sides')
require(msg.value >= totalPrice, 'invalid total price')
require(minNonce > userMinOrderNonce[msg.sender], 'nonce too low')
require(minNonce < userMinOrderNonce[msg.sender] + 1000000, 'too many')
require(numNonces > 0, 'cannot be empty')
require(orderNonces[i] >= userMinOrderNonce[msg.sender], 'nonce too low')
require(!isUserOrderNonceExecutedOrCancelled[msg.sender][orderNonces[i]]
require(verifyMatchOneToOneOrders(sellOrderHash, buyOrderHash, sell, buy), 'order not verified')
require(verifyMatchOneToManyOrders(buyOrderHash, false, sell, buy), 'order not verified')
require(verifyMatchOneToManyOrders(sellOrderHash, true, sell, buy), 'order not verified')
require(verifyMatchOrders(sellOrderHash, buyOrderHash, sell, buy), 'order not verified')
require(makerOrderValid && executionValid, 'order not verified')
require(sent, 'failed to send ether to seller')

InfinityOrderBookComplication.sol

require(tokenIdsIntersect, 'tokenIds dont intersect')

InfinityStaker.sol

require(amount != 0, 'stake amount cant be 0')
require(IERC20(INFINITY_TOKEN).balanceOf(msg.sender) >= amount, 'insufficient balance to stake')
require(amount != 0, 'amount cant be 0')
require(userstakedAmounts[msg.sender][oldDuration].amount >= amount,'insufficient staked amount to change duration')
require(newDuration > oldDuration, 'new duration must be greater than old duration')
require(amount != 0, 'stake amount cant be 0')
require(totalVested >= amount, 'insufficient balance to unstake')
require(totalStaked >= 0, 'nothing staked to rage quit')
require(sent, 'Failed to send Ether')

InfinityToken.sol

require(currentEpoch < getMaxEpochs(), 'no epochs left')
require(block.timestamp >= currentEpochTimestamp + getCliff(), 'cliff not passed')
require(block.timestamp >= previousEpochTimestamp + getEpochDuration(), 'not ready to advance')

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance, in InfinityExchange.sol:

Replace

require(msg.sender == MATCH_EXECUTOR, 'OME');

with

if (msg.sender != MATCH_EXECUTOR) {
		revert IsNotMatchExecutor(msg.sender);
}

and define the custom error in the contract

error IsNotMatchExecutor(address _caller);

Default value initialization

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 22 gas per variable initialized.

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0
uint256 i = 0

InfinityOrderBookComplication.sol

bool _isPriceValid = false
uint256 i = 0
uint256 j = 0
bool _isPriceValid = false
uint256 numConstructedItems = 0
uint256 i = 0
uint256 numTakerItems = 0
uint256 i = 0
uint256 numCollsMatched = 0
uint256 i = 0
uint256 j = 0
uint256 numTokenIdsPerCollMatched = 0
uint256 k = 0
uint256 l = 0
uint256 sum = 0
uint256 i = 0

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values.

Event emitting of local variable

PROBLEM

When emitting an event, using a local variable instead of a storage variable saves gas.

PROOF OF CONCEPT

Instances include:

InfinityToken.sol

emit EpochAdvanced(currentEpoch, supplyToMint)

TOOLS USED

Manual Analysis

MITIGATION

Emit the local variable epochsPassedSinceLastAdvance instead of the storage variable currentEpoch

Immutable variables save storage

PROBLEM

If a variable is set in the constructor and never modified afterwrds, marking it as immutable can save a storage operation - 20,000 gas.

PROOF OF CONCEPT

Instances include:

InfinityStaker.sol

INFINITY_TOKEN

InfinityToken.sol

currentEpochTimestamp

TOOLS USED

Manual Analysis

MITIGATION

Mark this variable as immutable.

Mathematical optimizations

PROBLEM

X += Y costs 22 more gas than X = X + Y. This can mean a lot of gas wasted in a function call when the computation is repeated n times (loops)

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

protocolFee += _matchOneMakerBuyToManyMakerSells(makerOrderHash,manyMakerOrders[i],makerOrder,protocolFeeBps)
totalPrice += execPrice
totalPrice += execPrice

InfinityOrderBookComplication.sol

numItems += manyMakerOrders[i].nfts[j].tokens.length
numConstructedItems += constructedNfts[i].tokens.length
numTakerItems += takerItems[i].tokens.length
sum += _getCurrentPrice(orders[i])

InfinityStaker.sol

userstakedAmounts[msg.sender][duration].amount += amount
userstakedAmounts[msg.sender][oldDuration].amount -= amount
userstakedAmounts[msg.sender][newDuration].amount += amount
userstakedAmounts[user][Duration.TWELVE_MONTHS].amount -= amount
userstakedAmounts[user][Duration.SIX_MONTHS].amount -= amount
userstakedAmounts[user][Duration.THREE_MONTHS].amount -= amount
userstakedAmounts[user][Duration.NONE].amount -= amount

InfinityToken.sol

currentEpoch += epochsPassedSinceLastAdvance

TOOLS USED

Manual Analysis

MITIGATION

use X = X + Y instead of X += Y (same with -)

Modifier instead of duplicate require

PROBLEM

When a require statement is used multiple times, it is cheaper to use a modifier instead.

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

require(msg.sender == MATCH_EXECUTOR, 'OME')
require(msg.sender == MATCH_EXECUTOR, 'OME') require(msg.sender == MATCH_EXECUTOR, 'OME')
require(currency != address(0), 'offers only in ERC20')
require(currency != address(0), 'offers only in ERC20')
require(msg.value >= totalPrice, 'invalid total price')
require(msg.value >= totalPrice, 'invalid total price')

InfinityStaker.sol

require(amount != 0, 'stake amount cant be 0')
require(amount != 0, 'amount cant be 0')
require(amount != 0, 'stake amount cant be 0')

TOOLS USED

Manual Analysis

MITIGATION

Use modifiers for these repeated statements

Multiplication by one

PROBLEM

Multiplying by one is a waste of gas. With the current compiler settings, removing it can save up to 140 gas per function call.

PROOF OF CONCEPT

Instances include:

InfinityStaker.sol

scope getUserStakePower()

• (userstakedAmounts[user][Duration.NONE].amount * 1

TOOLS USED

Manual Analysis

MITIGATION

Replace this line with

userstakedAmounts[user][Duration.NONE].amount

Require instead of AND

IMPACT

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas. It saves 50 gas per require statement broken down.

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

require(numSells == buys.length && numSells == constructs.length, 'mismatched lengths')
require(makerOrderValid && executionValid, 'order not verified')

TOOLS USED

Manual Analysis

MITIGATION

Break down the statements in multiple require statements.

-require(makerOrderValid && executionValid, 'order not verified');
+require(makerOrderValid) 
+require(executionValid);

You can also improve gas savings by using custom errors

Require tautologies

IMPACT

Require statements that check (uint >= 0) will always be true. They waste gas and should be removed

PROOF OF CONCEPT

Instances include:

InfinityStaker.sol

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

TOOLS USED

Manual Analysis

MITIGATION

Remove this require statement

Return statements

IMPACT

Named returns are the most gas efficient return statements, but there is no gas saving if the named return is unused and a return statement is used - costing an extra 2,000 gas per function call.

PROOF OF CONCEPT

Instances include:

InfinityToken.sol

return address(uint160(TimelockConfig.getConfig(TimelockConfig.ADMIN).value))
return TimelockConfig.getConfig(TimelockConfig.TIMELOCK).value
return TimelockConfig.getConfig(EPOCH_INFLATION).value
return TimelockConfig.getConfig(EPOCH_CLIFF).value
return TimelockConfig.getConfig(MAX_EPOCHS).value
return TimelockConfig.getConfig(EPOCH_DURATION).value

TOOLS USED

Manual Analysis

MITIGATION

Replace the return statements as explained, using a local variable instead.

Revert strings length

IMPACT

Revert strings cost more gas to deploy if the string is larger than 32 bytes. It costs an extra 9,500 gas per string exceeding that 32-byte size upon deployment.

PROOF OF CONCEPT

Revert strings exceeding 32 bytes include:

InfinityExchange.sol

nonce already executed or cancelled

InfinityStaker.sol

insufficient staked amount to change duration
new duration must be greater than old duration

TOOLS USED

Manual Analysis

MITIGATION

Write the error strings so that they do not exceed 32 bytes. For further gas savings, consider also using custom errors.

Tight Variable Packing

PROBLEM

Solidity contracts have contiguous 32 bytes (256 bits) slots used in storage. By arranging the variables, it is possible to minimize the number of slots used within a contract's storage and therefore reduce deployment costs.

address type variables are each of 20 bytes size (way less than 32 bytes). However, they here take up a whole 32 bytes slot (they are contiguous).

As uint16 type variables are of size 2 bytes, there's a slot here that can get saved by moving one uint16 closer to an address - saving a gsset, ie 20,000 gas

PROOF OF CONCEPT

Instances include:

InfinityStaker.sol

address public INFINITY_TOKEN; @audit - slot 1
  ///@dev Infinity treasury address - will be a EOA/multisig
address public INFINITY_TREASURY; @audit - slot 2

/**@dev Power levels to reach the specified stake thresholds. Users can reach these levels 
        either by staking the specified number of tokens for no duration or a less number of tokens but with higher durations.
        See getUserStakePower() to see how users can reach these levels.
*/
uint16 public BRONZE_STAKE_THRESHOLD = 1000;
uint16 public SILVER_STAKE_THRESHOLD = 5000;
uint16 public GOLD_STAKE_THRESHOLD = 10000;
uint16 public PLATINUM_STAKE_THRESHOLD = 20000;

uint16 public THREE_MONTH_PENALTY = 2;
uint16 public SIX_MONTH_PENALTY = 3;
uint16 public TWELVE_MONTH_PENALTY = 4; @audit - slot 3

TOOLS USED

Manual Analysis

MITIGATION

Place TWELVE_MONTH_PENALTY after INFINITY_TOKEN to save one storage slot

address public INFINITY_TOKEN; @audit - slot 1
+uint16 public TWELVE_MONTH_PENALTY = 4; 
  ///@dev Infinity treasury address - will be a EOA/multisig
address public INFINITY_TREASURY; @audit - slot 2

/**@dev Power levels to reach the specified stake thresholds. Users can reach these levels 
        either by staking the specified number of tokens for no duration or a less number of tokens but with higher durations.
        See getUserStakePower() to see how users can reach these levels.
*/
uint16 public BRONZE_STAKE_THRESHOLD = 1000;
uint16 public SILVER_STAKE_THRESHOLD = 5000;
uint16 public GOLD_STAKE_THRESHOLD = 10000;
uint16 public PLATINUM_STAKE_THRESHOLD = 20000;

uint16 public THREE_MONTH_PENALTY = 2;
uint16 public SIX_MONTH_PENALTY = 3;

Unchecked arithmetic

IMPACT

The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas

PROOF OF CONCEPT

Instances include:

InfinityExchange.sol

startPrice - endPrice : endPrice - startPrice

• Because of the condition startPrice > endPrice and the ternary operator, none of these can underflow

InfinityOrderBookComplication.sol

startPrice - endPrice : endPrice - startPrice

• Because of the condition startPrice > endPrice and the ternary operator, none of these can underflow

InfinityStaker.sol

userstakedAmounts[msg.sender][oldDuration].amount -= amount

• Because of the require statement line 99, it cannot underflow

amount = amount - noVesting

• Because of the condition amount > noVesting, it cannot underflow

amount = amount - vestedThreeMonths

• Because of the condition amount > vestedThreeMonths, it cannot underflow amount = amount - vestedSixMonths
• Because of the condition amount > vestedSixMonths, it cannot underflow

InfinityToken.sol

block.timestamp - previousEpochTimestamp

• Because of the require statement line 63, it cannot underflow

getMaxEpochs() - currentEpoch

• Because of the require statement line 61, it cannot underflow

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block