Uniswap / interface

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

Multistep tranasctions should always check deadlines in the first step. #2320

Open MicahZoltu opened 3 years ago

MicahZoltu commented 3 years ago

Bug Description When issuing a multicall transaction with multiple steps, if the first step doesn't check a deadline but later steps do then the transaction will unnecessarily burn a large amount of gas and then revert.

Steps to Reproduce https://etherscan.io/tx/0x6da942398b522165efe28432259ff67606f615852fe6de5da7fb11cdc6262cfe Notice that this is a multicall with:

  1. createAndInitializePoolIfNecessary
  2. mint
  3. refundETH

createAndInitializePoolIfNecessary costs over 4,000,000 gas and has no deadline check. mint has a deadline checked that is checked as one of the very first things it does.

Expected Behavior Any transaction with a deadline will check the deadline first, and fail out immediately without wasting much gas.

Additional Context With a router update, you could add a deadline check to every function where some allow it to be 0.

With a router update, you could add a deadline to the multicall itself. This would save gas on any transaction that has multiple steps with a deadline check, as well as save gas for failures in scenarios like this.

With a router update, you could add a new function that only does a deadline check that can be included at the start of any multicalls that need to do deadline checks.

Without a router update, you could find a function that already exists on the router that has a deadline and can be called in a no-op way and add it as a first transaction in the multicall as a mechanism for checking if the deadline is passed or not and failing fast. This would cost a bit of extra gas, but it has potential to save users millions of gas like in the linked example. I'm not sure what functions would be eligible for such a thing, or if there is a way to add new multicall entrypoints without doing a full router update.

moodysalem commented 3 years ago

Agreed, I created this PR but probably not worth a NFT contract upgrade yet. https://github.com/Uniswap/uniswap-v3-periphery/pull/188