LBRouter.removeLiquidity returning wrong values #105
Labels
2 (Med Risk)
Assets not at direct risk, but function/availability of the protocol could be impacted or leak value
bug
Something isn't working
M-01
selected for report
This submission will be included/highlighted in the audit report
sponsor confirmed
Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Lines of code
https://github.com/code-423n4/2022-10-traderjoe/blob/e81b78ddb7cc17f0ece921fbaef2c2521727094b/src/LBRouter.sol#L291
Vulnerability details
Impact
LBRouter.removeLiquidity
reorders tokens when the user did not pass them in the pair order (ascending order):However, when returning
amountX
andamountY
, it is ignored if the order was changed:Therefore, when the order of the tokens is swapped by the function, the return value
amountX
("Amount of token X returned") in reality is the amount of the user-provided token Y that is returned and vice versa.Because this is an exposed function that third-party protocols / contracts will use, this can cause them to malfunction. For instance, when integrating with Trader Joe, something natural to do is:
This snippet will only be correct when the token addresses are passed in the right order, which should not be the case. When they are not passed in the right order, the accounting of third-party contracts will be messed up, leading to vulnerabilities / lost funds there.
Proof Of Concept
First consider the following diff, which shows a scenario when
LBRouter
does not switchtokenX
andtokenY
, resulting in correct return values:This test passes (as it should). Now, consider the following diff, where
LBRouter
switchestokenX
andtokenY
:This test should also pass (the order of the tokens was only switched), but it does not because the return values are mixed up.
Recommended Mitigation Steps
Add the following statement in the end:
The text was updated successfully, but these errors were encountered: