search

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

6 stars 4 forks source link

Swap including native asset don't behaved as specified #112

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#L42

Vulnerability details

Impact

Here: https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibSwap.sol#L42, it should be fromAmount, otherwise it can be really misleading for users and decrease the flexibility as you can only have 1 swap (the first) involving native asset.

Proof of Concept

Assume Alice wants to swap 1 ETH for 3000 USDC in first swap using Uniswap, and then 1 ETH for 3000 USDC in a second swap using Sushiswap, all the eth would be send to Uniswap !

Another case would be if Alice wants to first swap USDC to DAI, and then ETH to DAI, all ETH would be sent to the first swap !

H3xept commented 2 years ago

Fixed in lifinance/lifi-contracts@0d3fface2c1f299ea2a91bb97fcd1ccaff6fe0da

maxklenk commented 2 years ago

We "disagree with severity" as this issue would not allow to access other users funds. As soon as the the contract would try to execute the second swap the whole contract call would revert as the msg.value has already been used and can't be send along again. While this is clearly a bug, it is not a high risk one.

gzeoneth commented 2 years ago

Agree with sponsor that no fund is lost. Changing to Med Risk due to potential tx revert.

gzeoneth commented 2 years ago

Duplicate of #207