Gas Optimizations - Githubissues

code423n4 commented 2 years ago

Error messages longer than 32 bytes

Lines of code

Vulnerability details

Impact

Error strings longer than 32 bytes are more expensive. Also you can use custom errors for revert statements which is more gas-efficient

Proof of Concept

https://blog.polymath.network/solidity-tips-and-tricks-to-save-gas-and-reduce-bytecode-size-c44580b218e6#c17b https://blog.soliditylang.org/2021/04/21/custom-errors/

Tools Used

Manual analysis

Recommended Mitigation Steps

Limit the error strings to 32 bytes max. Use custom errors for revert statements.

Checks can be performed before calling diamondStorage()

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L85 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L103 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L122

Vulnerability details

Impact

Zero facetAddress check can be performed before "DiamondStorage storage ds = diamondStorage();" If that check fails, unnecessary execution would be avoided.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Call the zero address check before "DiamondStorage storage ds = diamondStorage();"

For loop index increments can be made unchecked. Also prefix increments are cheaper than postfix increments.

Lines of code

Vulnerability details

Impact

There is no risk of overflowing the index of the for loops. Therefore, they can be changed to unchecked to save gas. This would save more gas as iterations in the loop increases.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Change the index increments to unchecked and prefix such as: for (uint256 i; i < s.dexs.length; ) { if (s.dexs[i] == _dex) { _removeDex(i); return; } unchecked { ++i; } }

Some internal functions can be made private

Lines of code

Vulnerability details

Impact

Calling private functions are cheaper than calling internal functions. Therefore, it is better to declare functions private if they are not called from inherited contracts.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Change the visibility to private where possible.

It's better to use calldata for the read-only function arguments on external functions

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L41 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L42

Vulnerability details

Impact

When external functions' read-only arguments are used as memory, intermediate memory operations are performed which costs gas. You can directly read these arguments from calldata.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Change from memory data location to calldata.

No need to cache _functionSelectors[selectorIndex]

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L93 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L111 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L126

Vulnerability details

Impact

At addFunctions(), replaceFunctions(), removeFunctions(); _functionSelectors[selectorIndex] is cached to selector and selector is used. _functionSelectors[selectorIndex] can be directly used where necessary.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

For example addFunctions() can be changed as below and so as the other 2 functions: for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) { //bytes4 selector = _functionSelectors[selectorIndex]; address oldFacetAddress = ds.selectorToFacetAndPosition[_functionSelectors[selectorIndex]].facetAddress; require(oldFacetAddress == address(0), "LibDiamondCut: Can't add function that already exists"); addFunction(ds, _functionSelectors[selectorIndex], selectorPosition, _facetAddress); selectorPosition++; }

Unused constant definition in LibSwap.sol

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L8

Vulnerability details

Impact

The MAX_INT is defined, but it is not used anywhere.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove the definition

Operation can be made unchecked

Lines of code

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

Vulnerability details

Impact

Subtraction operation can be safely made unchecked, since underflow is not possible. Saves ~2K gas without optimization.

Proof of Concept

Tools Used

Remix

Recommended Mitigation Steps

Change the line as: unchecked { finalBalance = postSwapBalance - startingBalance; }

Define _postSwapBalance first and reuse it,

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L103

Vulnerability details

Impact

_postSwapBalance can be defined before the require statement and used within the require. That would save making one less subtraction operation. Saves ~2.4K gas without optimization.

Proof of Concept

Tools Used

Remix

Recommended Mitigation Steps

Change the code as: // Swap _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;

Reorder lines to check for the contract owner first

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L44 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L46 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L35

Vulnerability details

Impact

It is better to make the contract owner check before calling the getStorage(). In case the check fails, getStorage() would not be executed unnecessarily.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Change the code as: function initCbridge(address _cBridge, uint64 _chainId) external { LibDiamond.enforceIsContractOwner(); Storage storage s = getStorage(); s.cBridge = _cBridge; s.cBridgeChainId = _chainId; emit Inited(s.cBridge, s.cBridgeChainId); }