Repository navigation
fix: OZ 2.3.0 audit fixes on the router path (L-02, N-01, N-05, N-08) - #608
Merged
Merged
Conversation
V4Quoter read the unspecified side of a swap's BalanceDelta with a bare truncating uint128(...). Only the specified side is guaranteed to have the expected sign. The unspecified side is whatever remains after Hooks.afterSwap applies the hook's returned delta, and a hook holding AFTER_SWAP_FLAG plus AFTER_SWAP_RETURNS_DELTA_FLAG can drive it past zero. The only existing guard, BaseV4Quoter._swap's NotEnoughLiquidity check, compares the specified side against the requested amount, so it cannot see this. The cast wrapped and the quoter returned roughly 2^128 as a successful quote with no revert, for a pool that in reality pays out nothing. A consumer that ranks candidate pools by raw quoter output picks that pool over every honest one. V4Router already handles the same two quantities correctly, so the quoter now uses those exact forms. The output sides at _quoteExactInput and _quoteExactInputSingle take the int128 overload of toUint128, matching _swapOutput. The input sides at _quoteExactOutput and _quoteExactOutputSingle widen before negating, uint256(-int256(x)).toUint128(), matching _swapInput. The widening matters on its own. Negating an int128 first would panic on type(int128).min before SafeCast ever ran, and that input is a legitimate magnitude, not a corrupted one. The widened form yields 2^127. Keeping the router's forms also preserves the hook-funded input case added in bbd4346: an input delta of exactly zero still quotes as an amountIn of 0 rather than reverting, so a route the router can execute stays quotable. Two related sites are fixed alongside. _quoteExactInput and _quoteExactInputSingle built the swap amount as -int256(int128(amountIn)) from a uint128. Above type(int128).max that reinterprets as negative, and the outer negation flips the sign, so an exact-input quote silently simulated an exact-output swap. Both now widen through uint256 as the router does. A hook that corrupts the delta makes its own pool unquotable rather than returning a clamped zero. Clamping would be wrong: a negative output delta means the caller owes the output currency, not that the pool pays nothing, and the router rejects that state too. Reverting keeps the quote and the swap in agreement about which pools are usable. Behavior on rejection is the existing shape. SafeCastOverflow bubbles out of the simulation and parseQuoteAmount wraps it in UnexpectedRevertBytes, the same outer error NotEnoughLiquidity already produces. No interface change. One divergence from the router is left in place. In multi-hop exact output, a fully hook-funded hop makes the router break out of its loop, while the quoter propagates the zero into the next swap and hits SwapAmountCannotBeZero. That is a revert rather than a wrong number. A test records the current behavior so a future change to it is visible. Reverting src/lens/V4Quoter.sol fails 8 of the 10 new tests. Four fail with "did not revert as expected", which is the wrapped value being returned as a valid quote, and the type(int128).min case fails with an arithmetic overflow panic. Snapshots regenerated with Foundry v1.3.6 to match the version CI pins. (cherry picked from commit 8d163f1)
…e input V4Router._swapExactOutput breaks out of its reverse loop once a hop's input delta reaches zero, since a hook that fully funds the input leaves the upstream pools with nothing to produce. V4Quoter._quoteExactOutput never gained the mirror guard, so it propagated the zero into the preceding pool and called swap with an amountSpecified of zero, which PoolManager rejects. The QuoteSwap payload was then replaced by SwapAmountCannotBeZero on the way out, surfacing as UnexpectedRevertBytes instead of a zero-input quote. Routes the router executes for free were therefore unquotable, so quote-dependent integrations excluded valid subsidy paths. test_quoteExactOutput_fullyFundedHop_revertsSwapAmountCannotBeZero asserted the broken behavior, so it passed against the bug. It is replaced by test_quoteExactOutput_fullyFundedHop_quotesZeroInput, which pins the quote to zero and matches the router's test_exactOutput_multiHop_hookFundsFinalHop_skipsUpstreamHops. Costs 29 gas per exact-output hop. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 7a1a256)
An exact-input or exact-output swap with a zero-length path ran its loop zero times and completed without swapping anything, so a route could succeed while delivering no output. Both multi-hop helpers now revert with EmptyPath before touching the register or the delta. OZ 2.3.0 audit L-02.
The comment claimed a hook pool could deliver more output than requested. It cannot: v4-core folds a hook's specified-side delta into the amount it swaps and afterSwap can only adjust the unspecified side, so the caller's output is at most the request. OZ 2.3.0 audit N-01.
toBytes read the offset word for element `_arg` without checking that the word lies inside the slice, so a params blob shorter than its declared layout had its hookData offset read from whatever followed it in calldata. Check the head word in whole words before reading it, which also covers a `_arg` large enough to wrap the byte offset. The five PositionManager decoders that rely on toBytes are the callers affected. OZ 2.3.0 audit N-08.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Co-authored-by: Eric Zhong <eric.zhong@uniswap.org>
Co-authored-by: Eric Zhong <eric.zhong@uniswap.org>
zhongeric
approved these changes
Oct 1, 2026
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
5 participants
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.
What
Fixes for the v4-periphery side of OpenZeppelin's Universal Router v2.3.0 audit (report dated 2026-09-21), one commit per finding, stacked on
dev-ur-2.3.0._swapExactInputand_swapExactOutputrevert withEmptyPathon a zero-length path instead of completing without a swapafterSwaponly touches the unspecified side-x: SafeCast on the delta conversions and the reverse-traversal break when a hook funds the whole inputCalldataDecoder.toByteschecks that the head word for element_arglies inside the slice before reading it, in whole words so a huge_argcannot wrapNotes
toBytes.main; with its commits carried here it can be closed once this merges, or retargeted if you would rather keep its review.