Gas Optimizations - Githubissues

code423n4 commented 2 years ago

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

    GenericSwapFacet.sol, swapTokensGeneric
    WithdrawFacet.sol, withdraw

Title: Unnecessary equals boolean Severity: GAS

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

    DexManagerFacet.sol, 34: if (s.dexWhitelist[_dexs[i]] == true) {
    DexManagerFacet.sol, 66: if (s.dexWhitelist[_dexs[i]] == false) {
    DexManagerFacet.sol, 47: if (s.dexWhitelist[_dex] == false) {
    DexManagerFacet.sol, 20: if (s.dexWhitelist[_dex] == true) {
    Swapper.sol, 16: ls.dexWhitelist[_swapData[i].approveTo] == true && ls.dexWhitelist[_swapData[i].callTo] == true,

Title: Consider inline the following functions to save gas Severity: 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.)

    LibAsset.sol, isNativeAsset, { return assetId == NATIVE_ASSETID; }
    LibAsset.sol, getOwnBalance, { return isNativeAsset(assetId) ? address(this).balance : IERC20(assetId).balanceOf(address(this)); }

Title: Unnecessary functions Severity: GAS

The following functions are not used at all. Therefore you can remove them to save deployment gas and improve code clearness.

    LibAsset.sol, increaseERC20Allowance
    LibAsset.sol, transferAsset
    Swapper.sol, _executeSwaps
    LibUtil.sol, getRevertMsg
    LibAsset.sol, transferFromERC20
    LibAsset.sol, getOwnBalance
    LibAsset.sol, decreaseERC20Allowance
    LibSwap.sol, swap
    LibAsset.sol, approveERC20

Title: Unnecessary array boundaries check when loading an array element twice Severity: GAS

There are places in the code (especially in for-each loops) that loads the same array element more than once. 
In such cases, only one array boundaries check should take place, and the rest are unnecessary.
Therefore, this array element should be cached in a local variable and then be loaded
again using this local variable, skipping the redundant second array boundaries check: 

    Swapper.sol._executeSwaps - double load of _swapData[i]

Title: Prefix increments are cheaper than postfix increments Severity: GAS

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:

    change to prefix increment and unchecked: DexManagerFacet.sol, i, 33
    change to prefix increment and unchecked: DexManagerFacet.sol, i, 52
    change to prefix increment and unchecked: DiamondLoupeFacet.sol, i, 24
    change to prefix increment and unchecked: DexManagerFacet.sol, i, 65
    change to prefix increment and unchecked: Swapper.sol, i, 14

Title: Inline one time use functions Severity: GAS

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

    LibAsset.sol, transferERC20
    LibAsset.sol, transferNativeAsset

Title: Unnecessary cast Severity: Gas

    IERC20 LibAsset.sol.approveERC20 - unnecessary casting IERC20(assetId)

Title: Unused state variables Severity: GAS

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.

    LibSwap.sol, MAX_INT

Title: Caching array length can save gas Severity: 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++) { ... }

    DexManagerFacet.sol, dexs.s, 52
    DexManagerFacet.sol, _dexs, 33
    Swapper.sol, _swapData, 14
    DexManagerFacet.sol, _dexs, 65

Title: Gas Optimization On The 2^256-1 Severity: GAS

Some projects (e.g. Uniswap - https://github.com/Uniswap/interface/blob/main/src/hooks/useApproveCallback.ts#L88) set the default value of the user's allowance to 2^256 - 1. Since the value 2^256 - 1 can also be represented in hex as 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff. From Ethereum's yellow paper we know that zeros are cheaper than non-zero values in the hex representation. Considering this fact, an alternative choice could be now 0x8000000000000000000000000000000000000000000000000000000000000000 or 2^255 to represent "infinity". If you do the calculations with Remix, you will see that the former costs 47'872 gas, while the latter costs 45'888 gas. If you accept that infinity can also be represented via 2^255 (instead of 2^256-1), which almost all projects can - you can already save about 4% gas leveraging this optimisation trick on those calculations.

    LibAsset.sol (L#15): uint256 private constant MAX_INT = 2**256 - 1; )
    LibSwap.sol (L#8): uint256 private constant MAX_INT = 2**256 - 1; )

Title: Internal functions to private Severity: GAS

The following functions could be set private to save gas and improve code quality:

    LibAsset.sol, increaseERC20Allowance
    LibAsset.sol, transferAsset
    LibAsset.sol, transferERC20
    Swapper.sol, _executeSwaps
    LibUtil.sol, getRevertMsg
    LibAsset.sol, getOwnBalance
    LibAsset.sol, isNativeAsset
    LibAsset.sol, transferFromERC20
    LibAsset.sol, decreaseERC20Allowance
    LibSwap.sol, swap
    LibAsset.sol, approveERC20
    LibAsset.sol, transferNativeAsset

Title: Unnecessary payable Severity: GAS

The following functions are payable but msg.value isn't used - therefore the function payable state modifier isn't necessary. Payable functions are more gas expensive than others, and it's danger the users if they send ETH by mistake.

    GenericSwapFacet.sol, swapTokensGeneric is payable but doesn't use msg.value

Title: uint8 index Severity: GAS

Due to how the EVM natively works on 256 numbers, using a 8 bit number here introduces additional costs as the EVM has to properly enforce the limits of this smaller type. See the warning at this link: https://docs.soliditylang.org/en/v0.8.0/internals/layout_in_storage.html#layout-of-state-variables-in-storage We recommend to use uint256 for the index in every for loop instead using uint8:

    Swapper.sol, uint8 i, 14