LI.FI contest - Jujic's results

Lines of code

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

Vulnerability details

Impact

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

Proof of Concept

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

Tools Used

Remix

Recommended Mitigation Steps

Add a timelock to setter functions of key/critical variables.

Lines of code

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

Vulnerability details

Impact

The use of transfer in WithdrawFacet.sol to send ether is now considered bad practice as gas costs can change which would break the code.

Proof of Concept

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

if (_assetAddress == NATIVE_ASSET) {
            address self = address(this); // workaround for a possible solidity bug
            assert(_amount <= self.balance);
            payable(sendTo).transfer(_amount);

Tools Used

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

https://chainsecurity.com/istanbul-hardfork-eips-increasing-gas-costs-and-more/

Recommended Mitigation Steps

Recommend using call instead, and make sure to check for reentrancy.

Lines of code

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

Vulnerability details

Impact

In the DexManagerFacet contract the owner register the address of a DEX contract to be approved for swapping but it is possible erroneously add zero address on s.dexWhitelist[_dex] whitelist because there is no check for zero address. In this case, the user who swap tokens on this zero Dex can lose their funds.

The same situation in the batchAddDex() function.

Proof of Concept

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

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

Tools Used

Remix

Recommended Mitigation Steps

Add the following line to the beginning of addDex() function: require(_dex != address(0));

1. Prefix increments are cheaper than postfix increments

Impact

There is no risk of overflow caused by increamenting the iteration index in for loops (the i++ in for for (uint8 i; i < _swapData.length; i++) {. Increments perform overflow checks that are not necessary in this case.

Proof of Concept

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

Recommended Mitigation Steps

Surround the increment expressions with an unchecked { ... } block to avoid the default overflow checks.

2. uint8 is less efficient than uint256 in loop iterations

Impact

This loop use uint8 for an index parameter (i). It does not give any efficiency, actually, it is the opposite as EVM operates on default of 256-bit values so uint8 is more expensive in this case as it needs a conversion. It only gives improvements in cases where you can pack variables together, e.g. structs.

Proof of Concept

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

for (uint8 i; i < _swapData.length; i++) {.

Recommended Mitigation Steps

Replace uint8 with uint256 in loop iterations.

3. > 0 can be replaced with != 0 for gas optimisation

Impact

!= 0 is a cheaper operation compared to > 0, when dealing with uint.

Proof of Concept

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/CBridgeFacet.sol#L116 There are several other places throughout the codebase where the same optimization can be used.

4. Use Custom Errors to save Gas

Impact

Custom errors are cheaper than revert strings.

Proof of Concept

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/: Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them. Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Recommended Mitigation Steps

Replace revert strings with custom errors.

5. Long Revert Strings

Impact

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met.

Proof of Concept

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

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

There are several other places throughout the codebase where the same optimization can be used.

https://planetcalc.com/9029/

Recommended Mitigation Steps

Shorten the revert strings to fit in 32 bytes

6. Adding unchecked directive can save gas (6 findings)

Impact

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

Proof of Concept

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

 if (postSwapBalance > startingBalance) {
            finalBalance = postSwapBalance - startingBalance;

Recommended Mitigation Steps

Consider using 'unchecked' where it is safe to do so.

6. && operator can use more gas

Impact

More expensive gas usage

Proof of Concept

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

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

Recommended Mitigation Steps

Instead of using operator && on single require check using double require check can save more gas

7. Avoid use of state variables in event emissions to save gas

Impact

Where possible, use equivalent function parameters or local variables in event emits instead of state variables to prevent expensive SLOADs. Post-Berlin, SLOADs on state variables accessed first-time in a transaction increased from 800 gas to 2100, which is a 2.5x increase.

Proof of Concept

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/CBridgeFacet.sol#L47 The Inited event i uses state variable s.cBridge and s.cBridgeChainId instead of using the equivalent function parameters _cBridge and _chainIdwhich were just used to set these state variables.

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

Recommended Mitigation Steps

Use equivalent function parameters or local variables in event emits instead of state variables.

8. Free gas savings for using solidity 0.8.10+

Impact

Gas savings

Proof of Concept

Solidity 0.8.10 has a useful change which reduced gas costs of external calls which expect a return value: https://blog.soliditylang.org/2021/11/09/solidity-0.8.10-release-announcement/

Code Generator: Skip existence check for external contract if return data is expected. In this case, the ABI decoder will revert if the contract does not exist

LIFI protocol is using 0.8.6: Updating to the newer version of solc will allow contracts to take advantage of these lower costs for external calls.

Recommended Mitigation Steps

Update to 0.8.10+

9. Caching variables (3 findings)

Impact

Some of the variable can be cached to slightly reduce gas usage.

Proof of Concept

LibAsset can be cached. https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/NXTPFacet.sol#L46-L76

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

IERC20(_assetAddress) can be cached. https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/WithdrawFacet.sol#L33-L35

Recommended Mitigation Steps

Consider caching those variable for read and make sure write back to storage

10. Gas optimization on 2**256 - 1

Vulnerability details

Impact

when using 2**256 - 1 it takes more gas, than using 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

Recommended Mitigation Steps

Use type(uint256).max