LI.FI contest - VAD37's results

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibAsset.sol#L59-L70 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibSwap.sol#L33-L35 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/GenericSwapFacet.sol#L28-L30

Vulnerability details

• Using LibAsset.sol approval exploit which LibSwap.sol approve all whitelist address access to transfer all token hold by LiFi contract. reference 1 reference 2

• most facet function does not check input LibSwap.SwapData[] before _executeSwaps(_lifiData, _swapData); pass on to LibSwap.sol. This allows anyone executes anything with whitelist DEX using Li.Fi as caller

Impact

Any malicious user can call LibSwap.sol execute to swap all token balance hold by Li.Fi contract to another token. Which can be used to transfer all token hold by Li.Fi contract to another address.

Proof of Concept

This only show the most simple, direct way to withdraw ERC20 token from contract. Consider Li.Fi already implement Whitelist function and token whitelist.

• Attacker call swapTokensGeneric() function in GenericSwapFacet.sol
• input _lifiData.receivingAssetId is USDC. This cache uint256 receivingAssetIdBalance equal to current USDC token balance.
• LibSwap.SwapData[] _swapData is custom compose to swap all token hold by Li.Fi contract to USDC through Uniswap.
• callTo and approveTo is Uniswap router address (whitelisted in dex.ts)
• sendingAssetId, receivingAssetId and fromAmount in SwapData can be any value
• bytes callData in LibSwap.SwapData is simply swapExactTokensForTokens with Uniswap
• Uniswap router have max approval token transfer, so it can call transferFrom to get all Li.Fi token directly.
• Li.Fi transfer all token to Uniswap router, then Uniswap Swap to USDC transfer back to Li.Fi contract.
• Do this for all token hold Li.Fi in array swap
• After postSwapBalance check. Li.Fi contract transfer all new swapped USDC to attacker. (attacker only have to spend 0.1 of any random ERC20 token)

Do this again with USDC and USDT to withdraw all USDC leftover.

Note: there are many more ways to exploit this using the same generic LibSwap.swap function. As long as Li.Fi contract hold ERC-20 token, there is no safe or fair way to prevent attacker make unfair trade with token.

Tools Used

Manual

Recommended Mitigation Steps

The best fix from my experience is to remove LibSwap.sol and restart from scratch. Try different approach instead of generic arbitrary call to mitigate this. Spending time on securing LibSwap.sol is simply not worth it.

From LibSwap documentation here, A library used to perform multiple swaps given an array of DEX addresses and calldata. This makes it very flexible but also dangerous in that it allows arbitrary calls to any contract

This is a wrong approach with diamond facet. Diamond facets are already flexible, no need for arbitrary call. Diamond allow adhoc implementation for each chain, different contracts which give flexibility to implement new features or new DEX. Sharing all swap logic into one single function and reuse it everywhere is not a good idea.

Current implementation, Swapper.sol raw call in loop, is no different from a multicall function to different DEX to swap and then cross bridge. Might as well use a multicall into facets function instead of facet use same lib function. It is much simpler structure wise.

For flashloan swap, as long as Li.Fi not check correct amount of token being swapped, attacker can use other people token like token hold by LiFi contract.

Lines of code

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/LibSwap.sol#L42 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/AnyswapFacet.sol#L85 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/CBridgeFacet.sol#L98 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/GenericSwapFacet.sol#L23 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/NXTPFacet.sol#L91

Vulnerability details

Swapper.sol function _executeSwaps() call in a loop that use the same msg.value sent by user. This allows anyone to call _executeSwaps() which can control Li.Fi contract ETH balance on raw call through function LibSwap.swap().

Note: Lines of code above include all facets swap function check wrong input token before swap. Which anyone can freely call whatever whitelist function on whitelist address without spend their token.

Impact

Attacker can withdraw main token (ETH) from LiFi contract. Dex Whitelist and Function whitelist cannot prevent this

Other User funds are not affected.

Proof of Concept

Attacker can exploit a function that does not check main token balance after _executeSwaps like in AnySwapFacet.sol. These lines of code here only check input balance of AnyswapData memory _anyswapData after execute and not balance of token inside LibSwap.SwapData[] calldata _swapData. (same go for GenericSwapFacet.sol with swapTokensGeneric() and every other facets)

Attacker can craft special msg to DODO RouteProxy (whitelist in dex.ts) and swap Li.Fi ETH to other token and transfer it to another address.

• Lifi contract have 1 ETH balance.
• Attacker call cross chain bridge swapAndStartBridgeTokensViaAnyswap with 1 ETH and another random token.
• Attacker craft Swap data with msg.value with 1 ETH and 2 swap with DODO router function.
• Swapper.sol call in array loop
• LibSwap.sol only take ERC20 from sender but not check or reduce amount from msg.value
• router swap 1 ETH send by sender
• router swap 1 ETH use LiFi contract balance
• Lifi contract have 0 ETH balance.
• LiFi start call _startBridge function to end bridge token with no relation to Dodo swap at all.
• Sender get 2 ETH worth of token.

The simpler way is to just swap 1 ETH to USDC with swapTokensGeneric() will just send back 2 ETH worth of USDC with same exploit above.

Note: This concept use other attack approach from different facets instead of attack through Uniswap like other report. Due to some other bugs exist in LibSwap.sol and Swapper.sol that I already found. It is possible swap ETH directly for some fake token.

Tools Used

Manual

Recommended Mitigation Steps

• track msg.value in Swapper.sol loop and check balance after each swap.
• Stop using the same LibSwap.SwapData[] as one data fit all for facet input.
• It would be the best to not use array in Swapper.sol and replace it with multicall function.

Migrate this might require change all logic in facets.

I would highly recommend create adhoc data function and execution for each facet instead of 1 Swapper.sol for all. This help reduces complexity and help reduce development time in long term. Also, 1 broken library does not require refactoring all facets code. Each facet should be modular and independent of each other.

Each facet should have its own unique SwapData or inputData.

LibSwap.SwapData[] should be a subset data of this SwapData and not a direct function input.

If possible, create new SwapData inside function and not input send by user. Input should be used to create data to pass on LibSwap.sol.

Lines of code

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

Vulnerability details

LibAsset.sol approve max for spender instead of replace or add approval amount. This is unintended behavior as swap suppose to use only amount user send to contract and not all token currently hold.

The docs inside LibAsset.sol for approveERC20 is confusing. It says Amount to approve for spending but it actually gives max allowance for spender. This might be error on Connext part since their implementation is different.

Technically, Li.Fi gives all whitelisted address inside config Dex.ts access to transfer all token balance currently hold by Li.Fi.

Note: suppose to be low severity, but there are multiple exploits using this to withdraw all tokens hold by LiFi contract. Also, if attacker have special angle attack in the future, attacker can force LibSwap.sol call router like Uniswap to call transferFrom directly with full approval by Li.Fi to withdraw all tokens.

Impact

Anyone can tell Li.Fi what to do with all token currently hold in contract without owner access.

Ex: attacker can force Li.Fi trade its own token for another token. Because LiFi give Uniswap router approval to spend any token.

Proof of Concept

Several angles of attack can happen because attacker can freely craft swapData to LibSwap.sol swap function without restriction

• Some whitelist DEX like DODO allow transfer token directly to another address after swap.
• Without token whitelist, it is simple to swap valuable token with fake token liquidity and rugpull that liquidity pool.
• Even with limited function whitelist and token whitelist, attacker can still force bad trade using flashloan price manipulation. This allows the contract to spend more token to swap than it should. This render safeTransferERC20 from sender before swap useless, because attacker can swap using fund hold by LiFi contract too.

Tools Used

Manual

Recommended Mitigation Steps

• replace approveERC20 here with increaseERC20Allowance since most ERC20 token support this.
• Due to how liquidity and flashloan work, it is unavoidable of having attacker make a manipulated trade with DEX like Uniswap. The best way is to make sure Li.Fi do not spend unexpected token balance during swap.

QA

1. QA
   1. Coin whitelist for planned improvement
      1. Impact
      2. Proof of Concept
      3. Recommended Mitigation Steps
   2. ExecuteSwap and PostSwapBalance check wrong input
      1. Impact
      2. Recommended Mitigation Steps
   3. Project does not follow clean code style and lack of organization

Coin whitelist for planned improvement

Low Severity

Planned improvement with function signature whitelist is not enough to prevent malicious call through LibSwap.sol and Swapper.sol.

• Attacker can create custom token and force Li.Fi contract to call transfer or approve to this custom contract token. Code

• Some coin take fee on safeTransferFrom raise some expected behavior. Relevance issue I read, https://github.com/code-423n4/rulebook/issues/3.

Impact

Attacker can use LiFi contract as call function outside of whitelist function. But without caller msg.sender as LiFi, the fund is safe.

Yet it is possible to create reentrancy attack through this contract token transfer or approval before it call to LibSwap.swap().

Proof of Concept

• Attacker create custom ERC20 token and liquidity pool on Uniswap with token like ETH.
• Attacker send fake ERC20 token to Li.Fi contract.
• Attacker call swapTokensGeneric to swap fake ERC20 token through liquid pool using UniswapV2Router02 (which is whitelisted in Dex.ts config)
• LiFi before call to Uniswap, it call approve function on malicious contract. At this point attacker can call anything they want.

Without msg.sender as Li.Fi, there is not much to do here except reentrancy back to Li.Fi.

Recommended Mitigation Steps

Add token whitelist as well along with function whitelist.

It would be safe to consider reentrancy guard at the cost of more 40000 gas for each facet.

ExecuteSwap and PostSwapBalance check wrong input

Rating: Low Severity

All Facets are implemented follow these swap token pattern.

function doSomething(
  LiFiData memory _lifiData, // for event
  LibSwap.SwapData[] calldata _swapData, // the swap data
  FacetData memory inputSpecialData
){
  tokenFromInput = inputSpecialData.token;
  // balance token before swap
  uint256 _fromTokenBalance = LibAsset.getOwnBalance(tokenFromInput);
  // Swap.
  _executeSwaps(_lifiData, _swapData);
  // Check balance to see if we have new token that was swapped and return it to user.
  uint256 _postSwapBalance = LibAsset.getOwnBalance(tokenFromInput) - _fromTokenBalance;
  require(_postSwapBalance > 0, "ERR_INVALID_AMOUNT");
  // give token back to user or bridge it over another chain
  // ....
}

The expected behavior is user use UI and the input data would be the same as FacetData made for each facet. But what happen is, attacker can use this to tell swap token unrelated to current facet. Which the same as call anything they want with whitelist address

Impact

This allow attack angle if _executeSwaps does not work correctly (it didn't check swap and balance correctly).

Recommended Mitigation Steps

I assume the expected behavior is user only send token to contract and contract swap that token only. Hence checking tokenFromInput, postswapbalance.

Split this into atomic function to do one thing and use multicall into these function is much better approach to this.

Project does not follow clean code style and lack of organization

Rating: NonCritical

This project break lots of software development practices and design principles which should translated well into security practice.

• Important Generic function like LibSwap have no document to explain reasoning and it try to do both swap and get token from user.
• LibSwap try to do everything in one function is bad idea all around.
• Code checking NativeAsset is duplicated and checking several times during swap from internal LibSwap to external facets function
• Swapper.sol send unnecessary information like entire LiFiData struct data when it only need transactionId to emit event
• All swap, bridge functions handle both flow of sending ETH and ERC20 token. It should be split.
• Flexible using generic arbitrary call everywhere is dangerous. It is hard to control and hard to maintain.