Uniswap / interface

🦄 Open source interfaces for the Uniswap protocol
https://app.uniswap.org
GNU General Public License v3.0
4.95k stars 5.02k forks source link

exact output buys with eth input errors are confusing because we try to pay with WETH #2006

Open Rachel-Eichenberger opened 3 years ago

Rachel-Eichenberger commented 3 years ago

Bug Description So I have come across some of these where the STF failure come from trying to transferFrom WETH when it has not been wrapped. The Users did not have WETH selected, and the multicall looks like it setup the call to use WETH In the Uniswap Discord tickets meekohi and nicovitale both txs have an ETH value used but still act like its using WETH It is trying to do transferFrom WETH - from the User to the pair

nicovitale tx https://etherscan.io/tx/0x94cf3531c2937f4ca6f4763320ab0545e06e26d6d54cce14b8cb2866cc5566ae/advanced this one doesn't wrap the eth any where in the parity trace and calls transferFrom on the WETH contract

meekohi tx https://etherscan.io/tx/0xdcdcc4c2f9897599ae75cc55759511323c42b40099ca67e7e6edf09d3570e03c it's routed but I didn't see a WETH deposit and calls transferFrom on the WETH contract

Steps to Reproduce

  1. ? have not been able to reproduce

Expected Behavior The multicall would include the wrap to WETH or use a call that wraps the ETH

moodysalem commented 3 years ago

both of these cases are exact output, and the way we decide whether to wrap is whether address(this).balance >= the amount required for input.

it is likely the price moved against the users in these cases, s.t. they would've had to pay more ether than they sent, so we tried to pay with WETH.

something to consider for our next version of swap router https://github.com/Uniswap/uniswap-v3-periphery/issues/165

we could check here that the amount required is less than the maximum amount in and return a better error, but it may be more expensive in terms of gas https://github.com/Uniswap/uniswap-v3-periphery/blob/8943ee4047ea7892685802e4baf5f913993844fa/contracts/SwapRouter.sol#L81

Rachel-Eichenberger commented 3 years ago

both of these cases are exact output, and the way we decide whether to wrap is whether address(this).balance >= the amount required for input.

it is likely the price moved against the users in these cases, s.t. they would've had to pay more ether than they sent, so we tried to pay with WETH.

something to consider for our next version of swap router Uniswap/uniswap-v3-periphery#165

we could check here that the amount required is less than the maximum amount in and return a better error, but it may be more expensive in terms of gas https://github.com/Uniswap/uniswap-v3-periphery/blob/8943ee4047ea7892685802e4baf5f913993844fa/contracts/SwapRouter.sol#L81

Ahh cool, well that makes sense. I suppose it wouldn't be a bad wish list item, since the fail on STF is confusing, and some kind of require wouldn't be a huge gas cost. However not the biggest issue, only seen 2 or 3 (of course probably more). I guess if we see more of these, but just knowing that will help. I'll let you decide if you want to close. I can Bring it back up if I see a large quantity of these. Thanks