fix: tolerate hook-funded input on exact-output swaps - #584
Open
dianakocsis wants to merge 4 commits into
Open
Conversation
A v4 hook can pay a swap's input on the caller's behalf. When it covers the cost in full, PoolManager hands the router the requested positive output against an input delta of exactly zero. That is solvent -- the hook settles the debt inside the unlock -- and core permits it explicitly: Hooks.sol:311 notes the caller "has to pay for (or receive) the hook's delta", and IHooks documents a negative afterSwap return as the hook owing/sending currency. But it is a shape no ordinary pool produces, where positive output always implied strictly negative input. Exact output is the only direction that has to DISCOVER its input by reading a delta, so it is the only one affected: - The per-hop price guard divides output by input. A zero input panics 0x12, even though the realized price is then infinite and clears every finite bound it is checked against. Hit _swapExactOutputSingle and, because the multihop guard is gated on array presence rather than each bound's value, _swapExactOutput whenever minHopPriceX36 was non-empty. - The multihop loop runs backwards, feeding each hop's input to the previous hop as its required output. A funded hop propagated zero, and PoolManager rejects amountSpecified == 0 with SwapAmountCannotBeZero. Skip the division when the input is zero, and stop the backward loop when a hop consumes nothing -- the upstream hops have nothing left to produce, and _take/_settle already no-op on zero, so untouched currencies settle cleanly and an amountIn of 0 trivially clears amountInMaximum. Over-funding (a hook paying MORE than the cost, leaving a credit whose owner is undefined in a route) is deliberately left unsupported and still reverts SafeCastOverflow on the negation, as it does today. Naming that condition would mean adding an error to IV4Router for no behavior change, so the interface is untouched; the reasoning is recorded on _swapInput instead. The fill check runs before the zero-input branch, so all-or-nothing still holds: a funded hop that underfills reverts V4ExactOutputUnfilled as before. Exact input is unaffected -- its divisor is the caller's own amountIn, never a delta, so it can neither divide by zero nor read the sign that overflows. Also corrects the "output delta will always be positive" comments, which named a hook case the code does not actually handle: a hook taking more than the whole output leaves a negative delta and the cast in _swapOutput rejects it. Documented rather than fixed, since exact input is out of scope here. Gas +26 single-hop, +54/hop multihop, +96 bytes. Snapshots regenerated (isolate mode). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Not related to the hook-funded fix in the previous commit. The committed Quoter numbers on fix/exact-output-underfill no longer match what the code produces, and CI runs with FORGE_SNAPSHOT_CHECK=true, so the check fails regardless of any change made here: Error: Snapshots differ from previous run - [Quoter_quoteExactOutput_twoHops] 204880 -> 207219 Confirmed independent of this branch by stashing the router change and re-running: the drift is still there. Both exactInput and exactOutput entries move, while this branch touches only exact-output paths, so it cannot be the cause. Kept as its own commit so it can be cherry-picked onto fix/exact-output-underfill, where the drift actually lives. If it is fixed there instead, drop this commit rather than merging both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dianakocsis
force-pushed
the
fix/hook-funded-exact-output
branch
from
August 5, 2026 16:53
df31027 to
a1bde74
Compare
This comment has been minimized.
This comment has been minimized.
Long afterSwap signature wraps onto separate lines. Only this file is touched: forge fmt locally also wants to reflow test/position-managers/PositionManager.t.sol, but that is an artifact of running 1.5.1 against CI's pinned 1.4.3, and CI flagged only this file. Reformatting it here would add an unrelated file and fail 1.4.3 anyway. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
quickquick |
This reverts commit a1bde74. The Quoter snapshots were never stale -- I misread a local toolchain difference as drift. CI pins Foundry v1.3.6 for tests; this machine runs 1.5.1, and the two measure Quoter gas differently. Locally FORGE_SNAPSHOT_CHECK failed against the committed values, which looked like drift on the base branch. It was not: CI regenerates exactly the values already on main, e.g. [Quoter_quoteExactInput_oneHop_initializedAfter] 148867 -> 147573 where 148867 was my regenerated value and 147573 is main's, which CI reproduces. Only Quoter entries diverge between the two versions; V4RouterTest.json matches, so the gas numbers for this fix are unaffected. Snapshot changes here should be verified against CI, not this machine, unless the local toolchain is pinned to v1.3.6. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A v4 hook can pay a swap's input on the caller's behalf. When it covers the cost in full,
PoolManagerhands the router the requested positive output against an input delta of exactly zero. That state is solvent — the hook settles the debt inside the unlock — and core permits it explicitly:Hooks.sol:311—// the caller has to pay for (or receive) the hook's deltaIHooks.sol:114— theafterSwapreturn is documented as "negative: the hook owes/sent currency"But it is a shape no ordinary pool produces, where positive output always implied a strictly negative input.
V4Routerassumed that invariant, so a legitimate fully-sponsored route reverted.Exact output is the only direction affected, because it is the only one that has to discover its input by reading a delta. Exact input divides by the caller's own
amountInparameter, so it can neither divide by zero nor read the sign that overflows.The two failures
1. Division by zero. The per-hop price guard computes execution price as output ÷ input. A zero input panics
0x12— even though the realized price is then infinite and clears every finite bound it is checked against. Hit_swapExactOutputSingle, and_swapExactOutputwheneverminHopPriceX36was non-empty (the multihop guard is gated on array presence, not on each bound's value, so it fired even when that hop's own bound was0).2. Zero propagation. The multihop loop runs backwards, feeding each hop's input to the previous hop as its required output. A funded hop propagated zero, and
PoolManager.sol:193rejectsamountSpecified == 0withSwapAmountCannotBeZero, unwinding the whole unlock.Neither risks funds — the unlock is atomic — but a caller loses gas and the route.
Fix
Three lines:
Skipping the division is correct rather than defensive: an infinite realized price genuinely satisfies every finite minimum. Breaking is likewise not a shortcut — if a hop consumes nothing, the upstream hops have nothing to produce, and
_take/_settlealready no-op on zero (DeltaResolver.sol:27,38), so untouched currencies settle cleanly and anamountInof0trivially clearsamountInMaximum.The fill check runs before the zero-input branch, so all-or-nothing still holds: a funded hop that underfills still reverts
V4ExactOutputUnfilled.Scope decisions
Over-funding is deliberately not supported. A hook paying more than the cost leaves a credit on the input currency, which still reverts
SafeCastOverflowon the negation, exactly as onmain. The surplus has no defined owner in a route — collecting it needs aTAKE-based plan no integrator writes today, and in multihop it can land on an intermediate currency the caller never asked to receive. Relaxing this later is non-breaking; shipping ownership semantics and walking them back would not be. Naming the condition would have meant adding an error toIV4Routerfor zero behavior change, so the interface is untouched and the reasoning lives on_swapInputinstead.Exact input is untouched. Verified structurally:
_swapInputis called only from the two exact-output functions.Corrected two stale comments.
// The output delta will always be positive, except for when interacting with certain hook poolsnamed a case the code does not handle — a hook taking more than the whole output leaves a negative delta and the cast in_swapOutputrejects it. Now documented as unsupported rather than reading like reassurance. Handling it properly is follow-up work.Tests
10 tests, and each of the three fixes has a test that fails without it (verified by reverting
src/V4Router.soland re-running):..._withMinHopPrice_succeedspanic 0x12..._multiHop_...withPerHopPrices_succeedspanic 0x12..._multiHop_...skipsUpstreamHopsSwapAmountCannotBeZero..._threeHop_hookFundsMiddleHop_...SwapAmountCannotBeZerotestFuzz_...onlyFullSubsidyClearsPriceBoundpanic 0x12The fuzz test pins the boundary from both sides: with an unsatisfiable price bound, only a full subsidy clears it, because only then is the price infinite. Remaining tests are regression and characterization guards.
MockFullySubsidizingHookabsorbs a share of whatever the pool actually charged — the existingDeltaReturningHookreturns a preset amount, which cannot land the input on exactly zero. It is exact-output-only by construction (theafterSwapreturn applies to the unspecified currency, which is the input side only for exact output) and reverts rather than misreporting if misused.Gas
+26single-hop,+54/hop multihop,+96bytes. Exact-input paths unchanged.Note on the second commit
chore: regenerate stale Quoter gas snapshotsis unrelated to this fix. CI runsFORGE_SNAPSHOT_CHECK=true, and the committed Quoter numbers were already stale on the base, so the check failed regardless of this change (confirmed by stashing the router change and re-running — the drift persists, and it movesexactInputentries this PR never touches). Kept as its own commit so it can be dropped or cherry-picked independently.Question for reviewers
Is the multihop
breakacceptable? When a hop is fully funded, the upstream hops the caller specified silently do not execute. I believe it is correct — there is genuinely nothing for them to produce, the caller receives their exact output, pays nothing, and no currency is left unsettled — but it is a real behavior change worth arguing rather than waving through.test_exactOutput_threeHop_hookFundsMiddleHop_skipsRemainingHopsdemonstrates exactly what it does to a longer route.Secondary: does anyone have a hook that deliberately over-pays? That is the case left unsupported above.
🤖 Generated with Claude Code