When swapping tokens, there are generally two types of swaps. Say a user wants to swap token A for token B. You either:
pay X amount of token A and receive as many of token B as the amount buys
or buy Y amount of token B but pay as little of token A as possible for it
Currently, the GenericSwapFacet only allows the first type of swaps. If the user tries to use the second type, they will receive the correct amount of token B. But, they won't get the remaining tokens of A.
The same thing also applies to the other facets that bridge the swapped tokens. In that case, the B tokens are bridged, but the remaining A tokens are simply left in the contract. The user loses them.
Since LiFi is supposed to be a generic solution to bridging & swapping tokens this is a pretty big issue. Especially considering that a user is able to call any function of a whitelisted dex contract.
Add the same check you do for token B for token A. You get the balance before the swap and before you pull the tokens from the user's address. Then execute the swap. Get the balance again. If there are any tokens left, return them to the user.
H3xept
commented
2 years ago
Lines of code
https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L28-L30 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L100-L114
Vulnerability details
Impact
When swapping tokens, there are generally two types of swaps. Say a user wants to swap token A for token B. You either:
Currently, the GenericSwapFacet only allows the first type of swaps. If the user tries to use the second type, they will receive the correct amount of token B. But, they won't get the remaining tokens of A.
Effectively, they lose some of their assets.
Here's the documentation for the "exact output" method for Uniswap: https://docs.uniswap.org/protocol/reference/periphery/SwapRouter#exactoutputsingle
The same thing also applies to the other facets that bridge the swapped tokens. In that case, the B tokens are bridged, but the remaining A tokens are simply left in the contract. The user loses them.
Since LiFi is supposed to be a generic solution to bridging & swapping tokens this is a pretty big issue. Especially considering that a user is able to call any function of a whitelisted dex contract.
Proof of Concept
The facet only transfers token B. It doesn't transfer the remains of token A: https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L28-L30
Example of a facet that also bridges tokens where the same thing happens: https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L100-L114
Tools Used
none
Recommended Mitigation Steps
Add the same check you do for token B for token A. You get the balance before the swap and before you pull the tokens from the user's address. Then execute the swap. Get the balance again. If there are any tokens left, return them to the user.