Tokens held in contract can be stolen

[code423n4]

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L33 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L76

Vulnerability details

Impact

Any ERC20 token balance held in the contract can be stolen by an attacker.

Proof of Concept

The swap function in the LibSwap.sol library checks if the contract's balance is sufficient to execute the trade (see line 33). If the contract does not hold the number of tokens to swap, the amount is transferred from the caller to the contract.

As long as the swap function is called with a swap amount of a token being less than the balance of the token in the contract, the function does not transfer the tokens from the caller but uses the contract's own tokens for the swap.

Furthermore, as the swapData argument is created by the caller (see for example AnyswapFacet.sol line 76), the contract called could be controlled by the caller and statically only returning 1 arbitrary token.

Recommended Mitigation Steps

Remove the second condition in the if-clause in the LibSwap::swap function to always transfer the tokens from the caller to the contract before executing a swap.

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