QA Report

[code423n4]

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.

[H3xept]

Re Coin whitelist for planned improvement

Duplicate of #108

[H3xept]

Re ExecuteSwap and PostSwapBalance check wrong input

Duplicate of #76