fix(audit): resolve sherlock contest findings against 2.3.0

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; 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.
 
@@ -118,7 +118,9 @@ 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.
 
-`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.
+`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. 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.
 

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});
             }
         }
 

contracts/base/Dispatcher.sol

@@ -43,8 +43,11 @@ 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 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
@@ -307,7 +310,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))
@@ -373,6 +389,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);
@@ -416,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

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

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) {

snapshots/UniversalRouterTest.json

@@ -1,3 +1,3 @@
 {
-  "UniversalRouter bytecode size": "24121"
+  "UniversalRouter bytecode size": "24065"
 }

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);
+    }
+}