LI.FI contest - CertoraInc's results

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibSwap.sol#L42-L46 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibAsset.sol#L49-L50

Vulnerability details

Impact

Low level calls (call, delegate call and static call) return success if the called contract doesn’t exist (not deployed or destructed).

That means that in LibSwap, if _swapData.callTo doesn't exist the call will fail but success will be set to true, which will act like the call was successful.

This bug can also be seen in the transferNativeAsset function of the LibAsset library:

function transferNativeAsset(address payable recipient, uint256 amount) internal {
    // solhint-disable-next-line avoid-low-level-calls
    (bool success, ) = recipient.call{ value: amount }("");
    require(success, "#TNA:028");
}

Proof of Concept

• https://github.com/Uniswap/v3-core/blob/main/audits/tob/audit.pdf (report #9)
• https://docs.soliditylang.org/en/develop/control-structures.html#error-handling-assert-require-revert-and-exceptions.

Tools Used

Remix & VS Code

Recommended Mitigation Steps

Check address existence before the low level call

Lows and Non-criticals

• The ITransactionManager intraface's solidity version is pragma solidity 0.8.7;, and the rest of the contracts are pragma solidity ^0.8.7; - this should be consistent.

• Missing space in error message in the initializeDiamondCut function of LibDiamond library:

  require(_calldata.length == 0, "LibDiamondCut: _init is address(0) but_calldata is not empty");// should be "... but _calldata ..."

• Pragmas should be locked to a specific compiler version (instead of ^0.8.7 for example), to avoid contracts getting deployed using a different version, which may have a greater risk of undiscovered bugs.

• Unsafe transferOwnership - it will be more safe to use a pattern for transferring the ownership, for example making the new pending owner to accept the ownership. That will make sure that the new owner is an actual existing address and not a self-distructed address or a non-existing address.

• Check that the array's lengths given to the initHop function as parameters are equal (can cause errors)

• The fallback function of the LiFiDiamond is re-enterable, so it will be a good thing to add a reentrency guard to it (one reentrency guard will lock all the functions of the facets).

• In the CBridgeFacet contract you make sure that msg.value >= _cBridgeData.amount, "ERR_INVALID_AMOUNT", but in the other facets you make sure that they are equal. This difference might not be wanted, and you should pay attention to it.

• In the comment before the _bridge function it says that the function is public, but it is clearly declared as internal.

  /*
  * @dev Public view function for the CBridge router address
  * @returns the router address
  */
  function _bridge() internal view returns (address) {
      Storage storage s = getStorage();
      return s.cBridge;
  }

Gas Optimizations

• In the _startBridge function of the CBridgeFacet contract, use s.cBridge instead of calling the _bridge() function

  Storage storage s = getStorage();
  address bridge = _bridge();

  This is the implementation of the function:

  /*
  * @dev Public view function for the CBridge router address
  * @returns the router address
  */
  function _bridge() internal view returns (address) {
      Storage storage s = getStorage();
      return s.cBridge;
  }

• There is no need to define two variables in the swapAndStartBridgeTokensViaCBridge function of the CBridgeFacet contract, you can simply use one variable for this calculation.

  Code before:

  uint256 _fromTokenBalance = LibAsset.getOwnBalance(_cBridgeData.token);
  
  // Swap
  _executeSwaps(_lifiData, _swapData);
  
  uint256 _postSwapBalance = LibAsset.getOwnBalance(_cBridgeData.token) - _fromTokenBalance;

  Code after:

  uint256 _balanceDiff = LibAsset.getOwnBalance(_cBridgeData.token);
  
  // Swap
  _executeSwaps(_lifiData, _swapData);
  
  _balanceDiff = LibAsset.getOwnBalance(_cBridgeData.token) - _balanceDiff;

• Using > costs more gas than using <, so you can replace all the > with < and switch the expressions, for example in the CBridgeFacet contract, use require(0 < _postSwapBalance, "ERR_INVALID_AMOUNT"); instead of require(_postSwapBalance > 0, "ERR_INVALID_AMOUNT");. This can be done in many places in the code, simply search > and switch the expressions to replace it with <

• Loop optimizations - there are multiple optimizations that can be done to your loops. Let's look at the loop in the batchAddDex function of the DexManagerFacet contract for example:

  1. use ++i instead of i++
  2. save the length in a local variable instead of accessing it in every iteration
  3. save the _dexs[i] in a local variable instead of accessing it 3 times in every iteration

  for (uint256 i; i < _dexs.length; i++) {
      if (s.dexWhitelist[_dexs[i]] == true) {
          continue;
      }
      s.dexWhitelist[_dexs[i]] = true;
      s.dexs.push(_dexs[i]);
  }

  These kind of optimizations can be done to more loops in your code. Now we'll take a look at the loop in the batchRemoveDex function of the DexManagerFacet contract. Here we can do multiple things, similar to the previous loop:

  1. use ++i and ++j instead of i++ and j++
  2. save the lengths in a local variable instead of accessing it in every iteration
  3. save the _dexs[i] in a local variable instead of accessing it many times in every iteration (here it is being accessed in every iteration of the inner loop, plus 2 times outside the loop).

  for (uint256 i; i < _dexs.length; i++) {
      if (s.dexWhitelist[_dexs[i]] == false) {
          continue;
      }
      s.dexWhitelist[_dexs[i]] = false;
      for (uint256 j; j < s.dexs.length; j++) {
          if (s.dexs[j] == _dexs[i]) {
              _removeDex(j);
              return;
          }
      }
  }

  More loops in the code:

  • _executeSwaps function of the Swapper contract
  • initHop function of the HopFacet contract
  • facets function of the DiamondLoupeFacet contract
  • removeDex function of the DexManagerFacet contract

• Calculate the hash before deployment, and just use the hash result. This can be done for example in the CBridgeFacet contract where you define bytes32 internal constant NAMESPACE = keccak256("com.lifi.facets.cbridge2");. This can also be done in the HopFacet, NXTPFacet and LibDiamond contracts.

• Use defined values instead of calculate the maximum value of uint256 - use type(uint256.max instead of 2**256 - 1 (this will save the gas spent on the calculation). This can be done in the LibAsset and LibSwap libraries.

• use boolean value instead of comparing to true or false. For example in the _executeSwaps function of the Swapper contract, the code in the require can be written as:

  function _executeSwaps(LiFiData memory _lifiData, LibSwap.SwapData[] calldata _swapData) internal {
      // Swap
      for (uint8 i; i < _swapData.length; i++) {
          require(
              ls.dexWhitelist[_swapData[i].approveTo] && ls.dexWhitelist[_swapData[i].callTo],
              "Contract call not allowed!"
          );
  
          LibSwap.swap(_lifiData.transactionId, _swapData[i]);
      }
  }

  instead of:

  function _executeSwaps(LiFiData memory _lifiData, LibSwap.SwapData[] calldata _swapData) internal {
      // Swap
      for (uint8 i; i < _swapData.length; i++) {
          require(
              ls.dexWhitelist[_swapData[i].approveTo] == true && ls.dexWhitelist[_swapData[i].callTo] == true,
              "Contract call not allowed!"
          );
  
          LibSwap.swap(_lifiData.transactionId, _swapData[i]);
      }
  }

• Use unchecked in the swapAndCompleteBridgeTokensViaNXTP of the NXTPFacet contract, this calculation won't underflow because we know that postSwapBalance > startingBalance.

  if (postSwapBalance > startingBalance) {
      finalBalance = postSwapBalance - startingBalance; // use unchecked on this calculation
      LibAsset.transferAsset(finalAssetId, payable(receiver), finalBalance);
  }

• Simplify the if statements in the swap function of LibSwap library - this will save the gas spent on checking the same condition and calling the same function twice.

  old code:

  if (!LibAsset.isNativeAsset(fromAssetId) && LibAsset.getOwnBalance(fromAssetId) < fromAmount) {
      LibAsset.transferFromERC20(fromAssetId, msg.sender, address(this), fromAmount);
  }
  
  if (!LibAsset.isNativeAsset(fromAssetId)) {
      LibAsset.approveERC20(IERC20(fromAssetId), _swapData.approveTo, fromAmount);
  }

  new code:

  if (!LibAsset.isNativeAsset(fromAssetId)) {
      if (LibAsset.getOwnBalance(fromAssetId) < fromAmount) {
          LibAsset.transferFromERC20(fromAssetId, msg.sender, address(this), fromAmount);
      }
      LibAsset.approveERC20(IERC20(fromAssetId), _swapData.approveTo, fromAmount);
  }

• call the LibDiamond.enforceIsContractOwner function before the getStorage function in the initNXTP function of the NXTPFacet contract and in the initCbridge function of the CBridgeFacet contract, in order to save gas in case that the LibDiamond.enforceIsContractOwner function will revert

  function initNXTP(ITransactionManager _txMgrAddr) external {
      Storage storage s = getStorage();
      LibDiamond.enforceIsContractOwner();
      s.nxtpTxManager = _txMgrAddr;
  }

  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);
  }