[audit-07] fix: [TRST-L-3] Add deterministic agreement ID

packages/horizon/contracts/interfaces/IRecurringCollector.sol

@@ -40,7 +40,6 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
 
     /**
      * @notice The Recurring Collection Agreement (RCA)
-     * @param agreementId The agreement ID of the RCA
      * @param deadline The deadline for accepting the RCA
      * @param endsAt The timestamp when the agreement ends
      * @param payer The address of the payer the RCA was issued by
@@ -52,11 +51,11 @@ 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 A unique nonce for preventing collisions (user-chosen)
      * @param metadata Arbitrary metadata to extend functionality if a data service requires it
      *
      */
     struct RecurringCollectionAgreement {
-        bytes16 agreementId;
         uint64 deadline;
         uint64 endsAt;
         address payer;
@@ -66,6 +65,7 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
         uint256 maxOngoingTokensPerSecond;
         uint32 minSecondsPerCollection;
         uint32 maxSecondsPerCollection;
+        uint256 nonce;
         bytes metadata;
     }
 
@@ -372,8 +372,9 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
     /**
      * @dev Accept an indexing agreement.
      * @param signedRCA The signed Recurring Collection Agreement which is to be accepted.
+     * @return agreementId The deterministically generated agreement ID
      */
-    function accept(SignedRCA calldata signedRCA) external;
+    function accept(SignedRCA calldata signedRCA) external returns (bytes16 agreementId);
 
     /**
      * @dev Cancel an indexing agreement.
@@ -433,4 +434,21 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
     function getCollectionInfo(
         AgreementData memory agreement
     ) external view returns (bool isCollectable, uint256 collectionSeconds);
+
+    /**
+     * @notice Generate a deterministic agreement ID from agreement parameters
+     * @param payer The address of the payer
+     * @param dataService The address of the data service
+     * @param serviceProvider The address of the service provider
+     * @param deadline The deadline for accepting the agreement
+     * @param nonce A unique nonce for preventing collisions
+     * @return agreementId The deterministically generated agreement ID
+     */
+    function generateAgreementId(
+        address payer,
+        address dataService,
+        address serviceProvider,
+        uint64 deadline,
+        uint256 nonce
+    ) external pure returns (bytes16 agreementId);
 }

packages/horizon/contracts/payments/collectors/RecurringCollector.sol

@@ -29,7 +29,7 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
     /// @notice The EIP712 typehash for the RecurringCollectionAgreement struct
     bytes32 public constant EIP712_RCA_TYPEHASH =
         keccak256(
-            "RecurringCollectionAgreement(bytes16 agreementId,uint256 deadline,uint256 endsAt,address payer,address dataService,address serviceProvider,uint256 maxInitialTokens,uint256 maxOngoingTokensPerSecond,uint32 minSecondsPerCollection,uint32 maxSecondsPerCollection,bytes metadata)"
+            "RecurringCollectionAgreement(uint64 deadline,uint64 endsAt,address payer,address dataService,address serviceProvider,uint256 maxInitialTokens,uint256 maxOngoingTokensPerSecond,uint32 minSecondsPerCollection,uint32 maxSecondsPerCollection,uint256 nonce,bytes metadata)"
         );
 
     /// @notice The EIP712 typehash for the RecurringCollectionAgreementUpdate struct
@@ -75,8 +75,16 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
      * See {IRecurringCollector.accept}.
      * @dev Caller must be the data service the RCA was issued to.
      */
-    function accept(SignedRCA calldata signedRCA) external {
-        require(signedRCA.rca.agreementId != bytes16(0), RecurringCollectorAgreementIdZero());
+    function accept(SignedRCA calldata signedRCA) external returns (bytes16) {
+        bytes16 agreementId = _generateAgreementId(
+            signedRCA.rca.payer,
+            signedRCA.rca.dataService,
+            signedRCA.rca.serviceProvider,
+            signedRCA.rca.deadline,
+            signedRCA.rca.nonce
+        );
+
+        require(agreementId != bytes16(0), RecurringCollectorAgreementIdZero());
         require(
             msg.sender == signedRCA.rca.dataService,
             RecurringCollectorUnauthorizedCaller(msg.sender, signedRCA.rca.dataService)
@@ -102,11 +110,11 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
             signedRCA.rca.maxSecondsPerCollection
         );
 
-        AgreementData storage agreement = _getAgreementStorage(signedRCA.rca.agreementId);
+        AgreementData storage agreement = _getAgreementStorage(agreementId);
         // check that the agreement is not already accepted
         require(
             agreement.state == AgreementState.NotAccepted,
-            RecurringCollectorAgreementIncorrectState(signedRCA.rca.agreementId, agreement.state)
+            RecurringCollectorAgreementIncorrectState(agreementId, agreement.state)
         );
 
         // accept the agreement
@@ -126,14 +134,16 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
             agreement.dataService,
             agreement.payer,
             agreement.serviceProvider,
-            signedRCA.rca.agreementId,
+            agreementId,
             agreement.acceptedAt,
             agreement.endsAt,
             agreement.maxInitialTokens,
             agreement.maxOngoingTokensPerSecond,
             agreement.minSecondsPerCollection,
             agreement.maxSecondsPerCollection
         );
+
+        return agreementId;
     }
 
     /**
@@ -261,6 +271,17 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
         return _getCollectionInfo(agreement);
     }
 
+    /// @inheritdoc IRecurringCollector
+    function generateAgreementId(
+        address payer,
+        address dataService,
+        address serviceProvider,
+        uint64 deadline,
+        uint256 nonce
+    ) external pure returns (bytes16) {
+        return _generateAgreementId(payer, dataService, serviceProvider, deadline, nonce);
+    }
+
     /**
      * @notice Decodes the collect data.
      * @param data The encoded collect parameters.
@@ -457,7 +478,6 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
                 keccak256(
                     abi.encode(
                         EIP712_RCA_TYPEHASH,
-                        _rca.agreementId,
                         _rca.deadline,
                         _rca.endsAt,
                         _rca.payer,
@@ -467,6 +487,7 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
                         _rca.maxOngoingTokensPerSecond,
                         _rca.minSecondsPerCollection,
                         _rca.maxSecondsPerCollection,
+                        _rca.nonce,
                         keccak256(_rca.metadata)
                     )
                 )
@@ -590,4 +611,23 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
     function _agreementCollectionStartAt(AgreementData memory _agreement) private pure returns (uint256) {
         return _agreement.lastCollectionAt > 0 ? _agreement.lastCollectionAt : _agreement.acceptedAt;
     }
+
+    /**
+     * @notice Internal function to generate deterministic agreement ID
+     * @param _payer The address of the payer
+     * @param _dataService The address of the data service
+     * @param _serviceProvider The address of the service provider
+     * @param _deadline The deadline for accepting the agreement
+     * @param _nonce A unique nonce for preventing collisions
+     * @return agreementId The deterministically generated agreement ID
+     */
+    function _generateAgreementId(
+        address _payer,
+        address _dataService,
+        address _serviceProvider,
+        uint64 _deadline,
+        uint256 _nonce
+    ) private pure returns (bytes16) {
+        return bytes16(keccak256(abi.encode(_payer, _dataService, _serviceProvider, _deadline, _nonce)));
+    }
 }

packages/horizon/test/unit/payments/recurring-collector/RecurringCollectorHelper.t.sol

@@ -45,17 +45,47 @@ contract RecurringCollectorHelper is AuthorizableHelper, Bounder {
         return signedRCAU;
     }
 
-    function generateSignedRCAUWithCorrectNonce(
+    function generateSignedRCAUForAgreement(
+        bytes16 agreementId,
         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);
+        IRecurringCollector.AgreementData memory agreement = collector.getAgreement(agreementId);
         rcau.nonce = agreement.updateNonce + 1;
 
         return generateSignedRCAU(rcau, signerPrivateKey);
     }
 
+    function generateSignedRCAUWithCorrectNonce(
+        IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau,
+        uint256 signerPrivateKey
+    ) public view returns (IRecurringCollector.SignedRCAU memory) {
+        // This is kept for backwards compatibility but should not be used with new interface
+        // since we can't determine agreementId without it being passed separately
+        return generateSignedRCAU(rcau, signerPrivateKey);
+    }
+
+    function generateSignedRCAWithCalculatedId(
+        IRecurringCollector.RecurringCollectionAgreement memory rca,
+        uint256 signerPrivateKey
+    ) public view returns (IRecurringCollector.SignedRCA memory, bytes16) {
+        // Ensure we have sensible values
+        rca = sensibleRCA(rca);
+
+        // Calculate the agreement ID
+        bytes16 agreementId = collector.generateAgreementId(
+            rca.payer,
+            rca.dataService,
+            rca.serviceProvider,
+            rca.deadline,
+            rca.nonce
+        );
+
+        IRecurringCollector.SignedRCA memory signedRCA = generateSignedRCA(rca, signerPrivateKey);
+        return (signedRCA, agreementId);
+    }
+
     function withElapsedAcceptDeadline(
         IRecurringCollector.RecurringCollectionAgreement memory rca
     ) public view returns (IRecurringCollector.RecurringCollectionAgreement memory) {
@@ -76,11 +106,15 @@ contract RecurringCollectorHelper is AuthorizableHelper, Bounder {
     function sensibleRCA(
         IRecurringCollector.RecurringCollectionAgreement memory rca
     ) public view returns (IRecurringCollector.RecurringCollectionAgreement memory) {
-        vm.assume(rca.agreementId != bytes16(0));
         vm.assume(rca.dataService != address(0));
         vm.assume(rca.payer != address(0));
         vm.assume(rca.serviceProvider != address(0));
 
+        // Ensure we have a nonce if it's zero
+        if (rca.nonce == 0) {
+            rca.nonce = 1;
+        }
+
         rca.minSecondsPerCollection = _sensibleMinSecondsPerCollection(rca.minSecondsPerCollection);
         rca.maxSecondsPerCollection = _sensibleMaxSecondsPerCollection(
             rca.maxSecondsPerCollection,

packages/horizon/test/unit/payments/recurring-collector/accept.t.sol

@@ -20,7 +20,15 @@ contract RecurringCollectorAcceptTest is RecurringCollectorSharedTest {
         IRecurringCollector.SignedRCA memory fuzzySignedRCA,
         uint256 unboundedSkip
     ) public {
-        vm.assume(fuzzySignedRCA.rca.agreementId != bytes16(0));
+        // Generate deterministic agreement ID for validation
+        bytes16 agreementId = _recurringCollector.generateAgreementId(
+            fuzzySignedRCA.rca.payer,
+            fuzzySignedRCA.rca.dataService,
+            fuzzySignedRCA.rca.serviceProvider,
+            fuzzySignedRCA.rca.deadline,
+            fuzzySignedRCA.rca.nonce
+        );
+        vm.assume(agreementId != bytes16(0));
         skip(boundSkip(unboundedSkip, 1, type(uint64).max - block.timestamp));
         fuzzySignedRCA.rca = _recurringCollectorHelper.withElapsedAcceptDeadline(fuzzySignedRCA.rca);
 
@@ -35,11 +43,13 @@ contract RecurringCollectorAcceptTest is RecurringCollectorSharedTest {
     }
 
     function test_Accept_Revert_WhenAlreadyAccepted(FuzzyTestAccept calldata fuzzyTestAccept) public {
-        (IRecurringCollector.SignedRCA memory accepted, ) = _sensibleAuthorizeAndAccept(fuzzyTestAccept);
+        (IRecurringCollector.SignedRCA memory accepted, , bytes16 agreementId) = _sensibleAuthorizeAndAccept(
+            fuzzyTestAccept
+        );
 
         bytes memory expectedErr = abi.encodeWithSelector(
             IRecurringCollector.RecurringCollectorAgreementIncorrectState.selector,
-            accepted.rca.agreementId,
+            agreementId,
             IRecurringCollector.AgreementState.Accepted
         );
         vm.expectRevert(expectedErr);

packages/horizon/test/unit/payments/recurring-collector/cancel.t.sol

@@ -13,22 +13,34 @@ contract RecurringCollectorCancelTest is RecurringCollectorSharedTest {
     /* solhint-disable graph/func-name-mixedcase */
 
     function test_Cancel(FuzzyTestAccept calldata fuzzyTestAccept, uint8 unboundedCanceler) public {
-        _sensibleAuthorizeAndAccept(fuzzyTestAccept);
-        _cancel(fuzzyTestAccept.rca, _fuzzyCancelAgreementBy(unboundedCanceler));
+        (IRecurringCollector.SignedRCA memory accepted, , bytes16 agreementId) = _sensibleAuthorizeAndAccept(
+            fuzzyTestAccept
+        );
+
+        _cancel(accepted.rca, agreementId, _fuzzyCancelAgreementBy(unboundedCanceler));
     }
 
     function test_Cancel_Revert_WhenNotAccepted(
         IRecurringCollector.RecurringCollectionAgreement memory fuzzyRCA,
         uint8 unboundedCanceler
     ) public {
+        // Generate deterministic agreement ID
+        bytes16 agreementId = _recurringCollector.generateAgreementId(
+            fuzzyRCA.payer,
+            fuzzyRCA.dataService,
+            fuzzyRCA.serviceProvider,
+            fuzzyRCA.deadline,
+            fuzzyRCA.nonce
+        );
+
         bytes memory expectedErr = abi.encodeWithSelector(
             IRecurringCollector.RecurringCollectorAgreementIncorrectState.selector,
-            fuzzyRCA.agreementId,
+            agreementId,
             IRecurringCollector.AgreementState.NotAccepted
         );
         vm.expectRevert(expectedErr);
         vm.prank(fuzzyRCA.dataService);
-        _recurringCollector.cancel(fuzzyRCA.agreementId, _fuzzyCancelAgreementBy(unboundedCanceler));
+        _recurringCollector.cancel(agreementId, _fuzzyCancelAgreementBy(unboundedCanceler));
     }
 
     function test_Cancel_Revert_WhenNotDataService(
@@ -38,16 +50,16 @@ contract RecurringCollectorCancelTest is RecurringCollectorSharedTest {
     ) public {
         vm.assume(fuzzyTestAccept.rca.dataService != notDataService);
 
-        _sensibleAuthorizeAndAccept(fuzzyTestAccept);
+        (, , bytes16 agreementId) = _sensibleAuthorizeAndAccept(fuzzyTestAccept);
 
         bytes memory expectedErr = abi.encodeWithSelector(
             IRecurringCollector.RecurringCollectorDataServiceNotAuthorized.selector,
-            fuzzyTestAccept.rca.agreementId,
+            agreementId,
             notDataService
         );
         vm.expectRevert(expectedErr);
         vm.prank(notDataService);
-        _recurringCollector.cancel(fuzzyTestAccept.rca.agreementId, _fuzzyCancelAgreementBy(unboundedCanceler));
+        _recurringCollector.cancel(agreementId, _fuzzyCancelAgreementBy(unboundedCanceler));
     }
     /* solhint-enable graph/func-name-mixedcase */
 }