Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gas Optimizations #16

Open
code423n4 opened this issue May 30, 2022 · 2 comments
Open

Gas Optimizations #16

code423n4 opened this issue May 30, 2022 · 2 comments
Labels
bug Something isn't working G (Gas Optimization) resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

[G-01] Cache array length before loop

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop. This saves gas.

This was found in many places
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/RewardHandler.sol#L42
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/access/RoleManager.sol#L82
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/zaps/PoolMigrationZap.sol#L22
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/zaps/PoolMigrationZap.sol#L39
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/FeeBurner.sol#L56
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/VestedEscrow.sol#L94
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/InflationManager.sol#L116
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/StakerVault.sol#L259

Recommended Mitigation Steps

Cache the array length before the for loop

[G-02] Use != 0 instead of > 0

Using > 0 uses slightly more gas than using != 0. Use != 0 when comparing uint variables to zero, which cannot hold values below zero

Locations where this was found include
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/BkdLocker.sol#L91
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/BkdLocker.sol#L92
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/BkdLocker.sol#L137
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/BkdLocker.sol#L139
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/BkdLocker.sol#L254
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/BkdLocker.sol#L301
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/RewardHandler.sol#L63
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/FeeBurner.sol#L117
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/LpGauge.sol#L68
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/LpGauge.sol#L114
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/VestedEscrow.sol#L84
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/InflationManager.sol#L575
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/InflationManager.sol#L589
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/InflationManager.sol#L602
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmGauge.sol#L88
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmGauge.sol#L104
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmGauge.sol#L125
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmGauge.sol#L147
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/KeeperGauge.sol#L140
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmConvexGauge.sol#L107
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmConvexGauge.sol#L129
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmConvexGauge.sol#L158
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmConvexGauge.sol#L171
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmConvexGauge.sol#L197

Recommended Mitigation Steps

Replace > 0 with != 0 to save gas

[G-03] For loop incrementing can be unsafe

For loops that use i++ do not need to use safemath for this operation because the loop would run out of gas long before this point. Making this addition operation unsafe using unchecked saves gas. This is already used in many places with i = i.uncheckedInc().

Sample code to make the for loop increment unsafe

for (uint i = 0; i < length; i = unchecked_inc(i)) {
    // do something that doesn't change the value of i
}

function unchecked_inc(uint i) returns (uint) {
    unchecked {
        return i + 1;
    }
}

Idea borrowed from
https://gist.github.com/hrkrshnn/ee8fabd532058307229d65dcd5836ddc#the-increment-in-for-loop-post-condition-can-be-made-unchecked

There is one loop and that can use this change
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/zaps/PoolMigrationZap.sol#L22

Recommended Mitigation Steps

Make the increment in for loops unsafe to save gas

[G-04] Use iszero assembly for zero checks

Comparing a value to zero can be done using the iszero EVM opcode. This can save gas

Source from t11s https://twitter.com/transmissions11/status/1474465495243898885

There are many places where a value is compared to zero
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/BkdLocker.sol#L59
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/BkdLocker.sol#L271
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/access/RoleManager.sol#L97
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/access/RoleManager.sol#L99
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/access/RoleManager.sol#L101
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/zaps/PoolMigrationZap.sol#L57
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/utils/Preparable.sol#L28
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/FeeBurner.sol#L61
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/FeeBurner.sol#L69
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/FeeBurner.sol#L75
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/VestedEscrowRevocable.sol#L53
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/VestedEscrow.sol#L144
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/Minter.sol#L105
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/Minter.sol#L133
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/Minter.sol#L165
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/Minter.sol#L174
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/Minter.sol#L183
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/InflationManager.sol#L496
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/InflationManager.sol#L514
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/InflationManager.sol#L522
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/Controller.sol#L106

Recommended Mitigation Steps

Use the assembly iszero evm opcode to compare values to zero

[G-05] Save gas with unchecked

Use unchecked math when there is no overflow risk to save gas. Before index is decreased in remove it is checked for zero condition. This means index will not underflow and can be unchecked.

These subtractions do not need to be checked for underflows because there is a require earlier that confirms the underflow will not happen. Use the uncheckedSub function
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/StakerVault.sol#L124
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/StakerVault.sol#L338

Recommended Mitigation Steps

Add unchecked around math that can't overflow for gas savings. In Solidity before 0.8.0, use the normal math operators instead of safe math functions.

[G-06] Add payable to functions that won't receive ETH

Identifying a function as payable saves gas. Functions that have a modifier like onlyGovernance or onlyAuthorizedToPause cannot be called by normal users and will not mistakenly receive ETH. These functions can be payable to save gas.

There are many functions that have the auth modifier in the contracts. Some examples are
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/BkdLocker.sol#L58
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/BkdLocker.sol#L70
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/access/RoleManager.sol#L37
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/access/RoleManager.sol#L41
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/access/RoleManager.sol#L45
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/access/RoleManager.sol#L50
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/access/RoleManager.sol#L54
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/access/RoleManager.sol#L111
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/Minter.sol#L99
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/Minter.sol#L104
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/Minter.sol#L147
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/InflationManager.sol#L58
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/InflationManager.sol#L70
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/InflationManager.sol#L92
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/InflationManager.sol#L136
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/InflationManager.sol#L167
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/InflationManager.sol#L342
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/InflationManager.sol#L408
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/InflationManager.sol#L435
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/InflationManager.sol#L449
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/InflationManager.sol#L469
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmConvexGauge.sol#L86
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmConvexGauge.sol#L92
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/Controller.sol#L33
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/Controller.sol#L62
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/Controller.sol#L81
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/Controller.sol#L89
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/AddressProvider.sol#L63
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/AddressProvider.sol#L70
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/AddressProvider.sol#L81
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/AddressProvider.sol#L216
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/AddressProvider.sol#L229
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/AddressProvider.sol#L239
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/AddressProvider.sol#L256
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/AddressProvider.sol#L278
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/StakerVault.sol#L75
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/StakerVault.sol#L82
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/StakerVault.sol#L87

Recommended Mitigation Steps

Add payable to these functions for gas savings

[G-07] Use internal function in place of modifier

An internal function can save gas vs. a modifier. A modifier inlines the code of the original function but an internal function does not.

Source https://blog.polymath.network/solidity-tips-and-tricks-to-save-gas-and-reduce-bytecode-size-c44580b218e6#dde7

Many modifiers can use this change
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/access/RoleManager.sol#L27
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/access/AuthorizationBase.sol#L16
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/access/AuthorizationBase.sol#L24
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/access/AuthorizationBase.sol#L32
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/access/AuthorizationBase.sol#L40
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/utils/Pausable.sol#L9
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/utils/Pausable.sol#L14
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/InflationManager.sol#L47
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/KeeperGauge.sol#L39

Recommended Mitigation Steps

Use internal functions in place of modifiers to save gas.

[G-08] Use newer solidity version

Solidity version 0.8.10 is used. The latest release of solidity includes changes that can provide gas savings. The improvements include:

Source https://gist.github.com/hrkrshnn/ee8fabd532058307229d65dcd5836ddc#upgrade-to-at-least-084

Recommended Mitigation Steps

Use solidity release 0.8.13 with Yul IR pipeline and other improvements for gas savings

[G-09] Non-public variables save gas

Many immutable variables are public, but changing the visibility of any of these variables to private or internal can save gas.

https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/BkdLocker.sol#L42
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/RewardHandler.sol#L20
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/RewardHandler.sol#L21
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/access/RoleManager.sol#L25
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/VestedEscrowRevocable.sol#L28
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/BkdToken.sol#L14
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/LpGauge.sol#L19
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/LpGauge.sol#L20
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/LpGauge.sol#L21
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/VestedEscrow.sol#L33
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/VestedEscrow.sol#L37
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/VestedEscrow.sol#L38
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/Minter.sol#L25
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/Minter.sol#L26
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/Minter.sol#L30
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/Minter.sol#L31
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/Minter.sol#L32
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/Minter.sol#L36
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/Minter.sol#L37
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/Minter.sol#L38
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/Minter.sol#L44
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/Minter.sol#L55
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/InflationManager.sol#L24
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmGauge.sol#L20
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmGauge.sol#L30
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/KeeperGauge.sol#L30
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/KeeperGauge.sol#L31
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmConvexGauge.sol#L19
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmConvexGauge.sol#L20
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmConvexGauge.sol#L21
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmConvexGauge.sol#L24
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/AmmConvexGauge.sol#L25
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/Controller.sol#L21
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/StakerVault.sol#L43
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/StakerVault.sol#L45
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/StakerVault.sol#L46

Recommended Mitigation Steps

Declare some public variables as private or internal to save gas

[G-10] Use calldata instead of memory for function arguments

Using calldata instead of memory for function arguments saves gas sometimes. This can happen when a function is called externally and the memory array values are kept in calldata and copied to memory during ABI decoding (using the opcode calldataload and mstore). If the array is used in a for loop, arr[i] accesses the value in memory using a mload. If calldata is used instead, then instead of going via memory, the value is directly read from calldata using calldataload. That is, there are no intermediate memory operations that carries this value.

One case of function arguments using memory instead of calldata can use this improvement to save gas
https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/FeeBurner.sol#L43

Source
https://gist.github.com/hrkrshnn/ee8fabd532058307229d65dcd5836ddc#use-calldata-instead-of-memory-for-function-parameters

Related DoS issue
https://twitter.com/danielvf/status/1519381832592199681

Recommended Mitigation Steps

Change function arguments from memory to calldata

[G-11] Write contracts in vyper

The contracts are all written entirely in solidity. Writing contracts with vyper instead of solidity can save gas.

Source https://twitter.com/eiber_david/status/1515737811881807876

Recommended Mitigation Steps

Write some or all of the contracts in vyper to save gas

@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels May 30, 2022
code423n4 added a commit that referenced this issue May 30, 2022
@chase-manning chase-manning added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Jun 6, 2022
@chase-manning
Copy link
Collaborator

Lol "Write contracts in vyper" 🤣

@chase-manning chase-manning added the resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) label Jun 7, 2022
@GalloDaSballo
Copy link
Collaborator

GalloDaSballo commented Jun 16, 2022

[G-01] Cache array length before loop

Would save 3 gas per instance
3 * 8 = 24

[G-02] Use != 0 instead of > 0

Works only for requires, saves 3 gas
Only 9 instances were requires

3 * 9 = 27

[G-03] For loop incrementing can be unsafe

Saves 25 gas

25

[G-04] Use iszero assembly for zero checks

Saves 2 gas per the discussion in #27
2 * 22 = 44

[G-05] Save gas with unchecked

Would save 20 gas per instance

40

[G-06] Add payable to functions that won't receive ETH

It would save gas but personally would not count it as it does change the contract behaviour even if by a small margin (also changes UX on Metamask)

[G-07] Use internal function in place of modifier

Indeed does save gas on deployment

[G-08] Use newer solidity version

Valid but hard to quantify

[G-09] Non-public variables save gas

Would save gas by removing functionality

[G-10] Use calldata instead of memory for function arguments

Finding is valid but in lack of math I'm not going to add it

[G-11] Write contracts in vyper

Same as above, I guess we'll burn hundreds of thousands of dollars of work and rewrite in vyper

I think this report could have been great had the warden done more comparison of the real code with improvements

Total Gas Saved:
160

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working G (Gas Optimization) resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

3 participants