fix: [TRST-M-3] Add nonce-based replay protection

matiasedgeandnode · matiasedgeandnode · commit e5fefbe0b1e6 · 2025-08-03T15:40:28.000-03:00
Fixes signature replay attack vulnerability where old signed RecurringCollectionAgreementUpdate
messages could be replayed to revert agreements to previous terms.

 ## Changes

- Add `nonce` field to RecurringCollectionAgreementUpdate struct (uint32)
- Add `updateNonce` field to AgreementData struct to track current nonce
- Add nonce validation in RecurringCollector.update() to ensure sequential updates
- Update EIP712_RCAU_TYPEHASH to include nonce field
- Add comprehensive tests for nonce validation and replay attack prevention
- Add RecurringCollectorInvalidUpdateNonce error for invalid nonce attempts

 ## Implementation Details

- Nonces start at 0 when agreement is accepted
- Each update must use current nonce + 1
- Nonce is incremented after successful update
- Uses uint32 for gas optimization (supports 4B+ updates per agreement)
- Single source of truth: nonce stored in AgreementData struct
diff --git a/packages/horizon/contracts/interfaces/IRecurringCollector.sol b/packages/horizon/contracts/interfaces/IRecurringCollector.sol
@@ -90,6 +90,7 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
      * except for the first collection
      * @param minSecondsPerCollection The minimum amount of seconds that must pass between collections
      * @param maxSecondsPerCollection The maximum amount of seconds that can pass between collections
+     * @param nonce The nonce for preventing replay attacks (must be current nonce + 1)
      * @param metadata Arbitrary metadata to extend functionality if a data service requires it
      */
     struct RecurringCollectionAgreementUpdate {
@@ -100,6 +101,7 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
         uint256 maxOngoingTokensPerSecond;
         uint32 minSecondsPerCollection;
         uint32 maxSecondsPerCollection;
+        uint32 nonce;
         bytes metadata;
     }
 
@@ -118,6 +120,7 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
      * except for the first collection
      * @param minSecondsPerCollection The minimum amount of seconds that must pass between collections
      * @param maxSecondsPerCollection The maximum amount of seconds that can pass between collections
+     * @param updateNonce The current nonce for updates (prevents replay attacks)
      * @param canceledAt The timestamp when the agreement was canceled
      * @param state The state of the agreement
      */
@@ -132,6 +135,7 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
         uint256 maxOngoingTokensPerSecond;
         uint32 minSecondsPerCollection;
         uint32 maxSecondsPerCollection;
+        uint32 updateNonce;
         uint64 canceledAt;
         AgreementState state;
     }
@@ -357,6 +361,14 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
      */
     error RecurringCollectorCollectionTooLate(bytes16 agreementId, uint64 secondsSinceLast, uint32 maxSeconds);
 
+    /**
+     * @notice Thrown when calling update() with an invalid nonce
+     * @param agreementId The agreement ID
+     * @param expected The expected nonce
+     * @param provided The provided nonce
+     */
+    error RecurringCollectorInvalidUpdateNonce(bytes16 agreementId, uint32 expected, uint32 provided);
+
     /**
      * @dev Accept an indexing agreement.
      * @param signedRCA The signed Recurring Collection Agreement which is to be accepted.
diff --git a/packages/horizon/contracts/payments/collectors/RecurringCollector.sol b/packages/horizon/contracts/payments/collectors/RecurringCollector.sol
@@ -35,7 +35,7 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
     /// @notice The EIP712 typehash for the RecurringCollectionAgreementUpdate struct
     bytes32 public constant EIP712_RCAU_TYPEHASH =
         keccak256(
-            "RecurringCollectionAgreementUpdate(bytes16 agreementId,uint64 deadline,uint64 endsAt,uint256 maxInitialTokens,uint256 maxOngoingTokensPerSecond,uint32 minSecondsPerCollection,uint32 maxSecondsPerCollection,bytes metadata)"
+            "RecurringCollectionAgreementUpdate(bytes16 agreementId,uint64 deadline,uint64 endsAt,uint256 maxInitialTokens,uint256 maxOngoingTokensPerSecond,uint32 minSecondsPerCollection,uint32 maxSecondsPerCollection,uint32 nonce,bytes metadata)"
         );
 
     /// @notice Tracks agreements
@@ -120,6 +120,7 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
         agreement.maxOngoingTokensPerSecond = signedRCA.rca.maxOngoingTokensPerSecond;
         agreement.minSecondsPerCollection = signedRCA.rca.minSecondsPerCollection;
         agreement.maxSecondsPerCollection = signedRCA.rca.maxSecondsPerCollection;
+        agreement.updateNonce = 0;
 
         emit AgreementAccepted(
             agreement.dataService,
@@ -193,6 +194,13 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
         // check that the voucher is signed by the payer (or proxy)
         _requireAuthorizedRCAUSigner(signedRCAU, agreement.payer);
 
+        // validate nonce to prevent replay attacks
+        uint32 expectedNonce = agreement.updateNonce + 1;
+        require(
+            signedRCAU.rcau.nonce == expectedNonce,
+            RecurringCollectorInvalidUpdateNonce(signedRCAU.rcau.agreementId, expectedNonce, signedRCAU.rcau.nonce)
+        );
+
         _requireValidCollectionWindowParams(
             signedRCAU.rcau.endsAt,
             signedRCAU.rcau.minSecondsPerCollection,
@@ -205,6 +213,7 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
         agreement.maxOngoingTokensPerSecond = signedRCAU.rcau.maxOngoingTokensPerSecond;
         agreement.minSecondsPerCollection = signedRCAU.rcau.minSecondsPerCollection;
         agreement.maxSecondsPerCollection = signedRCAU.rcau.maxSecondsPerCollection;
+        agreement.updateNonce = signedRCAU.rcau.nonce;
 
         emit AgreementUpdated(
             agreement.dataService,
@@ -482,6 +491,7 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
                         _rcau.maxOngoingTokensPerSecond,
                         _rcau.minSecondsPerCollection,
                         _rcau.maxSecondsPerCollection,
+                        _rcau.nonce,
                         keccak256(_rcau.metadata)
                     )
                 )
diff --git a/packages/horizon/test/unit/payments/recurring-collector/RecurringCollectorHelper.t.sol b/packages/horizon/test/unit/payments/recurring-collector/RecurringCollectorHelper.t.sol
@@ -45,6 +45,17 @@ contract RecurringCollectorHelper is AuthorizableHelper, Bounder {
         return signedRCAU;
     }
 
+    function generateSignedRCAUWithCorrectNonce(
+        IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau,
+        uint256 signerPrivateKey
+    ) public view returns (IRecurringCollector.SignedRCAU memory) {
+        // Automatically set the correct nonce based on current agreement state
+        IRecurringCollector.AgreementData memory agreement = collector.getAgreement(rcau.agreementId);
+        rcau.nonce = agreement.updateNonce + 1;
+
+        return generateSignedRCAU(rcau, signerPrivateKey);
+    }
+
     function withElapsedAcceptDeadline(
         IRecurringCollector.RecurringCollectionAgreement memory rca
     ) public view returns (IRecurringCollector.RecurringCollectionAgreement memory) {
diff --git a/packages/horizon/test/unit/payments/recurring-collector/update.t.sol b/packages/horizon/test/unit/payments/recurring-collector/update.t.sol
@@ -76,7 +76,7 @@ contract RecurringCollectorUpdateTest is RecurringCollectorSharedTest {
         );
         rcau.agreementId = accepted.rca.agreementId;
 
-        IRecurringCollector.SignedRCAU memory signedRCAU = _recurringCollectorHelper.generateSignedRCAU(
+        IRecurringCollector.SignedRCAU memory signedRCAU = _recurringCollectorHelper.generateSignedRCAUWithCorrectNonce(
             rcau,
             signerKey
         );
@@ -124,6 +124,8 @@ contract RecurringCollectorUpdateTest is RecurringCollectorSharedTest {
             fuzzyTestUpdate.rcau
         );
         rcau.agreementId = accepted.rca.agreementId;
+        // Don't use fuzzed nonce - use correct nonce for first update
+        rcau.nonce = 1;
         IRecurringCollector.SignedRCAU memory signedRCAU = _recurringCollectorHelper.generateSignedRCAU(
             rcau,
             signerKey
@@ -151,6 +153,151 @@ contract RecurringCollectorUpdateTest is RecurringCollectorSharedTest {
         assertEq(rcau.maxOngoingTokensPerSecond, agreement.maxOngoingTokensPerSecond);
         assertEq(rcau.minSecondsPerCollection, agreement.minSecondsPerCollection);
         assertEq(rcau.maxSecondsPerCollection, agreement.maxSecondsPerCollection);
+        assertEq(rcau.nonce, agreement.updateNonce);
+    }
+
+    function test_Update_Revert_WhenInvalidNonce_TooLow(FuzzyTestUpdate calldata fuzzyTestUpdate) public {
+        (IRecurringCollector.SignedRCA memory accepted, uint256 signerKey) = _sensibleAuthorizeAndAccept(
+            fuzzyTestUpdate.fuzzyTestAccept
+        );
+        IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau = _recurringCollectorHelper.sensibleRCAU(
+            fuzzyTestUpdate.rcau
+        );
+        rcau.agreementId = accepted.rca.agreementId;
+        rcau.nonce = 0; // Invalid: should be 1 for first update
+
+        IRecurringCollector.SignedRCAU memory signedRCAU = _recurringCollectorHelper.generateSignedRCAU(
+            rcau,
+            signerKey
+        );
+
+        bytes memory expectedErr = abi.encodeWithSelector(
+            IRecurringCollector.RecurringCollectorInvalidUpdateNonce.selector,
+            rcau.agreementId,
+            1, // expected
+            0 // provided
+        );
+        vm.expectRevert(expectedErr);
+        vm.prank(accepted.rca.dataService);
+        _recurringCollector.update(signedRCAU);
+    }
+
+    function test_Update_Revert_WhenInvalidNonce_TooHigh(FuzzyTestUpdate calldata fuzzyTestUpdate) public {
+        (IRecurringCollector.SignedRCA memory accepted, uint256 signerKey) = _sensibleAuthorizeAndAccept(
+            fuzzyTestUpdate.fuzzyTestAccept
+        );
+        IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau = _recurringCollectorHelper.sensibleRCAU(
+            fuzzyTestUpdate.rcau
+        );
+        rcau.agreementId = accepted.rca.agreementId;
+        rcau.nonce = 5; // Invalid: should be 1 for first update
+
+        IRecurringCollector.SignedRCAU memory signedRCAU = _recurringCollectorHelper.generateSignedRCAU(
+            rcau,
+            signerKey
+        );
+
+        bytes memory expectedErr = abi.encodeWithSelector(
+            IRecurringCollector.RecurringCollectorInvalidUpdateNonce.selector,
+            rcau.agreementId,
+            1, // expected
+            5 // provided
+        );
+        vm.expectRevert(expectedErr);
+        vm.prank(accepted.rca.dataService);
+        _recurringCollector.update(signedRCAU);
+    }
+
+    function test_Update_Revert_WhenReplayAttack(FuzzyTestUpdate calldata fuzzyTestUpdate) public {
+        (IRecurringCollector.SignedRCA memory accepted, uint256 signerKey) = _sensibleAuthorizeAndAccept(
+            fuzzyTestUpdate.fuzzyTestAccept
+        );
+        IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau1 = _recurringCollectorHelper.sensibleRCAU(
+            fuzzyTestUpdate.rcau
+        );
+        rcau1.agreementId = accepted.rca.agreementId;
+        rcau1.nonce = 1;
+
+        // First update succeeds
+        IRecurringCollector.SignedRCAU memory signedRCAU1 = _recurringCollectorHelper.generateSignedRCAU(
+            rcau1,
+            signerKey
+        );
+        vm.prank(accepted.rca.dataService);
+        _recurringCollector.update(signedRCAU1);
+
+        // Second update with different terms and nonce 2 succeeds
+        IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau2 = rcau1;
+        rcau2.nonce = 2;
+        rcau2.maxOngoingTokensPerSecond = rcau1.maxOngoingTokensPerSecond * 2; // Different terms
+
+        IRecurringCollector.SignedRCAU memory signedRCAU2 = _recurringCollectorHelper.generateSignedRCAU(
+            rcau2,
+            signerKey
+        );
+        vm.prank(accepted.rca.dataService);
+        _recurringCollector.update(signedRCAU2);
+
+        // Attempting to replay first update should fail
+        bytes memory expectedErr = abi.encodeWithSelector(
+            IRecurringCollector.RecurringCollectorInvalidUpdateNonce.selector,
+            rcau1.agreementId,
+            3, // expected (current nonce + 1)
+            1 // provided (old nonce)
+        );
+        vm.expectRevert(expectedErr);
+        vm.prank(accepted.rca.dataService);
+        _recurringCollector.update(signedRCAU1);
+    }
+
+    function test_Update_OK_NonceIncrementsCorrectly(FuzzyTestUpdate calldata fuzzyTestUpdate) public {
+        (IRecurringCollector.SignedRCA memory accepted, uint256 signerKey) = _sensibleAuthorizeAndAccept(
+            fuzzyTestUpdate.fuzzyTestAccept
+        );
+
+        // Initial nonce should be 0
+        IRecurringCollector.AgreementData memory initialAgreement = _recurringCollector.getAgreement(
+            accepted.rca.agreementId
+        );
+        assertEq(initialAgreement.updateNonce, 0);
+
+        // First update with nonce 1
+        IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau1 = _recurringCollectorHelper.sensibleRCAU(
+            fuzzyTestUpdate.rcau
+        );
+        rcau1.agreementId = accepted.rca.agreementId;
+        rcau1.nonce = 1;
+
+        IRecurringCollector.SignedRCAU memory signedRCAU1 = _recurringCollectorHelper.generateSignedRCAU(
+            rcau1,
+            signerKey
+        );
+        vm.prank(accepted.rca.dataService);
+        _recurringCollector.update(signedRCAU1);
+
+        // Verify nonce incremented to 1
+        IRecurringCollector.AgreementData memory updatedAgreement1 = _recurringCollector.getAgreement(
+            accepted.rca.agreementId
+        );
+        assertEq(updatedAgreement1.updateNonce, 1);
+
+        // Second update with nonce 2
+        IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau2 = rcau1;
+        rcau2.nonce = 2;
+        rcau2.maxOngoingTokensPerSecond = rcau1.maxOngoingTokensPerSecond * 2; // Different terms
+
+        IRecurringCollector.SignedRCAU memory signedRCAU2 = _recurringCollectorHelper.generateSignedRCAU(
+            rcau2,
+            signerKey
+        );
+        vm.prank(accepted.rca.dataService);
+        _recurringCollector.update(signedRCAU2);
+
+        // Verify nonce incremented to 2
+        IRecurringCollector.AgreementData memory updatedAgreement2 = _recurringCollector.getAgreement(
+            accepted.rca.agreementId
+        );
+        assertEq(updatedAgreement2.updateNonce, 2);
     }
 
     /* solhint-enable graph/func-name-mixedcase */
diff --git a/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/shared.t.sol b/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/shared.t.sol
@@ -255,11 +255,11 @@ contract SubgraphServiceIndexingAgreementSharedTest is SubgraphServiceTest, Boun
         Context storage _ctx,
         IRecurringCollector.RecurringCollectionAgreement memory _rca
     ) internal view returns (IRecurringCollector.SignedRCAU memory) {
-        return
-            _recurringCollectorHelper.generateSignedRCAU(
-                _generateAcceptableRecurringCollectionAgreementUpdate(_ctx, _rca),
-                _ctx.payer.signerPrivateKey
-            );
+        IRecurringCollector.RecurringCollectionAgreementUpdate
+            memory rcau = _generateAcceptableRecurringCollectionAgreementUpdate(_ctx, _rca);
+        // Set correct nonce for first update (should be 1)
+        rcau.nonce = 1;
+        return _recurringCollectorHelper.generateSignedRCAU(rcau, _ctx.payer.signerPrivateKey);
     }
 
     function _generateAcceptableRecurringCollectionAgreementUpdate(
diff --git a/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/update.t.sol b/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/update.t.sol
@@ -127,6 +127,8 @@ contract SubgraphServiceIndexingAgreementUpgradeTest is SubgraphServiceIndexingA
         IRecurringCollector.RecurringCollectionAgreementUpdate
             memory acceptableUpdate = _generateAcceptableRecurringCollectionAgreementUpdate(ctx, accepted.rca);
         acceptableUpdate.metadata = bytes("invalid");
+        // Set correct nonce for first update (should be 1)
+        acceptableUpdate.nonce = 1;
         IRecurringCollector.SignedRCAU memory unacceptableUpdate = _recurringCollectorHelper.generateSignedRCAU(
             acceptableUpdate,
             ctx.payer.signerPrivateKey
