search

code-423n4 / 2022-03-lifinance-findings

6 stars 4 forks source link

Swaps don't properly support fee-on-transfer tokens #106

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

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:

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.

H3xept commented 2 years ago

Duplicate of #66