From 73e00a0d48d9740d8ab95ac1fd90e615472cf5f7 Mon Sep 17 00:00:00 2001 From: Chris Cashwell Date: Thu, 29 Oct 2026 16:51:55 -0400 Subject: [PATCH 1/5] fix(dispatcher): refuse V4_POSITION_MANAGER_CALL inside a foreign unlock 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. --- README.md | 2 +- contracts/base/Dispatcher.sol | 7 +- snapshots/UniversalRouterTest.json | 2 +- .../V4PositionManagerCallWithinUnlock.t.sol | 171 ++++++++++++++++++ 4 files changed, 179 insertions(+), 3 deletions(-) create mode 100644 test/foundry-tests/V4PositionManagerCallWithinUnlock.t.sol diff --git a/README.md b/README.md index ba16b078..1689ea95 100644 --- a/README.md +++ b/README.md @@ -48,7 +48,7 @@ Each command is a `bytes1` containing the following 8 bits: └─┴─┴───────────┘ ``` -- `f` is a single bit flag, that signals whether or not the command should be allowed to revert. If `f` is `false`, and the command reverts, then the entire transaction will revert. If `f` is `true` and the command reverts then the transaction will continue, allowing us to achieve partial fills. The flag only has an effect on commands built on an external call: `PERMIT2_PERMIT`, `PERMIT2_PERMIT_BATCH`, `BALANCE_CHECK_ERC20`, `V3_POSITION_MANAGER_PERMIT`, `V3_POSITION_MANAGER_CALL`, `V4_INITIALIZE_POOL`, `V4_POSITION_MANAGER_CALL`, `RESOLVE`, `V4_PROTOCOL_FEE_UPDATE`, `V3_PROTOCOL_FEE_UPDATE` and `EXECUTE_SUB_PLAN`. Swaps, transfers, wraps, sweeps and the Across deposit run as internal calls and revert the whole transaction regardless of the flag; to tolerate their failure, wrap them in an `EXECUTE_SUB_PLAN` that sets it. One failure is never swallowed: a sub-plan that trips the nested-execution gate (`NestedExecutionNotPermitted`) reverts the transaction even with the flag set. If using this flag, be careful to include further commands that will remove any funds that could be left unused in the `UniversalRouter` contract. +- `f` is a single bit flag, that signals whether or not the command should be allowed to revert. If `f` is `false`, and the command reverts, then the entire transaction will revert. If `f` is `true` and the command reverts then the transaction will continue, allowing us to achieve partial fills. The flag only has an effect on commands built on an external call: `PERMIT2_PERMIT`, `PERMIT2_PERMIT_BATCH`, `BALANCE_CHECK_ERC20`, `V3_POSITION_MANAGER_PERMIT`, `V3_POSITION_MANAGER_CALL`, `V4_INITIALIZE_POOL`, `V4_POSITION_MANAGER_CALL`, `RESOLVE`, `V4_PROTOCOL_FEE_UPDATE`, `V3_PROTOCOL_FEE_UPDATE` and `EXECUTE_SUB_PLAN`. Swaps, transfers, wraps, sweeps and the Across deposit run as internal calls and revert the whole transaction regardless of the flag; to tolerate their failure, wrap them in an `EXECUTE_SUB_PLAN` that sets it. Some failures are never swallowed, with the flag set or through any `EXECUTE_SUB_PLAN`: the nested-execution gate (`NestedExecutionNotPermitted`), which `V4_POSITION_MANAGER_CALL` also trips inside any open PoolManager unlock because `modifyLiquidities` must open its own. If using this flag, be careful to include further commands that will remove any funds that could be left unused in the `UniversalRouter` contract. - `r` is one bit of reserved space. This will allow us to increase the space used for commands, or add new flags in future. diff --git a/contracts/base/Dispatcher.sol b/contracts/base/Dispatcher.sol index 66ddc2ea..0e61ebca 100644 --- a/contracts/base/Dispatcher.sol +++ b/contracts/base/Dispatcher.sol @@ -43,7 +43,8 @@ abstract contract Dispatcher is error InvalidCommandType(uint256 commandType); error BalanceTooLow(); - /// @notice Thrown when a route reaches V4_SWAP inside a foreign PoolManager unlock without opting in + /// @notice Thrown when a route reaches V4_SWAP inside a foreign PoolManager unlock without opting in, or + /// V4_POSITION_MANAGER_CALL inside any unlock (modifyLiquidities must open its own) error NestedExecutionNotPermitted(); /// @notice Executes encoded commands along with provided inputs. @@ -373,6 +374,10 @@ abstract contract Dispatcher is } else if (command == Commands.V4_POSITION_MANAGER_CALL) { // should only call modifyLiquidities() to mint _checkV4PositionManagerCall(inputs); + // modifyLiquidities opens its own unlock, so inside a foreign unlock it can only fail with + // AlreadyUnlocked. Fail loudly here instead, so FLAG_ALLOW_REVERT cannot turn an outsider's + // unlock into a silent skip of a leg that may already have funds parked in the position manager. + if (poolManager.isUnlocked()) revert NestedExecutionNotPermitted(); (success, output) = address(V4_POSITION_MANAGER).call{value: address(this).balance}(inputs); } else if (command == Commands.RESOLVE) { checkInputLength(inputs, 0x40); diff --git a/snapshots/UniversalRouterTest.json b/snapshots/UniversalRouterTest.json index c202fb48..c071a00c 100644 --- a/snapshots/UniversalRouterTest.json +++ b/snapshots/UniversalRouterTest.json @@ -1,3 +1,3 @@ { - "UniversalRouter bytecode size": "24121" + "UniversalRouter bytecode size": "24193" } \ No newline at end of file diff --git a/test/foundry-tests/V4PositionManagerCallWithinUnlock.t.sol b/test/foundry-tests/V4PositionManagerCallWithinUnlock.t.sol new file mode 100644 index 00000000..54d31b5a --- /dev/null +++ b/test/foundry-tests/V4PositionManagerCallWithinUnlock.t.sol @@ -0,0 +1,171 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.24; + +import {Test} from 'forge-std/Test.sol'; +import {Deployers} from '@uniswap/v4-core/test/utils/Deployers.sol'; +import {IPoolManager} from '@uniswap/v4-core/src/interfaces/IPoolManager.sol'; +import {IUnlockCallback} from '@uniswap/v4-core/src/interfaces/callback/IUnlockCallback.sol'; +import {Currency} from '@uniswap/v4-core/src/types/Currency.sol'; +import {IHooks} from '@uniswap/v4-core/src/interfaces/IHooks.sol'; +import {MockERC20} from 'solmate/src/test/utils/mocks/MockERC20.sol'; + +import {Plan, Planner} from '@uniswap/v4-periphery/test/shared/Planner.sol'; +import {Actions} from '@uniswap/v4-periphery/src/libraries/Actions.sol'; +import {ActionConstants} from '@uniswap/v4-periphery/src/libraries/ActionConstants.sol'; +import {IPositionManager} from '@uniswap/v4-periphery/src/interfaces/IPositionManager.sol'; +import {ERC721} from 'solmate/src/tokens/ERC721.sol'; + +import {UniversalRouter} from '../../contracts/UniversalRouter.sol'; +import {RouterParameters} from '../../contracts/types/RouterParameters.sol'; +import {Commands} from '../../contracts/libraries/Commands.sol'; +import {Dispatcher} from '../../contracts/base/Dispatcher.sol'; + +/// @notice Opens the PoolManager lock and runs a router plan from inside the callback, the shape of a contract +/// that composes router commands within its own unlock. +contract PositionManagerLockHolder is IUnlockCallback { + IPoolManager public immutable manager; + UniversalRouter public immutable router; + + bytes internal commands; + bytes[] internal inputs; + + constructor(IPoolManager _manager, UniversalRouter _router) { + manager = _manager; + router = _router; + } + + function executeWithinUnlock(bytes calldata _commands, bytes[] calldata _inputs) external { + commands = _commands; + inputs = _inputs; + manager.unlock(hex''); + } + + function unlockCallback(bytes calldata) external returns (bytes memory) { + require(msg.sender == address(manager), 'only manager'); + router.executeNested(commands, inputs, block.timestamp); + return hex''; + } +} + +/// @notice V4_POSITION_MANAGER_CALL may only reach modifyLiquidities, which opens its own PoolManager unlock, so +/// inside a foreign unlock the call can only fail with AlreadyUnlocked. The router refuses it up front instead, +/// so FLAG_ALLOW_REVERT cannot turn that failure into a silent skip that strands the funds the route parked in +/// the position manager. +contract V4PositionManagerCallWithinUnlockTest is Test, Deployers { + address constant USER = address(0xA11CE); + uint256 constant FUNDING = 1e18; + uint256 constant LIQUIDITY = 1e18; + int24 constant TICK_LOWER = -600; + int24 constant TICK_UPPER = 600; + + UniversalRouter router; + IPositionManager posm; + PositionManagerLockHolder lockHolder; + + function setUp() public { + deployFreshManagerAndRouters(); + (currency0, currency1) = deployMintAndApprove2Currencies(); + (key,) = initPoolAndAddLiquidity(currency0, currency1, IHooks(address(0)), 3000, SQRT_PRICE_1_1); + + posm = _deployPositionManager(); + + router = new UniversalRouter( + RouterParameters({ + permit2: address(0), + weth9: address(0), + v2Factory: address(0), + v3Factory: address(0), + pairInitCodeHash: bytes32(0), + poolInitCodeHash: bytes32(0), + v4PoolManager: address(manager), + permissionsAdapterFactory: address(0), + v3NFTPositionManager: address(0), + v4PositionManager: address(posm), + spokePool: address(0) + }) + ); + lockHolder = new PositionManagerLockHolder(IPoolManager(address(manager)), router); + + // the route pays the mint from the position manager's own balance, the shape the v3 to v4 migration uses + MockERC20(Currency.unwrap(currency0)).transfer(address(posm), FUNDING); + MockERC20(Currency.unwrap(currency1)).transfer(address(posm), FUNDING); + } + + /// @dev PositionManager.sol is pinned to its own optimizer profile in foundry.toml, which cannot share a + /// compilation job with UniversalRouter.sol, so the test deploys it from the compiled artifact instead of + /// importing it. The permit2, descriptor and WETH9 arguments are unused by a mint paid from its own balance. + function _deployPositionManager() internal returns (IPositionManager deployed) { + bytes memory initcode = abi.encodePacked( + vm.getCode('PositionManager.sol:PositionManager'), + abi.encode(address(manager), address(0), uint256(100_000), address(0), address(0)) + ); + address addr; + assembly ('memory-safe') { + addr := create(0, add(initcode, 0x20), mload(initcode)) + } + require(addr != address(0), 'position manager deploy failed'); + deployed = IPositionManager(addr); + } + + /// @dev a mint paid from the position manager's balance, sweeping whatever the mint did not use back to USER + function _mintCommand(bool allowRevert) internal view returns (bytes memory commands, bytes[] memory inputs) { + Plan memory plan = Planner.init(); + plan = plan.add( + Actions.MINT_POSITION, + abi.encode(key, TICK_LOWER, TICK_UPPER, LIQUIDITY, type(uint128).max, type(uint128).max, USER, new bytes(0)) + ); + plan = plan.add(Actions.SETTLE, abi.encode(currency0, ActionConstants.OPEN_DELTA, false)); + plan = plan.add(Actions.SETTLE, abi.encode(currency1, ActionConstants.OPEN_DELTA, false)); + plan = plan.add(Actions.SWEEP, abi.encode(currency0, USER)); + plan = plan.add(Actions.SWEEP, abi.encode(currency1, USER)); + + bytes1 command = bytes1(uint8(Commands.V4_POSITION_MANAGER_CALL)); + if (allowRevert) command |= Commands.FLAG_ALLOW_REVERT; + commands = abi.encodePacked(command); + inputs = new bytes[](1); + inputs[0] = abi.encodeCall(IPositionManager.modifyLiquidities, (plan.encode(), block.timestamp)); + } + + function _subPlan(bytes memory sub, bytes[] memory subInputs, bool allowRevert) + internal + pure + returns (bytes memory commands, bytes[] memory inputs) + { + bytes1 command = bytes1(uint8(Commands.EXECUTE_SUB_PLAN)); + if (allowRevert) command |= Commands.FLAG_ALLOW_REVERT; + commands = abi.encodePacked(command); + inputs = new bytes[](1); + inputs[0] = abi.encode(sub, subInputs); + } + + /// @notice control: the same calldata mints at the top level, so below it is only the gate that intervenes + function test_v4PositionManagerCall_topLevel_mints() public { + (bytes memory commands, bytes[] memory inputs) = _mintCommand(false); + router.execute(commands, inputs); + + assertEq(ERC721(address(posm)).ownerOf(1), USER); + assertEq(MockERC20(Currency.unwrap(currency0)).balanceOf(address(posm)), 0, 'currency0 swept'); + assertEq(MockERC20(Currency.unwrap(currency1)).balanceOf(address(posm)), 0, 'currency1 swept'); + } + + function test_v4PositionManagerCall_withinForeignUnlock_reverts() public { + (bytes memory commands, bytes[] memory inputs) = _mintCommand(false); + vm.expectRevert(Dispatcher.NestedExecutionNotPermitted.selector); + lockHolder.executeWithinUnlock(commands, inputs); + } + + /// @notice the gate fires in the router's own frame, so the flag that tolerates call failures cannot skip it + function test_v4PositionManagerCall_withinForeignUnlock_allowRevertCannotSkip() public { + (bytes memory commands, bytes[] memory inputs) = _mintCommand(true); + vm.expectRevert(Dispatcher.NestedExecutionNotPermitted.selector); + lockHolder.executeWithinUnlock(commands, inputs); + } + + /// @notice an allow-revert sub-plan re-throws the gate error instead of swallowing it + function test_v4PositionManagerCall_withinForeignUnlock_allowRevertSubPlanCannotSkip() public { + (bytes memory sub, bytes[] memory subInputs) = _mintCommand(false); + (bytes memory commands, bytes[] memory inputs) = _subPlan(sub, subInputs, true); + vm.expectRevert(Dispatcher.NestedExecutionNotPermitted.selector); + lockHolder.executeWithinUnlock(commands, inputs); + } +} From 14ae474579361d4d9c4e2355037308ac14c8f324 Mon Sep 17 00:00:00 2001 From: Chris Cashwell Date: Thu, 29 Oct 2026 16:57:39 -0400 Subject: [PATCH 2/5] fix(dispatcher): map batch transfer recipients through the sentinel table 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. --- README.md | 2 + contracts/base/Dispatcher.sol | 15 ++- contracts/modules/Permit2Payments.sol | 18 ---- snapshots/UniversalRouterTest.json | 2 +- .../Permit2TransferFromBatch.t.sol | 100 ++++++++++++++++++ 5 files changed, 117 insertions(+), 20 deletions(-) create mode 100644 test/foundry-tests/Permit2TransferFromBatch.t.sol diff --git a/README.md b/README.md index 1689ea95..42378bb9 100644 --- a/README.md +++ b/README.md @@ -118,6 +118,8 @@ Note that some of the commands in the middle of the series are unused. These gap `UNWRAP_WETH_EXACT` takes `(recipient, amount)` and unwraps an exact amount of the contract's WETH, reverting if the balance is insufficient. +`PERMIT2_TRANSFER_FROM` pulls from the caller, and every `PERMIT2_TRANSFER_FROM_BATCH` detail must name the caller as `from`. Both resolve their recipients through the same sentinel table as every other recipient field: `ActionConstants.MSG_SENDER` (address 1) delivers to the caller and `ActionConstants.ADDRESS_THIS` (address 2) to the router. + `V2_SWAP_EXACT_OUT` is all-or-nothing: it measures what the recipient actually receives and reverts with `V2TooLittleReceived` if that is less than `amountOut`, even by 1 wei. A token anywhere on the path that delivers less than the amount transferred, whether through a transfer fee, withholding, or share-based rounding such as stETH (which typically delivers 1-2 wei less), therefore makes the route revert. Swap such tokens with `V2_SWAP_EXACT_IN` instead. `V4_SWAP` takes an encoded v4 action plan `(bytes actions, bytes[] params)` and runs it through the router's `V4Router`. Exact-output swaps are all-or-nothing and revert on a shortfall. An exact-output swap paid from the router's own balance (`payerIsUser = false`, or a native input) spends only what the route actually costs, so whatever the plan pre-funded beyond that stays in the router. Such a plan must end with a full-balance return such as `SWEEP` for each input currency, since any balance left in the router can be swept by anyone. A hook that funds the swap's input itself is the extreme case: the route costs nothing, so the entire pre-funded amount stays in the router and must be swept back. diff --git a/contracts/base/Dispatcher.sol b/contracts/base/Dispatcher.sol index 0e61ebca..49ca6680 100644 --- a/contracts/base/Dispatcher.sol +++ b/contracts/base/Dispatcher.sol @@ -308,7 +308,20 @@ abstract contract Dispatcher is batchDetails.length := length batchDetails.offset := offset } - permit2TransferFrom(batchDetails, msgSender()); + // One detail at a time, so each amount can be mapped through the RESOLVE register and each + // recipient through the sentinel table without copying the array to memory. Permit2's own + // batch transferFrom performs the same per-detail transfers. + address owner = msgSender(); + for (uint256 i = 0; i < length; ++i) { + IAllowanceTransfer.AllowanceTransferDetails calldata details = batchDetails[i]; + if (details.from != owner) revert FromAddressIsNotOwner(); + permit2TransferFrom( + details.token, + owner, + _mapRecipient(details.to), + SafeCast160.toUint160(ResolvedAmount.map(details.amount)) + ); + } } else if (command == Commands.BALANCE_CHECK_ERC20) { checkInputLength(inputs, 0x60); // equivalent: abi.decode(inputs, (address, address, uint256)) diff --git a/contracts/modules/Permit2Payments.sol b/contracts/modules/Permit2Payments.sol index 46df5c1c..5ab6b846 100644 --- a/contracts/modules/Permit2Payments.sol +++ b/contracts/modules/Permit2Payments.sol @@ -1,10 +1,8 @@ // SPDX-License-Identifier: GPL-3.0-or-later pragma solidity ^0.8.24; -import {IAllowanceTransfer} from 'permit2/src/interfaces/IAllowanceTransfer.sol'; import {SafeCast160} from 'permit2/src/libraries/SafeCast160.sol'; import {Payments} from './Payments.sol'; -import {ResolvedAmount} from '../libraries/ResolvedAmount.sol'; /// @title Payments through Permit2 /// @notice Performs interactions with Permit2 to transfer tokens @@ -22,22 +20,6 @@ abstract contract Permit2Payments is Payments { PERMIT2.transferFrom(from, to, amount, token); } - /// @notice Performs a batch transferFrom on Permit2 - /// @dev Transfers one detail at a time so each amount can be mapped through the RESOLVE register without - /// copying the calldata array to memory. Permit2's batch transferFrom performs the same per-detail transfers. - /// @param batchDetails An array detailing each of the transfers that should occur - /// @param owner The address that should be the owner of all transfers - function permit2TransferFrom(IAllowanceTransfer.AllowanceTransferDetails[] calldata batchDetails, address owner) - internal - { - uint256 batchLength = batchDetails.length; - for (uint256 i = 0; i < batchLength; ++i) { - IAllowanceTransfer.AllowanceTransferDetails calldata details = batchDetails[i]; - if (details.from != owner) revert FromAddressIsNotOwner(); - permit2TransferFrom(details.token, owner, details.to, ResolvedAmount.map(details.amount).toUint160()); - } - } - /// @notice Either performs a regular payment or transferFrom on Permit2, depending on the payer address /// @param token The token to transfer /// @param payer The address to pay for the transfer diff --git a/snapshots/UniversalRouterTest.json b/snapshots/UniversalRouterTest.json index c071a00c..f74ad1fd 100644 --- a/snapshots/UniversalRouterTest.json +++ b/snapshots/UniversalRouterTest.json @@ -1,3 +1,3 @@ { - "UniversalRouter bytecode size": "24193" + "UniversalRouter bytecode size": "24201" } \ No newline at end of file diff --git a/test/foundry-tests/Permit2TransferFromBatch.t.sol b/test/foundry-tests/Permit2TransferFromBatch.t.sol new file mode 100644 index 00000000..3ade347d --- /dev/null +++ b/test/foundry-tests/Permit2TransferFromBatch.t.sol @@ -0,0 +1,100 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.24; + +import 'forge-std/Test.sol'; +import {DeployPermit2} from 'permit2/test/utils/DeployPermit2.sol'; +import {IAllowanceTransfer} from 'permit2/src/interfaces/IAllowanceTransfer.sol'; +import {ActionConstants} from '@uniswap/v4-periphery/src/libraries/ActionConstants.sol'; +import {UniversalRouter} from '../../contracts/UniversalRouter.sol'; +import {Commands} from '../../contracts/libraries/Commands.sol'; +import {RouterParameters} from '../../contracts/types/RouterParameters.sol'; +import {Permit2Payments} from '../../contracts/modules/Permit2Payments.sol'; +import {MockERC20} from './mock/MockERC20.sol'; + +/// @notice PERMIT2_TRANSFER_FROM_BATCH resolves each detail's recipient through the same sentinel table as every +/// other recipient field, so MSG_SENDER and ADDRESS_THIS deliver to the caller and the router rather than to the +/// literal addresses 1 and 2. +contract Permit2TransferFromBatchTest is Test, DeployPermit2 { + address constant RECIPIENT = address(1234); + uint160 constant AMOUNT = 5 ether; + + UniversalRouter router; + MockERC20 erc20; + address user = makeAddr('user'); + address other = makeAddr('other'); + + function setUp() public { + IAllowanceTransfer permit2 = IAllowanceTransfer(deployPermit2()); + router = new UniversalRouter( + RouterParameters({ + permit2: address(permit2), + weth9: address(0), + v2Factory: address(0), + v3Factory: address(0), + pairInitCodeHash: bytes32(0), + poolInitCodeHash: bytes32(0), + v4PoolManager: address(0), + permissionsAdapterFactory: address(0), + v3NFTPositionManager: address(0), + v4PositionManager: address(0), + spokePool: address(0) + }) + ); + erc20 = new MockERC20(); + + erc20.mint(user, 4 * AMOUNT); + vm.startPrank(user); + erc20.approve(address(permit2), type(uint256).max); + permit2.approve(address(erc20), address(router), type(uint160).max, type(uint48).max); + vm.stopPrank(); + } + + function _batch(address from, address to0, address to1) + internal + view + returns (bytes memory commands, bytes[] memory inputs) + { + IAllowanceTransfer.AllowanceTransferDetails[] memory details = + new IAllowanceTransfer.AllowanceTransferDetails[](2); + details[0] = + IAllowanceTransfer.AllowanceTransferDetails({from: from, to: to0, amount: AMOUNT, token: address(erc20)}); + details[1] = + IAllowanceTransfer.AllowanceTransferDetails({from: from, to: to1, amount: AMOUNT, token: address(erc20)}); + + commands = abi.encodePacked(bytes1(uint8(Commands.PERMIT2_TRANSFER_FROM_BATCH))); + inputs = new bytes[](1); + inputs[0] = abi.encode(details); + } + + function test_permit2TransferFromBatch_addressThisRecipient_deliversToRouter() public { + (bytes memory commands, bytes[] memory inputs) = _batch(user, ActionConstants.ADDRESS_THIS, RECIPIENT); + + vm.prank(user); + router.execute(commands, inputs); + + assertEq(erc20.balanceOf(address(router)), AMOUNT, 'ADDRESS_THIS maps to the router'); + assertEq(erc20.balanceOf(RECIPIENT), AMOUNT, 'a literal recipient is untouched'); + assertEq(erc20.balanceOf(ActionConstants.ADDRESS_THIS), 0, 'nothing reaches address(2)'); + assertEq(erc20.balanceOf(user), 2 * AMOUNT); + } + + function test_permit2TransferFromBatch_msgSenderRecipient_deliversToCaller() public { + (bytes memory commands, bytes[] memory inputs) = _batch(user, ActionConstants.MSG_SENDER, RECIPIENT); + + vm.prank(user); + router.execute(commands, inputs); + + // the MSG_SENDER detail round-trips to the caller, so only the literal detail leaves the user's balance + assertEq(erc20.balanceOf(user), 3 * AMOUNT, 'MSG_SENDER maps to the caller'); + assertEq(erc20.balanceOf(RECIPIENT), AMOUNT); + assertEq(erc20.balanceOf(ActionConstants.MSG_SENDER), 0, 'nothing reaches address(1)'); + } + + function test_permit2TransferFromBatch_fromIsNotCaller_reverts() public { + (bytes memory commands, bytes[] memory inputs) = _batch(other, RECIPIENT, RECIPIENT); + + vm.expectRevert(Permit2Payments.FromAddressIsNotOwner.selector); + vm.prank(user); + router.execute(commands, inputs); + } +} From a803b6fbe6f0fac439eaedc8809198a35311968f Mon Sep 17 00:00:00 2001 From: Chris Cashwell Date: Thu, 29 Oct 2026 16:59:50 -0400 Subject: [PATCH 3/5] fix(router): treat a gas-starved command failure as fatal 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. --- README.md | 2 +- contracts/UniversalRouter.sol | 10 ++- contracts/base/Dispatcher.sol | 13 ++- snapshots/UniversalRouterTest.json | 2 +- test/foundry-tests/GasStarvation.t.sol | 108 +++++++++++++++++++++++ test/foundry-tests/mock/MockResolver.sol | 9 ++ 6 files changed, 136 insertions(+), 8 deletions(-) create mode 100644 test/foundry-tests/GasStarvation.t.sol diff --git a/README.md b/README.md index 42378bb9..b8819690 100644 --- a/README.md +++ b/README.md @@ -48,7 +48,7 @@ Each command is a `bytes1` containing the following 8 bits: └─┴─┴───────────┘ ``` -- `f` is a single bit flag, that signals whether or not the command should be allowed to revert. If `f` is `false`, and the command reverts, then the entire transaction will revert. If `f` is `true` and the command reverts then the transaction will continue, allowing us to achieve partial fills. The flag only has an effect on commands built on an external call: `PERMIT2_PERMIT`, `PERMIT2_PERMIT_BATCH`, `BALANCE_CHECK_ERC20`, `V3_POSITION_MANAGER_PERMIT`, `V3_POSITION_MANAGER_CALL`, `V4_INITIALIZE_POOL`, `V4_POSITION_MANAGER_CALL`, `RESOLVE`, `V4_PROTOCOL_FEE_UPDATE`, `V3_PROTOCOL_FEE_UPDATE` and `EXECUTE_SUB_PLAN`. Swaps, transfers, wraps, sweeps and the Across deposit run as internal calls and revert the whole transaction regardless of the flag; to tolerate their failure, wrap them in an `EXECUTE_SUB_PLAN` that sets it. Some failures are never swallowed, with the flag set or through any `EXECUTE_SUB_PLAN`: the nested-execution gate (`NestedExecutionNotPermitted`), which `V4_POSITION_MANAGER_CALL` also trips inside any open PoolManager unlock because `modifyLiquidities` must open its own. If using this flag, be careful to include further commands that will remove any funds that could be left unused in the `UniversalRouter` contract. +- `f` is a single bit flag, that signals whether or not the command should be allowed to revert. If `f` is `false`, and the command reverts, then the entire transaction will revert. If `f` is `true` and the command reverts then the transaction will continue, allowing us to achieve partial fills. The flag only has an effect on commands built on an external call: `PERMIT2_PERMIT`, `PERMIT2_PERMIT_BATCH`, `BALANCE_CHECK_ERC20`, `V3_POSITION_MANAGER_PERMIT`, `V3_POSITION_MANAGER_CALL`, `V4_INITIALIZE_POOL`, `V4_POSITION_MANAGER_CALL`, `RESOLVE`, `V4_PROTOCOL_FEE_UPDATE`, `V3_PROTOCOL_FEE_UPDATE` and `EXECUTE_SUB_PLAN`. Swaps, transfers, wraps, sweeps and the Across deposit run as internal calls and revert the whole transaction regardless of the flag; to tolerate their failure, wrap them in an `EXECUTE_SUB_PLAN` that sets it. Some failures are never swallowed, with the flag set or through any `EXECUTE_SUB_PLAN`: the nested-execution gate (`NestedExecutionNotPermitted`), which `V4_POSITION_MANAGER_CALL` also trips inside any open PoolManager unlock because `modifyLiquidities` must open its own; and a command failure that leaves the router under 1/64 of the gas the command started with (`InsufficientGas`), which is what EIP-150 leaves behind a callee that ran out. The gas limit is the submitter's choice, so a starved call must not pass for a tolerated one. If using this flag, be careful to include further commands that will remove any funds that could be left unused in the `UniversalRouter` contract. - `r` is one bit of reserved space. This will allow us to increase the space used for commands, or add new flags in future. diff --git a/contracts/UniversalRouter.sol b/contracts/UniversalRouter.sol index 936a0ae5..50e00d3e 100644 --- a/contracts/UniversalRouter.sol +++ b/contracts/UniversalRouter.sol @@ -101,10 +101,16 @@ contract UniversalRouter is IUniversalRouter, ChainedActions, RouteSigner, Dispa bytes calldata input = inputs[commandIndex]; + uint256 gasBefore = gasleft(); (success, output) = dispatch(command, input); - if (!success && successRequired(command)) { - revert ExecutionFailed({commandIndex: commandIndex, message: output}); + if (!success) { + // A failure that leaves this frame under 1/64 of the gas it started with may be gas starvation + // rather than the command's own: the submitter picks the gas limit, and EIP-150 leaves exactly + // that much when a callee runs out. Treat it as fatal before consulting the flag; EXECUTE_SUB_PLAN + // re-throws it so no enclosing allow-revert boundary can swallow it either. + if (gasleft() < gasBefore >> 6) revert InsufficientGas(); + if (successRequired(command)) revert ExecutionFailed({commandIndex: commandIndex, message: output}); } } diff --git a/contracts/base/Dispatcher.sol b/contracts/base/Dispatcher.sol index 49ca6680..d0ffd627 100644 --- a/contracts/base/Dispatcher.sol +++ b/contracts/base/Dispatcher.sol @@ -46,6 +46,8 @@ abstract contract Dispatcher is /// @notice Thrown when a route reaches V4_SWAP inside a foreign PoolManager unlock without opting in, or /// V4_POSITION_MANAGER_CALL inside any unlock (modifyLiquidities must open its own) error NestedExecutionNotPermitted(); + /// @notice Thrown when a tolerated command failure left too little gas to rule out gas starvation + error InsufficientGas(); /// @notice Executes encoded commands along with provided inputs. /// @param commands A set of concatenated commands, each 1 byte in length @@ -434,10 +436,13 @@ abstract contract Dispatcher is if (command == Commands.EXECUTE_SUB_PLAN) { (bytes calldata _commands, bytes[] calldata _inputs) = inputs.decodeCommandsAndInputs(); (success, output) = (address(this)).call(abi.encodeCall(Dispatcher.execute, (_commands, _inputs))); - // The nested-execution gate is a consent check, not a route failure: FLAG_ALLOW_REVERT on the - // sub-plan must not turn it into a silent skip that leaves the route's input in the router. - if (!success && output.length == 4 && bytes4(output) == NestedExecutionNotPermitted.selector) { - revert NestedExecutionNotPermitted(); + // Neither the nested-execution gate (a consent check) nor gas starvation is a route failure: + // FLAG_ALLOW_REVERT on the sub-plan must not turn either into a silent skip that leaves the + // route's input in the router. + if (!success && output.length == 4) { + bytes4 selector = bytes4(output); + if (selector == NestedExecutionNotPermitted.selector) revert NestedExecutionNotPermitted(); + if (selector == InsufficientGas.selector) revert InsufficientGas(); } } else { // placeholder area for commands 0x22-0x3f diff --git a/snapshots/UniversalRouterTest.json b/snapshots/UniversalRouterTest.json index f74ad1fd..d0c4c8ac 100644 --- a/snapshots/UniversalRouterTest.json +++ b/snapshots/UniversalRouterTest.json @@ -1,3 +1,3 @@ { - "UniversalRouter bytecode size": "24201" + "UniversalRouter bytecode size": "23988" } \ No newline at end of file diff --git a/test/foundry-tests/GasStarvation.t.sol b/test/foundry-tests/GasStarvation.t.sol new file mode 100644 index 00000000..dc59d6ba --- /dev/null +++ b/test/foundry-tests/GasStarvation.t.sol @@ -0,0 +1,108 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.24; + +import 'forge-std/Test.sol'; +import {UniversalRouter} from '../../contracts/UniversalRouter.sol'; +import {Dispatcher} from '../../contracts/base/Dispatcher.sol'; +import {Commands} from '../../contracts/libraries/Commands.sol'; +import {RouterParameters} from '../../contracts/types/RouterParameters.sol'; +import {IUniversalRouter} from '../../contracts/interfaces/IUniversalRouter.sol'; +import {GasBurningResolver, RevertingResolver} from './mock/MockResolver.sol'; + +/// @notice A command built on an external call fails with no returndata when its callee runs out of gas, which +/// FLAG_ALLOW_REVERT would tolerate like any other failure even though the gas limit is the submitter's choice. +/// The router treats a failure that leaves it at the EIP-150 floor (under 1/64 of what the command started with) +/// as fatal, at the top level and through any allow-revert sub-plan boundary. +contract GasStarvationTest is Test { + /// @dev plenty for the plan's own work, so only the burner can exhaust it + uint256 constant GAS_LIMIT = 1_000_000; + + UniversalRouter router; + GasBurningResolver burner; + RevertingResolver reverter; + + function setUp() public { + router = new UniversalRouter( + RouterParameters({ + permit2: address(0), + weth9: address(0), + v2Factory: address(0), + v3Factory: address(0), + pairInitCodeHash: bytes32(0), + poolInitCodeHash: bytes32(0), + v4PoolManager: address(0), + permissionsAdapterFactory: address(0), + v3NFTPositionManager: address(0), + v4PositionManager: address(0), + spokePool: address(0) + }) + ); + burner = new GasBurningResolver(); + reverter = new RevertingResolver(); + } + + function _resolve(address resolver, bool allowRevert) + internal + pure + returns (bytes memory commands, bytes[] memory inputs) + { + bytes1 command = bytes1(uint8(Commands.RESOLVE)); + if (allowRevert) command |= Commands.FLAG_ALLOW_REVERT; + commands = abi.encodePacked(command); + inputs = new bytes[](1); + inputs[0] = abi.encode(resolver, bytes('')); + } + + function _subPlan(bytes memory sub, bytes[] memory subInputs, bool allowRevert) + internal + pure + returns (bytes memory commands, bytes[] memory inputs) + { + bytes1 command = bytes1(uint8(Commands.EXECUTE_SUB_PLAN)); + if (allowRevert) command |= Commands.FLAG_ALLOW_REVERT; + commands = abi.encodePacked(command); + inputs = new bytes[](1); + inputs[0] = abi.encode(sub, subInputs); + } + + function test_starvedAllowRevertCommand_reverts() public { + (bytes memory commands, bytes[] memory inputs) = _resolve(address(burner), true); + vm.expectRevert(Dispatcher.InsufficientGas.selector); + router.execute{gas: GAS_LIMIT}(commands, inputs); + } + + function test_starvedRequiredCommand_reverts() public { + (bytes memory commands, bytes[] memory inputs) = _resolve(address(burner), false); + vm.expectRevert(Dispatcher.InsufficientGas.selector); + router.execute{gas: GAS_LIMIT}(commands, inputs); + } + + /// @notice the starvation check runs before the required-command check, so the inner frame reports + /// InsufficientGas rather than an ExecutionFailed the allow-revert parent would swallow + function test_starvedRequiredCommandInAllowRevertSubPlan_reverts() public { + (bytes memory sub, bytes[] memory subInputs) = _resolve(address(burner), false); + (bytes memory commands, bytes[] memory inputs) = _subPlan(sub, subInputs, true); + vm.expectRevert(Dispatcher.InsufficientGas.selector); + router.execute{gas: GAS_LIMIT}(commands, inputs); + } + + function test_starvedAllowRevertCommandInAllowRevertSubPlan_reverts() public { + (bytes memory sub, bytes[] memory subInputs) = _resolve(address(burner), true); + (bytes memory commands, bytes[] memory inputs) = _subPlan(sub, subInputs, true); + vm.expectRevert(Dispatcher.InsufficientGas.selector); + router.execute{gas: GAS_LIMIT}(commands, inputs); + } + + /// @notice control: an ordinary failure under the flag is still tolerated + function test_ordinaryFailure_allowRevert_isTolerated() public { + (bytes memory commands, bytes[] memory inputs) = _resolve(address(reverter), true); + router.execute{gas: GAS_LIMIT}(commands, inputs); + } + + /// @notice control: an ordinary failure without the flag still surfaces as ExecutionFailed + function test_ordinaryFailure_required_isExecutionFailed() public { + (bytes memory commands, bytes[] memory inputs) = _resolve(address(reverter), false); + vm.expectPartialRevert(IUniversalRouter.ExecutionFailed.selector); + router.execute{gas: GAS_LIMIT}(commands, inputs); + } +} diff --git a/test/foundry-tests/mock/MockResolver.sol b/test/foundry-tests/mock/MockResolver.sol index a4d7d2d7..a107e592 100644 --- a/test/foundry-tests/mock/MockResolver.sol +++ b/test/foundry-tests/mock/MockResolver.sol @@ -25,6 +25,15 @@ contract RevertingResolver is IAmountResolver { } } +/// @notice Burns every unit of gas forwarded to it, so a RESOLVE that staticcalls it fails from gas exhaustion. +contract GasBurningResolver is IAmountResolver { + function resolveAmount(bytes calldata) external pure returns (uint256) { + assembly ('memory-safe') { + for {} 1 {} {} + } + } +} + /// @notice A resolver that returns fewer than 32 bytes, which the router must treat as a failure. contract ShortReturnResolver { fallback() external { From 0dfa95dceaecf1b24d5cb40070857d7f0e32e2d6 Mon Sep 17 00:00:00 2001 From: Chris Cashwell Date: Thu, 29 Oct 2026 17:00:21 -0400 Subject: [PATCH 4/5] fix(v2): measure 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 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. --- README.md | 2 +- contracts/modules/uniswap/v2/V2SwapRouter.sol | 5 +- snapshots/UniversalRouterTest.json | 2 +- test/foundry-tests/V2RecipientPairAlias.t.sol | 126 ++++++++++++++++++ 4 files changed, 132 insertions(+), 3 deletions(-) create mode 100644 test/foundry-tests/V2RecipientPairAlias.t.sol diff --git a/README.md b/README.md index b8819690..89cc2c89 100644 --- a/README.md +++ b/README.md @@ -120,7 +120,7 @@ Note that some of the commands in the middle of the series are unused. These gap `PERMIT2_TRANSFER_FROM` pulls from the caller, and every `PERMIT2_TRANSFER_FROM_BATCH` detail must name the caller as `from`. Both resolve their recipients through the same sentinel table as every other recipient field: `ActionConstants.MSG_SENDER` (address 1) delivers to the caller and `ActionConstants.ADDRESS_THIS` (address 2) to the router. -`V2_SWAP_EXACT_OUT` is all-or-nothing: it measures what the recipient actually receives and reverts with `V2TooLittleReceived` if that is less than `amountOut`, even by 1 wei. A token anywhere on the path that delivers less than the amount transferred, whether through a transfer fee, withholding, or share-based rounding such as stETH (which typically delivers 1-2 wei less), therefore makes the route revert. Swap such tokens with `V2_SWAP_EXACT_IN` instead. +`V2_SWAP_EXACT_OUT` is all-or-nothing: it measures what the recipient actually receives and reverts with `V2TooLittleReceived` if that is less than `amountOut`, even by 1 wei. A token anywhere on the path that delivers less than the amount transferred, whether through a transfer fee, withholding, or share-based rounding such as stETH (which typically delivers 1-2 wei less), therefore makes the route revert. Swap such tokens with `V2_SWAP_EXACT_IN` instead. Both v2 swap commands measure delivery across the final hop only, so a recipient that is itself a pair earlier on the path is not credited with the tokens the route moved through it on the way. `V4_SWAP` takes an encoded v4 action plan `(bytes actions, bytes[] params)` and runs it through the router's `V4Router`. Exact-output swaps are all-or-nothing and revert on a shortfall. An exact-output swap paid from the router's own balance (`payerIsUser = false`, or a native input) spends only what the route actually costs, so whatever the plan pre-funded beyond that stays in the router. Such a plan must end with a full-balance return such as `SWEEP` for each input currency, since any balance left in the router can be swept by anyone. A hook that funds the swap's input itself is the extreme case: the route costs nothing, so the entire pre-funded amount stays in the router and must be swept back. diff --git a/contracts/modules/uniswap/v2/V2SwapRouter.sol b/contracts/modules/uniswap/v2/V2SwapRouter.sol index 1c9a020f..500ecd4a 100644 --- a/contracts/modules/uniswap/v2/V2SwapRouter.sol +++ b/contracts/modules/uniswap/v2/V2SwapRouter.sol @@ -24,7 +24,7 @@ abstract contract V2SwapRouter is UniswapImmutables, Permit2Payments { uint256 amountOutMinimum ) private { ERC20 tokenOut = ERC20(path[path.length - 1]); - uint256 balanceBefore = tokenOut.balanceOf(recipient); + uint256 balanceBefore; unchecked { // cached to save on duplicate operations (address token0,) = UniswapV2Library.sortTokens(path[0], path[1]); @@ -50,6 +50,9 @@ abstract contract V2SwapRouter is UniswapImmutables, Permit2Payments { UNISWAP_V2_FACTORY, UNISWAP_V2_PAIR_INIT_CODE_HASH, output, path[i + 2] ) : (recipient, address(0)); + // Snapshot the recipient right before the final hop, so tokens an earlier hop routed through the same + // address (a pair that appears twice in the path) are not counted as delivery. + if (i == penultimatePairIndex) balanceBefore = tokenOut.balanceOf(recipient); // if minHopPrice is being used, we need to check output balance change if (minHopPriceEnabled && minHopPriceX36[i] != 0) { diff --git a/snapshots/UniversalRouterTest.json b/snapshots/UniversalRouterTest.json index d0c4c8ac..21a73b5b 100644 --- a/snapshots/UniversalRouterTest.json +++ b/snapshots/UniversalRouterTest.json @@ -1,3 +1,3 @@ { - "UniversalRouter bytecode size": "23988" + "UniversalRouter bytecode size": "24065" } \ No newline at end of file diff --git a/test/foundry-tests/V2RecipientPairAlias.t.sol b/test/foundry-tests/V2RecipientPairAlias.t.sol new file mode 100644 index 00000000..6b7a8456 --- /dev/null +++ b/test/foundry-tests/V2RecipientPairAlias.t.sol @@ -0,0 +1,126 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.24; + +import 'forge-std/Test.sol'; +import {IUniswapV2Factory} from '@uniswap/v2-core/contracts/interfaces/IUniswapV2Factory.sol'; +import {IUniswapV2Pair} from '@uniswap/v2-core/contracts/interfaces/IUniswapV2Pair.sol'; +import {UniversalRouter} from '../../contracts/UniversalRouter.sol'; +import {RouterParameters} from '../../contracts/types/RouterParameters.sol'; +import {Commands} from '../../contracts/libraries/Commands.sol'; +import {UniswapV2Library} from '../../contracts/modules/uniswap/v2/UniswapV2Library.sol'; +import {V2SwapRouter} from '../../contracts/modules/uniswap/v2/V2SwapRouter.sol'; +import {MockERC20} from './mock/MockERC20.sol'; + +/// @notice A v2 route measures delivery at the recipient. When the recipient is itself a pair the path passes +/// through, the tokens an earlier hop routed into it must not count toward amountOutMinimum, so the snapshot is +/// taken right before the final hop. +contract V2RecipientPairAliasTest is Test { + IUniswapV2Factory constant FACTORY = IUniswapV2Factory(0x5C69bEe701ef814a2B6a3EDD4B1652CB9cc5aA6f); + address constant USER = address(0xA11CE); + uint256 constant LIQUIDITY = 1_000 ether; + uint256 constant AMOUNT_IN = 10 ether; + + UniversalRouter router; + MockERC20 tokenA; + MockERC20 tokenB; + MockERC20 tokenC; + MockERC20 tokenD; + address pairBC; + + function setUp() public { + vm.createSelectFork(vm.envString('FORK_URL'), 20010000); + + router = new UniversalRouter( + RouterParameters({ + permit2: address(0), + weth9: address(0), + v2Factory: address(FACTORY), + v3Factory: address(0), + pairInitCodeHash: bytes32(0x96e8ac4277198ff8b6f785478aa9a39f403cb768dd02cbee326c3e7da348845f), + poolInitCodeHash: bytes32(0), + v4PoolManager: address(0), + permissionsAdapterFactory: address(0), + v3NFTPositionManager: address(0), + v4PositionManager: address(0), + spokePool: address(0) + }) + ); + + tokenA = new MockERC20(); + tokenB = new MockERC20(); + tokenC = new MockERC20(); + tokenD = new MockERC20(); + + _seedPair(tokenA, tokenB); + pairBC = _seedPair(tokenB, tokenC); + _seedPair(tokenC, tokenD); + _seedPair(tokenD, tokenB); + + // the router pays the first pair from its own balance (payerIsUser = false), so no Permit2 setup is needed + tokenA.mint(address(router), AMOUNT_IN); + } + + function _seedPair(MockERC20 a, MockERC20 b) internal returns (address pair) { + pair = FACTORY.createPair(address(a), address(b)); + a.mint(pair, LIQUIDITY); + b.mint(pair, LIQUIDITY); + IUniswapV2Pair(pair).sync(); + } + + /// @dev A -> B -> C -> D -> B: the output token is also the first intermediate, so the B -> C hop deposits B + /// into pairBC on the way through, before the final hop delivers there again + function _route(address recipient, uint256 amountOutMinimum) + internal + view + returns (bytes memory commands, bytes[] memory inputs) + { + address[] memory path = new address[](5); + path[0] = address(tokenA); + path[1] = address(tokenB); + path[2] = address(tokenC); + path[3] = address(tokenD); + path[4] = address(tokenB); + + commands = abi.encodePacked(bytes1(uint8(Commands.V2_SWAP_EXACT_IN))); + inputs = new bytes[](1); + inputs[0] = abi.encode(recipient, AMOUNT_IN, amountOutMinimum, path, false, new uint256[](0)); + } + + /// @dev every pair is used once and starts at LIQUIDITY on both sides + function _quote() internal pure returns (uint256 intermediateB, uint256 finalB) { + intermediateB = UniswapV2Library.getAmountOut(AMOUNT_IN, LIQUIDITY, LIQUIDITY); + uint256 amountC = UniswapV2Library.getAmountOut(intermediateB, LIQUIDITY, LIQUIDITY); + uint256 amountD = UniswapV2Library.getAmountOut(amountC, LIQUIDITY, LIQUIDITY); + finalB = UniswapV2Library.getAmountOut(amountD, LIQUIDITY, LIQUIDITY); + } + + function test_v2SwapExactIn_pairRecipientOnPath_finalHopBelowMinimum_reverts() public { + (uint256 intermediateB, uint256 finalB) = _quote(); + // above what the final hop delivers, below what the pair's balance rises by over the whole route + uint256 minimum = finalB + 1; + assertLt(minimum, intermediateB + finalB); + + (bytes memory commands, bytes[] memory inputs) = _route(pairBC, minimum); + vm.expectRevert(V2SwapRouter.V2TooLittleReceived.selector); + router.execute(commands, inputs); + } + + function test_v2SwapExactIn_pairRecipientOnPath_finalHopAtMinimum_succeeds() public { + (uint256 intermediateB, uint256 finalB) = _quote(); + uint256 balanceBefore = tokenB.balanceOf(pairBC); + + (bytes memory commands, bytes[] memory inputs) = _route(pairBC, finalB); + router.execute(commands, inputs); + + assertEq(tokenB.balanceOf(pairBC) - balanceBefore, intermediateB + finalB); + } + + function test_v2SwapExactIn_ordinaryRecipient_receivesFinalHop() public { + (, uint256 finalB) = _quote(); + + (bytes memory commands, bytes[] memory inputs) = _route(USER, finalB); + router.execute(commands, inputs); + + assertEq(tokenB.balanceOf(USER), finalB); + } +} From f9b7af847a7bd386472cf1d57b5f17ab81c624a7 Mon Sep 17 00:00:00 2001 From: Chris Cashwell Date: Thu, 29 Oct 2026 17:08:19 -0400 Subject: [PATCH 5/5] test: regenerate hardhat gas snapshots 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. --- .../CheckOwnership.gas.test.ts.snap | 2 +- .../__snapshots__/Payments.gas.test.ts.snap | 16 ++-- .../__snapshots__/Uniswap.gas.test.ts.snap | 78 +++++++++---------- .../UniversalVSSwapRouter.gas.test.ts.snap | 20 ++--- .../V3ToV4Migration.gas.test.ts.snap | 16 ++-- 5 files changed, 66 insertions(+), 66 deletions(-) diff --git a/test/integration-tests/gas-tests/__snapshots__/CheckOwnership.gas.test.ts.snap b/test/integration-tests/gas-tests/__snapshots__/CheckOwnership.gas.test.ts.snap index c453052d..053e5d3f 100644 --- a/test/integration-tests/gas-tests/__snapshots__/CheckOwnership.gas.test.ts.snap +++ b/test/integration-tests/gas-tests/__snapshots__/CheckOwnership.gas.test.ts.snap @@ -3,6 +3,6 @@ exports[`Check Ownership Gas gas: balance check ERC20 1`] = ` Object { "calldataByteLength": 356, - "gasUsed": 38075, + "gasUsed": 38060, } `; diff --git a/test/integration-tests/gas-tests/__snapshots__/Payments.gas.test.ts.snap b/test/integration-tests/gas-tests/__snapshots__/Payments.gas.test.ts.snap index b77eacdc..2e384276 100644 --- a/test/integration-tests/gas-tests/__snapshots__/Payments.gas.test.ts.snap +++ b/test/integration-tests/gas-tests/__snapshots__/Payments.gas.test.ts.snap @@ -3,55 +3,55 @@ exports[`Payments Gas Tests Individual Command Tests gas: SWEEP with ERC20 1`] = ` Object { "calldataByteLength": 356, - "gasUsed": 37553, + "gasUsed": 37532, } `; exports[`Payments Gas Tests Individual Command Tests gas: SWEEP_WITH_FEE 1`] = ` Object { "calldataByteLength": 516, - "gasUsed": 66374, + "gasUsed": 66346, } `; exports[`Payments Gas Tests Individual Command Tests gas: TRANSFER with ERC20 1`] = ` Object { "calldataByteLength": 356, - "gasUsed": 36552, + "gasUsed": 36531, } `; exports[`Payments Gas Tests Individual Command Tests gas: TRANSFER with ETH 1`] = ` Object { "calldataByteLength": 356, - "gasUsed": 32128, + "gasUsed": 32116, } `; exports[`Payments Gas Tests Individual Command Tests gas: UNWRAP_WETH 1`] = ` Object { "calldataByteLength": 324, - "gasUsed": 45031, + "gasUsed": 45023, } `; exports[`Payments Gas Tests Individual Command Tests gas: UNWRAP_WETH_EXACT partial amount 1`] = ` Object { "calldataByteLength": 324, - "gasUsed": 50033, + "gasUsed": 50019, } `; exports[`Payments Gas Tests Individual Command Tests gas: UNWRAP_WETH_WITH_FEE 1`] = ` Object { "calldataByteLength": 644, - "gasUsed": 51684, + "gasUsed": 51656, } `; exports[`Payments Gas Tests Individual Command Tests gas: WRAP_ETH 1`] = ` Object { "calldataByteLength": 324, - "gasUsed": 53847, + "gasUsed": 53832, } `; diff --git a/test/integration-tests/gas-tests/__snapshots__/Uniswap.gas.test.ts.snap b/test/integration-tests/gas-tests/__snapshots__/Uniswap.gas.test.ts.snap index f4b91f7c..50f17b1d 100644 --- a/test/integration-tests/gas-tests/__snapshots__/Uniswap.gas.test.ts.snap +++ b/test/integration-tests/gas-tests/__snapshots__/Uniswap.gas.test.ts.snap @@ -3,105 +3,105 @@ exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Batch reverts gas: 2 sub-plans, both fail but the transaction succeeds 1`] = ` Object { "calldataByteLength": 1956, - "gasUsed": 278437, + "gasUsed": 278488, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Batch reverts gas: 2 sub-plans, neither fails 1`] = ` Object { "calldataByteLength": 1956, - "gasUsed": 254002, + "gasUsed": 253963, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Batch reverts gas: 2 sub-plans, second sub plan fails 1`] = ` Object { "calldataByteLength": 1956, - "gasUsed": 254002, + "gasUsed": 253963, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Batch reverts gas: 2 sub-plans, the first fails 1`] = ` Object { "calldataByteLength": 1956, - "gasUsed": 278437, + "gasUsed": 278488, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Interleaving routes gas: V2, then V3 1`] = ` Object { "calldataByteLength": 964, - "gasUsed": 193975, + "gasUsed": 194025, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Interleaving routes gas: V3, then V2 1`] = ` Object { "calldataByteLength": 964, - "gasUsed": 181414, + "gasUsed": 181468, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Split routes gas: ERC20 --> ERC20 split V2 and V2 different routes, different input tokens, each two hop, with batch permit 1`] = ` Object { "calldataByteLength": 1668, - "gasUsed": 301581, + "gasUsed": 302027, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Split routes gas: ERC20 --> ERC20 split V2 and V2 different routes, each two hop, with explicit permit 1`] = ` Object { "calldataByteLength": 1348, - "gasUsed": 312389, + "gasUsed": 312826, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Split routes gas: ERC20 --> ERC20 split V2 and V2 different routes, each two hop, with explicit permit transfer from batch 1`] = ` Object { "calldataByteLength": 1412, - "gasUsed": 313192, + "gasUsed": 313840, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Split routes gas: ERC20 --> ERC20 split V2 and V2 different routes, each two hop, without explicit permit 1`] = ` Object { "calldataByteLength": 1028, - "gasUsed": 308511, + "gasUsed": 308980, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Split routes gas: ERC20 --> ERC20 split V2 and V3, one hop 1`] = ` Object { "calldataByteLength": 1124, - "gasUsed": 181637, + "gasUsed": 181678, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Split routes gas: ERC20 --> ERC20 split V2 and V3, one hop, ADDRESS_THIS flag 1`] = ` Object { "calldataByteLength": 1124, - "gasUsed": 181412, + "gasUsed": 181453, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Split routes gas: ERC20 --> ETH split V2 and V3, exactOut, one hop 1`] = ` Object { "calldataByteLength": 1092, - "gasUsed": 199062, + "gasUsed": 199152, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Split routes gas: ERC20 --> ETH split V2 and V3, one hop 1`] = ` Object { "calldataByteLength": 1092, - "gasUsed": 189422, + "gasUsed": 189476, } `; exports[`Uniswap Gas Tests Mixing V2 and V3 with Universal Router. Split routes gas: ETH --> ERC20 split V2 and V3, one hop 1`] = ` Object { "calldataByteLength": 1252, - "gasUsed": 196581, + "gasUsed": 196606, } `; @@ -143,98 +143,98 @@ Object { exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ERC20 gas: exactIn trade, where an output fee is taken 1`] = ` Object { "calldataByteLength": 900, - "gasUsed": 128760, + "gasUsed": 128863, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ERC20 gas: exactIn, one trade, one hop 1`] = ` Object { "calldataByteLength": 580, - "gasUsed": 108608, + "gasUsed": 108731, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ERC20 gas: exactIn, one trade, three hops 1`] = ` Object { "calldataByteLength": 644, - "gasUsed": 244182, + "gasUsed": 244520, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ERC20 gas: exactIn, one trade, three hops, no deadline 1`] = ` Object { "calldataByteLength": 612, - "gasUsed": 243915, + "gasUsed": 244253, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ERC20 gas: exactIn, one trade, two hops 1`] = ` Object { "calldataByteLength": 612, - "gasUsed": 176432, + "gasUsed": 176665, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ERC20 gas: exactIn, one trade, two hops, MSG_SENDER flag 1`] = ` Object { "calldataByteLength": 612, - "gasUsed": 176432, + "gasUsed": 176665, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ERC20 gas: exactOut, one trade, one hop 1`] = ` Object { "calldataByteLength": 580, - "gasUsed": 110530, + "gasUsed": 110637, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ERC20 gas: exactOut, one trade, three hops 1`] = ` Object { "calldataByteLength": 644, - "gasUsed": 251861, + "gasUsed": 252184, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ERC20 gas: exactOut, one trade, two hops 1`] = ` Object { "calldataByteLength": 612, - "gasUsed": 181266, + "gasUsed": 181478, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ETH gas: exactIn, one trade, one hop 1`] = ` Object { "calldataByteLength": 708, - "gasUsed": 124999, + "gasUsed": 125122, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ETH gas: exactOut, one trade, one hop 1`] = ` Object { "calldataByteLength": 868, - "gasUsed": 132157, + "gasUsed": 132257, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ERC20 --> ETH gas: exactOut, with ETH fee 1`] = ` Object { "calldataByteLength": 1028, - "gasUsed": 140360, + "gasUsed": 140453, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ETH --> ERC20 gas: exactIn, one trade, one hop 1`] = ` Object { "calldataByteLength": 708, - "gasUsed": 108548, + "gasUsed": 108654, } `; exports[`Uniswap Gas Tests Trade on UniswapV2 with Universal Router. ETH --> ERC20 gas: exactOut, one trade, one hop 1`] = ` Object { "calldataByteLength": 836, - "gasUsed": 129410, + "gasUsed": 129507, } `; @@ -283,69 +283,69 @@ Object { exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ERC20 --> ERC20 gas: exactIn, one trade, one hop 1`] = ` Object { "calldataByteLength": 580, - "gasUsed": 108582, + "gasUsed": 108506, } `; exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ERC20 --> ERC20 gas: exactIn, one trade, three hops 1`] = ` Object { "calldataByteLength": 612, - "gasUsed": 260647, + "gasUsed": 260514, } `; exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ERC20 --> ERC20 gas: exactIn, one trade, two hops 1`] = ` Object { "calldataByteLength": 612, - "gasUsed": 181992, + "gasUsed": 181887, } `; exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ERC20 --> ERC20 gas: exactOut, one trade, one hop 1`] = ` Object { "calldataByteLength": 580, - "gasUsed": 116146, + "gasUsed": 116115, } `; exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ERC20 --> ERC20 gas: exactOut, one trade, three hops 1`] = ` Object { "calldataByteLength": 612, - "gasUsed": 255526, + "gasUsed": 255475, } `; exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ERC20 --> ERC20 gas: exactOut, one trade, two hops 1`] = ` Object { "calldataByteLength": 612, - "gasUsed": 177535, + "gasUsed": 177494, } `; exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ERC20 --> ETH gas: exactIn swap 1`] = ` Object { "calldataByteLength": 708, - "gasUsed": 125021, + "gasUsed": 124945, } `; exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ERC20 --> ETH gas: exactOut swap 1`] = ` Object { "calldataByteLength": 708, - "gasUsed": 132657, + "gasUsed": 132626, } `; exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ETH --> ERC20 gas: exactIn swap 1`] = ` Object { "calldataByteLength": 708, - "gasUsed": 218532, + "gasUsed": 218446, } `; exports[`Uniswap Gas Tests Trade on UniswapV3 with Universal Router. ETH --> ERC20 gas: exactOut swap 1`] = ` Object { "calldataByteLength": 836, - "gasUsed": 127904, + "gasUsed": 127863, } `; diff --git a/test/integration-tests/gas-tests/__snapshots__/UniversalVSSwapRouter.gas.test.ts.snap b/test/integration-tests/gas-tests/__snapshots__/UniversalVSSwapRouter.gas.test.ts.snap index 4a9787c6..6e82ae29 100644 --- a/test/integration-tests/gas-tests/__snapshots__/UniversalVSSwapRouter.gas.test.ts.snap +++ b/test/integration-tests/gas-tests/__snapshots__/UniversalVSSwapRouter.gas.test.ts.snap @@ -7,32 +7,32 @@ Object { } `; -exports[`Uniswap UX Tests gas: Comparisons Casual Swapper - 3 swaps Permit2 Max Approval Swap 1`] = `1131132`; +exports[`Uniswap UX Tests gas: Comparisons Casual Swapper - 3 swaps Permit2 Max Approval Swap 1`] = `1130858`; -exports[`Uniswap UX Tests gas: Comparisons Casual Swapper - 3 swaps Permit2 Sign Per Swap 1`] = `1165713`; +exports[`Uniswap UX Tests gas: Comparisons Casual Swapper - 3 swaps Permit2 Sign Per Swap 1`] = `1165406`; exports[`Uniswap UX Tests gas: Comparisons Casual Swapper - 3 swaps SwapRouter02 1`] = `1124979`; -exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper - 10 swaps Permit2 Max Approval Swap 1`] = `3160725`; +exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper - 10 swaps Permit2 Max Approval Swap 1`] = `3159803`; -exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper - 10 swaps Permit2 Sign Per Swap 1`] = `3315034`; +exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper - 10 swaps Permit2 Sign Per Swap 1`] = `3313964`; exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper - 10 swaps SwapRouter02 1`] = `3195011`; -exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper across 3 swap router versions - 15 swaps across 3 versions Permit2 Max Approval Swap 1`] = `4207247`; +exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper across 3 swap router versions - 15 swaps across 3 versions Permit2 Max Approval Swap 1`] = `4205768`; -exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper across 3 swap router versions - 15 swaps across 3 versions Permit2 Sign Per Swap 1`] = `4412281`; +exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper across 3 swap router versions - 15 swaps across 3 versions Permit2 Sign Per Swap 1`] = `4410606`; exports[`Uniswap UX Tests gas: Comparisons Frequent Swapper across 3 swap router versions - 15 swaps across 3 versions SwapRouter02 1`] = `4282374`; -exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Complex Swap Permit2 Max Approval Swap 1`] = `519825`; +exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Complex Swap Permit2 Max Approval Swap 1`] = `519732`; -exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Complex Swap Permit2 Sign Per Swap 1`] = `520143`; +exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Complex Swap Permit2 Sign Per Swap 1`] = `520050`; exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Complex Swap SwapRouter02 1`] = `500008`; -exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Simple Swap Permit2 Max Approval Swap 1`] = `304629`; +exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Simple Swap Permit2 Max Approval Swap 1`] = `304508`; -exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Simple Swap Permit2 Sign Per Swap 1`] = `304565`; +exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Simple Swap Permit2 Sign Per Swap 1`] = `304444`; exports[`Uniswap UX Tests gas: Comparisons One Time Swapper - Simple Swap SwapRouter02 1`] = `270033`; diff --git a/test/integration-tests/gas-tests/__snapshots__/V3ToV4Migration.gas.test.ts.snap b/test/integration-tests/gas-tests/__snapshots__/V3ToV4Migration.gas.test.ts.snap index aa8d2a30..de6a3d64 100644 --- a/test/integration-tests/gas-tests/__snapshots__/V3ToV4Migration.gas.test.ts.snap +++ b/test/integration-tests/gas-tests/__snapshots__/V3ToV4Migration.gas.test.ts.snap @@ -3,55 +3,55 @@ exports[`V3 to V4 Migration Gas Tests V3 Commands burn gas: erc721permit + decreaseLiquidity + collect + burn 1`] = ` Object { "calldataByteLength": 1092, - "gasUsed": 256825, + "gasUsed": 256794, } `; exports[`V3 to V4 Migration Gas Tests V3 Commands collect gas: erc721permit + decreaseLiquidity + collect 1`] = ` Object { "calldataByteLength": 964, - "gasUsed": 224519, + "gasUsed": 224490, } `; exports[`V3 to V4 Migration Gas Tests V3 Commands decrease liquidity gas: erc721permit + decreaseLiquidity 1`] = ` Object { "calldataByteLength": 740, - "gasUsed": 202669, + "gasUsed": 202649, } `; exports[`V3 to V4 Migration Gas Tests V3 Commands erc721permit gas: erc721permit 1`] = ` Object { "calldataByteLength": 484, - "gasUsed": 66397, + "gasUsed": 66387, } `; exports[`V3 to V4 Migration Gas Tests V4 Commands initialize pool gas: initialize a pool 1`] = ` Object { "calldataByteLength": 452, - "gasUsed": 59314, + "gasUsed": 59296, } `; exports[`V3 to V4 Migration Gas Tests V4 Commands mint gas: migrate and mint 1`] = ` Object { "calldataByteLength": 2500, - "gasUsed": 600446, + "gasUsed": 601729, } `; exports[`V3 to V4 Migration Gas Tests V4 Commands mint gas: migrate weth position into eth position with forwarding 1`] = ` Object { "calldataByteLength": 2788, - "gasUsed": 577069, + "gasUsed": 578085, } `; exports[`V3 to V4 Migration Gas Tests V4 Commands mint gas: mint 1`] = ` Object { "calldataByteLength": 1604, - "gasUsed": 446534, + "gasUsed": 447848, } `;