search

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

6 stars 4 forks source link

Any user can recover the funds left in the contract #205

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#L33-L35

Vulnerability details

Impact

There is a WithdrawFacet such that only the owner/admin can recover the lost funds in the contract. However, any user can retrieve the funds by using the swapTokensGeneric function, which might be unexpected behavior.

Proof of Concept

  1. Suppose that 1000 USDC is left in the contract. Now, some random user wants to withdraw it.
  2. They call the swapTokensGeneric function with corresponding parameters to swap 1000 USDC to DAI via the Uniswap V2 router.
  3. The swapTokensGeneric function calls _executeSwaps, which calls LibSwap.swap. In this function, the contract checks whether LibAsset.getOwnBalance(fromAssetId) < fromAmount or not. If it has enough balance, it does not call LibAsset.transferFromERC20 to request the tokens from the caller.
  4. Therefore, the user doesn't have to send more USDC to the contract because the funds are enough. As a result, they get the swapped DAI without providing more USDC.

GenericSwapFacet.sol#L22-L44 Swapper.sol#L12-L23 LibSwap.sol#L33-L35

Recommended Mitigation Steps

If recovering funds by anyone is not the desired behavior, consider always transferring the funds from users before the first swap.

H3xept commented 2 years ago

Duplicate of #66

We are aware that the contract allows users to use latent funds, although we disagree on it being an issue as no funds (ERC20 or native) should ever lay in the contract. To make sure that no value is ever kept by the diamond, we now provide refunds for outstanding user value (after bridges/swaps).