[audit-13] fix: [TRST-M-2] improve _getCollectionInfo()

packages/horizon/contracts/interfaces/IRecurringCollector.sol

@@ -28,6 +28,14 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
         ThirdParty
     }
 
+    /// @notice Reasons why an agreement is not collectable
+    enum AgreementNotCollectableReason {
+        None,
+        InvalidAgreementState,
+        ZeroCollectionSeconds,
+        InvalidTemporalWindow
+    }
+
     /**
      * @notice A representation of a signed Recurring Collection Agreement (RCA)
      * @param rca The Recurring Collection Agreement to be signed
@@ -303,6 +311,13 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
      */
     error RecurringCollectorAgreementIncorrectState(bytes16 agreementId, AgreementState incorrectState);
 
+    /**
+     * @notice Thrown when an agreement is not collectable
+     * @param agreementId The agreement ID
+     * @param reason The reason why the agreement is not collectable
+     */
+    error RecurringCollectorAgreementNotCollectable(bytes16 agreementId, AgreementNotCollectableReason reason);
+
     /**
      * @notice Thrown when accepting an agreement with an address that is not set
      */
@@ -440,10 +455,11 @@ interface IRecurringCollector is IAuthorizable, IPaymentsCollector {
      * @return isCollectable Whether the agreement is in a valid state that allows collection attempts,
      * not that there are necessarily funds available to collect.
      * @return collectionSeconds The valid collection duration in seconds (0 if not collectable)
+     * @return reason The reason why the agreement is not collectable (None if collectable)
      */
     function getCollectionInfo(
         AgreementData memory agreement
-    ) external view returns (bool isCollectable, uint256 collectionSeconds);
+    ) external view returns (bool isCollectable, uint256 collectionSeconds, AgreementNotCollectableReason reason);
 
     /**
      * @notice Generate a deterministic agreement ID from agreement parameters

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

@@ -269,7 +269,7 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
     /// @inheritdoc IRecurringCollector
     function getCollectionInfo(
         AgreementData memory agreement
-    ) external view returns (bool isCollectable, uint256 collectionSeconds) {
+    ) external view returns (bool isCollectable, uint256 collectionSeconds, AgreementNotCollectableReason reason) {
         return _getCollectionInfo(agreement);
     }
 
@@ -309,14 +309,11 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
     ) private returns (uint256) {
         AgreementData storage agreement = _getAgreementStorage(_params.agreementId);
 
-        // Check if agreement exists first (for unknown agreements)
-        (bool isCollectable, uint256 collectionSeconds) = _getCollectionInfo(agreement);
-        require(isCollectable, RecurringCollectorAgreementIncorrectState(_params.agreementId, agreement.state));
-
-        require(
-            collectionSeconds > 0,
-            RecurringCollectorZeroCollectionSeconds(_params.agreementId, block.timestamp, agreement.lastCollectionAt)
+        // Check if agreement is collectable first
+        (bool isCollectable, uint256 collectionSeconds, AgreementNotCollectableReason reason) = _getCollectionInfo(
+            agreement
         );
+        require(isCollectable, RecurringCollectorAgreementNotCollectable(_params.agreementId, reason));
 
         require(
             msg.sender == agreement.dataService,
@@ -583,17 +580,17 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
      * @param _agreement The agreement data
      * @return isCollectable Whether the agreement can be collected from
      * @return collectionSeconds The valid collection duration in seconds (0 if not collectable)
+     * @return reason The reason why the agreement is not collectable (None if collectable)
      */
     function _getCollectionInfo(
         AgreementData memory _agreement
-    ) private view returns (bool isCollectable, uint256 collectionSeconds) {
+    ) private view returns (bool, uint256, AgreementNotCollectableReason) {
         // Check if agreement is in collectable state
-        isCollectable =
-            _agreement.state == AgreementState.Accepted ||
+        bool hasValidState = _agreement.state == AgreementState.Accepted ||
             _agreement.state == AgreementState.CanceledByPayer;
 
-        if (!isCollectable) {
-            return (false, 0);
+        if (!hasValidState) {
+            return (false, 0, AgreementNotCollectableReason.InvalidAgreementState);
         }
 
         bool canceledOrElapsed = _agreement.state == AgreementState.CanceledByPayer ||
@@ -606,11 +603,14 @@ contract RecurringCollector is EIP712, GraphDirectory, Authorizable, IRecurringC
         uint256 collectionStart = _agreementCollectionStartAt(_agreement);
 
         if (collectionEnd < collectionStart) {
-            return (false, 0);
+            return (false, 0, AgreementNotCollectableReason.InvalidTemporalWindow);
+        }
+
+        if (collectionStart == collectionEnd) {
+            return (false, 0, AgreementNotCollectableReason.ZeroCollectionSeconds);
         }
 
-        collectionSeconds = collectionEnd - collectionStart;
-        return (isCollectable, collectionSeconds);
+        return (true, collectionEnd - collectionStart, AgreementNotCollectableReason.None);
     }
 
     /**

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

@@ -90,9 +90,9 @@ contract RecurringCollectorCollectTest is RecurringCollectorSharedTest {
         bytes memory data = _generateCollectData(fuzzy.collectParams);
 
         bytes memory expectedErr = abi.encodeWithSelector(
-            IRecurringCollector.RecurringCollectorAgreementIncorrectState.selector,
+            IRecurringCollector.RecurringCollectorAgreementNotCollectable.selector,
             fuzzy.collectParams.agreementId,
-            IRecurringCollector.AgreementState.NotAccepted
+            IRecurringCollector.AgreementNotCollectableReason.InvalidAgreementState
         );
         vm.expectRevert(expectedErr);
         vm.prank(dataService);
@@ -116,9 +116,9 @@ contract RecurringCollectorCollectTest is RecurringCollectorSharedTest {
         bytes memory data = _generateCollectData(collectParams);
 
         bytes memory expectedErr = abi.encodeWithSelector(
-            IRecurringCollector.RecurringCollectorAgreementIncorrectState.selector,
+            IRecurringCollector.RecurringCollectorAgreementNotCollectable.selector,
             collectParams.agreementId,
-            IRecurringCollector.AgreementState.CanceledByServiceProvider
+            IRecurringCollector.AgreementNotCollectableReason.InvalidAgreementState
         );
         vm.expectRevert(expectedErr);
         vm.prank(accepted.rca.dataService);

packages/subgraph-service/contracts/libraries/IndexingAgreement.sol

@@ -554,7 +554,7 @@ library IndexingAgreement {
             IndexingAgreementNotAuthorized(params.agreementId, params.indexer)
         );
         // Get collection info from RecurringCollector (single source of truth for temporal logic)
-        (bool isCollectable, uint256 collectionSeconds) = _directory().recurringCollector().getCollectionInfo(
+        (bool isCollectable, uint256 collectionSeconds, ) = _directory().recurringCollector().getCollectionInfo(
             wrapper.collectorAgreement
         );
         require(_isValid(wrapper) && isCollectable, IndexingAgreementNotCollectable(params.agreementId));

packages/subgraph-service/test/unit/subgraphService/indexing-agreement/collect.t.sol

@@ -58,6 +58,8 @@ contract SubgraphServiceIndexingAgreementCollectTest is SubgraphServiceIndexingA
         );
         _expectCollectCallAndEmit(data, indexerState, accepted, acceptedAgreementId, tokensCollected, entities, poi);
 
+        skip(1); // To make agreement collectable
+
         subgraphService.collect(
             indexerState.addr,
             IGraphPayments.PaymentTypes.IndexingFee,
@@ -215,6 +217,8 @@ contract SubgraphServiceIndexingAgreementCollectTest is SubgraphServiceIndexingA
         );
         vm.expectRevert(expectedErr);
 
+        skip(1); // To make agreement collectable
+
         subgraphService.collect(
             indexerState.addr,
             IGraphPayments.PaymentTypes.IndexingFee,