Skip to content

Commit d1ba1e1

Browse files
dianakocsisclaude
andcommitted
fix: gate nested V4_SWAP behind an explicit opt-in entrypoint
#491 ran V4 actions inside a foreign PoolManager unlock whenever poolManager.isUnlocked() was true, inferring consent from chain state. All V4 deltas accrue under the router's address for the lifetime of that unlock while the router's own locker clears between external calls, so one call could leave unpaid debt that a later call's SETTLE_ALL or OPEN_DELTA settlement paid from a different caller's funds (audit finding M-01). Consent now lives on the entrypoint. executeNested sets a transient flag for the duration of the call and the nested branch requires it, so only a caller that opened the surrounding unlock can reach the path. It cannot live in the payload: EXECUTE_SIGNED_TYPEHASH commits to keccak256(commands), and a command byte would let the route author, rather than the composer whose capital is at risk, make the decision. Plain execute now reverts NestedExecutionNotPermitted instead. Signed routes cannot nest in this version. executeSignedNested would restore that, but costs 252 bytes against 473 remaining, and the RESOLVE proposal needs the same headroom. Documented on executeNested as a deliberate omission. Also carries the calldata re-encode (BP-1/2/3): the v4 parameter decoders follow struct offsets without bounding them against the enclosing input, so forwarding a slice of transaction calldata let them read bytes the signature never covered. executeV4SwapWithinUnlock re-enters through the ABI encoder so calldata ends where the input ends. This stays load-bearing under the opt-in, since an attacker can opt themselves in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent fb25ff0 commit d1ba1e1

12 files changed

Lines changed: 529 additions & 71 deletions

contracts/UniversalRouter.sol

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {PaymentsImmutables, PaymentsParameters} from './modules/PaymentsImmutabl
99
import {UniswapImmutables, UniswapParameters} from './modules/uniswap/UniswapImmutables.sol';
1010
import {V4SwapRouter} from './modules/uniswap/v4/V4SwapRouter.sol';
1111
import {Commands} from './libraries/Commands.sol';
12+
import {NestedUnlock} from './libraries/NestedUnlock.sol';
1213
import {IUniversalRouter} from './interfaces/IUniversalRouter.sol';
1314
import {MigratorImmutables, MigratorParameters} from './modules/MigratorImmutables.sol';
1415
import {EIP712} from '@openzeppelin/contracts/utils/cryptography/EIP712.sol';
@@ -66,6 +67,17 @@ contract UniversalRouter is IUniversalRouter, ChainedActions, RouteSigner, Dispa
6667
_resetSignatureContext();
6768
}
6869

70+
/// @inheritdoc IUniversalRouter
71+
function executeNested(bytes calldata commands, bytes[] calldata inputs, uint256 deadline)
72+
external
73+
payable
74+
checkDeadline(deadline)
75+
{
76+
NestedUnlock.set(true);
77+
execute(commands, inputs);
78+
NestedUnlock.set(false);
79+
}
80+
6981
/// @inheritdoc Dispatcher
7082
function execute(bytes calldata commands, bytes[] calldata inputs) public payable override isNotLocked {
7183
bool success;

contracts/base/Dispatcher.sol

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {V4SwapRouter} from '../modules/uniswap/v4/V4SwapRouter.sol';
77
import {BytesLib} from '../modules/uniswap/v3/BytesLib.sol';
88
import {Payments} from '../modules/Payments.sol';
99
import {Constants} from '../libraries/Constants.sol';
10+
import {NestedUnlock} from '../libraries/NestedUnlock.sol';
1011
import {PaymentsImmutables} from '../modules/PaymentsImmutables.sol';
1112
import {V3ToV4Migrator} from '../modules/V3ToV4Migrator.sol';
1213
import {Commands} from '../libraries/Commands.sol';
@@ -37,12 +38,29 @@ abstract contract Dispatcher is
3738

3839
error InvalidCommandType(uint256 commandType);
3940
error BalanceTooLow();
41+
error NotSelf();
42+
/// @notice Thrown when a route reaches V4_SWAP inside a foreign PoolManager unlock without opting in
43+
error NestedExecutionNotPermitted();
4044

4145
/// @notice Executes encoded commands along with provided inputs.
4246
/// @param commands A set of concatenated commands, each 1 byte in length
4347
/// @param inputs An array of byte strings containing abi encoded inputs for each command
4448
function execute(bytes calldata commands, bytes[] calldata inputs) external payable virtual;
4549

50+
/// @notice Runs the v4 actions of a V4_SWAP command inside a PoolManager unlock opened elsewhere
51+
/// @param inputs The V4_SWAP command input, re-encoded by the dispatcher
52+
/// @dev Only callable by this contract. It exists so that `inputs` arrives as a freshly abi encoded
53+
/// calldata copy rather than as a slice of the original transaction calldata. The v4 parameter decoders
54+
/// follow offsets without bounding them against the enclosing input, so a slice would let them read
55+
/// bytes past `inputs.length` -- bytes that executeSigned never commits to. Encoding the argument makes
56+
/// calldata end where the input ends, which is the same guarantee poolManager.unlock() gives the
57+
/// ordinary path when it re-encodes for unlockCallback.
58+
function executeV4SwapWithinUnlock(bytes calldata inputs) external {
59+
if (msg.sender != address(this)) revert NotSelf();
60+
(bytes calldata actions, bytes[] calldata params) = inputs.decodeActionsRouterParams();
61+
_executeActionsWithoutUnlock(actions, params);
62+
}
63+
4664
/// @notice Public view function to be used instead of msg.sender, as the contract performs self-reentrancy and at
4765
/// times msg.sender == address(this). Instead msgSender() returns the initiator of the lock
4866
/// @dev overrides BaseActionsRouter.msgSender in V4Router
@@ -299,8 +317,22 @@ abstract contract Dispatcher is
299317
// contract's unlock callback. Opening a new lock via _executeActions would revert with
300318
// AlreadyUnlocked, so instead run the v4 actions within the existing lock.
301319
if (poolManager.isUnlocked()) {
302-
(bytes calldata actions, bytes[] calldata params) = inputs.decodeActionsRouterParams();
303-
_executeActionsWithoutUnlock(actions, params);
320+
// Nesting shares the router's delta account with every other call in this
321+
// unlock, so it runs only when the caller explicitly opted in via executeNested.
322+
if (!NestedUnlock.isPermitted()) revert NestedExecutionNotPermitted();
323+
// Re-enter externally so the abi encoder produces a canonical, self-contained copy of
324+
// `inputs`. Decoding it in place leaves it a slice of the original transaction calldata,
325+
// where the v4 parameter decoders can follow offsets past inputs.length into bytes the
326+
// signature never covered. See executeV4SwapWithinUnlock.
327+
(bool ok, bytes memory reason) =
328+
address(this).call(abi.encodeCall(Dispatcher.executeV4SwapWithinUnlock, (inputs)));
329+
// bubble unconditionally: the _executeActions path below propagates reverts regardless
330+
// of FLAG_ALLOW_REVERT, and routing through a call must not quietly change that
331+
if (!ok) {
332+
assembly ('memory-safe') {
333+
revert(add(reason, 0x20), mload(reason))
334+
}
335+
}
304336
} else {
305337
// pass the calldata provided to V4SwapRouter._executeActions (defined in BaseActionsRouter)
306338
_executeActions(inputs);

contracts/interfaces/IUniversalRouter.sol

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,24 @@ interface IUniversalRouter {
6565
uint256 deadline
6666
) external payable;
6767

68+
/// @notice Executes encoded commands, opting in to running inside a PoolManager unlock opened by
69+
/// another contract
70+
/// @param commands A set of concatenated commands, each 1 byte in length
71+
/// @param inputs An array of byte strings containing abi encoded inputs for each command
72+
/// @param deadline The deadline by which the transaction must be executed
73+
/// @dev Use this ONLY from a contract that itself opened the surrounding PoolManager unlock. V4 deltas
74+
/// accrue under the router's address for the lifetime of that unlock, not per call, so an unsettled
75+
/// delta left by one call can be paid by a later call's `SETTLE_ALL` or `OPEN_DELTA` settlement, out of
76+
/// that later caller's funds. Opting in accepts responsibility for that: a contract exposing a
77+
/// permissionless function that reaches this entrypoint can have its own capital drained. If a function
78+
/// does not open its own unlock, call `execute` instead, which refuses to run nested.
79+
/// @dev Signed routes cannot nest in this version. `executeSigned` does not set the opt-in, so a signed
80+
/// route reaching V4_SWAP inside a foreign unlock reverts NestedExecutionNotPermitted. This is a
81+
/// deliberate omission for bytecode headroom, not a security boundary: a composer that needs a hook to
82+
/// authorize on the signer rather than on msgSender() has no way to express that here. Restoring it
83+
/// requires an `executeSignedNested` entrypoint and therefore a new router deployment.
84+
function executeNested(bytes calldata commands, bytes[] calldata inputs, uint256 deadline) external payable;
85+
6886
/// @notice Returns all signed execution context (signer, intent, data) in a single call
6987
/// @return signer The address that signed the current execution, or address(0) if not in a signed execution
7088
/// @return intent The intent value from the signed execution, or bytes32(0) if not in a signed execution
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// SPDX-License-Identifier: GPL-3.0-or-later
2+
pragma solidity ^0.8.24;
3+
4+
/// @notice A library to record, in transient storage, that the caller has opted into executing a
5+
/// route inside a PoolManager unlock opened by another contract.
6+
/// @dev Set for the duration of an executeNested / executeSignedNested call only. Deltas accrue under
7+
/// the router's address for the whole of the surrounding unlock, so an opted-in caller is responsible
8+
/// for the delta hygiene the router can no longer guarantee on its behalf. See IUniversalRouter.
9+
/// TODO: This library can be deleted when we have the transient keyword support in solidity.
10+
library NestedUnlock {
11+
// The slot holding the opt-in state, transiently. bytes32(uint256(keccak256("NestedUnlock")) - 1)
12+
bytes32 constant NESTED_UNLOCK_SLOT = 0x904918ff601603e47e61e77f6dc30e7705c7e82b0e1bf84efb9b137e90ac8643;
13+
14+
function set(bool permitted) internal {
15+
assembly ('memory-safe') {
16+
tstore(NESTED_UNLOCK_SLOT, permitted)
17+
}
18+
}
19+
20+
function isPermitted() internal view returns (bool permitted) {
21+
assembly ('memory-safe') {
22+
permitted := tload(NESTED_UNLOCK_SLOT)
23+
}
24+
}
25+
}

snapshots/UniversalRouterTest.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11
{
2-
"UniversalRouter bytecode size": "23705"
2+
"UniversalRouter bytecode size": "24103"
33
}

0 commit comments

Comments
 (0)