QA Report

[code423n4]

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.

[H3xept]

Re deprecated transfer()

Duplicate of #14

[H3xept]

Re Assert is used instead of require

Duplicate of #165

[H3xept]

Re Native value with ERC20

Duplicate of #53