LI.FI contest - 0xkatana's results

1 - Low - calldata is not limited by whitelisting

Impact

The _executeSwaps function requires the swapData.approveTo and swapData.callTo to be approved in the dexWhitelist mapping. But there are several fields of the SwapData struct that are not whitelisted:

1. sendingAssetId
2. receivingAssetId
3. fromAmount
4. callData

The riskiest of these non-whitelisted values is callData, which allows any function of the whitelisted callTo contracts to be called. Even if the intent is for a user to only call one or two different swap functions with the DEX, the lack of limitations on the callData value allows any function to be called. Issues can even be caused if the whitelisted DEX address is a proxy address and the DEX adds new functions that cause LiFi to become insecure.

Proof of Concept

Only swapData.approveTo and swapData.callTo are validated in _executeSwaps https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/Swapper.sol#L16

_executeSwaps is an internal function, but the swapTokensGeneric function is a public entrypoint which permits any _swapData value https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L22

Tools Used

Manual analysis

Recommended Mitigation Steps

The design of GenericSwapFacet is very open ended today, but it should be more restricted. Because the purpose of this contract is to perform swaps with specific whitelisted DEXes, a whitelist preventing calls to other DEX functions should be added. This whitelist for callData should be checked at the same time as approveTo and callTo.

2 - Low - Owner has unlimited modification privileges

Impact

The diamond proxy EIP-2535 documentation states the dangers of adding/replacing functions using the diamondCut function. The documentation suggests limiting who, when, and the transparency of changes. The only limit currently in place is limiting who can use diamondCut to the contract owner, but a rogue contract owner could quickly rug users of LiFi because there are no checks on the owner's privileges.

Proof of Concept

The EIP2535 documentation explaining these risks https://eips.ethereum.org/EIPS/eip-2535#security-of-diamond-storage

The only check in place to prevent arbitrary changes to diamond storage is to permit only the contract owner to call diamondCut. There are no protections for when these changes can take place, no timelock to prevent surprise changes without notice, and no checks and balances for the owner's power. These are centralization risks and can cause problems for the users of the protocol if the owner is either compromised or acting maliciously. https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DiamondCutFacet.sol#L19

Tools Used

Manual analysis

Recommended Mitigation Steps

Apply recommendations from EIP2535 to limit when diamondCut changes can occur. Make it clear to users what the purpose of this function is, when upgrades will happen, and add a timelock if this could increase user trust that no sudden changes will happen.

3 - Low - Infinite allowance used

Impact

The approveERC20 function in LibAsset sets an infinite allowance for each token/bridge pair that it is called on. Doing this increases the risk of LiFi token holdings, because a security vulnerability in a trusted bridge could drain the tokens with infinite allowance from LiFi.

Proof of Concept

The approveERC20 function sets the allowance to MAX_INT for every token that is sent using AnySwap, Hop, NXTP, or CBridge https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibAsset.sol#L68

This implies that these bridges are completely trusted by LiFi, and if any of these bridges has a security issue, LiFi may be vulnerable because of this allowance setting.

Tools Used

Manual analysis

Recommended Mitigation Steps

Set the allowance to the token value sent in this transaction. It will use more gas but will increase the security of the protocol.

1 - Short require strings save gas

Impact

Strings in solidity are handled in 32 byte chunks. A require string longer than 32 bytes uses more gas. Shortening these strings will save gas.

Proof of Concept

Several cases of this gas optimization were found. These are a few examples, but more may exist

1. https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L84
2. https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L95
3. https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L102
4. https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L104
5. https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L113
6. https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L121
7. https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L124
8. https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L187

Tools Used

Manual analysis

Recommended Mitigation Steps

Shorten require strings

2 - Redundant assert

Impact

There are two asserts in WithdrawFacet that are not necessary. The transfer that happens immediately after the assert will revert if there is insufficient balance, so the assert does not provide any value.

Proof of Concept

The redundant assert calls are found at WithdrawFacet 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

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove the two asserts to save gas

3 - Public functions can be external

Impact

Declaring a function as external instead of public saves gas

Proof of Concept

Some functions can be declared external instead of public

• startBridgeTokensViaAnyswap(ILiFi.LiFiData,AnyswapFacet.AnyswapData) in AnyswapFacet.sol
• startBridgeTokensViaCBridge(ILiFi.LiFiData,CBridgeFacet.CBridgeData) in CBridgeFacet.sol
• swapTokensGeneric(ILiFi.LiFiData,LibSwap.SwapData[]) in GenericSwapFacet.sol
• startBridgeTokensViaHop(ILiFi.LiFiData,HopFacet.HopData) in HopFacet.sol
• startBridgeTokensViaNXTP(ILiFi.LiFiData,ITransactionManager.PrepareArgs) in NXTPFacet.sol
• completeBridgeTokensViaNXTP(ILiFi.LiFiData,address,address,uint256) in NXTPFacet.sol
• swapAndCompleteBridgeTokensViaNXTP(ILiFi.LiFiData,LibSwap.SwapData[],address,address) in NXTPFacet.sol
• withdraw(address,address,uint256) in WithdrawFacet.sol

Tools Used

Slither

Recommended Mitigation Steps

Change function from public to external to save gas