Gas Optimizations

[code423n4]

More efficient Struct packing of Market in the contract ComptrollerStorage.sol

    The following structs could change the order of their stored elements to decrease memory uses.
    and number of occupied slots. Therefore will save gas at every store and load from memory.

In ComptrollerStorage.sol, Market is optimized to: 3 slots from: 4 slots.
The new order of types (you choose the actual variables):
1. uint256
2. mapping
3. bool
4. bool

Unnecessary equals boolean

Boolean variables can be checked within conditionals directly without the use of equality operators to true/false.

Code instances:

    RoleManager.sol, 137: account == addressProvider.getAddress(AddressProviderKeys._POOL_FACTORY_KEY, false);
    BkdTriHopCvx.sol, 169: if (_lpBalance() == 0) return false;
    Vault.sol, 647: if (address(strategy) == address(0)) return false;
    CvxCrvRewardsLocker.sol, 283: if (IERC20(CVX_CRV).balanceOf(address(this)) == 0) return false;
    Vault.sol, 434: if (address(strategy) == address(0)) return false;

State variables that could be set immutable

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

Code instances:

    _decimals in LpToken.sol
    minter in LpToken.sol
    token in StakerVault.sol
    minWithdrawalDelay in VaultReserve.sol

Unused state variables

Unused state variables are gas consuming at deployment (since they are located in storage) and are
a bad code practice. Removing those variables will decrease deployment gas cost and improve code quality.
This is a full list of all the unused storage variables we found in your code base.

Code instances:

    Errors.sol, INVALID_TOKEN_TO_REMOVE
    AddressProviderKeys.sol, _REWARD_HANDLER_KEY
    Errors.sol, ADDRESS_NOT_ACTION
    Errors.sol, INVALID_INDEX
    InterestRateModel.sol, isInterestRateModel

Unused declared local variables

Unused local variables are gas consuming, since the initial value assignment costs gas. And are
a bad code practice. Removing those variables will decrease the gas cost and improve code quality.
This is a full list of all the unused storage variables we found in your code base.

Code instances:

    PoolMigrationZap.sol, migrate, ethValue_
    RewardHandler.sol, burnFees, ethBalance
    FeeBurner.sol, _depositInPool, ethBalance_

Caching array length can save gas

Caching the array length is more gas efficient.
This is because access to a local variable in solidity is more efficient than query storage / calldata / memory.
We recommend to change from:

for (uint256 i=0; i<array.length; i++) { ... }

to:

uint len = array.length  
for (uint256 i=0; i<len; i++) { ... }

Code instances:

    StakerVault.sol, actions, 259
    InflationManager.sol, stakerVaults, 116
    PoolMigrationZap.sol, newPools_, 22
    VestedEscrow.sol, amounts, 94
    PoolMigrationZap.sol, oldPoolAddresses_, 39

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:

    just change to unchecked: PoolMigrationZap.sol, i, 22

Unnecessary default assignment

Unnecessary default assignments, you can just declare and it will save gas and have the same meaning.

Code instances:

    Vault.sol (L#43) : uint256 internal constant _INITIAL_STRATEGIST_FEE = 0.1e18;
    Vault.sol (L#46) : uint256 public constant MAX_PERFORMANCE_FEE = 0.5e18;
    Vault.sol (L#42) : uint256 internal constant _INITIAL_RESERVE_FEE = 0.01e18;
    CvxCrvRewardsLocker.sol (L#43) : int128 private constant _CRV_INDEX = 0;
    Vault.sol (L#44) : uint256 internal constant _INITIAL_PERFORMANCE_FEE = 0; 

Rearrange state variables

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

Code instances:

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

In KeeperGauge.sol,rearranging the storage fields can optimize to: 3 slots from: 4 slots.
The new order of types (you choose the actual variables):
1. IController
2. uint256
3. address
4. uint48
5. bool

Use bytes32 instead of string to save gas whenever possible

Use bytes32 instead of string to save gas whenever possible.
String is a dynamic data structure and therefore is more gas consuming then bytes32.

Code instances:

    Errors.sol (L22), string internal constant INVALID_IMPLEMENTATION = "invalid pool implementation for given coin";
    Errors.sol (L59), string internal constant FAILED_MINT = "mint failed";
    Errors.sol (L62), string internal constant NOTHING_TO_CLAIM = "there is no claimable balance";
    Errors.sol (L55), string internal constant UNDERLYING_NOT_SUPPORTED = "underlying token not supported";
    Errors.sol (L91), string internal constant STRATEGY_DOES_NOT_EXIST = "Strategy does not exist";

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: Minter.sol, In line 150, Require message length to shorten: 38, The message: Maximum non-inflation amount exceeded.

Use != 0 instead of > 0

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

Code instances:

    AmmConvexGauge.sol, 171: change 'amount > 0' to 'amount != 0'
    StakerVault.sol, 323: change 'balance > 0' to 'balance != 0'
    LiquidityPool.sol, 469: change 'underlyingAmount > 0' to 'underlyingAmount != 0'
    InflationManager.sol, 602: change 'totalAmmTokenWeight > 0' to 'totalAmmTokenWeight != 0'
    Controller.sol, 107: change 'balance > 0' to 'balance != 0'

Unnecessary cast

Code instances:

    IController PoolFactory.sol.constructor - unnecessary casting IController(_controller)
    IController LpGauge.sol.constructor - unnecessary casting IController(_controller)
    address CompoundHandler.sol._repayAnyDebt - unnecessary casting address(ctoken)
    IController LiquidityPool.sol.constructor - unnecessary casting IController(_controller)

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:

    BkdLocker.sol (L#124) - stashedGovTokens[msg.sender].push( WithdrawStash(block.timestamp + currentUInts256[_WITHDRAW_DELAY], amount) );
    Minter.sol (L#188) - totalAvailableToNow += (currentTotalInflation * (block.timestamp - lastEvent));
    AmmGauge.sol (L#89) - ammStakedIntegral_ += (controller.inflationManager().getAmmRateForToken(ammToken) * (block.timestamp - uint256(ammLastUpdated))).scaledDiv(totalStaked);
    BkdLocker.sol (L#276) - newBoost += (block.timestamp - lastUpdated[user]) .scaledDiv(currentUInts256[_INCREASE_PERIOD]) .scaledMul(maxBoost - startBoost);
    LpGauge.sol (L#69) - poolStakedIntegral_ += (inflationManager.getLpRateForStakerVault(address(stakerVault)) * (block.timestamp - poolLastUpdate)).scaledDiv(poolTotalStaked);

Consider inline the following functions to save gas

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)

Code instances

    ScaledMath.sol, scaledDiv, { return (a * DECIMAL_SCALE) / b; }
    ExponentialNoError.sol, lessThanOrEqualExp, { return left.mantissa <= right.mantissa; }
    Preparable.sol, _prepare, { return _prepare(key, value, _MIN_DELAY); }
    AddressProviderHelpers.sol, getSafeRewardHandler, { return provider.getAddress(AddressProviderKeys._REWARD_HANDLER_KEY, false); }
    TopUpKeeperHelper.sol, _positionToTopup, { return TopupData(user, position.account, position.protocol, position.record); }

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:

    ExponentialNoError.sol, sub_
    BkdTriHopCvx.sol, _minLpAccepted
    FeeBurner.sol, _swapperRouter
    LiquidityPool.sol, _doTransferIn
    CompoundHandler.sol, _getAccountBorrowsAndSupply

Cache powers of 10 used several times

You calculate the power of 10 every time you use it instead of caching it once as a constant variable and using it instead.
Fix the following code lines:

Code instances:

DecimalScale.sol, 22 : You should cache the used power of 10 as constant state variable since it's used several times (4): return value / 10**(_DECIMALS - decimals);

DecimalScale.sol, 12 : You should cache the used power of 10 as constant state variable since it's used several times (4): return value * 10**(_DECIMALS - decimals);

DecimalScale.sol, 20 : You should cache the used power of 10 as constant state variable since it's used several times (4): return value * 10**(decimals - _DECIMALS);

DecimalScale.sol, 10 : You should cache the used power of 10 as constant state variable since it's used several times (4): return value / 10**(decimals - _DECIMALS);

Change if -> revert pattern to require

Change if -> revert pattern to 'require' to save gas and improve code quality,
if (some_condition) {
revert(revert_message)
}

to: require(!some_condition, revert_message)

In the following locations:

Code instance:

    CTokenRegistry.sol, 62

Do not cache msg.sender

We recommend not to cache msg.sender since calling it is 2 gas while reading a variable is more.

Code instance:

    https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/VestedEscrow.sol#L63

[GalloDaSballo]

More efficient Struct packing of Market in the contract ComptrollerStorage.sol

Yes but in lack of POC of gas saved for end users I cannot give it any points

Unnecessary equals boolean

The list provided doesn't match the statement

State variables that could be set immutable

Finding is valid, in lack of any further details will give each variable a saved SLOAD
2100 * 4 = 8400

Unused state variables

Valid for the errors

Unused declared local variables

They are used in the value

Caching array length can save gas

Valid 3 gas per instance
5 * 3 = 15

Prefix increments are cheaper than postfix increments

5 gas

Unnecessary default assignment

Valid for 2 assignments, that said these are deployment costs

Rearrange state variables

In lack of POC I cannot give it points

## Use bytes32 instead of string to save gas whenever possible
Mostly disagree in lack of POC

Short the following require messages

Saves 6 gas

Use != 0 instead of > 0

Valid for 4 instances, 3 gas each
12

[GalloDaSballo]

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

20 per instance * 5
100

Consider inline the following functions to save gas

Disagree as the optimizer takes cases

Inline one time use functions

Agreed for
BkdTriHopCvx.sol, _minLpAccepted
FeeBurner.sol, _swapperRouter
LiquidityPool.sol, _doTransferIn
CompoundHandler.sol, _getAccountBorrowsAndSupply

Would save 2 jumps (8 gas each)
4 * 16 = 64

Cache powers of 10 used several times

Disagree as they are calculated once and at the time of need, with variable inputs

Change if -> revert pattern to require

Agree but in lack of math I can't quantify

Do not cache msg.sender

Saves 1 gas

Total Gas Saved
8603

[GalloDaSballo]

In reviewing, this is the only report that found 4 storage -> immutable gas improvements which ultimately save the most gas.

My advice for most wardens is not to spam submission but just focus on finding those big gas savings, which despite all the unactionable findings, ultimately this report was the only one that did

[IllIllI000]

@GalloDaSballo LpToken.sol and VaultReserve.sol are not in scope, so three out of the four storage->immutable are invalid, and therefore #108 saves more gas

[GalloDaSballo]

Thank you @IllIllI000 looking into it

[GalloDaSballo]

I've confirmed that LpToken and VaultReserve are outside of scope, will have to recalculate gas savings for this report

[GalloDaSballo]

Removing 8400 gas from the immutable, the updated score for this report is:
203 gas saved

[GalloDaSballo]

My math was off, as one of the immutable variables is correct, adding 2100 gas

2303 gas saved