LI.FI contest - catchup's results

Lines of code

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

Vulnerability details

Impact

When batchRemoveDex() function is called with an array of dexes to be removed, the first dex in the _dexs array would be removed, but then the inner for loop would return from the function without removing the rest of the dexes. Hence, the rest of the dexes would still be whitelisted and operational.

Proof of Concept

1- Add 2 dexes using batchAddDex(): 2- Call approvedDexs(). It will return the 2 added adresses: address[]: 0xDEF171Fe48CF0115B1d80b88dc8eAB59176FEe57,0x216B4B4Ba9F3e719726886d34a177484278Bfcae 3- Call batchRemoveDex() passing this address array as argument 4- Call approvedDexs() again. It will return 1 adress whereas there should be none: address[]: 0x216B4B4Ba9F3e719726886d34a177484278Bfcae

Tools Used

Remix

Recommended Mitigation Steps

Replace the return statement on line 55 with break, so that only the inner loop would break, but the outer loop will continue execution.

Lines of code

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

Vulnerability details

Impact

approveERC20() is using unlimited approval. Although unlimited approval is used by various DeFi platforms to minimize transaction fees and improve user experience, it introduces security risks as well.

Proof of Concept

https://kalis.me/unlimited-erc20-allowances/ https://blocksecteam.medium.com/unlimited-approval-in-erc20-convenience-or-security-1c8dce421ed7 https://medium.com/@rodrigoherrerai/understanding-the-problem-of-erc20-unlimited-approval-from-first-principles-d2eaf6b4ea0e

Tools Used

Manual analysis

Recommended Mitigation Steps

Consider changing approveERC20() to approve only the required amount, or rather using increaseERC20Allowance() and decreaseERC20Allowance().

Lines of code

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

Vulnerability details

Impact

removeDex(), batchRemoveDex() should have a timelock. Such functions that make critical changes should have a timelock, so that users can see the upcoming changes and have time to react to these changes.

Proof of Concept

See below for similar findings: https://consensys.net/diligence/audits/2020/12/1inch-liquidity-protocol/#unpredictable-behavior-for-users-due-to-admin-front-running-or-general-bad-timing https://github.com/code-423n4/2021-11-overlay-findings/issues/120

Tools Used

Manual analysis

Recommended Mitigation Steps

Add timelock for critical changes which will effect user experience and decisions.

C4 finding submitted: (risk = 1 (Low Risk)) Missing non-zero address check when setting the contract owner

Lines of code

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

Vulnerability details

Impact

setContractOwner(address _newOwner) does not check _newOwner against zero address. Zero address check should be performed especially when changing critical addresses.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Add zero address check

C4 finding submitted: (risk = 1 (Low Risk)) Setting new contract owner should be a 2-step process

Lines of code

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

Vulnerability details

Impact

setContractOwner(address _newOwner) sets the new contract owner directly. If a wrong new owner address is entered, the ownership will be lost and can not be fixed.

Proof of Concept

https://github.com/code-423n4/2022-01-livepeer-findings/issues/143

Tools Used

Manual analysis

Recommended Mitigation Steps

Use a 2-step process where the current owner sets the new owner candidate, and the candidate claims the ownership.

C4 finding submitted: (risk = 1 (Low Risk)) Missing non-zero address check when setting the cBridge router contract

Lines of code

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

Vulnerability details

Impact

initCbridge() does not check _cBridge against zero address.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Add zero address check

C4 finding submitted: (risk = 0 (Non-critical)) Floating pragma

Lines of code

All the contracts in scope, some of which https://github.com/code-423n4/2022-03-lifinance/blob/main/src/LiFiDiamond.sol#L2 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L2 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L2

Vulnerability details

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.

Impact

Proof of Concept

https://swcregistry.io/docs/SWC-103

Tools Used

Manual analysis

Recommended Mitigation Steps

Lock the pragma

C4 finding submitted: (risk = 0 (Non-critical)) Unused import

Lines of code

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

Vulnerability details

Impact

LibDiamond.sol is imported (twice) in AnyswapAsset.sol, but it is not used.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Those lines can be deleted.

Error messages longer than 32 bytes

Lines of code

There are a lot of lines. Some of them written below. https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L56 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L76 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L84 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L86 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L95 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L102 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L104 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L113 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L121

Vulnerability details

Impact

Error strings longer than 32 bytes are more expensive. Also you can use custom errors for revert statements which is more gas-efficient

Proof of Concept

https://blog.polymath.network/solidity-tips-and-tricks-to-save-gas-and-reduce-bytecode-size-c44580b218e6#c17b https://blog.soliditylang.org/2021/04/21/custom-errors/

Tools Used

Manual analysis

Recommended Mitigation Steps

Limit the error strings to 32 bytes max. Use custom errors for revert statements.

Checks can be performed before calling diamondStorage()

Lines of code

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

Vulnerability details

Impact

Zero facetAddress check can be performed before "DiamondStorage storage ds = diamondStorage();" If that check fails, unnecessary execution would be avoided.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Call the zero address check before "DiamondStorage storage ds = diamondStorage();"

For loop index increments can be made unchecked. Also prefix increments are cheaper than postfix increments.

Lines of code

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 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/Swapper.sol#L14

Vulnerability details

Impact

There is no risk of overflowing the index of the for loops. Therefore, they can be changed to unchecked to save gas. This would save more gas as iterations in the loop increases.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Change the index increments to unchecked and prefix such as: for (uint256 i; i < s.dexs.length; ) { if (s.dexs[i] == _dex) { _removeDex(i); return; } unchecked { ++i; } }

Some internal functions can be made private

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L131 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L142 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L176 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L184 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L134 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L180 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L185 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L175 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L190

Vulnerability details

Impact

Calling private functions are cheaper than calling internal functions. Therefore, it is better to declare functions private if they are not called from inherited contracts.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Change the visibility to private where possible.

It's better to use calldata for the read-only function arguments on external functions

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L41 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L42

Vulnerability details

Impact

When external functions' read-only arguments are used as memory, intermediate memory operations are performed which costs gas. You can directly read these arguments from calldata.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Change from memory data location to calldata.

No need to cache _functionSelectors[selectorIndex]

Lines of code

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

Vulnerability details

Impact

At addFunctions(), replaceFunctions(), removeFunctions(); _functionSelectors[selectorIndex] is cached to selector and selector is used. _functionSelectors[selectorIndex] can be directly used where necessary.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

For example addFunctions() can be changed as below and so as the other 2 functions: for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) { //bytes4 selector = _functionSelectors[selectorIndex]; address oldFacetAddress = ds.selectorToFacetAndPosition[_functionSelectors[selectorIndex]].facetAddress; require(oldFacetAddress == address(0), "LibDiamondCut: Can't add function that already exists"); addFunction(ds, _functionSelectors[selectorIndex], selectorPosition, _facetAddress); selectorPosition++; }

Unused constant definition in LibSwap.sol

Lines of code

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

Vulnerability details

Impact

The MAX_INT is defined, but it is not used anywhere.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove the definition

Operation can be made unchecked

Lines of code

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

Vulnerability details

Impact

Subtraction operation can be safely made unchecked, since underflow is not possible. Saves ~2K gas without optimization.

Proof of Concept

Tools Used

Remix

Recommended Mitigation Steps

Change the line as: unchecked { finalBalance = postSwapBalance - startingBalance; }

Define _postSwapBalance first and reuse it,

Lines of code

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

Vulnerability details

Impact

_postSwapBalance can be defined before the require statement and used within the require. That would save making one less subtraction operation. Saves ~2.4K gas without optimization.

Proof of Concept

Tools Used

Remix

Recommended Mitigation Steps

Change the code as: // Swap _executeSwaps(_lifiData, _swapData);

        uint256 _postSwapBalance = address(this).balance - _fromBalance;

        require(_postSwapBalance >= _anyswapData.amount, "ERR_INVALID_AMOUNT");

        require(_postSwapBalance > 0, "ERR_INVALID_AMOUNT");

        _anyswapData.amount = _postSwapBalance;

Reorder lines to check for the contract owner first

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L44 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L46 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L35

Vulnerability details

Impact

It is better to make the contract owner check before calling the getStorage(). In case the check fails, getStorage() would not be executed unnecessarily.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

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