LI.FI contest - kenta's results

2022-03-Li.finance

1 Lock pragmas to specific compiler version. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

https://github.com/code-423n4/2022-03-Li.finance/blob/main/src/LiFiDiamond.sol#L2 Other files too.

pragma solidity 0.8.7;

2 delete deprecated import statement.

https://github.com/code-423n4/2022-03-Li.finance/blob/main/src/Facets/AnyswapFacet.sol#L6 https://github.com/code-423n4/2022-03-Li.finance/blob/main/src/Facets/AnyswapFacet.sol#L9

Delete one of them.

3 missing input validations in initHop.

The length of_tokens and _bridgeConfigs must be checked and they have the same length to avoid errors.

https://github.com/code-423n4/2022-03-Li.finance/blob/main/src/Facets/HopFacet.sol#L40-L52

require(_tokens.length == _bridgeConfigs.length, “ERROR MESSAGE”);

4 use call instead of transfer.

According to consensys, .transfer() and .send() forward exactly 2,300 gas to the recipient. The goal of this hardcoded gas stipend was to prevent reentrancy vulnerabilities, but this only makes sense under the assumption that gas costs are constant. Recently EIP 1884 was included in the Istanbul hard fork. One of the changes included in EIP 1884 is an increase to the gas cost of the SLOAD operation, causing a contract's fallback function to cost more than 2300 gas. It's recommended to stop using .transfer() and .send() and instead use .call(). https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/WithdrawFacet.sol#L31

(bool success, ) = sendTo.call{value: _amount}(“”); require(success, “Transfer failed”);

5 import IERC20 from openzeppelin and delete unused import statement.

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

import { LibAsset } from "../Libraries/LibAsset.sol"; import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";

Delete IERC20 in the following line.

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L4 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L5 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L6 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L6 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/WithdrawFacet.sol#L4

6 call appStorage same in different facets. In the following lines, you define the appStorage and each variable name has a different name. Of course, it is ok, but how about setting these variable same names to avoid confusion and make readability better.

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L13 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/Swapper.sol#L10

For example, we call these variables both

LibStorage internal s;

2022-03-Li.finance gas optimization

1 use unchecked and prefix for loop

https://github.com/code-423n4/2022-03-Li.finance/blob/main/src/Facets/Swapper.sol#L14 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L33 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L52 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L65 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L70 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DiamondLoupeFacet.sol#L24 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L48 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L67 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L92 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L110 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L125

For example,

for (uint8 i; i < _swapData.length;) { // some executions

unchecked {
    ++i;
}

}

2 == true is unnecessary in require.

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

require( ls.dexWhitelist[_swapData[i].approveTo] && ls.dexWhitelist[_swapData[i].callTo], "Contract call not allowed!" )

3 use unchecked. Underflow will not happen in the following part because of an increase in the balance.

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

unchecked { toAmount = LibAsset.getOwnBalance(_swapData.receivingAssetId) - toAmount; }

4 use unchecked. Underflow is already checked in if statement before the following line. Since Solidity 0.8.0, all arithmetic operations revert on over- and underflow by default, thus making the use of these libraries unnecessary. To obtain the previous behaviour, an unchecked block can be used and this is cheaper from the view of gas.

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

Unchecked { finalBalance = postSwapBalance - startingBalance; }

5 use memory instead of storage and you don’t need to call getStorage() to get s.cBridge, so you don’t use _bridge()any more. You can delete this function and save deployment costs.

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

Storage memory s = getStorage();

    // Do CBridge stuff
    require(s.cBridgeChainId != _cBridgeData.dstChainId, "Cannot bridge to the same network.");

    if (LibAsset.isNativeAsset(_cBridgeData.token)) {
        ICBridge(s.cBridge).sendNative(
            _cBridgeData.receiver,
            _cBridgeData.amount,
            _cBridgeData.dstChainId,
            _cBridgeData.nonce,
            _cBridgeData.maxSlippage
        );
    } else {
        // Give CBridge approval to bridge tokens
        LibAsset.approveERC20(IERC20(_cBridgeData.token), s.cBridge, _cBridgeData.amount);
        // solhint-disable check-send-result
        ICBridge(s.cBridge).send(
            _cBridgeData.receiver,
            _cBridgeData.token,
            _cBridgeData.amount,
            _cBridgeData.dstChainId,
            _cBridgeData.nonce,
            _cBridgeData.maxSlippage
        );
    }

and delete _bridge()in CBridgeFacet.sol.

6 Don’t use == (equality) to save gas. The following lines use == (equality) for boolean. You don’t need this syntax to check boolean.

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L20 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L34

For example,

if (s.dexWhitelist[_dex]){}

7 Use ! instead of == false. ! must be cheaper than == false.

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L47 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L66

if (!s.dexWhitelist[_dex)