fix: harden poller replay protection (#125)

Track every funded digest across schedule updates, document salt construction, and remove redundant funding tests.

Author: igorroncevic

README.md

@@ -103,7 +103,8 @@ To create a TWAP order:
 
 1. ABI-Encode the `IConditionalOrder.ConditionalOrderParams` struct with:
    - `handler`: set to the `TWAP` smart contract deployment.
-   - `salt`: set to a unique value.
+   - `salt`: use a value unique to the user and logical order. For poller schedules, a good default is
+     the hash of the order-defining `staticInput` values with any `appData` field set to zero.
    - `staticInput`: the ABI-encoded `TWAP.Data` struct.
 2. Use the `struct` from (1) as either a Merkle leaf, or with `ComposableCoW.create` to create a single conditional order.
 3. Approve `GPv2VaultRelayer` to trade `n x partSellAmount` of the safe's `sellToken` tokens (in the example above, `GPv2VaultRelayer` would receive approval for spending 12,000,000 DAI tokens).

src/types/ComposableCowPoller.sol

@@ -11,7 +11,8 @@ import {IConditionalOrder, IConditionalOrderGenerator} from "src/interfaces/ICon
 contract ComposableCowPoller {
     using GPv2SafeERC20 for IERC20;
 
-    ComposableCoW public immutable composableCow;
+    /// @dev `ComposableCoW` stores the settlement domain separator supplied at deployment.
+    ComposableCoW public immutable COMPOSABLE_COW;
 
     /// @notice Parameters for a JIT funding schedule.
     /// @dev A schedule is uniquely identified by its funder, handler, owner, and salt.
@@ -25,8 +26,8 @@ contract ComposableCowPoller {
         /// @dev It can be an EOA or contract and may be the same address as `funder`.
         address owner;
         /// @notice A user-controlled namespace for this schedule.
-        /// @dev Use a value unique to the user and order. Deriving it from order-defining static-input values,
-        ///      excluding appData, is recommended.
+        /// @dev Keep it unique to the user and logical order. A good default is the hash of the
+        ///      order-defining static-input values with any appData field set to zero.
         bytes32 salt;
         /// @notice The static input passed to the handler when it generates an order.
         bytes staticInput;
@@ -35,8 +36,8 @@ contract ComposableCowPoller {
     /// @dev Keyed by `id == scheduleId(schedule)`, which excludes the order's `appData`.
     mapping(bytes32 => Schedule) public schedules;
 
-    /// @dev `id => digest` of the order last funded, so each order is funded at most once.
-    mapping(bytes32 => bytes32) public lastFunded;
+    /// @dev `id => digest => funded`. History survives schedule updates so an old order cannot be replayed.
+    mapping(bytes32 => mapping(bytes32 => bool)) public funded;
 
     /// @notice Thrown when someone other than the schedule funder registers or updates a schedule.
     error OnlyFunder();
@@ -51,12 +52,13 @@ contract ComposableCowPoller {
     event Pulled(bytes32 indexed id, bytes32 orderDigest, uint256 amount);
 
     constructor(ComposableCoW _composableCow) {
-        composableCow = _composableCow;
+        COMPOSABLE_COW = _composableCow;
     }
 
     /// @notice Computes the deterministic, appData-independent schedule key.
     /// @dev `staticInput` is excluded because its appData can depend on this key.
-    ///      Use a different salt for concurrent schedules with the same funder, handler, and owner.
+    ///      Keep the salt unique to the user and logical order. A good default is the hash of the
+    ///      order-defining static-input values with any appData field set to zero.
     /// @param schedule The schedule whose identity fields determine the key.
     /// @return The schedule key.
     function scheduleId(Schedule memory schedule) public pure returns (bytes32) {
@@ -65,14 +67,14 @@ contract ComposableCowPoller {
 
     /// @notice Registers or updates a schedule.
     /// @dev Registering the same funder, handler, owner, and salt replaces the stored schedule.
-    ///      Only the funds source may register, and the ID is namespaced by the funder.
+    ///      Only the funds source may register, and the ID is namespaced by the funder. Funding
+    ///      history is preserved across updates; use a new salt for a new logical schedule.
     /// @param schedule The schedule to store.
     /// @return id The deterministic key of the stored schedule.
     function register(Schedule calldata schedule) external returns (bytes32 id) {
         if (msg.sender != schedule.funder) revert OnlyFunder();
         id = scheduleId(schedule);
         schedules[id] = schedule;
-        delete lastFunded[id]; // reset funding history
         emit ScheduleRegistered(id, schedule.owner, schedule.funder);
     }
 
@@ -84,25 +86,23 @@ contract ComposableCowPoller {
         if (schedule.funder == address(0)) revert NoSchedule();
 
         // Re-derive `ctx` on-chain, so `pollFunds(id)` stays independent of the order's `appData`.
-        bytes32 ctx = composableCow.hash(
+        bytes32 ctx = COMPOSABLE_COW.hash(
             IConditionalOrder.ConditionalOrderParams({
-                handler: schedule.handler,
-                salt: schedule.salt,
-                staticInput: schedule.staticInput
+                handler: schedule.handler, salt: schedule.salt, staticInput: schedule.staticInput
             })
         );
 
         // The order must still be authorised; `remove` disables the poller.
-        if (!composableCow.singleOrders(schedule.owner, ctx)) revert OrderNotLive();
+        if (!COMPOSABLE_COW.singleOrders(schedule.owner, ctx)) revert OrderNotLive();
 
         // The handler yields the current order and reverts outside its window.
         GPv2Order.Data memory order =
             schedule.handler.getTradeableOrder(schedule.owner, address(this), ctx, schedule.staticInput, bytes(""));
 
-        // Fund each order at most once.
-        bytes32 digest = GPv2Order.hash(order, composableCow.domainSeparator());
-        if (digest == lastFunded[id]) return;
-        lastFunded[id] = digest;
+        // `ComposableCoW` exposes the settlement domain separator it received at deployment.
+        bytes32 digest = GPv2Order.hash(order, COMPOSABLE_COW.domainSeparator());
+        if (funded[id][digest]) return;
+        funded[id][digest] = true;
 
         order.sellToken.safeTransferFrom(schedule.funder, schedule.owner, order.sellAmount);
         emit Pulled(id, digest, order.sellAmount);

test/ComposableCowPoller.t.sol

@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: GPL-3.0
 pragma solidity >=0.8.0 <0.9.0;
 
+import {GPv2Order} from "cowprotocol/contracts/libraries/GPv2Order.sol";
+
 import {IConditionalOrder, IValueFactory, BaseComposableCoWTest} from "test/ComposableCoW.base.t.sol";
 
 import {TWAP} from "src/types/twap/TWAP.sol";
@@ -166,68 +168,88 @@ contract ComposableCowPollerTest is BaseComposableCoWTest {
         );
     }
 
-    /// @dev A single poll moves exactly the current part into the owner.
-    function test_pollFunds_fundsCurrentPart() public {
+    /// @dev Funds move unconditionally: even if the owner already holds a balance (e.g. from another
+    ///      concurrent order), the full part is still pulled, so orders never share funding.
+    function test_pollFunds_movesFullAmountUnconditionally() public {
         (, bytes32 ctx, bytes32 id) = _setupSchedule();
         vm.warp(_t0(ctx));
 
-        assertEq(
-            token0.balanceOf(address(safe1)),
-            0,
-            "owner empty before pull"
-        );
+        // The owner already holds an unrelated balance (e.g. funded for another order).
+        deal(address(token0), address(safe1), TWAP_PART_AMOUNT);
 
         poller.pollFunds(id);
 
         assertEq(
-            token0.balanceOf(address(safe1)),
-            PART,
-            "owner funded with exactly one part"
-        );
-        assertEq(
-            token0.balanceOf(funder),
-            PART * N - PART,
-            "exactly one part left the funder"
+            token0.balanceOf(address(safe1)), TWAP_PART_AMOUNT * 2, "full part pulled on top of the existing balance"
         );
+        assertEq(token0.balanceOf(funder), TWAP_PART_AMOUNT * N - TWAP_PART_AMOUNT, "a full part left the funder");
     }
 
-    /// @dev Funds move unconditionally: even if the owner already holds a balance (e.g. from another
-    ///      concurrent order), the full part is still pulled, so orders never share funding.
-    function test_pollFunds_movesFullAmountUnconditionally() public {
+    /// @dev A repeated call in the same part is a no-op, even after settlement drains the owner.
+    function test_pollFunds_idempotentWithinPartAfterSettlement() public {
         (, bytes32 ctx, bytes32 id) = _setupSchedule();
         vm.warp(_t0(ctx));
 
-        // The owner already holds an unrelated balance (e.g. funded for another order).
-        deal(address(token0), address(safe1), PART);
-
         poller.pollFunds(id);
 
-        assertEq(
-            token0.balanceOf(address(safe1)),
-            PART * 2,
-            "full part pulled on top of the existing balance"
-        );
-        assertEq(
-            token0.balanceOf(funder),
-            PART * N - PART,
-            "a full part left the funder"
-        );
+        vm.prank(address(safe1));
+        assertTrue(token0.transfer(bob.addr, TWAP_PART_AMOUNT), "part settled");
+        assertEq(token0.balanceOf(address(safe1)), 0, "part settled");
+
+        poller.pollFunds(id); // no-op: this part has already been funded
+
+        assertEq(token0.balanceOf(address(safe1)), 0, "next part not funded early");
+        assertEq(token0.balanceOf(funder), TWAP_PART_AMOUNT * N - TWAP_PART_AMOUNT, "no extra pull");
     }
 
-    /// @dev Repeated calls for the same part are a no-op (guarded by the order digest).
-    function test_pollFunds_idempotentWithinPart() public {
+    /// @dev A handler returning A, then B, then A cannot refund A, even after schedule registration.
+    function test_pollFunds_doesNotRefundEarlierDigestAfterReregister() public {
         (, bytes32 ctx, bytes32 id) = _setupSchedule();
         vm.warp(_t0(ctx));
 
+        bytes memory staticInput = abi.encode(_bundle());
+        bytes memory handlerCall = abi.encodeCall(
+            IConditionalOrderGenerator.getTradeableOrder, (address(safe1), address(poller), ctx, staticInput, bytes(""))
+        );
+        GPv2Order.Data memory orderA =
+            twap.getTradeableOrder(address(safe1), address(poller), ctx, staticInput, bytes(""));
+        GPv2Order.Data memory orderB = abi.decode(abi.encode(orderA), (GPv2Order.Data));
+        orderB.appData = keccak256("second valid order");
+
+        vm.mockCall(address(twap), handlerCall, abi.encode(orderA));
         poller.pollFunds(id);
-        poller.pollFunds(id); // no-op: this part has already been funded
+        vm.prank(address(safe1));
+        assertTrue(token0.transfer(bob.addr, TWAP_PART_AMOUNT), "first order settled");
 
+        vm.clearMockedCalls();
+        vm.mockCall(address(twap), handlerCall, abi.encode(orderB));
+        poller.pollFunds(id);
+        vm.prank(address(safe1));
+        assertTrue(token0.transfer(bob.addr, TWAP_PART_AMOUNT), "second order settled");
+
+        vm.prank(funder);
         assertEq(
-            token0.balanceOf(address(safe1)),
-            PART,
-            "still exactly one part"
+            poller.register(
+                ComposableCowPoller.Schedule({
+                    handler: IConditionalOrderGenerator(address(twap)),
+                    funder: funder,
+                    owner: address(safe1),
+                    salt: SALT,
+                    staticInput: staticInput
+                })
+            ),
+            id,
+            "same schedule id"
         );
-        assertEq(token0.balanceOf(funder), PART * N - PART, "no extra pull");
+
+        vm.clearMockedCalls();
+        vm.mockCall(address(twap), handlerCall, abi.encode(orderA));
+        poller.pollFunds(id);
+
+        assertEq(token0.balanceOf(address(safe1)), 0, "first order not funded twice");
+        assertEq(token0.balanceOf(funder), TWAP_PART_AMOUNT, "only two distinct orders funded");
+        assertTrue(poller.funded(id, GPv2Order.hash(orderA, composableCow.domainSeparator())));
+        assertTrue(poller.funded(id, GPv2Order.hash(orderB, composableCow.domainSeparator())));
     }
 
     /// @dev A failed ERC-20 transfer must not mark this part as funded.
@@ -237,19 +259,12 @@ contract ComposableCowPollerTest is BaseComposableCoWTest {
 
         vm.mockCall(
             address(token0),
-            abi.encodeWithSelector(
-                token0.transferFrom.selector,
-                funder,
-                address(safe1),
-                PART
-            ),
+            abi.encodeWithSelector(token0.transferFrom.selector, funder, address(safe1), TWAP_PART_AMOUNT),
             abi.encode(false)
         );
 
         vm.expectRevert(bytes("GPv2: failed transferFrom"));
         poller.pollFunds(id);
-
-        assertEq(poller.lastFunded(id), bytes32(0), "failed pull is not recorded");
     }
 
     /// @dev The headline flow: each part is funded JIT and the owner holds nothing in between.
@@ -260,61 +275,22 @@ contract ComposableCowPollerTest is BaseComposableCoWTest {
         for (uint256 part = 0; part < N; part++) {
             vm.warp(t0 + part * FREQ);
 
-            assertEq(
-                token0.balanceOf(address(safe1)),
-                0,
-                "owner empty before part"
-            );
+            assertEq(token0.balanceOf(address(safe1)), 0, "owner empty before part");
             poller.pollFunds(id);
-            assertEq(token0.balanceOf(address(safe1)), PART, "part funded");
+            assertEq(token0.balanceOf(address(safe1)), TWAP_PART_AMOUNT, "part funded");
 
             // Simulate the part settling: the owner's balance is consumed.
             vm.prank(address(safe1));
-            token0.transfer(bob.addr, PART);
+            assertTrue(token0.transfer(bob.addr, TWAP_PART_AMOUNT), "part settled");
 
             assertEq(
                 token0.balanceOf(funder),
-                PART * N - PART * (part + 1),
+                TWAP_PART_AMOUNT * N - TWAP_PART_AMOUNT * (part + 1),
                 "one part funded per window"
             );
         }
     }
 
-    /// @dev The anti-premature-execution guard: once a part is funded, the *next* part's funds
-    ///      cannot be pulled until time advances into its window — even after the part settles and
-    ///      drains the owner. Without the per-order guard, the drained owner would be refilled
-    ///      immediately and that balance would be sold as the next part, a full interval early.
-    function test_pollFunds_cannotFundFuturePartEarly() public {
-        (, bytes32 ctx, bytes32 id) = _setupSchedule();
-        vm.warp(_t0(ctx)); // part 0 window
-
-        poller.pollFunds(id);
-        assertEq(token0.balanceOf(address(safe1)), PART, "part 0 funded");
-
-        // Simulate the part settling: the owner's balance is consumed.
-        vm.prank(address(safe1));
-        token0.transfer(bob.addr, PART);
-        assertEq(
-            token0.balanceOf(address(safe1)),
-            0,
-            "owner drained by the fill"
-        );
-
-        // Still inside part 0's window: a fresh pull must NOT refill.
-        poller.pollFunds(id);
-
-        assertEq(
-            token0.balanceOf(address(safe1)),
-            0,
-            "next part not funded early"
-        );
-        assertEq(
-            token0.balanceOf(funder),
-            PART * N - PART,
-            "exactly one part ever left the funder"
-        );
-    }
-
     /// @dev The pull is bounded to the schedule window: after it ends, `getTradeableOrder` reverts.
     function test_pollFunds_RevertWhen_scheduleEnded() public {
         (, bytes32 ctx, bytes32 id) = _setupSchedule();