LI.FI contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

Some tokens take a fee on every transfer or might in the future, e.g. USDT. Currently, the protocol doesn't block those tokens from being used. But, neither does it properly support them. It doesn't check how many tokens it actually received after it transferred them from the user's address. It simply continues with the original amount it had called the transfer function with. Since the protocol holds funds itself, subsequent operations using that amount won't fail. Instead, they will simply access funds that don't belong to the user.

I'm not sure how many exchanges actually support fee-on-transfer tokens. I just know of Uniswap V2: https://docs.uniswap.org/protocol/V2/reference/smart-contracts/router-02#swapexacttokensfortokenssupportingfeeontransfertokens

Since the attacker is able to access funds they shouldn't be able to, I'd rate this as high.

Proof of Concept

Now, this whole thing expects the contract to have some funds before the user swaps their tokens. Since there is a withdraw function for the admin, I'd argue that's possible.

Let's also assume there is a token X that takes a 1% fee.

1. LiFi contract has 10 X tokens
2. User executes a swap through the GenericSwapFacet using 100 X tokens.
3. When the facet pulls the funds from the user's address, it only receives 99 X tokens because of the fees.
4. It proceeds to call the user's specified DEX with 100 X tokens
5. The user calls Uniswap V2 which supports the fee-on-transfer tokens. The swap is executed and the user gets their tokens.

The new state is:

• a happy user who got their tokens
• unhappy LiFi because they now only have 9 X tokens instead of the original 10.

Tools Used

none

Recommended Mitigation Steps

You can either block the usage of fee-on-transfer tokens by verifying that the contract's new balance matches the amount transferred:

uint oldBalance = ERC20(fromAssetId).balanceOf(address(this));
LibAsset.transferFromERC20(fromAssetId, msg.sender, address(this), fromAmount);
uint newBalance = ERC20(fromAssetId).balanceOf(address(this));
require(newBalance - oldBalance == fromAmount);

Or, you overwrite the swap data with the actual amount the contract received.

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L28-L30 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L100-L114

Vulnerability details

Impact

When swapping tokens, there are generally two types of swaps. Say a user wants to swap token A for token B. You either:

1. pay X amount of token A and receive as many of token B as the amount buys
2. or buy Y amount of token B but pay as little of token A as possible for it

Currently, the GenericSwapFacet only allows the first type of swaps. If the user tries to use the second type, they will receive the correct amount of token B. But, they won't get the remaining tokens of A.

Effectively, they lose some of their assets.

Here's the documentation for the "exact output" method for Uniswap: https://docs.uniswap.org/protocol/reference/periphery/SwapRouter#exactoutputsingle

The same thing also applies to the other facets that bridge the swapped tokens. In that case, the B tokens are bridged, but the remaining A tokens are simply left in the contract. The user loses them.

Since LiFi is supposed to be a generic solution to bridging & swapping tokens this is a pretty big issue. Especially considering that a user is able to call any function of a whitelisted dex contract.

Proof of Concept

The facet only transfers token B. It doesn't transfer the remains of token A: https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L28-L30

Example of a facet that also bridges tokens where the same thing happens: https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L100-L114

Tools Used

none

Recommended Mitigation Steps

Add the same check you do for token B for token A. You get the balance before the swap and before you pull the tokens from the user's address. Then execute the swap. Get the balance again. If there are any tokens left, return them to the user.

Report

Use a two-step process for ownership transfer

Transferring the owner in a two-step process is less error-prone than assigning the new owner directly.

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

Here's an example from the Compound codebase: https://github.com/compound-finance/compound-protocol/blob/master/contracts/Timelock.sol#L45-L58

use .call() instead of .transfer() to send ETH

This makes the transfer prone to failure in case the receiving address is a contract: https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/WithdrawFacet.sol#L31

See SWC-134 for more infos.