code-423n4 / 2022-06-infinity-findings

4 stars 0 forks source link

Gas Optimizations #297

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Gas Report

Table of Contents

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()

line 380\ line 381

InfinityStaker.sol

scope: stake()

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

line 141\ line 142

scope: getUserStakeLevel()

line 213\ line 215

line 215\ line 217

line 217\ line 219

InfinityToken.sol

scope: advanceEpoch()

line 61\ line 66

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()

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

InfinityOrderBookComplication.sol

startPrice - endPrice : endPrice - startPrice

InfinityStaker.sol

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

amount = amount - noVesting

amount = amount - vestedThreeMonths

InfinityToken.sol

block.timestamp - previousEpochTimestamp

getMaxEpochs() - currentEpoch

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block

nneverlander commented 2 years ago

Thanks

HardlyDifficult commented 2 years ago

🔥