If contract holds balance of any ERC20 token, any user can take it

[code423n4]

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/GenericSwapFacet.sol#L28 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibSwap.sol#L33

Vulnerability details

Impact

If the LiFiDiamond contract ends up holding any ERC20 token, any user is able to perform a swap from the held asset to another asset, and the swap will use the contracts' funds as input instead of their own. The result is that a user can take all of the funds from the contract.

Proof of Concept

• When a user submits a swap to the GenericSwapFacet.sol contract, it calls _executeSwaps() from Swapper.sol, which calls swap() from LibSwap.sol.
• The GenericSwapFacet.sol and Swapper.sol contracts do not take the users funds — that is left to LibSwap.sol.
• However, in LibSwap.sol, line 33 contains the logic for when the token should be transferred from the user, and it only takes the user's tokens if the fromAmount is greater than the contract's balance of that token.
• If a user submits a transaction with a fromAmount <= the contract's balance of that token, the contract tokens will be used for the input, while the user will be send the outputted tokens.

Here is a link to a screenshot of a test running a simulation: https://drive.google.com/file/d/1Ac3KLufObZV7djHxVrmRlp9yjT7CW7E1/view?usp=sharing

Recommended Mitigation Steps

• remove the && LibAsset.getOwnBalance(fromAssetId) < fromAmount) check from the if statement on line 33 of LibSwap.sol.

[H3xept]

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).