search

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

6 stars 4 forks source link

Behaviour of swapper can be erratic and lead to loss of user funds #110

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

The GenericSwapFacet allows for sequences of swaps using different DEX or routers.

Here https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibSwap.sol#L34, the code is the following: if the contract has enough for the swap, do nothing, otherwise transfer fromAmount from the user.

However the code of GenericSwapFacet accepts only one receivingAsset, therefore, if the contract has some balance of the fromAsset but not enough, it should only transfer fromAmount - balance from the user, otherwise funds will be lost.

Proof of Concept

Assume Alice wants to swap 10 A for 10 B during swap 1, and then 11 B for 11 C during swap 2. The first swap will happen correctly, but then 11 B will be transferred from Alice instead of only 1 B.

H3xept commented 2 years ago

We now return any excess value left in the contract (fixed in another issue).

maxklenk commented 2 years ago

We "disagree with severity" as this issue would not allow to access other users funds and only happens if the user passes these kind of swaps himself. The multi-swap feature is mainly used to allow swapping multiple different tokens into one, which is then fully swapped. In the example given 10 A could be swapped for 10 C and 1 B to 1 C as well, which would also be more clear for the user which funds will be access from his wallet.

gzeoneth commented 2 years ago

Agree with sponsor the scenario is unlikely and require the user to pass weird parameter to the contract. Downgrading to Low/QA.

gzeoneth commented 2 years ago

Consider with warden's QA Report #114