LI.FI contest - robee's results

Title: Assert instead require to validate user inputs Severity: Low Risk

    From solidity docs: Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix.
    With assert the user pays the gas and with require it doesn't. The ETH network gas isn't cheap and users can see it as a scam.
    
    WithdrawFacet.sol : reachable assert in line 33
    WithdrawFacet.sol : reachable assert in line 29

Title: Not verified owner Severity: Low Risk

    owner param should be validated to make sure the owner address is not address(0).
    Otherwise if not given the right input all only owner accessible functions will be unaccessible.
    
    
    OwnershipFacet.sol.transferOwnership _newOwner

Title: Never used parameters Severity: Low Risk

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

    LibAsset.sol: function transferNativeAsset parameter recipient isn't used. (transferNativeAsset is internal)
    LibAsset.sol: function transferNativeAsset parameter amount isn't used. (transferNativeAsset is internal)

Title: safeApprove of openZeppelin is deprecated Severity: Low Risk

    You use safeApprove of openZeppelin although it's deprecated.
    (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38)
    You should change it to increase/decrease Allowance as OpenZeppilin says
    This appears in the following locations in the code base
    

Deprecated safeApprove in LibAsset.sol line 66: if (allowance > 0) SafeERC20.safeApprove(IERC20(assetId), spender, 0);

Deprecated safeApprove in LibAsset.sol line 67: SafeERC20.safeApprove(IERC20(assetId), spender, MAX_INT);

Deprecated safeApprove in LibSwap.sol line 37: LibAsset.approveERC20(IERC20(fromAssetId), _swapData.approveTo, fromAmount);

Deprecated safeApprove in Swapper.sol line 15: ls.dexWhitelist[_swapData[i].approveTo] == true && ls.dexWhitelist[_swapData[i].callTo] == true,

Title: Two Steps Verification before Transferring Ownership Severity: Low Risk

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked. It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership. A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

    IERC173.sol
    OwnershipFacet.sol

Title: Not verified input Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    
    LibAsset.sol.getOwnBalance assetId
    LibAsset.sol.approveERC20 spender
    LibAsset.sol.transferFromERC20 assetId
    LibAsset.sol.increaseERC20Allowance spender
    LibAsset.sol.transferERC20 assetId
    DexManagerFacet.sol.removeDex _dex

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