LI.FI contest - saian's results

QA Report

Low severity findings

DexManagerFacet:batchRemoveDex removes only first matched dex

In batchRemoveDex second for-loop the first matched dex is removed using _removeDexand return statement returns from function, the other dexes in the array are not removed from whitelist

Proof of concept

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

Mitigation

return can be replaced with break statement

Non-critical findings

No anyswap.token == 0 check

In AnyswapFacet:startBridgeTokensViaAnyswap function , if _anyswapData.token equals to 0 and _anyswap.amount equals to 0, functions does not fail, if conditions are not executed, but event LifiTransferStarted is emitted.

Proof of concept

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

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

library imported twice

Impact

library LibDiamond is imported twice

Proof of concept

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

Mitigation

second import statement can be removed

Assert can be replaced with require

In WithdrawFacet:withdraw assert statements are used to validate user inputs. Assert statements should be used to check internal errors and invariants, and require should be used for validating user inputs

https://docs.soliditylang.org/en/v0.8.4/control-structures.html#panic-via-assert-and-error-via-require

Proof of concept

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

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

Mitigation

Assert statement can be replaced with require

Wrong order of arguments in event

In LogWithdraw event declaration assetaddress is the first argument, but in the withdraw function event emmission a variable assetAddress is sent in the second position and address sendTo is sent in first position

Proof of concept

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

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

Mitigation

Order of arguments can be changed, comments can be added

constant declaration can be changed

In LibAsset constant MAX_INT is defined with an expression, it can be replaced with type(uint256).max

Proof of concept

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

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

Mitigation

expression can be replaced with type(uint256).max

Lack of zero address check

In LifiDiamond:constructor and OwnershipFacet:transferOwnership sddress validation checks can be added to prevent setting wrong owner address

Proof of concept

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

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

Unlimited approval

In LibAsset:approveERC20 spender is approved MAX_INT instead of amount tokens

Proof of concept

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

Order of layout

In LibDiamond events can be placed above functions

https://docs.soliditylang.org/en/v0.8.4/style-guide.html#order-of-layout

Proof of concept

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

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

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

https://github.com/ethereum/solidity/issues/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);
}