search

Uniswap / v4-core

🦄 🦄 🦄 🦄 Core smart contracts of Uniswap v4
https://blog.uniswap.org/uniswap-v4
Other
1.97k stars 934 forks source link

rm unnecessary unlocked check in sync() #885

Open wjmelements opened 4 days ago

wjmelements commented 4 days ago

Resolves #883 Reviewers @hensha256 @snreynolds I believe a bad tradeoff was made in response to TOB-UNI4-3. The manager contract does not need to be fool-proof. Similar to TOB-UNI4-6, it can remain safely unresolved.

Not every issue raised by an audit must be resolved.

Trail Of Bits raised TOB-UNI4-6 but it was ignored.

Mistakes can be made between sync and settle.

The main issue raised by TOB-UNI4-3 is that mistakes can be made between sync and settle. A particular kind of mistake is hypothesized. This mistake is not the only kind of mistake that can be made, because the interface is not fool-proof. Here is an example of another mistake that is possible between sync and settle: take. take, like collectProtocolFees, can be invoked between sync and settle. This can cause a deposit not to be fully credited; an amount of balance equal to the amount taken is burned, just as in TOB-UNI4-3. However, the manager does not prevent this with an additional TLOAD. Instead, the users are supposed to not call take on that currency between sync and settle. Similarly, the users should not call collectProtocolFees between sync and settle.

collectProtocolFees can be safely called without restricting sync

It isn't necessary to call collectProtocolFees (or take) between sync and settle. Proper usage can prevent loss of funds. settle should be called as soon after sync as possible. Warnings can be added to the source code.

There are valid reasons to sync before unlock

The callback may not be the originator of the funds. It is sometimes better to transfer-in the funds before calling unlock. One such flow looks like this:

  1. sync()
  2. transfer()
  3. unlock() 3a. settle 3b. swap 3c. take

Preventing this flow requires the unlock callback contract to have access to the originating funds, which isn't always good or necessary. The originating funds could be coming from a separate system entirely.

Impact: Additional transfers

This section describes example of the aforementioned scenario, where being able to sync before unlock saves thousands of gas. For my project the gas impact is between 8371 and 25292 gas when the settled token is DAI or WETH.

Custodian Contract (CC)

This contract custodies funds and is the ultimate counterparty performing the swap. Perhaps it is an ERC-4337 smart account. Perhaps it is a multisig. But for whatever reason it doesn't have support for the UniswapV4 callback.

Swap Callback (SC)

This contract has support for the UniswapV4 callback. Its logic might be shared between many CC or it may be specific to one CC.

The swap with versatile sync: transfer

CC: PoolManager.sync(TokenA) CC: TokenA.transfer(PoolManager, AmountIn) CC: SC.performSwap() SC: PoolManager.unlock() PoolManager: SC.unlockCallback() SC: PoolManager.settle(TokenA) SC: PoolManager.swap() SC: PoolManager.take(TokenB, CC)

The swap without versatile sync: approve/transferFrom

CC: TokenA.approve(SC, AmountIn) CC: SC.performSwap() SC: PoolManager.unlock() PoolManager: SC.unlockCallback() SC: PoolManager.sync(TokenA) SC: TokenA.transferFrom(CC, PoolManager, AmountIn) SC: PoolManager.settle(TokenA) SC: PoolManager.swap() SC: PoolManager.take(TokenB, CC)

The swap without versatile sync: transfer/transfer

CC: TokenA.transfer(SC, AmountIn) CC: SC.performSwap() SC: PoolManager.unlock() PoolManager: SC.unlockCallback() SC: PoolManager.sync(TokenA) SC: TokenA.transfer(PoolManager, AmountIn) SC: PoolManager.settle(TokenA) SC: PoolManager.swap() SC: PoolManager.take(TokenB, CC)

Analysis

The additional approval/transferFrom or transfer/transfer in comparison to a single transfer amounts to thousands to tens of thousands of gas. Any account that wants to swap but doesn't have uniswap v4 callbacks benefits from sync outside of unlock.

Changes

Succinctly, the trade-off being made is whether collectProtocolFees should be fool-proofed against use between sync and settle, at the cost of custodial smart contracts without a uniswapv4 hook needing additional ERC20 operations to perform a swap. However, it is unclear that it is more important to fool-proof collectProtocolFees than other methods, such as take, that are still not fool-proofed against the same mistake. Therefore #856 is reverted and TOB-UNI4-3 is accepted as permissible.

How to solve the issue without requiring sync() to be unlocked.

If collectProtocolFees must be fool-proof, it can add a check that the collected token is not set in CURRENCY_SLOT. This solution is discussed more in its pull request: #886

wjmelements commented 4 days ago

I have updated the original description to describe the scenario where the gas impact is largest and extrapolate who all is impacted.