code-423n4 / 2022-03-lifinance-findings

6 stars 4 forks source link

Gas Optimizations #100

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Gas optimization report

For uint use != 0 instead of > 0

For the type uint it is cheaper to use the logical operator != instead of the > when checking the value is not 0. The value can never be below 0 since it is unsigned and hence would underflow instead.

The relevant code:

AnyswapFacet.sol line 92:          _postSwapBalance > 0
AnyswapFacet.sol line 105:         _postSwapBalance > 0

CBridgeFacet.sol line 105:         _postSwapBalance > 0
CBridgeFacet.sol line 116:         _postSwapBalance > 0

HopFacet.sol line 109:             _postSwapBalance > 0

NXTPFacet.sol line 98:             _postSwapBalance > 0

LibAsset.sol line 67:              allowance > 0

LibDiamond.sol line 84:            _functionSelectors.length > 0
LibDiamond.sol line 102:           _functionSelectors.length > 0
LibDiamond.sol line 121:           _functionSelectors.length > 0
LibDiamond.sol line 189:           _calldata.length > 0
LibDiamond.sol line 196:           error.length > 0
LibDiamond.sol line 212:           contractSize > 0

Keep revert strings below 32 bytes

Strings are stored in slots of 32 bytes, and hence the length of the revert string should be at max 32 bytes to fit inside 1 slot. If the string is just 33 bytes it will occupy 2 slots (64 bytes). Keeping the string size at 32 bytes or below will save gas on both deployment and when the revert condition is met.

Since the used version of Solidity is >=0.8.4 it would also be worth considering using Custom Errors which is both more gas efficient and allows error descriptions using NatSpec.

The relevant code:

AnyswapFacet.sol line 133:          "Cannot bridge to the same network."

CBridgeFacet.sol line 147:          "Cannot bridge to the same network."

HopFacet.sol line 146:              "Cannot bridge to the same network."

LibDiamond:sol line 56:             "LibDiamond: Must be contract owner"
LibDiamond:sol line 84:             "LibDiamondCut: No selectors in facet to cut"
LibDiamond:sol line 86:             "LibDiamondCut: Add facet can't be address(0)"
LibDiamond:sol line 95:             "LibDiamondCut: Can't add function that already exists"
LibDiamond:sol line 102:            "LibDiamondCut: No selectors in facet to cut"
LibDiamond:sol line 104:            "LibDiamondCut: Add facet can't be address(0)"
LibDiamond:sol line 113:            "LibDiamondCut: Can't replace function with same function"
LibDiamond:sol line 121:            "LibDiamondCut: No selectors in facet to cut"
LibDiamond:sol line 124:            "LibDiamondCut: Remove facet address must be address(0)"
LibDiamond:sol line 133:            "LibDiamondCut: New facet has no code"
LibDiamond:sol line 154:            "LibDiamondCut: Can't remove function that doesn't exist"
LibDiamond:sol line 156:            "LibDiamondCut: Can't remove immutable function" 
LibDiamond:sol line 187:            "LibDiamondCut: _init is address(0) but_calldata is not empty"
LibDiamond:sol line 189:            "LibDiamondCut: _calldata is empty but _init is not address(0)" 
LibDiamond:sol line 191:            "LibDiamondCut: _init address has no code"
LibDiamond:sol line 200:            "LibDiamondCut: _init function reverted" 

Unused variable MAX_INT in LibSwap.sol

The constant variable MAX_INT is never used in LibSwap.sol and can hence be removed.

The relevant code:

LibSwap.sol line 8:              uint256 private constant MAX_INT = 2**256 - 1;

For loop gas optimization

It is cheaper to prefix increment (++i) a loop counter rather than postfix increment it (i++). Furthermore, there is no reason to use checked arithmetic on a uint/uint256 loop counter, since there is little to no risk of it overflowing. In newer versions of Solidity there is a built-in overflow check for increments, which is not necessary in this case and is hence a waste of gas. Instead it is possible to skip the overflow check by using unchecked arithmetic like unchecked{++i}

This means that a loop looking like this

for (uint i; i < array.length; i++) {
    // ...
}

Should be rewritten into this

for (uint i; i < array.length; ) {
    // ...

    unchecked { ++i; }
}

These optimizations will save gas for each iteration and hence the total amount of saved gas depends on the size of the element that is looped over.

The relevant code:

DexManagerFacet.sol line 33-39
DexManagerFacet.sol line 52-57
DexManagerFacet.sol line 65-76
DexManagerFacet.sol line 70-75

DiamondLoupeFacet.sol line 24-28

LibDiamond.sol line 67-78
LibDiamond.sol line 92-98
LibDiamond.sol line 110-117
LibDiamond.sol line 125-129

// The below use uint8 and hence only the prefix increment is relevant here

HopFacet.sol line 48-51

Swapper.sol line 14-21

Unused library import

The library LibDiamond is imported twice in AnySwapFacet.sol and the library is not used in the file and hence should be removed.

The relevant code:

AnySwapFacet.sol line 6
AnySwapFacet.sol line 9

Cache _postSwapBalance earlier

In AnySwapFacet.sol the _postSwapBalance is cached in line 103, however, the exact same operation is used in the line above the caching (line 101). Therefore, it is possible to save a BALANCE instruction by swapping the two lines such that:

_executeSwaps(_lifiData, _swapData);

require(address(this).balance - _fromBalance >= _anyswapData.amount, "ERR_INVALID_AMOUNT");

uint256 _postSwapBalance = address(this).balance - _fromBalance;

require(_postSwapBalance > 0, "ERR_INVALID_AMOUNT");

_anyswapData.amount = _postSwapBalance;

Becomes this:

_executeSwaps(_lifiData, _swapData);

uint256 _postSwapBalance = address(this).balance - _fromBalance;

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

require(_postSwapBalance > 0, "ERR_INVALID_AMOUNT");

_anyswapData.amount = _postSwapBalance;
H3xept commented 2 years ago

Re For uint use != 0 instead of > 0: -- Fixed in previous commit.

H3xept commented 2 years ago

Re Keep revert strings below 32 bytes

Fixed in lifinance/lifi-contracts@f35ed79a266a69b363d72332b7861d15d18b98cb

H3xept commented 2 years ago

Re Unused library import

Removed in previous commit.

Re ++i

We internally decided to avoid prefix increments for now.

Re Cache _postBalance earlier -- Fixed in lifinance/lifi-contracts@87a27cee2fbde337c4ab873971f37573d2240994

H3xept commented 2 years ago

Re prefix increments

We internally decided to avoid previx increments for now.