diff --git a/packages/horizon/contracts/interfaces/IRecurringCollector.sol b/packages/horizon/contracts/interfaces/IRecurringCollector.sol index cb31125c3..19388062b 100644 --- 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 index 945f34279..e16db74db 100644 --- 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 index b3ccbc3b8..611f554e7 100644 --- 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 index 4fd8af1e7..1676fc0bc 100644 --- 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 index 2a5b2385a..c23727f20 100644 --- 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 index 336ef97de..ebd9200d1 100644 --- 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