search

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

6 stars 4 forks source link

QA Report #81

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

Tokens sent to the contract can be retrieved by anyone, which opens up the possibility for WithdrawFacet.withdraw() to be frontrun.

When swapping ERC20 tokens, LibSwap.swap only tries to transfer tokens if the contract doesn't have enough balance to swap. As long as a market for the token exists, anyone can call GenericSwapFacet.swapTokensGeneric() to swap any token in the contract and retrieve them.

Proof of Concept

  1. Alice mistakenly sent 1000 USDC to LiFi contract.
  2. Governance tries to rescue the USDC by calling WithdrawFacet.withdraw().
  3. An attacker front run the transaction by calling GenericSwapFacet.swapTokensGeneric(), using USDC as sendingAssetId and WETH as receivingAssetId. The attacker receives free WETH.

Recommended Mitigation Steps

Consider transferring the token (sendingAssetId of the first _swapData) to the contract before executing the swaps. Return the value of toAmount in LibSwap.swap() and use it as the fromAmount value for the next swap. This way an attacker cannot utilise funds left in the contract for swapping.

H3xept commented 2 years ago

By fixing other audit reports, we now have made sure that no value can be left in the contract via swap and bridge methods by refunding any outstanding token.

To our knowledge, the only way a user can transfer value to our contract and have it sit there is to directly send it. We highly discourage this behaviour and we internally decided that such a deliberate action is unlikely. Therefore we believe that it is not worth mitigating, as the necessary checks would increase the transaction costs for all contract calls.

gzeoneth commented 2 years ago

Downgrading to Low/QA. Treating as warden's QA Report.

JeeberC4 commented 2 years ago

Preserving original title: ERC20 withdrawals can be frontrun