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 #51

Open
code423n4 opened this issue Mar 28, 2022 · 8 comments
Open

Gas Optimizations #51

code423n4 opened this issue Mar 28, 2022 · 8 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 acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

Gas Optimizations

constant expressions

expression assigned to constants are recomputed every time it is called, so keccak operation is done every time the variable is used
This can be prevented by using value directly or using immutable so that value is computed once

Proof of concept

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/CBridgeFacet.sol#L18

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/NXTPFacet.sol#L18

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/HopFacet.sol#L18

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibDiamond.sol#L7

Mitigation

Use immutable instead of constant

using unchecked can save gas

arithmetic operations that cant overflow/underflow can placed inside a unchecked block to avoid underflow/overflow check and save gas

Proof of concept

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/NXTPFacet.sol#L166

Mitigation

statement can be placed inside a unchecked block

Storage variable can be cached and re-used to save gas

Storing storage variable re-using it can save ~100 gas on repeated reads

Proof of concept

In DexManagerFacet:removeDex s.dex.length is read in every iteration, this value can be cached in a variable and save ~100 gas in repeated storage reads

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/DexManagerFacet.sol#L52

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/DexManagerFacet.sol#L70

In HopFacet:_startBridge s.hopChainId can be cached

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/HopFacet.sol#L139-L153

Replacing > with != can save gas

using != can save gas than > with uint and optimizer enabled

Proof of concept

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/AnyswapFacet.sol#L92

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/CBridgeFacet.sol#L105

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/NXTPFacet.sol#L98

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/DexManagerFacet.sol#L70

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/HopFacet.sol#L109

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibDiamond.sol#L84

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibDiamond.sol#L102

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibDiamond.sol#L121

Repeating statements

In AnyswapFacet:swapAndStartBridgeTokensViaAnyswap, require and update statements can be placed outside if-else statements

    require(_postSwapBalance > 0, "ERR_INVALID_AMOUNT");
    _anyswapData.amount = _postSwapBalance;

Proof of concept

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/AnyswapFacet.sol#L92-L94

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/CBridgeFacet.sol#L105-L107

Mitigation

the two statements can be placed outide the if-else block

Revert strings can <= 32 bytes

reducing revert string to under 32 bytes can save deployment costs as well as when require condition fails

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

Proof of concept

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/AnyswapFacet.sol#L133

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/CBridgeFacet.sol#L147

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/HopFacet.sol#L146

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibDiamond.sol#L76

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibDiamond.sol#L84

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibDiamond.sol#L133

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibDiamond.sol#L154

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibDiamond.sol#L189

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibDiamond.sol#L200

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/LiFiDiamond.sol#L38

Mitigation

Pre-increment is more efficient than post increment

prefix increment uses less gas when compared to postfix increment
unchecked can be added to save more gas as the variable wont overflow

ethereum/solidity#10695

Proof of concept

In all for loop in the project

Examples
https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/DexManagerFacet.sol#L33

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/DexManagerFacet.sol#L52

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/DiamondLoupeFacet.sol#L24

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/HopFacet.sol#L48

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/Swapper.sol#L14

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibDiamond.sol#L67

In _executeSwaps() LifiData can be replaced with bytes32

In function _executeSwaps() input LifiData only transaction ID is used from the structure lifiData , so it can be replaced with a bytes32 lifiData.transactionID

Proof of concept

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/Swapper.sol#L12

Statements can be reordered to save gas on revert

In LibDiamond:removeFunctions second require statement can be placed above storage read to save gas on revert

    require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut");

    DiamondStorage storage ds = diamondStorage();
    
    // if function does not exist then do nothing and return
    require(_facetAddress == address(0), "LibDiamondCut: Remove facet address must be address(0)");

Proof of concept

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibDiamond.sol#L84-L86

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibDiamond.sol#L102-L104

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibDiamond.sol#L124

Mitigation

Place require statement above storage read

if statements can be nested

In LibSwap:swap if statement can be combined to reduce one call

    if (!LibAsset.isNativeAsset(fromAssetId) && LibAsset.getOwnBalance(fromAssetId) < fromAmount) {
        LibAsset.transferFromERC20(fromAssetId, msg.sender, address(this), fromAmount);
    }

    if (!LibAsset.isNativeAsset(fromAssetId)) {
        LibAsset.approveERC20(IERC20(fromAssetId), _swapData.approveTo, fromAmount);
    }

Proof of concept

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibSwap.sol#L33

Mitigation

if blocks can be nested


if (!LibAsset.isNativeAsset(fromAssetId)) { 
    
    if(LibAsset.getOwnBalance(fromAssetId) < fromAmount) {
        LibAsset.transferFromERC20(fromAssetId, msg.sender, address(this), fromAmount);
    }
    
    LibAsset.approveERC20(IERC20(fromAssetId), _swapData.approveTo, fromAmount);
}

@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels Mar 28, 2022
code423n4 added a commit that referenced this issue Mar 28, 2022
@H3xept
Copy link
Collaborator

H3xept commented Apr 4, 2022

Re unchecked & prefix increment

We internally decided to avoid unchecked statements and prefix increments for now.

@H3xept
Copy link
Collaborator

H3xept commented Apr 4, 2022

  1. Constant expressions: Fixed in lifinfance/lifi-contracts/@39dd074acf47b40f9a544439427e58de0208b961
  2. Storage variable caching: Fixed in lifinance/lifi-contracts/@2b0c057fb05c62d95c0b04edd1864c184ccf9ad8
  3. 32 bytes revert string: Fixed in lifinance/lifi-contracts/@45edddfb56028db3cfd070b57990ae8a455f0109
  4. In _executeSwaps() LifiData can be replaced with bytes32: To maintain the same interface in future updates we should leave this as it is

@H3xept H3xept closed this as completed Apr 4, 2022
@H3xept
Copy link
Collaborator

H3xept commented Apr 8, 2022

Re nesting Ifs

Duplicate of #39

@H3xept
Copy link
Collaborator

H3xept commented Apr 11, 2022

Re Revert strings can <= 32 bytes

Duplicate of #100

@H3xept
Copy link
Collaborator

H3xept commented Apr 11, 2022

Re Storage variable can be cached and re-used to save gas

Duplicate of #196

@H3xept
Copy link
Collaborator

H3xept commented Apr 11, 2022

Re Replacing > with != can save gas

Duplicate of #100

@H3xept H3xept added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Apr 11, 2022
@H3xept
Copy link
Collaborator

H3xept commented Apr 11, 2022

Re prefix increments

We internally decided to avoid previx increments for now.

@H3xept H3xept marked this as a duplicate and then as not a duplicate of #182 Apr 11, 2022
@H3xept
Copy link
Collaborator

H3xept commented Apr 11, 2022

Re constant pre-computation

Duplicate of #182

@H3xept H3xept added the resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) label Apr 11, 2022
@H3xept H3xept reopened this Apr 11, 2022
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 acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Development

No branches or pull requests

2 participants