Skip to content

fix(audit): resolve sherlock contest findings against 2.3.0 - #531

Open
ccashwell wants to merge 5 commits into
dev-2.3.0from
fix/contest-findings-2.3.0
Open

ccashwell wants to merge 5 commits into
dev-2.3.0from
fix/contest-findings-2.3.0

Conversation

@ccashwell

Copy link
Copy Markdown
Member

Fixes for four of the eight contest findings filed against dev-2.3.0 at 26f89e6, one commit each with a regression test that fails on the unfixed code, plus README text for the new behavior and regenerated Hardhat gas snapshots. The OZ findings were already resolved on the base. The other four findings are covered at the end: three acknowledged without code changes, one a comment fix in v4-periphery.

What's in it

Finding Commit Change
7 9108e53 V4_POSITION_MANAGER_CALL refuses to run inside a foreign PoolManager unlock. The command only admits modifyLiquidities, which opens its own unlock, so inside an unlock someone else opened it could only fail with AlreadyUnlocked. That failure fell under FLAG_ALLOW_REVERT, so a plan built with the flag completed with the leg skipped and the route's funds left in the position manager: the L-01 shape, which the 2.3.0 gate closed for V4_SWAP but not here. The branch now reads poolManager.isUnlocked() after the calldata check and reverts NestedExecutionNotPermitted in the router's own frame, where the flag does not reach. The existing EXECUTE_SUB_PLAN re-throw carries it out of nested plans.
11 69ebd2b PERMIT2_TRANSFER_FROM_BATCH maps each detail's to through _mapRecipient. The batch passed recipients straight to Permit2, so the MSG_SENDER and ADDRESS_THIS sentinels every other recipient field honors sent tokens to address 1 and address 2 instead. Permit2Payments sits below the contract that defines the mapper, so the per-detail loop moves into the Dispatcher branch beside the single-transfer command, and the batch overload in Permit2Payments is removed with its imports.
8 669b869 A gas-starved command failure is fatal regardless of FLAG_ALLOW_REVERT. A call-based command fails with no returndata when its callee runs out of gas, and the flag tolerated that like any other failure, even though the gas limit is the one input a route's submitter controls. The execute loop snapshots gasleft() before dispatch and, on failure, reverts InsufficientGas when the frame holds under 1/64 of it, the share EIP-150 leaves behind a callee that ran dry (the check OpenZeppelin's forwarder uses). It runs before the required-command check on purpose: after it, a starved required command inside an allow-revert sub-plan would surface as a plain ExecutionFailed that the parent frame, still holding about 2/64 of its own gas, would swallow. EXECUTE_SUB_PLAN re-throws InsufficientGas the same way it re-throws the nested-execution gate. Pre-existing since v1.
1 6247453 _v2Swap measures delivery across the final hop only. The recipient's balance was snapshotted before the first hop, so when the recipient is itself a pair the path passes through, the tokens an earlier hop routed into it counted toward amountOutMinimum. The snapshot now happens inside the loop at the penultimate pair, right before the final swap; the check after the loop is unchanged. Pre-existing since v1.
2a9bdac Regenerated Hardhat gas snapshots.

The README documents the failures the flag never swallows, the batch recipient sentinels, and the final-hop measurement.

Alternatives considered
  • 7. Decoding the returned AlreadyUnlocked selector and re-throwing costs more bytes and ties the router to the position manager's error. Making the command ignore the flag altogether breaks plans that legitimately tolerate a failed mint at top level. Gating on the nested opt-in instead of the unlock state is wrong, since an opted-in route still cannot run it. The gate sits after the calldata check so a malformed input still reports the slice error first, and a chain without a pool manager never reads one before validating the input.
  • 11. Copying the array to memory and rewriting recipients before calling Permit2's own batch function costs a memory copy and more bytes. Redefining the mapper lower in the hierarchy clashes with the periphery's definition on linearization.
  • 8. Reverting with invalid() saves 12 bytes and burns the submitter's remaining gas, which is what OpenZeppelin does, but gives integrators nothing to decode. Checking only on the tolerated path leaves the sub-plan hole above. A per-command gas stipend or a minimum-gas field in the plan changes the format and the SDK.
  • 1. Rejecting recipients that are pairs on the path needs a scan per hop and blocks legitimate pair-delivery composition. Trusting the pair-computed output instead of the balance delta reopens the fee-on-transfer gap the delivery check exists to close.

Not in this branch

  • 5, a permissioned fee-on-transfer underlying consuming the fee adapter's verification surplus. Acknowledged: the token and the surplus are the adapter owner's own, fee-on-transfer tokens are unsupported, and any fix belongs in the adapter.
  • 6, a buy-tax intermediate in a v3 multi-hop spending other router-held balance. Acknowledged: pre-existing since v1 and shared with SwapRouter02, and fee-on-transfer tokens are unsupported.
  • 10, aggregate v4 deltas at or above 2^127 reverting SafeCastOverflow. Acknowledged: core bounds each operation to int128, the amounts exceed any supply, and a plan can split.
  • 4, the PositionInfoLibrary field list naming tickUpper before tickLower while claiming LSB order. Fixed in v4-periphery as a comment-only commit, a14b2e80 on fix/position-info-layout-comment off periphery main. The file is identical on the 7ed84398 pin, so the submodule pointer here is unchanged.

Bytecode

UniversalRouter runtime, from snapshots/UniversalRouterTest.json at each commit:

Runtime Free under 24,576
dev-2.3.0 (26f89e6) 24,121 455
this branch 24,065 511

Gas

66 Hardhat snapshot entries changed, median 13 gas lower. PositionManager mint paths pay about 1.0k to 1.3k more for the isUnlocked read; long multi-swap plans are down by as much as 1.7k.

Testing

New Foundry tests, each run against the unfixed code first and failing there:

  • V4PositionManagerCallWithinUnlock.t.sol: deploys a real PositionManager from its artifact, since importing it clashes with the per-file optimizer_runs restrictions in foundry.toml. A top-level mint succeeds as a control; inside a foreign unlock the command reverts with the flag clear, with the flag set, and with the flag set on an enclosing sub-plan.
  • Permit2TransferFromBatch.t.sol: ADDRESS_THIS delivers to the router, MSG_SENDER to the caller, and a detail whose from is not the caller reverts FromAddressIsNotOwner.
  • GasStarvation.t.sol: a RESOLVE against a resolver that burns all forwarded gas, under a 1M gas limit, reverts InsufficientGas as a required command, as an allow-revert command, and as either inside an allow-revert sub-plan. Two controls show ordinary failures still follow the flag.
  • V2RecipientPairAlias.t.sol: fork test at block 20010000 against the real v2 factory with four mock tokens, path A, B, C, D, B, delivering to the B-C pair. A minimum one wei above the final hop's output reverts V2TooLittleReceived; the exact final-hop output passes; an ordinary recipient receives the final hop's output.

modifyLiquidities opens its own PoolManager unlock, so when a route runs
inside an unlock someone else opened the call can only fail with
AlreadyUnlocked, and FLAG_ALLOW_REVERT would turn that into a silent skip
of a leg whose funds are already parked in the position manager. Gate the
command on isUnlocked in the router's own frame, where the flag does not
reach, and let the existing sub-plan re-throw carry the error out.
…able

PERMIT2_TRANSFER_FROM_BATCH passed each detail's `to` straight to Permit2,
so the MSG_SENDER and ADDRESS_THIS sentinels every other recipient field
honours sent tokens to address(1) and address(2) instead. Permit2Payments
cannot reach _mapRecipient, so move the per-detail loop into the
Dispatcher branch beside the single-transfer command and map there.
A command built on an external call fails with no returndata when its
callee runs out of gas, and FLAG_ALLOW_REVERT tolerated that like any
other failure even though the transaction's gas limit is the submitter's
choice. Snapshot gasleft before dispatch and, on failure, revert with
InsufficientGas when the frame holds under 1/64 of it, the share EIP-150
leaves behind a callee that ran dry. The check runs before the
required-command check: after it, a starved required command inside an
allow-revert sub-plan would surface as a plain ExecutionFailed that the
parent swallows. EXECUTE_SUB_PLAN re-throws InsufficientGas the same way
it re-throws the nested-execution gate.
The recipient's balance was snapshotted before the first hop, so when the
recipient is itself a pair the path passes through, the tokens an earlier
hop routed into it counted toward amountOutMinimum along with the final
hop's output. Take the snapshot right before the final hop instead, so a
route whose output token reappears mid-path cannot satisfy its minimum
with its own intermediate transfer.
The gas-starvation check adds a gasleft read and a comparison per
command, the batch transfer loop moved into the Dispatcher, and the v2
delivery snapshot moved inside the hop loop. Regenerated with forge
v1.4.3 on the path, the version CI pins, since forge 1.8.3 emits
context remappings that hardhat-foundry 1.1.2 cannot parse.
@ccashwell
ccashwell requested a review from a team as a code owner October 30, 2026 16:39
@github-actions

Copy link
Copy Markdown

🤖 Claude PR Metadata Generation

Status: ❌ Error
Job: View workflow run


⚠️ Generation Failed

The automated PR title and description generation encountered an error.

Next Steps:

  1. Check the workflow logs for details
  2. Verify the GitHub Actions configuration
  3. Ensure the Anthropic API key is valid

You can manually set the PR title and description, or re-trigger by pushing a new commit.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant