diff --git a/packages/contracts-test/tests/unit/rewards/rewards-interface.test.ts b/packages/contracts-test/tests/unit/rewards/rewards-interface.test.ts index 63280f5e8..7bbfebe6b 100644 --- a/packages/contracts-test/tests/unit/rewards/rewards-interface.test.ts +++ b/packages/contracts-test/tests/unit/rewards/rewards-interface.test.ts @@ -54,11 +54,11 @@ describe('RewardsManager interfaces', () => { }) it('IIssuanceTarget should have stable interface ID', () => { - expect(IIssuanceTarget__factory.interfaceId).to.equal('0xaee4dc43') + expect(IIssuanceTarget__factory.interfaceId).to.equal('0x19f6601a') }) it('IRewardsManager should have stable interface ID', () => { - expect(IRewardsManager__factory.interfaceId).to.equal('0x337b092e') + expect(IRewardsManager__factory.interfaceId).to.equal('0x8469b577') }) }) diff --git a/packages/contracts/contracts/rewards/RewardsManager.sol b/packages/contracts/contracts/rewards/RewardsManager.sol index a0ca5ca20..f251dc5f8 100644 --- a/packages/contracts/contracts/rewards/RewardsManager.sol +++ b/packages/contracts/contracts/rewards/RewardsManager.sol @@ -173,24 +173,25 @@ contract RewardsManager is * Note that the IssuanceAllocator can be set to the zero address to disable use of an allocator, and * use the local `issuancePerBlock` variable instead to control issuance. */ - function setIssuanceAllocator(address newIssuanceAllocator) external override onlyGovernor { - if (address(issuanceAllocator) != newIssuanceAllocator) { + function setIssuanceAllocator(IIssuanceAllocationDistribution newIssuanceAllocator) external override onlyGovernor { + if (issuanceAllocator != newIssuanceAllocator) { // Update rewards calculation before changing the issuance allocator updateAccRewardsPerSignal(); // Check that the contract supports the IIssuanceAllocationDistribution interface // Allow zero address to disable the allocator - if (newIssuanceAllocator != address(0)) { + if (address(newIssuanceAllocator) != address(0)) { // solhint-disable-next-line gas-small-strings require( - IERC165(newIssuanceAllocator).supportsInterface(type(IIssuanceAllocationDistribution).interfaceId), + IERC165(address(newIssuanceAllocator)).supportsInterface( + type(IIssuanceAllocationDistribution).interfaceId + ), "Contract does not support IIssuanceAllocationDistribution interface" ); } - address oldIssuanceAllocator = address(issuanceAllocator); - issuanceAllocator = IIssuanceAllocationDistribution(newIssuanceAllocator); - emit IssuanceAllocatorSet(oldIssuanceAllocator, newIssuanceAllocator); + emit IssuanceAllocatorSet(issuanceAllocator, newIssuanceAllocator); + issuanceAllocator = newIssuanceAllocator; } } @@ -325,7 +326,7 @@ contract RewardsManager is } /** - * @inheritdoc IRewardsManager + * @inheritdoc IIssuanceTarget */ function getIssuanceAllocator() external view override returns (IIssuanceAllocationDistribution) { return issuanceAllocator; diff --git a/packages/deployment/lib/abis.ts b/packages/deployment/lib/abis.ts index e9894d213..b7b0868b2 100644 --- a/packages/deployment/lib/abis.ts +++ b/packages/deployment/lib/abis.ts @@ -17,12 +17,12 @@ function loadAbi(artifactPath: string): Abi { return artifact.abi as Abi } -// Interface IDs - these match the generated values from TypeChain factories -// Verified by tests: packages/issuance/testing/tests/allocate/InterfaceIdStability.test.ts -// and packages/contracts-test/tests/unit/rewards/rewards-interface.test.ts +// Interface IDs - these mirror the values the compiler derives from the +// corresponding ABI. Cross-checked by test/interface-id-stability.test.ts; +// update both together whenever an interface changes. export const IERC165_INTERFACE_ID = '0x01ffc9a7' as const -export const IISSUANCE_TARGET_INTERFACE_ID = '0xaee4dc43' as const -export const IREWARDS_MANAGER_INTERFACE_ID = '0xa0a2f219' as const +export const IISSUANCE_TARGET_INTERFACE_ID = '0x19f6601a' as const +export const IREWARDS_MANAGER_INTERFACE_ID = '0x8469b577' as const export const REWARDS_MANAGER_ABI = loadAbi( '@graphprotocol/interfaces/artifacts/contracts/contracts/rewards/IRewardsManager.sol/IRewardsManager.json', diff --git a/packages/deployment/test/interface-id-stability.test.ts b/packages/deployment/test/interface-id-stability.test.ts new file mode 100644 index 000000000..5d0ed1225 --- /dev/null +++ b/packages/deployment/test/interface-id-stability.test.ts @@ -0,0 +1,34 @@ +import { expect } from 'chai' +import type { Abi } from 'viem' +import { toFunctionSelector } from 'viem' + +import { + IERC165_ABI, + IERC165_INTERFACE_ID, + IISSUANCE_TARGET_INTERFACE_ID, + IREWARDS_MANAGER_INTERFACE_ID, + ISSUANCE_TARGET_ABI, + REWARDS_MANAGER_ABI, +} from '../lib/abis.js' + +function computeInterfaceId(abi: Abi): `0x${string}` { + const xor = abi + .filter((item): item is Extract<(typeof abi)[number], { type: 'function' }> => item.type === 'function') + .map((f) => Number.parseInt(toFunctionSelector(f).slice(2), 16) >>> 0) + .reduce((a, s) => (a ^ s) >>> 0, 0) + return `0x${xor.toString(16).padStart(8, '0')}` +} + +describe('Interface ID Stability', function () { + it('IERC165_INTERFACE_ID matches the IERC165 ABI', function () { + expect(IERC165_INTERFACE_ID).to.equal(computeInterfaceId(IERC165_ABI)) + }) + + it('IISSUANCE_TARGET_INTERFACE_ID matches the IIssuanceTarget ABI', function () { + expect(IISSUANCE_TARGET_INTERFACE_ID).to.equal(computeInterfaceId(ISSUANCE_TARGET_ABI)) + }) + + it('IREWARDS_MANAGER_INTERFACE_ID matches the IRewardsManager ABI', function () { + expect(IREWARDS_MANAGER_INTERFACE_ID).to.equal(computeInterfaceId(REWARDS_MANAGER_ABI)) + }) +}) diff --git a/packages/horizon/contracts/payments/collectors/RecurringCollector.sol b/packages/horizon/contracts/payments/collectors/RecurringCollector.sol index 171e6e8f0..088e94e12 100644 --- a/packages/horizon/contracts/payments/collectors/RecurringCollector.sol +++ b/packages/horizon/contracts/payments/collectors/RecurringCollector.sol @@ -1,6 +1,8 @@ // SPDX-License-Identifier: GPL-3.0-or-later pragma solidity ^0.8.27; +// solhint-disable gas-strict-inequalities + import { EIP712Upgradeable } from "@openzeppelin/contracts-upgradeable/utils/cryptography/EIP712Upgradeable.sol"; import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; import { PausableUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/PausableUpgradeable.sol"; @@ -15,13 +17,21 @@ import { IPaymentsCollector } from "@graphprotocol/interfaces/contracts/horizon/ import { IAgreementOwner } from "@graphprotocol/interfaces/contracts/horizon/IAgreementOwner.sol"; import { IAgreementCollector, + OFFER_TYPE_NONE, OFFER_TYPE_NEW, OFFER_TYPE_UPDATE, ACCEPTED, REGISTERED, + NOTICE_GIVEN, + SETTLED, + BY_PAYER, + BY_PROVIDER, UPDATE, + VERSION_CURRENT, + VERSION_NEXT, SCOPE_ACTIVE, - SCOPE_PENDING + SCOPE_PENDING, + SCOPE_SIGNED } from "@graphprotocol/interfaces/contracts/horizon/IAgreementCollector.sol"; import { IRecurringCollector } from "@graphprotocol/interfaces/contracts/horizon/IRecurringCollector.sol"; import { IGraphPayments } from "@graphprotocol/interfaces/contracts/horizon/IGraphPayments.sol"; @@ -34,6 +44,11 @@ import { PPMMath } from "../../libraries/PPMMath.sol"; * @author Edge & Node * @dev Implements the {IRecurringCollector} interface. * @notice A payments collector contract that can be used to collect payments using a RCA (Recurring Collection Agreement). + * + * @custom:security Self-authorization: RC overrides {_isAuthorized} to return true whenever + * `signer == address(this)`, so RC itself must perform the appropriate authorization check + * before any external call. + * * @custom:security-contact Please email security+contracts@thegraph.com if you find any * bugs. We may have an active bug bounty program. */ @@ -58,6 +73,12 @@ contract RecurringCollector is /// that could starve the core collect() call of gas. uint256 private constant MAX_PAYER_CALLBACK_GAS = 1_500_000; + /// @notice Gas overhead between the gasleft() precheck and the actual CALL/STATICCALL opcode. + /// Covers ABI encoding, stack/memory setup, and the CALL base cost so that at least + /// MAX_PAYER_CALLBACK_GAS is forwarded to the callee. Sized to cover the cold-account + /// EIP-2929 access cost (2_600) plus Solidity framing. + uint256 private constant CALLBACK_GAS_OVERHEAD = 3_000; + /* solhint-disable gas-small-strings */ /// @notice The EIP712 typehash for the RecurringCollectionAgreement struct bytes32 public constant EIP712_RCA_TYPEHASH = @@ -72,10 +93,18 @@ contract RecurringCollector is ); /* solhint-enable gas-small-strings */ - /// @notice A stored offer (RCA or RCAU) with its EIP-712 hash - struct StoredOffer { - bytes32 offerHash; - bytes data; + /// @notice Decoded agreement terms keyed by EIP-712 hash. `data` preserves the + /// original ABI-encoded RCA or RCAU for {getAgreementOfferAt} consumers. + struct AgreementTerms { + uint8 offerType; // 1 byte ─┐ slot 0 (27/32) + uint64 endsAt; // 8 bytes ─┤ + uint64 deadline; // 8 bytes ─┤ + uint32 minSecondsPerCollection; // 4 bytes ─┤ + uint32 maxSecondsPerCollection; // 4 bytes ─┤ + uint16 conditions; // 2 bytes ─┘ + uint256 maxInitialTokens; // 32 bytes ── slot 1 + uint256 maxOngoingTokensPerSecond; // 32 bytes ── slot 2 + bytes data; // ── slot 3 (pointer) } /// @custom:storage-location erc7201:graphprotocol.storage.RecurringCollector @@ -84,10 +113,12 @@ contract RecurringCollector is mapping(address pauseGuardian => bool allowed) pauseGuardians; /// @notice Tracks agreements mapping(bytes16 agreementId => AgreementData data) agreements; - /// @notice Stored RCA offers (pre-approval), keyed by agreement ID - mapping(bytes16 agreementId => StoredOffer offer) rcaOffers; - /// @notice Stored RCAU offers (pre-approval), keyed by agreement ID - mapping(bytes16 agreementId => StoredOffer offer) rcauOffers; + /// @notice Decoded agreement terms, keyed by EIP-712 hash. + /// Referenced by AgreementData.activeTermsHash and pendingTermsHash. + mapping(bytes32 termsHash => AgreementTerms terms) terms; + /// @notice Cancelled offer hashes, keyed by signer then EIP-712 hash. + /// Stores the agreementId that is blocked; bytes16(0) means not cancelled. + mapping(address signer => mapping(bytes32 hash => bytes16 agreementId)) cancelledOffers; } /// @dev keccak256(abi.encode(uint256(keccak256("graphprotocol.storage.RecurringCollector")) - 1)) & ~bytes32(uint256(0xff)) @@ -199,92 +230,69 @@ contract RecurringCollector is function accept( RecurringCollectionAgreement calldata rca, bytes calldata signature - ) external whenNotPaused returns (bytes16) { - /* solhint-disable gas-strict-inequalities */ + ) external whenNotPaused returns (bytes16 agreementId) { + require(msg.sender == rca.dataService, RecurringCollectorUnauthorizedCaller(msg.sender, rca.dataService)); + + bytes32 rcaHash; + (agreementId, rcaHash) = _rcaIdAndHash(rca); + + AgreementData storage agreement = _getStorage().agreements[agreementId]; + // Idempotent: already accepted at this hash → return silently, no state change. + // Short-circuits before deadline + auth; callable after the RCA deadline. + if (agreement.state == AgreementState.Accepted && agreement.activeTermsHash == rcaHash) return agreementId; + require( - rca.deadline >= block.timestamp, + block.timestamp <= rca.deadline, RecurringCollectorAgreementDeadlineElapsed(block.timestamp, rca.deadline) ); - /* solhint-enable gas-strict-inequalities */ - bool isSigned = 0 < signature.length; - bytes32 rcaHash = _hashRCA(rca); - bytes16 agreementId = _generateAgreementId( - rca.payer, + _requireAuthorization(rca.payer, rcaHash, signature, agreementId, OFFER_TYPE_NEW); + + if (_validateAndStoreTerms(agreementId, rcaHash, _termsFromRCA(rca), rca.payer, VERSION_CURRENT, rca.deadline)) + _storeAgreement(agreementId, rca); + + require( + agreement.state == AgreementState.NotAccepted, + RecurringCollectorAgreementIncorrectState(agreementId, agreement.state) + ); + agreement.acceptedAt = uint64(block.timestamp); + agreement.state = AgreementState.Accepted; + + emit AgreementAccepted( rca.dataService, + rca.payer, rca.serviceProvider, - rca.deadline, - rca.nonce + agreementId, + rca.endsAt, + rca.maxInitialTokens, + rca.maxOngoingTokensPerSecond, + rca.minSecondsPerCollection, + rca.maxSecondsPerCollection ); - - _requireAuthorization(rca.payer, rcaHash, signature, isSigned, agreementId, OFFER_TYPE_NEW); - - return _validateAndStoreAgreement(rca, agreementId, rcaHash); } /** - * @notice Validates RCA fields and stores the agreement. - * @param _rca The Recurring Collection Agreement to validate and store - * @return agreementId The deterministically generated agreement ID + * @notice Stores agreement participants and state identity. Does not store terms or hashes. + * @param agreementId The agreement ID + * @param rca The Recurring Collection Agreement (source for identity fields) */ - /* solhint-disable function-max-lines */ - function _validateAndStoreAgreement( - RecurringCollectionAgreement memory _rca, - bytes16 agreementId, - bytes32 _rcaHash - ) private returns (bytes16) { - require(agreementId != bytes16(0), RecurringCollectorAgreementIdZero()); - require(msg.sender == _rca.dataService, RecurringCollectorUnauthorizedCaller(msg.sender, _rca.dataService)); - + function _storeAgreement(bytes16 agreementId, RecurringCollectionAgreement memory rca) private { require( - _rca.dataService != address(0) && _rca.payer != address(0) && _rca.serviceProvider != address(0), + rca.dataService != address(0) && rca.payer != address(0) && rca.serviceProvider != address(0), RecurringCollectorAgreementAddressNotSet() ); - _requireValidCollectionWindowParams(_rca.endsAt, _rca.minSecondsPerCollection, _rca.maxSecondsPerCollection); - _requirePayerToSupportEligibilityCheck(_rca.payer, _rca.conditions); - AgreementData storage agreement = _getAgreementStorage(agreementId); - // check that the agreement is not already accepted require( agreement.state == AgreementState.NotAccepted, RecurringCollectorAgreementIncorrectState(agreementId, agreement.state) ); - // Reverts on overflow — rejecting excessive terms that could prevent collection - _rca.maxOngoingTokensPerSecond * _rca.maxSecondsPerCollection * 1024; - - // accept the agreement - agreement.acceptedAt = uint64(block.timestamp); - agreement.state = AgreementState.Accepted; - agreement.dataService = _rca.dataService; - agreement.payer = _rca.payer; - agreement.serviceProvider = _rca.serviceProvider; - agreement.endsAt = _rca.endsAt; - agreement.maxInitialTokens = _rca.maxInitialTokens; - agreement.maxOngoingTokensPerSecond = _rca.maxOngoingTokensPerSecond; - agreement.minSecondsPerCollection = _rca.minSecondsPerCollection; - agreement.maxSecondsPerCollection = _rca.maxSecondsPerCollection; - agreement.conditions = _rca.conditions; - agreement.activeTermsHash = _rcaHash; + agreement.dataService = rca.dataService; + agreement.payer = rca.payer; + agreement.serviceProvider = rca.serviceProvider; agreement.updateNonce = 0; - - emit AgreementAccepted( - agreement.dataService, - agreement.payer, - agreement.serviceProvider, - agreementId, - agreement.acceptedAt, - agreement.endsAt, - agreement.maxInitialTokens, - agreement.maxOngoingTokensPerSecond, - agreement.minSecondsPerCollection, - agreement.maxSecondsPerCollection - ); - - return agreementId; } - /* solhint-enable function-max-lines */ /** * @inheritdoc IRecurringCollector @@ -309,14 +317,7 @@ contract RecurringCollector is agreement.state = AgreementState.CanceledByServiceProvider; } - emit AgreementCanceled( - agreement.dataService, - agreement.payer, - agreement.serviceProvider, - agreementId, - agreement.canceledAt, - by - ); + emit AgreementCanceled(agreement.dataService, agreement.payer, agreement.serviceProvider, agreementId, by); } /** @@ -329,19 +330,54 @@ contract RecurringCollector is function update(RecurringCollectionAgreementUpdate calldata rcau, bytes calldata signature) external whenNotPaused { AgreementData storage agreement = _requireValidUpdateTarget(rcau.agreementId); - /* solhint-disable gas-strict-inequalities */ + bytes32 rcauHash = _hashRCAU(rcau); + + // Idempotent: already at this version (state is Accepted per _requireValidUpdateTarget). + // Skip deadline + auth since no state change happens. + if (agreement.activeTermsHash == rcauHash) return; + require( - rcau.deadline >= block.timestamp, + block.timestamp <= rcau.deadline, RecurringCollectorAgreementDeadlineElapsed(block.timestamp, rcau.deadline) ); - /* solhint-enable gas-strict-inequalities */ - bool isSigned = 0 < signature.length; - bytes32 rcauHash = _hashRCAU(rcau); + _requireAuthorization(agreement.payer, rcauHash, signature, rcau.agreementId, OFFER_TYPE_UPDATE); + + uint32 expectedNonce = agreement.updateNonce + 1; + require( + rcau.nonce == expectedNonce, + RecurringCollectorInvalidUpdateNonce(rcau.agreementId, expectedNonce, rcau.nonce) + ); - _requireAuthorization(agreement.payer, rcauHash, signature, isSigned, rcau.agreementId, OFFER_TYPE_UPDATE); + RecurringCollectorStorage storage $ = _getStorage(); + if (agreement.pendingTermsHash == rcauHash) { + // Accept pending offer: move hash from pending to active (terms already stored + validated) + delete $.terms[agreement.activeTermsHash]; + agreement.activeTermsHash = rcauHash; + agreement.pendingTermsHash = bytes32(0); + } else + _validateAndStoreTerms( + rcau.agreementId, + rcauHash, + _termsFromRCAU(rcau), + agreement.payer, + VERSION_CURRENT, + rcau.deadline + ); + + agreement.updateNonce = rcau.nonce; - _validateAndStoreUpdate(agreement, rcau, rcauHash); + emit AgreementUpdated( + agreement.dataService, + agreement.payer, + agreement.serviceProvider, + rcau.agreementId, + rcau.endsAt, + rcau.maxInitialTokens, + rcau.maxOngoingTokensPerSecond, + rcau.minSecondsPerCollection, + rcau.maxSecondsPerCollection + ); } /// @inheritdoc IRecurringCollector @@ -384,7 +420,7 @@ contract RecurringCollector is /// @inheritdoc IAgreementCollector function getMaxNextClaim(bytes16 agreementId) external view returns (uint256) { - return _getMaxNextClaimScoped(agreementId, SCOPE_ACTIVE | SCOPE_PENDING); + return _getMaxNextClaimScoped(agreementId, 0); } /// @inheritdoc IRecurringCollector @@ -409,6 +445,8 @@ contract RecurringCollector is if (offerType == OFFER_TYPE_NEW) details = _offerNew(data); else if (offerType == OFFER_TYPE_UPDATE) details = _offerUpdate(data); else revert RecurringCollectorInvalidCollectData(data); + + require(msg.sender == details.payer, RecurringCollectorUnauthorizedCaller(msg.sender, details.payer)); } /** @@ -417,30 +455,18 @@ contract RecurringCollector is * @return details The agreement details */ function _offerNew(bytes calldata _data) private returns (AgreementDetails memory details) { - RecurringCollectorStorage storage $ = _getStorage(); RecurringCollectionAgreement memory rca = abi.decode(_data, (RecurringCollectionAgreement)); - require(msg.sender == rca.payer, RecurringCollectorUnauthorizedCaller(msg.sender, rca.payer)); - _requirePayerToSupportEligibilityCheck(rca.payer, rca.conditions); - - bytes16 agreementId = _generateAgreementId( - rca.payer, - rca.dataService, - rca.serviceProvider, - rca.deadline, - rca.nonce + require( + block.timestamp <= rca.deadline, + RecurringCollectorAgreementDeadlineElapsed(block.timestamp, rca.deadline) ); - bytes32 offerHash = _hashRCA(rca); - $.rcaOffers[agreementId] = StoredOffer({ offerHash: offerHash, data: _data }); + (bytes16 agreementId, bytes32 rcaHash) = _rcaIdAndHash(rca); - details.agreementId = agreementId; - details.payer = rca.payer; - details.dataService = rca.dataService; - details.serviceProvider = rca.serviceProvider; - details.versionHash = offerHash; - details.state = REGISTERED; + if (_validateAndStoreTerms(agreementId, rcaHash, _termsFromRCA(rca), rca.payer, VERSION_CURRENT, rca.deadline)) + _storeAgreement(agreementId, rca); - emit OfferStored(agreementId, rca.payer, OFFER_TYPE_NEW, offerHash); + return _getAgreementDetails(_getStorage(), agreementId, rcaHash); } /** @@ -449,116 +475,142 @@ contract RecurringCollector is * @return details The agreement details */ function _offerUpdate(bytes calldata _data) private returns (AgreementDetails memory details) { - RecurringCollectorStorage storage $ = _getStorage(); RecurringCollectionAgreementUpdate memory rcau = abi.decode(_data, (RecurringCollectionAgreementUpdate)); + require( + block.timestamp <= rcau.deadline, + RecurringCollectorAgreementDeadlineElapsed(block.timestamp, rcau.deadline) + ); bytes16 agreementId = rcau.agreementId; - // Payer check: look up the existing agreement or the stored RCA offer - AgreementData storage agreement = $.agreements[agreementId]; + AgreementData storage agreement = _getStorage().agreements[agreementId]; address payer = agreement.payer; - if (payer == address(0)) { - // Not yet accepted — check stored RCA offer payer - require( - $.rcaOffers[agreementId].offerHash != bytes32(0), - RecurringCollectorAgreementIncorrectState(agreementId, AgreementState.NotAccepted) - ); - RecurringCollectionAgreement memory rca = abi.decode( - $.rcaOffers[agreementId].data, - (RecurringCollectionAgreement) - ); - payer = rca.payer; - details.dataService = rca.dataService; - details.serviceProvider = rca.serviceProvider; - } else { - details.dataService = agreement.dataService; - details.serviceProvider = agreement.serviceProvider; - } - require(msg.sender == payer, RecurringCollectorUnauthorizedCaller(msg.sender, payer)); - _requirePayerToSupportEligibilityCheck(payer, rcau.conditions); bytes32 offerHash = _hashRCAU(rcau); - $.rcauOffers[agreementId] = StoredOffer({ offerHash: offerHash, data: _data }); + _validateAndStoreTerms(agreementId, offerHash, _termsFromRCAU(rcau), payer, VERSION_NEXT, rcau.deadline); - details.agreementId = agreementId; - details.payer = payer; - details.versionHash = offerHash; - details.state = REGISTERED | UPDATE; - - emit OfferStored(agreementId, payer, OFFER_TYPE_UPDATE, offerHash); + return _getAgreementDetails(_getStorage(), agreementId, offerHash); } /// @inheritdoc IAgreementCollector + /// @dev This implementation targets only the payer side of the agreement. + /// SCOPE_PENDING and SCOPE_ACTIVE enforce `msg.sender == agreement.payer`. + /// SCOPE_SIGNED has no caller check in this function; the entry it writes is + /// self-keyed by msg.sender and is consulted only later, during payer + /// authorization of a signed accept or update. Extending cancel to data-service + /// or service-provider callers is left for a future revision. function cancel(bytes16 agreementId, bytes32 termsHash, uint16 options) external whenNotPaused { RecurringCollectorStorage storage $ = _getStorage(); AgreementData storage agreement = $.agreements[agreementId]; - _requirePayer($, agreement, agreementId); - - if (agreement.activeTermsHash != termsHash) { - if (options & SCOPE_PENDING != 0) - // Pending scope: delete stored offer if hash matches and terms are not currently active - if ($.rcaOffers[agreementId].offerHash == termsHash) delete $.rcaOffers[agreementId]; - else if ($.rcauOffers[agreementId].offerHash == termsHash) delete $.rcauOffers[agreementId]; - } else if (options & SCOPE_ACTIVE != 0 && agreement.state == AgreementState.Accepted) - // Active scope and hash matches: cancel accepted agreement - IDataServiceAgreements(agreement.dataService).cancelIndexingAgreementByPayer(agreementId); - } - /** - * @notice Requires that msg.sender is the payer for an agreement. - * @dev Checks the on-chain agreement first, then falls back to stored RCA offer. - * @param agreement The agreement data - * @param agreementId The agreement ID - */ - // solhint-disable-next-line use-natspec - function _requirePayer( - RecurringCollectorStorage storage $, - AgreementData storage agreement, - bytes16 agreementId - ) private view { - if (agreement.payer == msg.sender) return; - - // Not payer on accepted agreement — check stored RCA offer - StoredOffer storage rcaOffer = $.rcaOffers[agreementId]; - if (rcaOffer.offerHash != bytes32(0)) { - RecurringCollectionAgreement memory rca = abi.decode(rcaOffer.data, (RecurringCollectionAgreement)); - require(msg.sender == rca.payer, RecurringCollectorUnauthorizedCaller(msg.sender, rca.payer)); - return; + // Signed scope: record cancelledOffers[msg.sender][termsHash] = agreementId. + // Self-authenticating — only blocks when msg.sender matches the recovered ECDSA signer. + // The stored agreementId is checked in _requireAuthorization (!=); calling again + // with bytes16(0) undoes the cancellation, calling with a different agreementId + // redirects it. + if (options & SCOPE_SIGNED != 0) { + if ($.cancelledOffers[msg.sender][termsHash] != agreementId) { + $.cancelledOffers[msg.sender][termsHash] = agreementId; + emit OfferCancelled(msg.sender, agreementId, termsHash); + } + } + + // Pending / active scopes: revert if on-chain data exists but caller is not the payer. + // No-op if nothing exists on-chain (nothing to cancel). + if (options & (SCOPE_PENDING | SCOPE_ACTIVE) != 0) { + address payer = agreement.payer; + if (payer == address(0)) return; + require(payer == msg.sender, RecurringCollectorUnauthorizedCaller(msg.sender, payer)); } - if (agreement.payer == address(0)) revert RecurringCollectorAgreementNotFound(agreementId); - revert RecurringCollectorUnauthorizedCaller(msg.sender, agreement.payer); + if (options & SCOPE_PENDING != 0) { + if (agreement.pendingTermsHash == termsHash) { + // Pending update matches — delete it. + delete $.terms[termsHash]; + agreement.pendingTermsHash = bytes32(0); + emit OfferCancelled(msg.sender, agreementId, termsHash); + } else if (agreement.activeTermsHash == termsHash && agreement.state == AgreementState.NotAccepted) { + // Pre-acceptance RCA offer matches — delete it. Any pending RCAU is + // independent and can be cancelled separately in either order. + delete $.terms[termsHash]; + agreement.activeTermsHash = bytes32(0); + emit OfferCancelled(msg.sender, agreementId, termsHash); + } + } + if ( + options & SCOPE_ACTIVE != 0 && + agreement.state == AgreementState.Accepted && + agreement.activeTermsHash == termsHash + ) + // Active scope and hash matches accepted agreement — cancel via data service. + IDataServiceAgreements(agreement.dataService).cancelIndexingAgreementByPayer(agreementId); } /// @inheritdoc IAgreementCollector function getAgreementDetails( bytes16 agreementId, - uint256 /* index */ + uint256 index ) external view returns (AgreementDetails memory details) { RecurringCollectorStorage storage $ = _getStorage(); AgreementData storage agreement = $.agreements[agreementId]; - if (agreement.state != AgreementState.NotAccepted) { - details.agreementId = agreementId; - details.payer = agreement.payer; - details.dataService = agreement.dataService; - details.serviceProvider = agreement.serviceProvider; - details.versionHash = agreement.activeTermsHash; - details.state = ACCEPTED; - return details; - } + return _getAgreementDetails($, agreementId, _getVersionHash(agreement, index)); + } - // Not yet accepted — check stored RCA offer - StoredOffer storage rcaOffer = $.rcaOffers[agreementId]; - if (rcaOffer.offerHash != bytes32(0)) { - RecurringCollectionAgreement memory rca = abi.decode(rcaOffer.data, (RecurringCollectionAgreement)); - details.agreementId = agreementId; - details.payer = rca.payer; - details.dataService = rca.dataService; - details.serviceProvider = rca.serviceProvider; - details.versionHash = rcaOffer.offerHash; - details.state = REGISTERED; - } + /** + * @notice Build an AgreementDetails view for a given version hash. + * @dev Empty struct when no offer is stored for the version. Otherwise composes state flags + * per the {IAgreementCollector} flag spec. Shared by getAgreementDetails, _offerNew, and + * _offerUpdate so all three consistently reflect the same on-chain state. + * @param $ The collector storage root + * @param agreementId The agreement ID + * @param versionHash The version hash (activeTermsHash or pendingTermsHash) + * @return details The composed agreement details + */ + // solhint-disable-next-line use-natspec + function _getAgreementDetails( + RecurringCollectorStorage storage $, + bytes16 agreementId, + bytes32 versionHash + ) private view returns (AgreementDetails memory details) { + uint8 offerType = $.terms[versionHash].offerType; + if (offerType == OFFER_TYPE_NONE) return details; + + AgreementData storage agreement = $.agreements[agreementId]; + AgreementState agreementState = agreement.state; + + details.agreementId = agreementId; + details.versionHash = versionHash; + details.payer = agreement.payer; + details.dataService = agreement.dataService; + details.serviceProvider = agreement.serviceProvider; + + details.state = REGISTERED; + if (agreementState != AgreementState.NotAccepted && versionHash == agreement.activeTermsHash) + details.state |= ACCEPTED; + + if (offerType == OFFER_TYPE_UPDATE) details.state |= UPDATE; + + if (agreementState == AgreementState.CanceledByPayer) details.state |= NOTICE_GIVEN | BY_PAYER; + else if (agreementState == AgreementState.CanceledByServiceProvider) + details.state |= NOTICE_GIVEN | BY_PROVIDER; + + if (_getMaxNextClaimScoped(agreementId, 0) == 0) details.state |= SETTLED; + } + + /** + * @notice Resolves a version index to the corresponding terms hash. + * @dev Returns bytes32(0) for unknown indexes or when the slot is empty. + * @param agreement The agreement data + * @param index The version index ({VERSION_CURRENT} or {VERSION_NEXT}) + * @return versionHash The resolved terms hash + */ + function _getVersionHash( + AgreementData storage agreement, + uint256 index + ) private view returns (bytes32 versionHash) { + if (index == VERSION_CURRENT) versionHash = agreement.activeTermsHash; + else if (index == VERSION_NEXT) versionHash = agreement.pendingTermsHash; } /// @inheritdoc IAgreementCollector @@ -572,13 +624,13 @@ contract RecurringCollector is uint256 index ) external view returns (uint8 offerType, bytes memory offerData) { RecurringCollectorStorage storage $ = _getStorage(); - if (index == OFFER_TYPE_NEW) { - StoredOffer storage rca = $.rcaOffers[agreementId]; - if (rca.offerHash != bytes32(0)) return (OFFER_TYPE_NEW, rca.data); - } else if (index == OFFER_TYPE_UPDATE) { - StoredOffer storage rcau = $.rcauOffers[agreementId]; - if (rcau.offerHash != bytes32(0)) return (OFFER_TYPE_UPDATE, rcau.data); - } + AgreementData storage agreement = $.agreements[agreementId]; + + bytes32 versionHash = _getVersionHash(agreement, index); + if (versionHash == bytes32(0)) return (OFFER_TYPE_NONE, ""); + + AgreementTerms storage stored = $.terms[versionHash]; + return (stored.offerType, stored.data); } /** @@ -650,12 +702,10 @@ contract RecurringCollector is if (_params.tokens != 0) { uint256 slippage = _params.tokens - tokensToCollect; - /* solhint-disable gas-strict-inequalities */ require( slippage <= _params.maxSlippage, RecurringCollectorExcessiveSlippage(_params.tokens, tokensToCollect, _params.maxSlippage) ); - /* solhint-enable gas-strict-inequalities */ } agreement.lastCollectionAt = uint64(block.timestamp); @@ -725,34 +775,40 @@ contract RecurringCollector is ) private { address payer = agreement.payer; address provider = agreement.serviceProvider; - // Payer callbacks use gas-capped low-level calls to prevent gas siphoning and - // caller-side ABI decode reverts. Failures emit events but do not block collection. - if ((agreement.conditions & CONDITION_ELIGIBILITY_CHECK) != 0) { - // 64/63 accounts for EIP-150 63/64 gas forwarding rule. - if (gasleft() < (MAX_PAYER_CALLBACK_GAS * 64) / 63) revert RecurringCollectorInsufficientCallbackGas(); - - // Eligibility gate (opt-in via conditions bitmask): low-level staticcall avoids - // caller-side ABI decode reverts. Only an explicit return of 0 blocks collection; - // reverts, short returndata, and malformed responses are treated as "no opinion" - // (collection proceeds). - // solhint-disable-next-line avoid-low-level-calls - (bool success, bytes memory result) = payer.staticcall{ gas: MAX_PAYER_CALLBACK_GAS }( - abi.encodeCall(IProviderEligibility.isEligible, (provider)) - ); - if (success && !(result.length < 32) && abi.decode(result, (uint256)) == 0) + // Eligibility gate (opt-in via conditions bitmask). Assembly staticcall caps returndata + // copy to 32 bytes, preventing returndata bombing. Only an explicit return of 0 blocks + // collection; reverts, short returndata, and malformed responses are treated as "no + // opinion" (collection proceeds). + if ((_getStorage().terms[agreement.activeTermsHash].conditions & CONDITION_ELIGIBILITY_CHECK) != 0) { + if (gasleft() < (MAX_PAYER_CALLBACK_GAS * 64) / 63 + CALLBACK_GAS_OVERHEAD) + revert RecurringCollectorInsufficientCallbackGas(); + bytes memory cd = abi.encodeCall(IProviderEligibility.isEligible, (provider)); + bool success; + uint256 returnLen; + uint256 result; + // solhint-disable-next-line no-inline-assembly + assembly { + success := staticcall(MAX_PAYER_CALLBACK_GAS, payer, add(cd, 0x20), mload(cd), 0x00, 0x20) + returnLen := returndatasize() + result := mload(0x00) + } + if (success && !(returnLen < 32) && result == 0) revert RecurringCollectorCollectionNotEligible(agreementId, provider); - if (!success || result.length < 32) + if (!success || returnLen < 32) emit PayerCallbackFailed(agreementId, payer, PayerCallbackStage.EligibilityCheck); } + // Assembly call copies 0 bytes of returndata, preventing returndata bombing. if (payer.code.length != 0 && payer != msg.sender) { - if (gasleft() < (MAX_PAYER_CALLBACK_GAS * 64) / 63) revert RecurringCollectorInsufficientCallbackGas(); - - // solhint-disable-next-line avoid-low-level-calls - (bool beforeOk, ) = payer.call{ gas: MAX_PAYER_CALLBACK_GAS }( - abi.encodeCall(IAgreementOwner.beforeCollection, (agreementId, tokensToCollect)) - ); + if (gasleft() < (MAX_PAYER_CALLBACK_GAS * 64) / 63 + CALLBACK_GAS_OVERHEAD) + revert RecurringCollectorInsufficientCallbackGas(); + bytes memory cd = abi.encodeCall(IAgreementOwner.beforeCollection, (agreementId, tokensToCollect)); + bool beforeOk; + // solhint-disable-next-line no-inline-assembly + assembly { + beforeOk := call(MAX_PAYER_CALLBACK_GAS, payer, 0, add(cd, 0x20), mload(cd), 0, 0) + } if (!beforeOk) emit PayerCallbackFailed(agreementId, payer, PayerCallbackStage.BeforeCollection); } } @@ -768,34 +824,45 @@ contract RecurringCollector is // Notify contract payers so they can reconcile escrow in the same transaction. if (payer != msg.sender && payer.code.length != 0) { // 64/63 accounts for EIP-150 63/64 gas forwarding rule. - if (gasleft() < (MAX_PAYER_CALLBACK_GAS * 64) / 63) revert RecurringCollectorInsufficientCallbackGas(); - // solhint-disable-next-line avoid-low-level-calls - (bool afterOk, ) = payer.call{ gas: MAX_PAYER_CALLBACK_GAS }( - abi.encodeCall(IAgreementOwner.afterCollection, (agreementId, tokensToCollect)) + if (gasleft() < (MAX_PAYER_CALLBACK_GAS * 64) / 63 + CALLBACK_GAS_OVERHEAD) + revert RecurringCollectorInsufficientCallbackGas(); + // Assembly call copies 0 bytes of returndata, preventing returndata bombing. + bytes memory afterCallData = abi.encodeCall( + IAgreementOwner.afterCollection, + (agreementId, tokensToCollect) ); + bool afterOk; + // solhint-disable-next-line no-inline-assembly + assembly { + afterOk := call(MAX_PAYER_CALLBACK_GAS, payer, 0, add(afterCallData, 0x20), mload(afterCallData), 0, 0) + } if (!afterOk) emit PayerCallbackFailed(agreementId, payer, PayerCallbackStage.AfterCollection); } } /** * @notice Requires that the collection window parameters are valid. - * + * @dev Validated against `_deadline` (the offer's acceptance deadline) rather than + * `block.timestamp`, making this check time-independent: if terms pass here they remain + * valid for any acceptance that happens on or before `_deadline`. Callers must enforce + * `block.timestamp <= _deadline` at the acceptance entry point. + * @param _deadline The offer's acceptance deadline * @param _endsAt The end time of the agreement * @param _minSecondsPerCollection The minimum seconds per collection * @param _maxSecondsPerCollection The maximum seconds per collection */ function _requireValidCollectionWindowParams( + uint64 _deadline, uint64 _endsAt, uint32 _minSecondsPerCollection, uint32 _maxSecondsPerCollection - ) private view { - // Agreement needs to end in the future - require(_endsAt > block.timestamp, RecurringCollectorAgreementElapsedEndsAt(block.timestamp, _endsAt)); + ) private pure { + // Agreement must end after the deadline + require(_deadline < _endsAt, RecurringCollectorAgreementEndsBeforeDeadline(_deadline, _endsAt)); // Collection window needs to be at least MIN_SECONDS_COLLECTION_WINDOW require( _maxSecondsPerCollection > _minSecondsPerCollection && - // solhint-disable-next-line gas-strict-inequalities (_maxSecondsPerCollection - _minSecondsPerCollection >= MIN_SECONDS_COLLECTION_WINDOW), RecurringCollectorAgreementInvalidCollectionWindow( MIN_SECONDS_COLLECTION_WINDOW, @@ -804,23 +871,81 @@ contract RecurringCollector is ) ); - // Agreement needs to last at least one min collection window + // Even if accepted at the deadline at least one min collection window must remain require( - // solhint-disable-next-line gas-strict-inequalities - _endsAt - block.timestamp >= _minSecondsPerCollection + MIN_SECONDS_COLLECTION_WINDOW, + _minSecondsPerCollection + MIN_SECONDS_COLLECTION_WINDOW <= _endsAt - _deadline, RecurringCollectorAgreementInvalidDuration( _minSecondsPerCollection + MIN_SECONDS_COLLECTION_WINDOW, - _endsAt - block.timestamp + _endsAt - _deadline ) ); } + /** + * @notice Validates offer terms: collection window, eligibility support, and overflow. + * @dev Called once per unique hash by _validateAndStoreTerms (on first store). Time-independent + * — validates against the offer's deadline. + * @param _deadline The offer's acceptance deadline + * @param _endsAt The end time of the agreement + * @param _minSecondsPerCollection The minimum seconds per collection + * @param _maxSecondsPerCollection The maximum seconds per collection + * @param _payer The payer address (for eligibility validation) + * @param _conditions The conditions bitmask + * @param _maxOngoingTokensPerSecond The maximum ongoing tokens per second + */ + function _requireValidTerms( + uint64 _deadline, + uint64 _endsAt, + uint32 _minSecondsPerCollection, + uint32 _maxSecondsPerCollection, + address _payer, + uint16 _conditions, + uint256 _maxOngoingTokensPerSecond + ) private view { + _requireValidCollectionWindowParams(_deadline, _endsAt, _minSecondsPerCollection, _maxSecondsPerCollection); + _requirePayerToSupportEligibilityCheck(_payer, _conditions); + // Reverts on overflow — rejecting excessive terms that could prevent collection + _maxOngoingTokensPerSecond * _maxSecondsPerCollection * 1024; + } + + /// @notice Extract AgreementTerms from an RCA. + /// @param rca The Recurring Collection Agreement + /// @return terms The decoded agreement terms + function _termsFromRCA(RecurringCollectionAgreement memory rca) private pure returns (AgreementTerms memory terms) { + terms.offerType = OFFER_TYPE_NEW; + terms.endsAt = rca.endsAt; + terms.deadline = rca.deadline; + terms.minSecondsPerCollection = rca.minSecondsPerCollection; + terms.maxSecondsPerCollection = rca.maxSecondsPerCollection; + terms.conditions = rca.conditions; + terms.maxInitialTokens = rca.maxInitialTokens; + terms.maxOngoingTokensPerSecond = rca.maxOngoingTokensPerSecond; + terms.data = abi.encode(rca); + } + + /// @notice Extract AgreementTerms from an RCAU. + /// @param rcau The Recurring Collection Agreement Update + /// @return terms The decoded agreement terms + function _termsFromRCAU( + RecurringCollectionAgreementUpdate memory rcau + ) private pure returns (AgreementTerms memory terms) { + terms.offerType = OFFER_TYPE_UPDATE; + terms.endsAt = rcau.endsAt; + terms.deadline = rcau.deadline; + terms.minSecondsPerCollection = rcau.minSecondsPerCollection; + terms.maxSecondsPerCollection = rcau.maxSecondsPerCollection; + terms.conditions = rcau.conditions; + terms.maxInitialTokens = rcau.maxInitialTokens; + terms.maxOngoingTokensPerSecond = rcau.maxOngoingTokensPerSecond; + terms.data = abi.encode(rcau); + } + /** * @notice Validates temporal constraints and caps the requested token amount. * @dev Enforces `minSecondsPerCollection` (unless canceled/elapsed) and returns the lesser of * the requested amount and the RCA payer's per-collection cap * (`maxOngoingTokensPerSecond * collectionSeconds`, plus `maxInitialTokens` on first collection). - * @param _agreement The agreement data + * @param _agreement The agreement data (lifecycle) * @param _agreementId The ID of the agreement * @param _tokens The requested token amount (upper bound from data service) * @param _collectionSeconds Collection duration, already capped at maxSecondsPerCollection @@ -832,24 +957,23 @@ contract RecurringCollector is uint256 _tokens, uint256 _collectionSeconds ) private view returns (uint256) { - bool canceledOrElapsed = _agreement.state == AgreementState.CanceledByPayer || - block.timestamp > _agreement.endsAt; + AgreementTerms storage terms = _getStorage().terms[_agreement.activeTermsHash]; + bool canceledOrElapsed = _agreement.state == AgreementState.CanceledByPayer || block.timestamp > terms.endsAt; if (!canceledOrElapsed) { require( - // solhint-disable-next-line gas-strict-inequalities - _collectionSeconds >= _agreement.minSecondsPerCollection, + _collectionSeconds >= terms.minSecondsPerCollection, RecurringCollectorCollectionTooSoon( _agreementId, // casting to uint32 is safe because _collectionSeconds < minSecondsPerCollection (uint32) // forge-lint: disable-next-line(unsafe-typecast) uint32(_collectionSeconds), - _agreement.minSecondsPerCollection + terms.minSecondsPerCollection ) ); } // _collectionSeconds is already capped at maxSecondsPerCollection by _getCollectionInfo - uint256 maxTokens = _agreement.maxOngoingTokensPerSecond * _collectionSeconds; - maxTokens += _agreement.lastCollectionAt == 0 ? _agreement.maxInitialTokens : 0; + uint256 maxTokens = terms.maxOngoingTokensPerSecond * _collectionSeconds; + maxTokens += _agreement.lastCollectionAt == 0 ? terms.maxInitialTokens : 0; return Math.min(_tokens, maxTokens); } @@ -950,30 +1074,29 @@ contract RecurringCollector is * @notice Verifies authorization for an EIP712 hash using the given basis. * @param _payer The payer address (signer owner for ECDSA, contract for approval) * @param _hash The EIP712 typed data hash - * @param _signature The ECDSA signature (only used when basis is Signature) - * @param _isSigned True if ECDSA-signed, false if pre-approved via stored offer - * @param _agreementId The agreement ID (used to look up stored offer when not signed) - * @param _offerType OFFER_TYPE_NEW or OFFER_TYPE_UPDATE (selects which stored offer to check) + * @param _signature The ECDSA signature, zero length for no signature (pre-approved via stored terms) + * @param _agreementId The agreement ID (used to look up stored terms when not signed) + * @param _offerType OFFER_TYPE_NEW or OFFER_TYPE_UPDATE (selects which terms hash to check) */ function _requireAuthorization( address _payer, bytes32 _hash, bytes memory _signature, - bool _isSigned, bytes16 _agreementId, uint8 _offerType ) private view { RecurringCollectorStorage storage $ = _getStorage(); - if (_isSigned) - require(_isAuthorized(_payer, ECDSA.recover(_hash, _signature)), RecurringCollectorInvalidSigner()); - else - // Check stored offer hash instead of callback - require( - (_offerType == OFFER_TYPE_NEW ? $.rcaOffers[_agreementId] : $.rcauOffers[_agreementId]).offerHash == - _hash, - RecurringCollectorInvalidSigner() - ); + if (0 < _signature.length) { + address signer = ECDSA.recover(_hash, _signature); + require(_isAuthorized(_payer, signer), RecurringCollectorInvalidSigner()); + require($.cancelledOffers[signer][_hash] != _agreementId, RecurringCollectorOfferCancelled(signer, _hash)); + } else { + // Pre-approval: the hash must match the expected version of this agreement. + AgreementData storage agreement = $.agreements[_agreementId]; + bytes32 versionHash = _offerType == OFFER_TYPE_NEW ? agreement.activeTermsHash : agreement.pendingTermsHash; + require(versionHash == _hash, RecurringCollectorInvalidSigner()); + } } /** @@ -995,64 +1118,56 @@ contract RecurringCollector is } /** - * @notice Validates and stores an update to a Recurring Collection Agreement. - * Shared validation/storage/emit logic for the update function. - * @param _agreement The storage reference to the agreement data - * @param _rcau The Recurring Collection Agreement Update to apply - * @param _rcauHash The EIP-712 hash of the RCAU + * @notice Validate, store, and link agreement terms to a version slot. + * @dev Idempotent: if the hash is already stored for this agreement (in any slot), returns + * false without side effects. Validation is time-independent (uses deadline) so terms only + * need validating once — on first store. Caller must separately handle the "accept pending + * into active" transition (done in {update}): this function is only for storing new terms. + * @param agreementId The agreement ID + * @param hash The EIP-712 terms hash + * @param newTerms The decoded terms to store + * @param payer The payer address (for eligibility validation) + * @param index The version slot to link: {VERSION_CURRENT} (active) or {VERSION_NEXT} (pending). + * @param deadline The offer deadline (for offer-window validation) + * @return added False if the hash was already stored (idempotent no-op). */ - function _validateAndStoreUpdate( - AgreementData storage _agreement, - RecurringCollectionAgreementUpdate calldata _rcau, - bytes32 _rcauHash - ) private { + function _validateAndStoreTerms( + bytes16 agreementId, + bytes32 hash, + AgreementTerms memory newTerms, + address payer, + uint256 index, + uint64 deadline + ) private returns (bool added) { RecurringCollectorStorage storage $ = _getStorage(); - // validate nonce to prevent replay attacks - uint32 expectedNonce = _agreement.updateNonce + 1; - require( - _rcau.nonce == expectedNonce, - RecurringCollectorInvalidUpdateNonce(_rcau.agreementId, expectedNonce, _rcau.nonce) - ); - - _requireValidCollectionWindowParams(_rcau.endsAt, _rcau.minSecondsPerCollection, _rcau.maxSecondsPerCollection); - _requirePayerToSupportEligibilityCheck(_agreement.payer, _rcau.conditions); - - // Reverts on overflow — rejecting excessive terms that could prevent collection - _rcau.maxOngoingTokensPerSecond * _rcau.maxSecondsPerCollection * 1024; - - // Clean up stored replaced offer - bytes32 oldHash = _agreement.activeTermsHash; - if (oldHash != bytes32(0)) - if ($.rcaOffers[_rcau.agreementId].offerHash == oldHash) delete $.rcaOffers[_rcau.agreementId]; - else if ($.rcauOffers[_rcau.agreementId].offerHash == oldHash) delete $.rcauOffers[_rcau.agreementId]; - - // update the agreement - _agreement.endsAt = _rcau.endsAt; - _agreement.maxInitialTokens = _rcau.maxInitialTokens; - _agreement.maxOngoingTokensPerSecond = _rcau.maxOngoingTokensPerSecond; - _agreement.minSecondsPerCollection = _rcau.minSecondsPerCollection; - _agreement.maxSecondsPerCollection = _rcau.maxSecondsPerCollection; - _agreement.conditions = _rcau.conditions; - _agreement.activeTermsHash = _rcauHash; - _agreement.updateNonce = _rcau.nonce; + if ($.terms[hash].offerType != OFFER_TYPE_NONE) return false; - emit AgreementUpdated( - _agreement.dataService, - _agreement.payer, - _agreement.serviceProvider, - _rcau.agreementId, - uint64(block.timestamp), - _agreement.endsAt, - _agreement.maxInitialTokens, - _agreement.maxOngoingTokensPerSecond, - _agreement.minSecondsPerCollection, - _agreement.maxSecondsPerCollection + _requireValidTerms( + deadline, + newTerms.endsAt, + newTerms.minSecondsPerCollection, + newTerms.maxSecondsPerCollection, + payer, + newTerms.conditions, + newTerms.maxOngoingTokensPerSecond ); + $.terms[hash] = newTerms; + + AgreementData storage agreement = $.agreements[agreementId]; + if (index == VERSION_NEXT) { + if (agreement.pendingTermsHash != bytes32(0)) delete $.terms[agreement.pendingTermsHash]; + agreement.pendingTermsHash = hash; + } else { + if (agreement.activeTermsHash != bytes32(0)) delete $.terms[agreement.activeTermsHash]; + agreement.activeTermsHash = hash; + } + emit OfferStored(agreementId, payer, newTerms.offerType, hash); + return true; } /** - * @notice Gets an agreement to be updated. + * @notice Gets an agreement storage reference. * @param _agreementId The ID of the agreement to get * @return The storage reference to the agreement data */ @@ -1074,7 +1189,7 @@ contract RecurringCollector is * @dev Single source of truth for collection window logic. The returned `collectionSeconds` * is capped at `maxSecondsPerCollection` — this is a cap on tokens, not a deadline; late * collections succeed but receive at most `maxSecondsPerCollection` worth of tokens. - * @param _agreement The agreement data + * @param _agreement The agreement data (lifecycle); terms loaded from terms[activeTermsHash] * @return isCollectable Whether the agreement can be collected from * @return collectionSeconds The valid collection duration in seconds, capped at * maxSecondsPerCollection (0 if not collectable) @@ -1091,13 +1206,14 @@ contract RecurringCollector is return (false, 0, AgreementNotCollectableReason.InvalidAgreementState); } - bool canceledOrElapsed = _agreement.state == AgreementState.CanceledByPayer || - block.timestamp > _agreement.endsAt; + AgreementTerms storage _terms = _getStorage().terms[_agreement.activeTermsHash]; + + bool canceledOrElapsed = _agreement.state == AgreementState.CanceledByPayer || block.timestamp > _terms.endsAt; uint256 canceledOrNow = _agreement.state == AgreementState.CanceledByPayer ? _agreement.canceledAt : block.timestamp; - uint256 collectionEnd = canceledOrElapsed ? Math.min(canceledOrNow, _agreement.endsAt) : block.timestamp; + uint256 collectionEnd = canceledOrElapsed ? Math.min(canceledOrNow, _terms.endsAt) : block.timestamp; uint256 collectionStart = _agreementCollectionStartAt(_agreement); if (collectionEnd < collectionStart) { @@ -1109,11 +1225,7 @@ contract RecurringCollector is } uint256 elapsed = collectionEnd - collectionStart; - return ( - true, - Math.min(elapsed, uint256(_agreement.maxSecondsPerCollection)), - AgreementNotCollectableReason.None - ); + return (true, Math.min(elapsed, uint256(_terms.maxSecondsPerCollection)), AgreementNotCollectableReason.None); } /** @@ -1125,124 +1237,74 @@ contract RecurringCollector is return _agreement.lastCollectionAt > 0 ? _agreement.lastCollectionAt : _agreement.acceptedAt; } - /** - * @notice Compute the maximum tokens collectable in the next collection (worst case). - * @dev Determines the collection window from agreement state, then delegates to {_maxClaim}. - * Returns 0 for non-collectable states. - * @param _a The agreement data - * @return The maximum tokens that could be collected - */ - function _getMaxNextClaim(AgreementData storage _a) private view returns (uint256) { - // CanceledByServiceProvider = immediately non-collectable - if (_a.state == AgreementState.CanceledByServiceProvider) return 0; - // Only Accepted and CanceledByPayer are collectable - if (_a.state != AgreementState.Accepted && _a.state != AgreementState.CanceledByPayer) return 0; - - // Collection starts from last collection (or acceptance if never collected) - uint256 collectionStart = 0 < _a.lastCollectionAt ? _a.lastCollectionAt : _a.acceptedAt; - - // Determine the latest possible collection end - uint256 collectionEnd; - if (_a.state == AgreementState.CanceledByPayer) { - // Payer cancel freezes the window at min(canceledAt, endsAt) - collectionEnd = _a.canceledAt < _a.endsAt ? _a.canceledAt : _a.endsAt; - } else { - // Active: collection window capped at endsAt - collectionEnd = _a.endsAt; - } - - return - _maxClaim( - collectionStart, - collectionEnd, - _a.maxSecondsPerCollection, - _a.maxOngoingTokensPerSecond, - _a.lastCollectionAt == 0 ? _a.maxInitialTokens : 0 - ); - } - /** * @notice Compute max next claim with scope control (active, pending, or both). - * @dev Adapts the refactored _getMaxNextClaim(agreementId, agreementScope) pattern. - * Active claim comes from the on-chain agreement state. Pending claim comes from - * stored offers (RCA if not yet accepted, RCAU if pending update). + * @dev Terms are loaded from terms[hash]. Active window is state-dependent (pre-acceptance + * time-caps from now; accepted starts from last collection; cancelled caps at canceledAt or + * yields a zero-width window). Pending window is always time-capped from now to endsAt. * @param agreementId The agreement ID * @param agreementScope Bitmask: SCOPE_ACTIVE (1), SCOPE_PENDING (2), or both (3) * @return maxClaim The maximum tokens claimable under the requested scope */ function _getMaxNextClaimScoped(bytes16 agreementId, uint8 agreementScope) private view returns (uint256 maxClaim) { + if (agreementScope == 0) agreementScope = SCOPE_ACTIVE | SCOPE_PENDING; + RecurringCollectorStorage storage $ = _getStorage(); AgreementData storage _a = $.agreements[agreementId]; - - uint256 maxActiveClaim = 0; - uint256 maxPendingClaim = 0; - - if (agreementScope & SCOPE_ACTIVE != 0) { - if (_a.state == AgreementState.NotAccepted) { - // Not yet accepted — check stored RCA offer - StoredOffer storage rcaOffer = $.rcaOffers[agreementId]; - if (rcaOffer.offerHash != bytes32(0)) { - RecurringCollectionAgreement memory rca = abi.decode(rcaOffer.data, (RecurringCollectionAgreement)); - // Use block.timestamp as proxy for acceptedAt, deadline as expiry - if (block.timestamp < rca.deadline) { - maxActiveClaim = _maxClaim( - block.timestamp, - rca.endsAt, - rca.maxSecondsPerCollection, - rca.maxOngoingTokensPerSecond, - rca.maxInitialTokens - ); - } - } - } else { - maxActiveClaim = _getMaxNextClaim(_a); - } + bool hasCollected = _a.lastCollectionAt != 0; + + if (agreementScope & SCOPE_ACTIVE != 0 && _a.activeTermsHash != bytes32(0)) { + AgreementTerms storage terms = $.terms[_a.activeTermsHash]; + AgreementState state = _a.state; + // Pre-acceptance: claimable while accept() is still admissible (block.timestamp <= + // deadline); past the deadline, start == end → zero window. + // Accepted / CanceledByPayer: start from last collection (or acceptedAt). + uint256 start = state == AgreementState.NotAccepted + ? (block.timestamp <= terms.deadline ? block.timestamp : terms.endsAt) + : _agreementCollectionStartAt(_a); + // CanceledByServiceProvider: zero window (end == start). CanceledByPayer: cap at canceledAt. + uint256 end = state == AgreementState.CanceledByServiceProvider + ? start + : ( + state == AgreementState.CanceledByPayer && _a.canceledAt < terms.endsAt + ? _a.canceledAt + : terms.endsAt + ); + maxClaim = _maxClaimForTerms(start, end, terms, hasCollected); } - if (agreementScope & SCOPE_PENDING != 0) { - StoredOffer storage rcauOffer = $.rcauOffers[agreementId]; - if (rcauOffer.offerHash != bytes32(0)) { - RecurringCollectionAgreementUpdate memory rcau = abi.decode( - rcauOffer.data, - (RecurringCollectionAgreementUpdate) - ); - // Ongoing claim: time-capped from now to rcau.endsAt - maxPendingClaim = _maxClaim( - block.timestamp, - rcau.endsAt, - rcau.maxSecondsPerCollection, - rcau.maxOngoingTokensPerSecond, - _a.lastCollectionAt == 0 ? rcau.maxInitialTokens : 0 - ); + if (agreementScope & SCOPE_PENDING != 0 && _a.pendingTermsHash != bytes32(0)) { + AgreementTerms storage terms = $.terms[_a.pendingTermsHash]; + // Skip expired pending — {update} rejects past-deadline RCAUs, so a stale pending + // offer can never be promoted to active and cannot be claimed on. + if (block.timestamp <= terms.deadline) { + uint256 pending = _maxClaimForTerms(block.timestamp, terms.endsAt, terms, hasCollected); + if (maxClaim < pending) maxClaim = pending; } } - - maxClaim = maxActiveClaim < maxPendingClaim ? maxPendingClaim : maxActiveClaim; } /** - * @notice Core claim formula: rate * min(window, maxSeconds) + initialBonus. - * @dev Single source of truth for all max-claim calculations. Returns 0 when - * windowEnd <= windowStart (empty or inverted window). + * @notice Compute max claim from a window and agreement terms. * @param windowStart Start of the collection window * @param windowEnd End of the collection window - * @param maxSecondsPerCollection Maximum seconds per collection period - * @param maxOngoingTokensPerSecond Maximum ongoing tokens per second - * @param maxInitialTokens Initial bonus tokens (0 if already collected) - * @return The maximum possible claim amount + * @param terms The agreement terms + * @param hasCollected Whether a collection has already occurred (suppresses initial bonus) + * @return maxClaim The maximum possible claim amount */ - function _maxClaim( + function _maxClaimForTerms( uint256 windowStart, uint256 windowEnd, - uint256 maxSecondsPerCollection, - uint256 maxOngoingTokensPerSecond, - uint256 maxInitialTokens - ) private pure returns (uint256) { - // solhint-disable-next-line gas-strict-inequalities + AgreementTerms storage terms, + bool hasCollected + ) private view returns (uint256 maxClaim) { if (windowEnd <= windowStart) return 0; uint256 windowSeconds = windowEnd - windowStart; - uint256 effectiveSeconds = windowSeconds < maxSecondsPerCollection ? windowSeconds : maxSecondsPerCollection; - return maxOngoingTokensPerSecond * effectiveSeconds + maxInitialTokens; + uint256 effectiveSeconds = windowSeconds < terms.maxSecondsPerCollection + ? windowSeconds + : terms.maxSecondsPerCollection; + maxClaim = terms.maxOngoingTokensPerSecond * effectiveSeconds; + if (!hasCollected) maxClaim += terms.maxInitialTokens; } /** @@ -1277,4 +1339,24 @@ contract RecurringCollector is ) private pure returns (bytes16) { return bytes16(keccak256(abi.encode(payer, dataService, serviceProvider, deadline, nonce))); } + + /** + * @notice Compute the agreement ID and EIP-712 hash for an RCA. + * @dev These are always used together when accepting or offering an RCA. + * @param _rca The Recurring Collection Agreement + * @return agreementId The deterministic agreement ID + * @return rcaHash The EIP-712 hash of the RCA + */ + function _rcaIdAndHash( + RecurringCollectionAgreement memory _rca + ) private view returns (bytes16 agreementId, bytes32 rcaHash) { + agreementId = _generateAgreementId( + _rca.payer, + _rca.dataService, + _rca.serviceProvider, + _rca.deadline, + _rca.nonce + ); + rcaHash = _hashRCA(_rca); + } } diff --git a/packages/horizon/test/unit/payments/recurring-collector/accept.t.sol b/packages/horizon/test/unit/payments/recurring-collector/accept.t.sol index d1742b690..fb7c06cb1 100644 --- a/packages/horizon/test/unit/payments/recurring-collector/accept.t.sol +++ b/packages/horizon/test/unit/payments/recurring-collector/accept.t.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.27; import { IRecurringCollector } from "@graphprotocol/interfaces/contracts/horizon/IRecurringCollector.sol"; +import { OFFER_TYPE_NEW } from "@graphprotocol/interfaces/contracts/horizon/IAgreementCollector.sol"; import { RecurringCollectorSharedTest } from "./shared.t.sol"; @@ -25,6 +26,8 @@ contract RecurringCollectorAcceptTest is RecurringCollectorSharedTest { ) public { // Ensure non-empty signature so the signed path is taken (which checks deadline first) vm.assume(fuzzySignature.length > 0); + // Pranking as the proxy admin hits ProxyDeniedAdminAccess before the deadline check. + vm.assume(fuzzyRCA.dataService != _proxyAdmin); // Generate deterministic agreement ID for validation bytes16 agreementId = _recurringCollector.generateAgreementId( fuzzyRCA.payer, @@ -47,7 +50,7 @@ contract RecurringCollectorAcceptTest is RecurringCollectorSharedTest { _recurringCollector.accept(fuzzyRCA, fuzzySignature); } - function test_Accept_Revert_WhenAlreadyAccepted(FuzzyTestAccept calldata fuzzyTestAccept) public { + function test_Accept_Idempotent_WhenAlreadyAccepted(FuzzyTestAccept calldata fuzzyTestAccept) public { ( IRecurringCollector.RecurringCollectionAgreement memory acceptedRca, bytes memory signature, @@ -55,12 +58,176 @@ contract RecurringCollectorAcceptTest is RecurringCollectorSharedTest { bytes16 agreementId ) = _sensibleAuthorizeAndAccept(fuzzyTestAccept); - bytes memory expectedErr = abi.encodeWithSelector( - IRecurringCollector.RecurringCollectorAgreementIncorrectState.selector, + // Re-accepting the same RCA is a no-op — succeeds without reverting or re-emitting. + vm.recordLogs(); + vm.prank(acceptedRca.dataService); + bytes16 returnedId = _recurringCollector.accept(acceptedRca, signature); + assertEq(returnedId, agreementId); + assertEq(vm.getRecordedLogs().length, 0, "no event emitted on idempotent re-accept"); + } + + /// @notice Re-accepting an already-accepted RCA at the same hash must still succeed after + /// the RCA's acceptance deadline has elapsed. The idempotent short-circuit runs before the + /// deadline check so signature lifetime is not consumed — this is the path the SubgraphService + /// relies on to rebind an agreement to a new allocation after the original acceptance window + /// has closed. + function test_Accept_Idempotent_AfterDeadline_SameHash(FuzzyTestAccept calldata fuzzyTestAccept) public { + ( + IRecurringCollector.RecurringCollectionAgreement memory acceptedRca, + bytes memory signature, + , + bytes16 agreementId + ) = _sensibleAuthorizeAndAccept(fuzzyTestAccept); + + // Warp past the RCA's deadline — a fresh accept would now revert with + // RecurringCollectorAgreementDeadlineElapsed. + vm.warp(uint256(acceptedRca.deadline) + 1); + + vm.recordLogs(); + vm.prank(acceptedRca.dataService); + bytes16 returnedId = _recurringCollector.accept(acceptedRca, signature); + assertEq(returnedId, agreementId, "returns the same agreementId"); + assertEq(vm.getRecordedLogs().length, 0, "no event emitted on idempotent re-accept after deadline"); + + // Sanity: the collector-side agreement is still in Accepted state, unchanged by the no-op. + IRecurringCollector.AgreementData memory agreement = _recurringCollector.getAgreement(agreementId); + assertEq(uint8(agreement.state), uint8(IRecurringCollector.AgreementState.Accepted)); + } + + /// @notice A fresh accept (no prior offer()) stores terms via _validateAndStoreTerms, which must + /// emit OfferStored. AgreementAccepted follows. Both events observable in order. + function test_Accept_EmitsOfferStored_WhenFreshTerms(FuzzyTestAccept calldata fuzzyTestAccept) public { + IRecurringCollector.RecurringCollectionAgreement memory rca = _recurringCollectorHelper.sensibleRCA( + fuzzyTestAccept.rca + ); + uint256 signerKey = boundKey(fuzzyTestAccept.unboundedSignerKey); + _recurringCollectorHelper.authorizeSignerWithChecks(rca.payer, signerKey); + (, bytes memory signature) = _recurringCollectorHelper.generateSignedRCA(rca, signerKey); + bytes32 rcaHash = _recurringCollector.hashRCA(rca); + bytes16 agreementId = _recurringCollector.generateAgreementId( + rca.payer, + rca.dataService, + rca.serviceProvider, + rca.deadline, + rca.nonce + ); + _setupValidProvision(rca.serviceProvider, rca.dataService); + + // OfferStored fires from _validateAndStoreTerms before _storeAgreement; AgreementAccepted + // follows the state transition at the end of accept(). + vm.expectEmit(address(_recurringCollector)); + emit IRecurringCollector.OfferStored(agreementId, rca.payer, OFFER_TYPE_NEW, rcaHash); + vm.expectEmit(address(_recurringCollector)); + emit IRecurringCollector.AgreementAccepted( + rca.dataService, + rca.payer, + rca.serviceProvider, agreementId, - IRecurringCollector.AgreementState.Accepted + rca.endsAt, + rca.maxInitialTokens, + rca.maxOngoingTokensPerSecond, + rca.minSecondsPerCollection, + rca.maxSecondsPerCollection + ); + vm.prank(rca.dataService); + _recurringCollector.accept(rca, signature); + } + + /// @notice A second RCA sharing the same agreementId seed (payer, dataService, serviceProvider, + /// deadline, nonce) but with different other fields — so different rcaHash — must not be + /// accepted against an already-Accepted agreement. The idempotent short-circuit only fires on + /// exact hash match; everything else must fall through to the state guard and revert. Proves + /// the short-circuit can't be abused as an overwrite path even in an imagined 128-bit + /// agreementId collision. + function test_Accept_Revert_WhenDifferentHashSameAgreementId(FuzzyTestAccept calldata fuzzyTestAccept) public { + ( + IRecurringCollector.RecurringCollectionAgreement memory acceptedRca, + , + uint256 signerKey, + bytes16 agreementId + ) = _sensibleAuthorizeAndAccept(fuzzyTestAccept); + + // Snapshot the original hash before constructing the variant. `variant = acceptedRca` in + // Solidity memory is a reference copy, so rebuild explicitly to vary one pricing field + // while keeping the 5 agreementId-seed fields (payer, dataService, serviceProvider, + // deadline, nonce) verbatim. + bytes32 originalHash = _recurringCollector.hashRCA(acceptedRca); + IRecurringCollector.RecurringCollectionAgreement memory variant = IRecurringCollector + .RecurringCollectionAgreement({ + deadline: acceptedRca.deadline, + endsAt: acceptedRca.endsAt, + payer: acceptedRca.payer, + dataService: acceptedRca.dataService, + serviceProvider: acceptedRca.serviceProvider, + maxInitialTokens: acceptedRca.maxInitialTokens + 1, // <-- vary + maxOngoingTokensPerSecond: acceptedRca.maxOngoingTokensPerSecond, + minSecondsPerCollection: acceptedRca.minSecondsPerCollection, + maxSecondsPerCollection: acceptedRca.maxSecondsPerCollection, + conditions: acceptedRca.conditions, + nonce: acceptedRca.nonce, + metadata: acceptedRca.metadata + }); + + bytes32 variantHash = _recurringCollector.hashRCA(variant); + assertTrue(originalHash != variantHash, "hashes must differ when any field differs"); + assertEq( + _recurringCollector.generateAgreementId( + variant.payer, + variant.dataService, + variant.serviceProvider, + variant.deadline, + variant.nonce + ), + agreementId, + "same agreementId seed yields same id" + ); + + (, bytes memory variantSig) = _recurringCollectorHelper.generateSignedRCA(variant, signerKey); + + // Short-circuit doesn't fire (hash differs); falls through to _storeAgreement's state guard. + vm.expectRevert( + abi.encodeWithSelector( + IRecurringCollector.RecurringCollectorAgreementIncorrectState.selector, + agreementId, + IRecurringCollector.AgreementState.Accepted + ) + ); + vm.prank(acceptedRca.dataService); + _recurringCollector.accept(variant, variantSig); + + // Post-revert sanity: storage reflects the original, not the variant. + IRecurringCollector.AgreementData memory agreement = _recurringCollector.getAgreement(agreementId); + assertEq(agreement.activeTermsHash, originalHash, "activeTermsHash unchanged"); + } + + /// @notice After a cancellation, re-accepting the same RCA at the same hash must revert — the + /// short-circuit only fires when state == Accepted, so a cancelled agreement falls through to + /// the NotAccepted state guard. Proves cancelled is terminal and the short-circuit cannot + /// resurrect it. + function test_Accept_Revert_AfterCancellation_SameHash(FuzzyTestAccept calldata fuzzyTestAccept) public { + ( + IRecurringCollector.RecurringCollectionAgreement memory acceptedRca, + bytes memory signature, + , + bytes16 agreementId + ) = _sensibleAuthorizeAndAccept(fuzzyTestAccept); + + vm.prank(acceptedRca.dataService); + _recurringCollector.cancel(agreementId, IRecurringCollector.CancelAgreementBy.ServiceProvider); + + assertEq( + uint8(_recurringCollector.getAgreement(agreementId).state), + uint8(IRecurringCollector.AgreementState.CanceledByServiceProvider), + "precondition: cancelled" + ); + + vm.expectRevert( + abi.encodeWithSelector( + IRecurringCollector.RecurringCollectorAgreementIncorrectState.selector, + agreementId, + IRecurringCollector.AgreementState.CanceledByServiceProvider + ) ); - vm.expectRevert(expectedErr); vm.prank(acceptedRca.dataService); _recurringCollector.accept(acceptedRca, signature); } diff --git a/packages/horizon/test/unit/payments/recurring-collector/acceptUnsigned.t.sol b/packages/horizon/test/unit/payments/recurring-collector/acceptUnsigned.t.sol index 7feca10c9..e535cd130 100644 --- a/packages/horizon/test/unit/payments/recurring-collector/acceptUnsigned.t.sol +++ b/packages/horizon/test/unit/payments/recurring-collector/acceptUnsigned.t.sol @@ -60,7 +60,6 @@ contract RecurringCollectorAcceptUnsignedTest is RecurringCollectorSharedTest { rca.payer, rca.serviceProvider, expectedId, - uint64(block.timestamp), rca.endsAt, rca.maxInitialTokens, rca.maxOngoingTokensPerSecond, @@ -129,7 +128,7 @@ contract RecurringCollectorAcceptUnsignedTest is RecurringCollectorSharedTest { _recurringCollector.accept(rca, ""); } - function test_AcceptUnsigned_Revert_WhenAlreadyAccepted(FuzzyTestAccept calldata fuzzyTestAccept) public { + function test_AcceptUnsigned_Idempotent_WhenAlreadyAccepted(FuzzyTestAccept calldata fuzzyTestAccept) public { MockAgreementOwner approver = _newApprover(); IRecurringCollector.RecurringCollectionAgreement memory rca = _recurringCollectorHelper.sensibleRCA( fuzzyTestAccept.rca @@ -144,16 +143,10 @@ contract RecurringCollectorAcceptUnsignedTest is RecurringCollectorSharedTest { vm.prank(rca.dataService); bytes16 agreementId = _recurringCollector.accept(rca, ""); - // Stored offer persists, so authorization passes but state check fails - vm.expectRevert( - abi.encodeWithSelector( - IRecurringCollector.RecurringCollectorAgreementIncorrectState.selector, - agreementId, - IRecurringCollector.AgreementState.Accepted - ) - ); + // Re-accepting the same RCA is a no-op — succeeds without reverting. vm.prank(rca.dataService); - _recurringCollector.accept(rca, ""); + bytes16 returnedId = _recurringCollector.accept(rca, ""); + assertEq(returnedId, agreementId); } function test_AcceptUnsigned_Revert_WhenDeadlineElapsed() public { diff --git a/packages/horizon/test/unit/payments/recurring-collector/acceptValidation.t.sol b/packages/horizon/test/unit/payments/recurring-collector/acceptValidation.t.sol index 5e47e2fb4..91e3e0bdd 100644 --- a/packages/horizon/test/unit/payments/recurring-collector/acceptValidation.t.sol +++ b/packages/horizon/test/unit/payments/recurring-collector/acceptValidation.t.sol @@ -69,11 +69,11 @@ contract RecurringCollectorAcceptValidationTest is RecurringCollectorSharedTest _recurringCollector.accept(rca, signature); } - // ==================== endsAt validation (L545) ==================== + // ==================== endsAt validation ==================== - function test_Accept_Revert_WhenEndsAtInPast() public { + function test_Accept_Revert_WhenEndsAtNotAfterDeadline() public { IRecurringCollector.RecurringCollectionAgreement memory rca = _makeValidRCA(); - rca.endsAt = uint64(block.timestamp); // endsAt == now, fails "endsAt > block.timestamp" + rca.endsAt = rca.deadline; // endsAt == deadline, fails "endsAt > deadline" _recurringCollectorHelper.authorizeSignerWithChecks(rca.payer, SIGNER_KEY); (, bytes memory signature) = _recurringCollectorHelper.generateSignedRCA(rca, SIGNER_KEY); @@ -81,8 +81,8 @@ contract RecurringCollectorAcceptValidationTest is RecurringCollectorSharedTest vm.expectRevert( abi.encodeWithSelector( - IRecurringCollector.RecurringCollectorAgreementElapsedEndsAt.selector, - block.timestamp, + IRecurringCollector.RecurringCollectorAgreementEndsBeforeDeadline.selector, + rca.deadline, rca.endsAt ) ); @@ -142,12 +142,12 @@ contract RecurringCollectorAcceptValidationTest is RecurringCollectorSharedTest function test_Accept_Revert_WhenDurationTooShort() public { IRecurringCollector.RecurringCollectionAgreement memory rca = _makeValidRCA(); - // Need: endsAt - now >= minSecondsPerCollection + MIN_SECONDS_COLLECTION_WINDOW + // Need: endsAt - deadline >= minSecondsPerCollection + MIN_SECONDS_COLLECTION_WINDOW // Set duration just under the minimum uint32 minWindow = _recurringCollector.MIN_SECONDS_COLLECTION_WINDOW(); rca.minSecondsPerCollection = 600; rca.maxSecondsPerCollection = 600 + minWindow; // valid window - rca.endsAt = uint64(block.timestamp + rca.minSecondsPerCollection + minWindow - 1); // 1 second too short + rca.endsAt = rca.deadline + rca.minSecondsPerCollection + minWindow - 1; // 1 second too short _recurringCollectorHelper.authorizeSignerWithChecks(rca.payer, SIGNER_KEY); (, bytes memory signature) = _recurringCollectorHelper.generateSignedRCA(rca, SIGNER_KEY); @@ -157,7 +157,7 @@ contract RecurringCollectorAcceptValidationTest is RecurringCollectorSharedTest abi.encodeWithSelector( IRecurringCollector.RecurringCollectorAgreementInvalidDuration.selector, rca.minSecondsPerCollection + minWindow, - rca.endsAt - block.timestamp + uint256(rca.endsAt - rca.deadline) ) ); vm.prank(rca.dataService); diff --git a/packages/horizon/test/unit/payments/recurring-collector/afterCollection.t.sol b/packages/horizon/test/unit/payments/recurring-collector/afterCollection.t.sol index 3e7396178..90ae638e7 100644 --- a/packages/horizon/test/unit/payments/recurring-collector/afterCollection.t.sol +++ b/packages/horizon/test/unit/payments/recurring-collector/afterCollection.t.sol @@ -143,7 +143,7 @@ contract RecurringCollectorAfterCollectionTest is RecurringCollectorSharedTest { ); // Binary-search for a gas limit that passes core collect logic but trips the - // callback gas guard (gasleft < MAX_PAYER_CALLBACK_GAS * 64/63 ≈ 1_523_810). + // callback gas guard (gasleft < MAX_PAYER_CALLBACK_GAS * 64/63 + CALLBACK_GAS_OVERHEAD ≈ 1_526_810). // Core logic + escrow call + beforeCollection + events uses ~200k gas. bool triggered; for (uint256 gasLimit = 1_700_000; gasLimit > 1_500_000; gasLimit -= 10_000) { @@ -166,6 +166,68 @@ contract RecurringCollectorAfterCollectionTest is RecurringCollectorSharedTest { assertTrue(triggered, "Should have triggered InsufficientCallbackGas at some gas limit"); } + /// @notice TRST-L-9: the CALLBACK_GAS_OVERHEAD precheck also guards the eligibility staticcall + /// (first of three callback prechecks). Binary-search for a gas limit that reaches the + /// eligibility precheck and trips it, confirming the buffer logic applies there too. + function test_Collect_Revert_WhenInsufficientCallbackGas_EligibilityPrecheck() public { + MockAgreementOwner approver = _newApprover(); + IRecurringCollector.RecurringCollectionAgreement memory rca = _recurringCollectorHelper.sensibleRCA( + IRecurringCollector.RecurringCollectionAgreement({ + deadline: uint64(block.timestamp + 1 hours), + endsAt: uint64(block.timestamp + 365 days), + payer: address(approver), + dataService: makeAddr("ds"), + serviceProvider: makeAddr("sp"), + maxInitialTokens: 100 ether, + maxOngoingTokensPerSecond: 1 ether, + minSecondsPerCollection: 600, + maxSecondsPerCollection: 3600, + conditions: 0, + nonce: 1, + metadata: "" + }) + ); + rca.conditions = 1; // CONDITION_ELIGIBILITY_CHECK — activates the eligibility precheck first + + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_NEW, abi.encode(rca), 0); + _setupValidProvision(rca.serviceProvider, rca.dataService); + + vm.prank(rca.dataService); + bytes16 agreementId = _recurringCollector.accept(rca, ""); + + skip(rca.minSecondsPerCollection); + uint256 tokens = 1 ether; + bytes memory data = _generateCollectData(_generateCollectParams(rca, agreementId, bytes32("col1"), tokens, 0)); + bytes memory callData = abi.encodeCall( + _recurringCollector.collect, + (IGraphPayments.PaymentTypes.IndexingFee, data) + ); + + // With eligibility enabled, three sequential callbacks each need the buffer. The test + // confirms at least one (the first, eligibility) trips InsufficientCallbackGas. + bool triggered; + for (uint256 gasLimit = 1_700_000; gasLimit > 1_500_000; gasLimit -= 10_000) { + uint256 snap = vm.snapshot(); + vm.prank(rca.dataService); + (bool success, bytes memory returnData) = address(_recurringCollector).call{ gas: gasLimit }(callData); + if (!success && returnData.length >= 4) { + bytes4 selector; + // solhint-disable-next-line no-inline-assembly + assembly { + selector := mload(add(returnData, 32)) + } + if (selector == IRecurringCollector.RecurringCollectorInsufficientCallbackGas.selector) { + triggered = true; + assertTrue(vm.revertTo(snap)); + break; + } + } + assertTrue(vm.revertTo(snap)); + } + assertTrue(triggered, "eligibility precheck must trip InsufficientCallbackGas under tight gas"); + } + function test_AfterCollection_NotCalledForEOAPayer(FuzzyTestCollect calldata fuzzy) public { // Use standard ECDSA-signed path (EOA payer, no contract) (IRecurringCollector.RecurringCollectionAgreement memory acceptedRca, , , ) = _sensibleAuthorizeAndAccept( diff --git a/packages/horizon/test/unit/payments/recurring-collector/cancel.t.sol b/packages/horizon/test/unit/payments/recurring-collector/cancel.t.sol index cf1da6743..1b19a2fc8 100644 --- a/packages/horizon/test/unit/payments/recurring-collector/cancel.t.sol +++ b/packages/horizon/test/unit/payments/recurring-collector/cancel.t.sol @@ -27,6 +27,8 @@ contract RecurringCollectorCancelTest is RecurringCollectorSharedTest { IRecurringCollector.RecurringCollectionAgreement memory fuzzyRCA, uint8 unboundedCanceler ) public { + vm.assume(fuzzyRCA.dataService != _proxyAdmin); + // Generate deterministic agreement ID bytes16 agreementId = _recurringCollector.generateAgreementId( fuzzyRCA.payer, diff --git a/packages/horizon/test/unit/payments/recurring-collector/cancelSignature.t.sol b/packages/horizon/test/unit/payments/recurring-collector/cancelSignature.t.sol new file mode 100644 index 000000000..9dadf2f6a --- /dev/null +++ b/packages/horizon/test/unit/payments/recurring-collector/cancelSignature.t.sol @@ -0,0 +1,256 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.27; + +import { IRecurringCollector } from "@graphprotocol/interfaces/contracts/horizon/IRecurringCollector.sol"; +import { + SCOPE_SIGNED, + SCOPE_ACTIVE, + SCOPE_PENDING +} from "@graphprotocol/interfaces/contracts/horizon/IAgreementCollector.sol"; + +import { RecurringCollectorSharedTest } from "./shared.t.sol"; + +contract RecurringCollectorCancelSignedOfferTest is RecurringCollectorSharedTest { + /* + * TESTS + */ + + /* solhint-disable graph/func-name-mixedcase */ + + function test_CancelSigned_BlocksAccept(FuzzyTestAccept calldata fuzzyTestAccept) public { + IRecurringCollector.RecurringCollectionAgreement memory rca = _recurringCollectorHelper.sensibleRCA( + fuzzyTestAccept.rca + ); + uint256 signerKey = boundKey(fuzzyTestAccept.unboundedSignerKey); + _recurringCollectorHelper.authorizeSignerWithChecks(rca.payer, signerKey); + + (, bytes memory signature) = _recurringCollectorHelper.generateSignedRCA(rca, signerKey); + bytes32 rcaHash = _recurringCollector.hashRCA(rca); + address signer = vm.addr(signerKey); + bytes16 agreementId = _recurringCollector.generateAgreementId( + rca.payer, + rca.dataService, + rca.serviceProvider, + rca.deadline, + rca.nonce + ); + + vm.prank(signer); + _recurringCollector.cancel(agreementId, rcaHash, SCOPE_SIGNED); + + // Accepting with the cancelled signature should revert + vm.expectRevert( + abi.encodeWithSelector(IRecurringCollector.RecurringCollectorOfferCancelled.selector, signer, rcaHash) + ); + vm.prank(rca.dataService); + _recurringCollector.accept(rca, signature); + } + + function test_CancelSigned_EmitsEvent(FuzzyTestAccept calldata fuzzyTestAccept) public { + IRecurringCollector.RecurringCollectionAgreement memory rca = _recurringCollectorHelper.sensibleRCA( + fuzzyTestAccept.rca + ); + uint256 signerKey = boundKey(fuzzyTestAccept.unboundedSignerKey); + _recurringCollectorHelper.authorizeSignerWithChecks(rca.payer, signerKey); + + bytes32 rcaHash = _recurringCollector.hashRCA(rca); + address signer = vm.addr(signerKey); + bytes16 agreementId = _recurringCollector.generateAgreementId( + rca.payer, + rca.dataService, + rca.serviceProvider, + rca.deadline, + rca.nonce + ); + + vm.expectEmit(address(_recurringCollector)); + emit IRecurringCollector.OfferCancelled(signer, agreementId, rcaHash); + vm.prank(signer); + _recurringCollector.cancel(agreementId, rcaHash, SCOPE_SIGNED); + } + + function test_CancelSigned_BlocksUpdate(FuzzyTestUpdate calldata fuzzyTestUpdate) public { + ( + IRecurringCollector.RecurringCollectionAgreement memory rca, + , + uint256 signerKey, + bytes16 agreementId + ) = _sensibleAuthorizeAndAccept(fuzzyTestUpdate.fuzzyTestAccept); + + IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau = _recurringCollectorHelper.sensibleRCAU( + fuzzyTestUpdate.rcau + ); + rcau.agreementId = agreementId; + + ( + IRecurringCollector.RecurringCollectionAgreementUpdate memory signedRcau, + bytes memory rcauSig + ) = _recurringCollectorHelper.generateSignedRCAUForAgreement(agreementId, rcau, signerKey); + bytes32 rcauHash = _recurringCollector.hashRCAU(signedRcau); + address signer = vm.addr(signerKey); + + vm.prank(signer); + _recurringCollector.cancel(agreementId, rcauHash, SCOPE_SIGNED); + + // Updating with the cancelled signature should revert + vm.expectRevert( + abi.encodeWithSelector(IRecurringCollector.RecurringCollectorOfferCancelled.selector, signer, rcauHash) + ); + vm.prank(rca.dataService); + _recurringCollector.update(signedRcau, rcauSig); + } + + function test_CancelSigned_Idempotent(FuzzyTestAccept calldata fuzzyTestAccept) public { + IRecurringCollector.RecurringCollectionAgreement memory rca = _recurringCollectorHelper.sensibleRCA( + fuzzyTestAccept.rca + ); + uint256 signerKey = boundKey(fuzzyTestAccept.unboundedSignerKey); + _recurringCollectorHelper.authorizeSignerWithChecks(rca.payer, signerKey); + + bytes32 rcaHash = _recurringCollector.hashRCA(rca); + address signer = vm.addr(signerKey); + bytes16 agreementId = _recurringCollector.generateAgreementId( + rca.payer, + rca.dataService, + rca.serviceProvider, + rca.deadline, + rca.nonce + ); + + vm.prank(signer); + _recurringCollector.cancel(agreementId, rcaHash, SCOPE_SIGNED); + + // Second call succeeds silently — no revert, no event + vm.recordLogs(); + vm.prank(signer); + _recurringCollector.cancel(agreementId, rcaHash, SCOPE_SIGNED); + assertEq(vm.getRecordedLogs().length, 0); + } + + function test_CancelSigned_DoesNotAffectDifferentSigner( + FuzzyTestAccept calldata fuzzyTestAccept1, + FuzzyTestAccept calldata fuzzyTestAccept2 + ) public { + IRecurringCollector.RecurringCollectionAgreement memory rca1 = _recurringCollectorHelper.sensibleRCA( + fuzzyTestAccept1.rca + ); + uint256 signerKey1 = boundKey(fuzzyTestAccept1.unboundedSignerKey); + + IRecurringCollector.RecurringCollectionAgreement memory rca2 = _recurringCollectorHelper.sensibleRCA( + fuzzyTestAccept2.rca + ); + uint256 signerKey2 = boundKey(fuzzyTestAccept2.unboundedSignerKey); + + vm.assume(rca1.payer != rca2.payer); + vm.assume(vm.addr(signerKey1) != vm.addr(signerKey2)); + + _recurringCollectorHelper.authorizeSignerWithChecks(rca1.payer, signerKey1); + _recurringCollectorHelper.authorizeSignerWithChecks(rca2.payer, signerKey2); + + bytes32 rcaHash = _recurringCollector.hashRCA(rca1); + + // Signer1 cancels — should not affect signer2 + vm.prank(vm.addr(signerKey1)); + _recurringCollector.cancel(bytes16(0), rcaHash, SCOPE_SIGNED); + + // Signer2's signatures for the same hash are unaffected + // (signer-scoped, not hash-global) + } + + function test_CancelSigned_SelfAuthenticating(FuzzyTestAccept calldata fuzzyTestAccept, address anyAddress) public { + // Any address can call cancel with SCOPE_SIGNED — it only records for msg.sender + IRecurringCollector.RecurringCollectionAgreement memory rca = _recurringCollectorHelper.sensibleRCA( + fuzzyTestAccept.rca + ); + bytes32 rcaHash = _recurringCollector.hashRCA(rca); + vm.assume(anyAddress != address(0)); + vm.assume(anyAddress != _proxyAdmin); + + // Should not revert — self-authenticating, no _requirePayer + vm.prank(anyAddress); + _recurringCollector.cancel(bytes16(0), rcaHash, SCOPE_SIGNED); + } + + function test_CancelSigned_CombinedWithActiveDoesNotRevert(FuzzyTestAccept calldata fuzzyTestAccept) public { + IRecurringCollector.RecurringCollectionAgreement memory rca = _recurringCollectorHelper.sensibleRCA( + fuzzyTestAccept.rca + ); + uint256 signerKey = boundKey(fuzzyTestAccept.unboundedSignerKey); + _recurringCollectorHelper.authorizeSignerWithChecks(rca.payer, signerKey); + + bytes32 rcaHash = _recurringCollector.hashRCA(rca); + address signer = vm.addr(signerKey); + bytes16 agreementId = _recurringCollector.generateAgreementId( + rca.payer, + rca.dataService, + rca.serviceProvider, + rca.deadline, + rca.nonce + ); + + // SCOPE_SIGNED | SCOPE_ACTIVE with no accepted agreement — should not revert. + // The signed recording succeeds; the active scope is skipped because nothing on-chain. + vm.expectEmit(address(_recurringCollector)); + emit IRecurringCollector.OfferCancelled(signer, agreementId, rcaHash); + vm.prank(signer); + _recurringCollector.cancel(agreementId, rcaHash, SCOPE_SIGNED | SCOPE_ACTIVE); + } + + function test_CancelSigned_CombinedWithPendingDoesNotRevert(FuzzyTestAccept calldata fuzzyTestAccept) public { + IRecurringCollector.RecurringCollectionAgreement memory rca = _recurringCollectorHelper.sensibleRCA( + fuzzyTestAccept.rca + ); + uint256 signerKey = boundKey(fuzzyTestAccept.unboundedSignerKey); + _recurringCollectorHelper.authorizeSignerWithChecks(rca.payer, signerKey); + + bytes32 rcaHash = _recurringCollector.hashRCA(rca); + address signer = vm.addr(signerKey); + bytes16 agreementId = _recurringCollector.generateAgreementId( + rca.payer, + rca.dataService, + rca.serviceProvider, + rca.deadline, + rca.nonce + ); + + // SCOPE_SIGNED | SCOPE_PENDING with no agreement — should not revert. + vm.expectEmit(address(_recurringCollector)); + emit IRecurringCollector.OfferCancelled(signer, agreementId, rcaHash); + vm.prank(signer); + _recurringCollector.cancel(agreementId, rcaHash, SCOPE_SIGNED | SCOPE_PENDING); + } + + function test_CancelSigned_UndoWithZero(FuzzyTestAccept calldata fuzzyTestAccept) public { + IRecurringCollector.RecurringCollectionAgreement memory rca = _recurringCollectorHelper.sensibleRCA( + fuzzyTestAccept.rca + ); + uint256 signerKey = boundKey(fuzzyTestAccept.unboundedSignerKey); + _recurringCollectorHelper.authorizeSignerWithChecks(rca.payer, signerKey); + + (, bytes memory signature) = _recurringCollectorHelper.generateSignedRCA(rca, signerKey); + bytes32 rcaHash = _recurringCollector.hashRCA(rca); + address signer = vm.addr(signerKey); + bytes16 agreementId = _recurringCollector.generateAgreementId( + rca.payer, + rca.dataService, + rca.serviceProvider, + rca.deadline, + rca.nonce + ); + + // Cancel + vm.prank(signer); + _recurringCollector.cancel(agreementId, rcaHash, SCOPE_SIGNED); + + // Undo by calling with bytes16(0) + vm.prank(signer); + _recurringCollector.cancel(bytes16(0), rcaHash, SCOPE_SIGNED); + + // Accept should now succeed + _setupValidProvision(rca.serviceProvider, rca.dataService); + vm.prank(rca.dataService); + _recurringCollector.accept(rca, signature); + } + + /* solhint-enable graph/func-name-mixedcase */ +} diff --git a/packages/horizon/test/unit/payments/recurring-collector/coverageGaps.t.sol b/packages/horizon/test/unit/payments/recurring-collector/coverageGaps.t.sol index 696f97584..cc693637f 100644 --- a/packages/horizon/test/unit/payments/recurring-collector/coverageGaps.t.sol +++ b/packages/horizon/test/unit/payments/recurring-collector/coverageGaps.t.sol @@ -849,17 +849,88 @@ contract RecurringCollectorCoverageGapsTest is RecurringCollectorSharedTest { assertEq(dataAfter.length, 0, "Offer data should be empty after cancel"); } + function test_Cancel_PendingRcaAndRcau_IndependentOrder() public { + MockAgreementOwner approver = new MockAgreementOwner(); + + IRecurringCollector.RecurringCollectionAgreement memory rca = IRecurringCollector.RecurringCollectionAgreement({ + deadline: uint64(block.timestamp + 1 hours), + endsAt: uint64(block.timestamp + 365 days), + payer: address(approver), + dataService: makeAddr("ds"), + serviceProvider: makeAddr("sp"), + maxInitialTokens: 100 ether, + maxOngoingTokensPerSecond: 1 ether, + minSecondsPerCollection: 600, + maxSecondsPerCollection: 3600, + conditions: 0, + nonce: 1, + metadata: "" + }); + + // Offer RCA (not yet accepted) + vm.prank(address(approver)); + IAgreementCollector.AgreementDetails memory details = _recurringCollector.offer( + OFFER_TYPE_NEW, + abi.encode(rca), + 0 + ); + bytes16 agreementId = details.agreementId; + bytes32 rcaHash = details.versionHash; + + // Offer RCAU on top of the pending RCA + IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau = IRecurringCollector + .RecurringCollectionAgreementUpdate({ + agreementId: agreementId, + deadline: uint64(block.timestamp + 1 hours), + endsAt: uint64(block.timestamp + 365 days), + maxInitialTokens: 200 ether, + maxOngoingTokensPerSecond: 2 ether, + minSecondsPerCollection: 600, + maxSecondsPerCollection: 3600, + conditions: 0, + nonce: 1, + metadata: "" + }); + vm.prank(address(approver)); + IAgreementCollector.AgreementDetails memory updateDetails = _recurringCollector.offer( + OFFER_TYPE_UPDATE, + abi.encode(rcau), + 0 + ); + bytes32 rcauHash = updateDetails.versionHash; + + // Cancel the RCA offer first — pending RCAU survives independently + vm.expectEmit(true, true, false, true); + emit IRecurringCollector.OfferCancelled(address(approver), agreementId, rcaHash); + vm.prank(address(approver)); + _recurringCollector.cancel(agreementId, rcaHash, SCOPE_PENDING); + + IRecurringCollector.AgreementData memory after1 = _recurringCollector.getAgreement(agreementId); + assertEq(after1.activeTermsHash, bytes32(0), "active should be cleared"); + assertEq(after1.pendingTermsHash, rcauHash, "pending RCAU should survive RCA cancel"); + assertEq(after1.payer, address(approver), "agreement.payer persists for subsequent auth"); + + // Now cancel the pending RCAU — payer auth still works via persistent agreement.payer + vm.expectEmit(true, true, false, true); + emit IRecurringCollector.OfferCancelled(address(approver), agreementId, rcauHash); + vm.prank(address(approver)); + _recurringCollector.cancel(agreementId, rcauHash, SCOPE_PENDING); + + (uint8 activeType, ) = _recurringCollector.getAgreementOfferAt(agreementId, 0); + assertEq(activeType, 0, "Active offer should be gone"); + (uint8 pendingType, ) = _recurringCollector.getAgreementOfferAt(agreementId, 1); + assertEq(pendingType, 0, "Pending offer should be gone"); + } + // ══════════════════════════════════════════════════════════════════════ - // Gap 16 — _requirePayer: agreement not found (L528) + // Gap 16 — cancel: silent no-op when agreement not found // ══════════════════════════════════════════════════════════════════════ - function test_Cancel_Revert_WhenAgreementNotFound() public { + function test_Cancel_NoOp_WhenAgreementNotFound() public { bytes16 fakeId = bytes16(keccak256("nonexistent")); address caller = makeAddr("randomCaller"); - vm.expectRevert( - abi.encodeWithSelector(IRecurringCollector.RecurringCollectorAgreementNotFound.selector, fakeId) - ); + // Should not revert — nothing exists on-chain, so cancel is a no-op vm.prank(caller); _recurringCollector.cancel(fakeId, bytes32(0), SCOPE_ACTIVE); } diff --git a/packages/horizon/test/unit/payments/recurring-collector/getAgreementDetails.t.sol b/packages/horizon/test/unit/payments/recurring-collector/getAgreementDetails.t.sol index 91d788020..42c847394 100644 --- a/packages/horizon/test/unit/payments/recurring-collector/getAgreementDetails.t.sol +++ b/packages/horizon/test/unit/payments/recurring-collector/getAgreementDetails.t.sol @@ -5,7 +5,13 @@ import { IRecurringCollector } from "@graphprotocol/interfaces/contracts/horizon import { IAgreementCollector, OFFER_TYPE_NEW, - REGISTERED + REGISTERED, + ACCEPTED, + NOTICE_GIVEN, + SETTLED, + BY_PAYER, + BY_PROVIDER, + VERSION_CURRENT } from "@graphprotocol/interfaces/contracts/horizon/IAgreementCollector.sol"; import { RecurringCollectorSharedTest } from "./shared.t.sol"; @@ -107,4 +113,83 @@ contract RecurringCollectorGetAgreementDetailsTest is RecurringCollectorSharedTe assertEq(details.serviceProvider, rca.serviceProvider); assertNotEq(details.versionHash, bytes32(0)); } + + // -- Cancel sets NOTICE_GIVEN + origin flag; provider cancel is always SETTLED -- + + function test_GetAgreementDetails_CanceledByServiceProvider_Flags(FuzzyTestAccept calldata fuzzyTestAccept) public { + ( + IRecurringCollector.RecurringCollectionAgreement memory rca, + , + , + bytes16 agreementId + ) = _sensibleAuthorizeAndAccept(fuzzyTestAccept); + + vm.prank(rca.dataService); + _recurringCollector.cancel(agreementId, IRecurringCollector.CancelAgreementBy.ServiceProvider); + + IAgreementCollector.AgreementDetails memory details = _recurringCollector.getAgreementDetails( + agreementId, + VERSION_CURRENT + ); + + assertEq( + details.state, + REGISTERED | ACCEPTED | NOTICE_GIVEN | BY_PROVIDER | SETTLED, + "provider cancel: REGISTERED|ACCEPTED|NOTICE_GIVEN|BY_PROVIDER|SETTLED" + ); + } + + function test_GetAgreementDetails_CanceledByPayer_Flags(FuzzyTestAccept calldata fuzzyTestAccept) public { + ( + IRecurringCollector.RecurringCollectionAgreement memory rca, + , + , + bytes16 agreementId + ) = _sensibleAuthorizeAndAccept(fuzzyTestAccept); + + vm.prank(rca.dataService); + _recurringCollector.cancel(agreementId, IRecurringCollector.CancelAgreementBy.Payer); + + IAgreementCollector.AgreementDetails memory details = _recurringCollector.getAgreementDetails( + agreementId, + VERSION_CURRENT + ); + + uint16 baseline = REGISTERED | ACCEPTED | NOTICE_GIVEN | BY_PAYER; + assertTrue( + details.state == baseline || details.state == (baseline | SETTLED), + "payer cancel: REGISTERED|ACCEPTED|NOTICE_GIVEN|BY_PAYER (+SETTLED if fully elapsed)" + ); + assertEq(details.state & NOTICE_GIVEN, NOTICE_GIVEN, "NOTICE_GIVEN set"); + assertEq(details.state & BY_PAYER, BY_PAYER, "BY_PAYER set"); + assertEq(details.state & BY_PROVIDER, 0, "BY_PROVIDER not set"); + } + + // -- Accepted agreement with nothing left to claim reports SETTLED -- + + function test_GetAgreementDetails_Accepted_ElapsedSetsSettled(FuzzyTestAccept calldata fuzzyTestAccept) public { + ( + IRecurringCollector.RecurringCollectionAgreement memory rca, + , + , + bytes16 agreementId + ) = _sensibleAuthorizeAndAccept(fuzzyTestAccept); + + // Jump past the agreement's end so no further collection is possible once lastCollectionAt + // catches up. Without any collections, _getMaxNextClaim still returns a non-zero value + // (late-collection semantics), so the clearest SETTLED case is via provider cancel — but + // we want to assert the non-cancel path here too. Simulate fully-collected state by + // advancing to endsAt + 1 and marking lastCollectionAt == endsAt via a well-formed path: + // easiest is a payer cancel far in the past (canceledAt in the past → window empty). + vm.prank(rca.dataService); + _recurringCollector.cancel(agreementId, IRecurringCollector.CancelAgreementBy.Payer); + vm.warp(rca.endsAt + 1); + + IAgreementCollector.AgreementDetails memory details = _recurringCollector.getAgreementDetails( + agreementId, + VERSION_CURRENT + ); + + assertEq(details.state & SETTLED, SETTLED, "SETTLED set when nothing left to claim"); + } } diff --git a/packages/horizon/test/unit/payments/recurring-collector/getMaxNextClaim.t.sol b/packages/horizon/test/unit/payments/recurring-collector/getMaxNextClaim.t.sol index 58aa6961d..fb46ba2dc 100644 --- a/packages/horizon/test/unit/payments/recurring-collector/getMaxNextClaim.t.sol +++ b/packages/horizon/test/unit/payments/recurring-collector/getMaxNextClaim.t.sol @@ -505,5 +505,189 @@ contract RecurringCollectorGetMaxNextClaimTest is RecurringCollectorSharedTest { assertLt(windowSeconds, maxSecondsPerCollection, "Window should be smaller than maxSecondsPerCollection"); } + /// @notice Symmetry of the pending-deadline fix for the pre-acceptance active branch. + /// An agreement that has been offered but not yet accepted (state == NotAccepted, but + /// activeTermsHash set) is admissible for acceptance at exactly `terms.deadline` because + /// accept() gates on `block.timestamp <= rca.deadline`. RAM's reservation envelope must + /// therefore still cover the potential claim window at that block. One second past, accept() + /// would revert and the agreement is unreachable, so max-claim drops to zero. + function test_GetMaxNextClaim_PreAcceptanceActiveAtExactDeadline_StillCounts() public { + MockAgreementOwner approver = new MockAgreementOwner(); + + // Build RCA manually so we control the exact deadline. + uint64 rcaDeadline = uint64(block.timestamp + 1 hours); + IRecurringCollector.RecurringCollectionAgreement memory rca = IRecurringCollector.RecurringCollectionAgreement({ + deadline: rcaDeadline, + endsAt: uint64(block.timestamp + 365 days), + payer: address(approver), + dataService: makeAddr("ds"), + serviceProvider: makeAddr("sp"), + maxInitialTokens: 100 ether, + maxOngoingTokensPerSecond: 1 ether, + minSecondsPerCollection: 600, + maxSecondsPerCollection: 3600, + conditions: 0, + nonce: 1, + metadata: "" + }); + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_NEW, abi.encode(rca), 0); + + bytes16 agreementId = _recurringCollector.generateAgreementId( + rca.payer, + rca.dataService, + rca.serviceProvider, + rca.deadline, + rca.nonce + ); + // Agreement is in NotAccepted state — activeTermsHash is set (by offer) but no accept() yet. + assertEq( + uint8(_recurringCollector.getAgreement(agreementId).state), + uint8(IRecurringCollector.AgreementState.NotAccepted), + "precondition: NotAccepted" + ); + + // One second before the deadline: pre-acceptance active counts. + vm.warp(uint256(rcaDeadline) - 1); + assertGt(_recurringCollector.getMaxNextClaim(agreementId, 1), 0, "active counts before deadline"); + + // At the exact deadline: accept() is still admissible (<=), so the pre-acceptance window + // must still count in the reservation envelope. + vm.warp(uint256(rcaDeadline)); + assertGt(_recurringCollector.getMaxNextClaim(agreementId, 1), 0, "active should still count at exact deadline"); + + // One second past the deadline: accept() would revert, so max-claim drops to zero. + vm.warp(uint256(rcaDeadline) + 1); + assertEq(_recurringCollector.getMaxNextClaim(agreementId, 1), 0, "active zero one second past deadline"); + } + + /// @notice Boundary: the guard uses `block.timestamp <= terms.deadline` (inclusive) to match + /// {update}'s admissibility — at the exact deadline block, update() can still promote the + /// pending to active, so RAM must keep reserving for it. One second past the deadline, the + /// pending is no longer admissible and drops to zero. + function test_GetMaxNextClaim_PendingAtExactDeadline_StillCounts() public { + MockAgreementOwner approver = new MockAgreementOwner(); + IRecurringCollector.RecurringCollectionAgreement memory rca = _recurringCollectorHelper.sensibleRCA( + IRecurringCollector.RecurringCollectionAgreement({ + deadline: uint64(block.timestamp + 1 hours), + endsAt: uint64(block.timestamp + 365 days), + payer: address(approver), + dataService: makeAddr("ds"), + serviceProvider: makeAddr("sp"), + maxInitialTokens: 100 ether, + maxOngoingTokensPerSecond: 1 ether, + minSecondsPerCollection: 600, + maxSecondsPerCollection: 3600, + conditions: 0, + nonce: 1, + metadata: "" + }) + ); + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_NEW, abi.encode(rca), 0); + _setupValidProvision(rca.serviceProvider, rca.dataService); + vm.prank(rca.dataService); + bytes16 agreementId = _recurringCollector.accept(rca, ""); + + // Build RCAU manually (not via sensibleRCAU, which overrides deadline to a tight window) + // so we can pick a deadline we control and warp exactly to its boundary. + uint64 pendingDeadline = uint64(block.timestamp + 1 hours); + IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau = IRecurringCollector + .RecurringCollectionAgreementUpdate({ + agreementId: agreementId, + deadline: pendingDeadline, + endsAt: uint64(block.timestamp + 730 days), + maxInitialTokens: 200 ether, + maxOngoingTokensPerSecond: 10 ether, + minSecondsPerCollection: 600, + maxSecondsPerCollection: 7200, + conditions: 0, + nonce: 1, + metadata: "" + }); + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_UPDATE, abi.encode(rcau), 0); + + // One second before the deadline: pending counts. + vm.warp(uint256(pendingDeadline) - 1); + assertGt(_recurringCollector.getMaxNextClaim(agreementId, 2), 0, "pending counts before deadline"); + + // At the exact deadline: guard is inclusive `<=`, matching update()'s admissibility. + // update() can still promote the pending to active on this block, so RAM must keep it + // in the reservation envelope. + vm.warp(uint256(pendingDeadline)); + assertGt(_recurringCollector.getMaxNextClaim(agreementId, 2), 0, "pending counts at exact deadline"); + + // One second past the deadline: update() would revert, so pending drops to zero. + vm.warp(uint256(pendingDeadline) + 1); + assertEq(_recurringCollector.getMaxNextClaim(agreementId, 2), 0, "pending zero one second past deadline"); + } + + /// @notice An expired pending offer (deadline in the past, endsAt still in the future) must not + /// contribute to max-claim. {update} rejects past-deadline RCAUs so the pending can never be + /// promoted to active; counting it would over-reserve escrow in RAM. + function test_GetMaxNextClaim_PendingIgnored_AfterDeadline() public { + MockAgreementOwner approver = new MockAgreementOwner(); + IRecurringCollector.RecurringCollectionAgreement memory rca = _recurringCollectorHelper.sensibleRCA( + IRecurringCollector.RecurringCollectionAgreement({ + deadline: uint64(block.timestamp + 1 hours), + endsAt: uint64(block.timestamp + 365 days), + payer: address(approver), + dataService: makeAddr("ds"), + serviceProvider: makeAddr("sp"), + maxInitialTokens: 100 ether, + maxOngoingTokensPerSecond: 1 ether, + minSecondsPerCollection: 600, + maxSecondsPerCollection: 3600, + conditions: 0, + nonce: 1, + metadata: "" + }) + ); + + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_NEW, abi.encode(rca), 0); + _setupValidProvision(rca.serviceProvider, rca.dataService); + vm.prank(rca.dataService); + bytes16 agreementId = _recurringCollector.accept(rca, ""); + + // Pending RCAU with higher rate + short acceptance deadline but long endsAt. Build manually + // so we control the deadline exactly (sensibleRCAU would override it to a bounded window). + uint64 pendingDeadline = uint64(block.timestamp + 1 hours); + IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau = IRecurringCollector + .RecurringCollectionAgreementUpdate({ + agreementId: agreementId, + deadline: pendingDeadline, + endsAt: uint64(block.timestamp + 730 days), + maxInitialTokens: 200 ether, + maxOngoingTokensPerSecond: 10 ether, + minSecondsPerCollection: 600, + maxSecondsPerCollection: 7200, + conditions: 0, + nonce: 1, + metadata: "" + }); + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_UPDATE, abi.encode(rcau), 0); + + uint256 activeClaim = _recurringCollector.getMaxNextClaim(agreementId, 1); // SCOPE_ACTIVE + + // Before deadline: higher-rate pending dominates the combined claim. + uint256 beforeDeadline = _recurringCollector.getMaxNextClaim(agreementId); + assertGt(beforeDeadline, activeClaim, "live pending dominates before its deadline"); + + // Warp one second past the pending's deadline. endsAt is still well in the future, so + // _maxClaimForTerms would still return a large number — but the pending can no longer + // be accepted via update(), so it must not contribute. + vm.warp(uint256(pendingDeadline) + 1); + + uint256 pendingScopeAfter = _recurringCollector.getMaxNextClaim(agreementId, 2); // SCOPE_PENDING + assertEq(pendingScopeAfter, 0, "expired pending returns 0 under SCOPE_PENDING"); + + uint256 combinedAfter = _recurringCollector.getMaxNextClaim(agreementId); + uint256 activeAfter = _recurringCollector.getMaxNextClaim(agreementId, 1); + assertEq(combinedAfter, activeAfter, "combined scope falls back to active-only after pending expires"); + } + /* solhint-enable graph/func-name-mixedcase */ } diff --git a/packages/horizon/test/unit/payments/recurring-collector/hashRoundTrip.t.sol b/packages/horizon/test/unit/payments/recurring-collector/hashRoundTrip.t.sol index 7c5c73cbe..cc75e78d9 100644 --- a/packages/horizon/test/unit/payments/recurring-collector/hashRoundTrip.t.sol +++ b/packages/horizon/test/unit/payments/recurring-collector/hashRoundTrip.t.sol @@ -177,8 +177,8 @@ contract RecurringCollectorHashRoundTripTest is RecurringCollectorSharedTest { IRecurringCollector.AgreementData memory agreement = _recurringCollector.getAgreement(agreementId); assertEq(agreement.activeTermsHash, rcauHash, "activeTermsHash should be RCAU hash after update"); - // Stored update offer persists after update - _verifyOfferRoundTrip(agreementId, 1, rcauHash); + // After update, RCAU becomes the active version (VERSION_CURRENT = 0) + _verifyOfferRoundTrip(agreementId, 0, rcauHash); } // ==================== Cancel pending, active stays ==================== diff --git a/packages/horizon/test/unit/payments/recurring-collector/mixedPath.t.sol b/packages/horizon/test/unit/payments/recurring-collector/mixedPath.t.sol index f81aa0f04..659979dee 100644 --- a/packages/horizon/test/unit/payments/recurring-collector/mixedPath.t.sol +++ b/packages/horizon/test/unit/payments/recurring-collector/mixedPath.t.sol @@ -64,7 +64,6 @@ contract RecurringCollectorMixedPathTest is RecurringCollectorSharedTest { address(approver), rca.serviceProvider, agreementId, - uint64(block.timestamp), rcau.endsAt, rcau.maxInitialTokens, rcau.maxOngoingTokensPerSecond, @@ -77,8 +76,7 @@ contract RecurringCollectorMixedPathTest is RecurringCollectorSharedTest { // Verify updated terms IRecurringCollector.AgreementData memory agreement = _recurringCollector.getAgreement(agreementId); - assertEq(agreement.maxOngoingTokensPerSecond, rcau.maxOngoingTokensPerSecond); - assertEq(agreement.maxSecondsPerCollection, rcau.maxSecondsPerCollection); + assertEq(agreement.activeTermsHash, _recurringCollector.hashRCAU(rcau)); assertEq(agreement.updateNonce, 1); } @@ -185,7 +183,6 @@ contract RecurringCollectorMixedPathTest is RecurringCollectorSharedTest { payer, rca.serviceProvider, agreementId, - uint64(block.timestamp), rcau.endsAt, rcau.maxInitialTokens, rcau.maxOngoingTokensPerSecond, @@ -197,9 +194,87 @@ contract RecurringCollectorMixedPathTest is RecurringCollectorSharedTest { _recurringCollector.update(rcau, updateSig); IRecurringCollector.AgreementData memory agreement = _recurringCollector.getAgreement(agreementId); - assertEq(agreement.maxOngoingTokensPerSecond, rcau.maxOngoingTokensPerSecond); + assertEq(agreement.activeTermsHash, _recurringCollector.hashRCAU(rcau)); assertEq(agreement.updateNonce, 1); } + /// @notice Replacing the active offer preserves an independent pending RCAU. The update is + /// still a valid signed offer against the same agreementId; the payer may cancel it + /// explicitly if they don't want it. The contract shouldn't silently invalidate it. + function test_MixedPath_OfferNew_PreservesPendingRcau() public { + MockAgreementOwner approver = new MockAgreementOwner(); + + IRecurringCollector.RecurringCollectionAgreement memory rca1 = _recurringCollectorHelper.sensibleRCA( + IRecurringCollector.RecurringCollectionAgreement({ + deadline: 0, + endsAt: 0, + payer: address(approver), + dataService: makeAddr("ds"), + serviceProvider: makeAddr("sp"), + maxInitialTokens: 100 ether, + maxOngoingTokensPerSecond: 1 ether, + minSecondsPerCollection: 600, + maxSecondsPerCollection: 3600, + conditions: 0, + nonce: 1, + metadata: "" + }) + ); + // Derive the deterministic agreement ID from rca1's post-sensible fields. + bytes16 agreementId = _recurringCollector.generateAgreementId( + rca1.payer, + rca1.dataService, + rca1.serviceProvider, + rca1.deadline, + rca1.nonce + ); + + // Step 1: offer RCA → active = hashRCA(rca1) + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_NEW, abi.encode(rca1), 0); + bytes32 rca1Hash = _recurringCollector.hashRCA(rca1); + + // Step 2: offer RCAU → pending = hashRCAU(rcau) + IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau = _recurringCollectorHelper.sensibleRCAU( + IRecurringCollector.RecurringCollectionAgreementUpdate({ + agreementId: agreementId, + deadline: 0, + endsAt: 0, + maxInitialTokens: 200 ether, + maxOngoingTokensPerSecond: 2 ether, + minSecondsPerCollection: 600, + maxSecondsPerCollection: 7200, + conditions: 0, + nonce: 1, + metadata: "" + }) + ); + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_UPDATE, abi.encode(rcau), 0); + bytes32 rcauHash = _recurringCollector.hashRCAU(rcau); + + // Pre-check: pending is set + IRecurringCollector.AgreementData memory before = _recurringCollector.getAgreement(agreementId); + assertEq(before.activeTermsHash, rca1Hash, "active should be rca1Hash after offer"); + assertEq(before.pendingTermsHash, rcauHash, "pending should be rcauHash after offer UPDATE"); + + // Step 3: offer different RCA with same primary fields (same agreementId, different terms) + IRecurringCollector.RecurringCollectionAgreement memory rca2 = rca1; + rca2.maxInitialTokens = 999 ether; // different terms → different hash, same agreementId + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_NEW, abi.encode(rca2), 0); + bytes32 rca2Hash = _recurringCollector.hashRCA(rca2); + + // Post-check: active replaced, pending preserved (still the original RCAU) + IRecurringCollector.AgreementData memory afterOffer = _recurringCollector.getAgreement(agreementId); + assertEq(afterOffer.activeTermsHash, rca2Hash, "active should be rca2Hash"); + assertEq(afterOffer.pendingTermsHash, rcauHash, "pending RCAU should still be queued"); + + // The pending offer's $.terms entry must still be retrievable — payer can still accept it + (uint8 pendingType, bytes memory pendingData) = _recurringCollector.getAgreementOfferAt(agreementId, 1); + assertEq(pendingType, OFFER_TYPE_UPDATE, "pending slot should still hold update offer"); + assertEq(keccak256(pendingData), keccak256(abi.encode(rcau)), "pending data should be the original RCAU"); + } + /* solhint-enable graph/func-name-mixedcase */ } diff --git a/packages/horizon/test/unit/payments/recurring-collector/offerStorageLifecycle.t.sol b/packages/horizon/test/unit/payments/recurring-collector/offerStorageLifecycle.t.sol new file mode 100644 index 000000000..a4988b4b0 --- /dev/null +++ b/packages/horizon/test/unit/payments/recurring-collector/offerStorageLifecycle.t.sol @@ -0,0 +1,354 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.27; + +import { Vm } from "forge-std/Vm.sol"; + +import { IRecurringCollector } from "@graphprotocol/interfaces/contracts/horizon/IRecurringCollector.sol"; +import { + IAgreementCollector, + OFFER_TYPE_NONE, + OFFER_TYPE_NEW, + OFFER_TYPE_UPDATE, + SCOPE_PENDING, + VERSION_CURRENT, + VERSION_NEXT +} from "@graphprotocol/interfaces/contracts/horizon/IAgreementCollector.sol"; + +import { RecurringCollectorSharedTest } from "./shared.t.sol"; +import { MockAgreementOwner } from "./MockAgreementOwner.t.sol"; + +/// @notice Targeted coverage for the hash-keyed offer storage refactor. +contract RecurringCollectorOfferStorageLifecycleTest is RecurringCollectorSharedTest { + /* solhint-disable graph/func-name-mixedcase */ + + function _makeRca(address payer) internal returns (IRecurringCollector.RecurringCollectionAgreement memory) { + return + _recurringCollectorHelper.sensibleRCA( + IRecurringCollector.RecurringCollectionAgreement({ + deadline: uint64(block.timestamp + 1 hours), + endsAt: uint64(block.timestamp + 365 days), + payer: payer, + dataService: makeAddr("ds"), + serviceProvider: makeAddr("sp"), + maxInitialTokens: 100 ether, + maxOngoingTokensPerSecond: 1 ether, + minSecondsPerCollection: 600, + maxSecondsPerCollection: 3600, + conditions: 0, + nonce: 1, + metadata: "" + }) + ); + } + + function _makeRcau( + bytes16 agreementId, + IRecurringCollector.RecurringCollectionAgreement memory rca, + uint32 nonce + ) internal view returns (IRecurringCollector.RecurringCollectionAgreementUpdate memory) { + return + IRecurringCollector.RecurringCollectionAgreementUpdate({ + agreementId: agreementId, + deadline: uint64(block.timestamp + 1 hours), + endsAt: rca.endsAt + 30 days, + maxInitialTokens: rca.maxInitialTokens, + maxOngoingTokensPerSecond: rca.maxOngoingTokensPerSecond * 2, + minSecondsPerCollection: rca.minSecondsPerCollection, + maxSecondsPerCollection: rca.maxSecondsPerCollection, + conditions: 0, + nonce: nonce, + metadata: "" + }); + } + + // ────────────────────────────────────────────────────────────────────── + // Hash-keyed offer storage lifecycle + // ────────────────────────────────────────────────────────────────────── + + /// @notice offer(RCA) creates a storage entry at the EIP-712 hash and emits OfferStored. + function test_OfferNew_StoresEntryAtHash_EmitsEvent() public { + MockAgreementOwner approver = new MockAgreementOwner(); + IRecurringCollector.RecurringCollectionAgreement memory rca = _makeRca(address(approver)); + bytes32 rcaHash = _recurringCollector.hashRCA(rca); + bytes16 agreementId = _recurringCollector.generateAgreementId( + rca.payer, + rca.dataService, + rca.serviceProvider, + rca.deadline, + rca.nonce + ); + + vm.expectEmit(address(_recurringCollector)); + emit IRecurringCollector.OfferStored(agreementId, rca.payer, OFFER_TYPE_NEW, rcaHash); + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_NEW, abi.encode(rca), 0); + + (uint8 offerType, bytes memory offerData) = _recurringCollector.getAgreementOfferAt( + agreementId, + VERSION_CURRENT + ); + assertEq(offerType, OFFER_TYPE_NEW, "stored entry at rcaHash"); + assertTrue(offerData.length > 0, "stored data non-empty"); + + IRecurringCollector.AgreementData memory agreement = _recurringCollector.getAgreement(agreementId); + assertEq(agreement.activeTermsHash, rcaHash, "agreement.activeTermsHash points at offer hash"); + assertEq(agreement.pendingTermsHash, bytes32(0), "no pending before update"); + } + + /// @notice Re-offering the identical RCA is idempotent — no second OfferStored event, storage unchanged. + function test_OfferNew_Idempotent_WhenResubmittedSameHash() public { + MockAgreementOwner approver = new MockAgreementOwner(); + IRecurringCollector.RecurringCollectionAgreement memory rca = _makeRca(address(approver)); + + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_NEW, abi.encode(rca), 0); + + // Second call with the same RCA must not emit OfferStored again + vm.recordLogs(); + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_NEW, abi.encode(rca), 0); + Vm.Log[] memory logs = vm.getRecordedLogs(); + bytes32 offerStoredSig = keccak256("OfferStored(bytes16,address,uint8,bytes32)"); + for (uint256 i = 0; i < logs.length; i++) { + if (logs[i].topics.length > 0) { + assertFalse(logs[i].topics[0] == offerStoredSig, "no duplicate OfferStored on re-offer"); + } + } + } + + /// @notice Accepting a stored offer preserves the offer entry — getAgreementOfferAt still returns it. + function test_OfferNew_EntryPersistsAcrossAccept() public { + MockAgreementOwner approver = new MockAgreementOwner(); + IRecurringCollector.RecurringCollectionAgreement memory rca = _makeRca(address(approver)); + _setupValidProvision(rca.serviceProvider, rca.dataService); + + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_NEW, abi.encode(rca), 0); + vm.prank(rca.dataService); + bytes16 agreementId = _recurringCollector.accept(rca, ""); + + (uint8 offerType, bytes memory offerData) = _recurringCollector.getAgreementOfferAt( + agreementId, + VERSION_CURRENT + ); + assertEq(offerType, OFFER_TYPE_NEW, "accept does not delete the RCA offer entry"); + assertTrue(offerData.length > 0, "accept preserves stored data"); + } + + /// @notice A successful update deletes the prior active offer from storage; the new RCAU terms + /// become VERSION_CURRENT (OFFER_TYPE_UPDATE) and the pending slot clears. + function test_Update_DeletesPriorActiveOffer_PromotesRcauToCurrent() public { + MockAgreementOwner approver = new MockAgreementOwner(); + IRecurringCollector.RecurringCollectionAgreement memory rca = _makeRca(address(approver)); + _setupValidProvision(rca.serviceProvider, rca.dataService); + + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_NEW, abi.encode(rca), 0); + vm.prank(rca.dataService); + bytes16 agreementId = _recurringCollector.accept(rca, ""); + bytes32 rcaHash = _recurringCollector.hashRCA(rca); + + IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau = _makeRcau(agreementId, rca, 1); + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_UPDATE, abi.encode(rcau), 0); + bytes32 rcauHash = _recurringCollector.hashRCAU(rcau); + + vm.prank(rca.dataService); + _recurringCollector.update(rcau, ""); + + // Prior active (RCA) offer deleted from storage — since activeTermsHash now points at rcauHash, + // a fresh agreementId derived with mismatched hash should return empty at the rcaHash slot. + // We assert via getAgreementDetails: rcaHash is no longer a current version. + IRecurringCollector.AgreementData memory agreement = _recurringCollector.getAgreement(agreementId); + assertEq(agreement.activeTermsHash, rcauHash, "activeTermsHash = rcauHash after update"); + assertEq(agreement.pendingTermsHash, bytes32(0), "pendingTermsHash cleared after update"); + + (uint8 currentType, ) = _recurringCollector.getAgreementOfferAt(agreementId, VERSION_CURRENT); + assertEq(currentType, OFFER_TYPE_UPDATE, "current offer type now OFFER_TYPE_UPDATE"); + + (uint8 nextType, bytes memory nextData) = _recurringCollector.getAgreementOfferAt(agreementId, VERSION_NEXT); + assertEq(nextType, OFFER_TYPE_NONE, "no pending offer after update"); + assertEq(nextData.length, 0, "pending data empty after update"); + + // Old RCA hash is no longer referenced; since getAgreementOfferAt only resolves via version + // indices, confirm indirectly that no version maps to rcaHash. + bytes32 currentHash = _recurringCollector.getAgreementDetails(agreementId, VERSION_CURRENT).versionHash; + assertTrue(currentHash != rcaHash, "no version maps to old rcaHash"); + } + + /// @notice Offering a different pending update replaces the prior pending RCAU — the replaced + /// entry is deleted from storage. + function test_OfferUpdate_ReplacesPriorPending_DeletesReplaced() public { + MockAgreementOwner approver = new MockAgreementOwner(); + IRecurringCollector.RecurringCollectionAgreement memory rca = _makeRca(address(approver)); + _setupValidProvision(rca.serviceProvider, rca.dataService); + + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_NEW, abi.encode(rca), 0); + vm.prank(rca.dataService); + bytes16 agreementId = _recurringCollector.accept(rca, ""); + + IRecurringCollector.RecurringCollectionAgreementUpdate memory rcauA = _makeRcau(agreementId, rca, 1); + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_UPDATE, abi.encode(rcauA), 0); + bytes32 rcauAHash = _recurringCollector.hashRCAU(rcauA); + + // Second update with different terms (different maxInitialTokens) replaces the pending RCAU + IRecurringCollector.RecurringCollectionAgreementUpdate memory rcauB = rcauA; + rcauB.maxInitialTokens = rcauA.maxInitialTokens + 1; + bytes32 rcauBHash = _recurringCollector.hashRCAU(rcauB); + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_UPDATE, abi.encode(rcauB), 0); + + IRecurringCollector.AgreementData memory agreement = _recurringCollector.getAgreement(agreementId); + assertEq(agreement.pendingTermsHash, rcauBHash, "pending now points to rcauB"); + + // Replaced rcauA entry no longer referenced by any version — VERSION_NEXT is now rcauB. + bytes32 pendingHash = _recurringCollector.getAgreementDetails(agreementId, VERSION_NEXT).versionHash; + assertEq(pendingHash, rcauBHash, "VERSION_NEXT resolves to rcauB"); + assertTrue(pendingHash != rcauAHash, "old rcauA no longer reachable via version index"); + } + + // ────────────────────────────────────────────────────────────────────── + // Pre-acceptance cancel cascades deletion of any pending RCAU + // ────────────────────────────────────────────────────────────────────── + + /// @notice Pre-acceptance cancel of the RCA under SCOPE_PENDING deletes BOTH the RCA offer + /// and any pending RCAU offer. After cascade, both slots are empty. + function test_CancelPreAcceptanceRca_PreservesPendingRcau() public { + MockAgreementOwner approver = new MockAgreementOwner(); + IRecurringCollector.RecurringCollectionAgreement memory rca = _makeRca(address(approver)); + bytes32 rcaHash = _recurringCollector.hashRCA(rca); + + vm.prank(address(approver)); + IAgreementCollector.AgreementDetails memory rcaDetails = _recurringCollector.offer( + OFFER_TYPE_NEW, + abi.encode(rca), + 0 + ); + bytes16 agreementId = rcaDetails.agreementId; + + IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau = _makeRcau(agreementId, rca, 1); + bytes32 rcauHash = _recurringCollector.hashRCAU(rcau); + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_UPDATE, abi.encode(rcau), 0); + + // Sanity: both slots populated before the cancel + (uint8 preCurrentType, ) = _recurringCollector.getAgreementOfferAt(agreementId, VERSION_CURRENT); + (uint8 preNextType, ) = _recurringCollector.getAgreementOfferAt(agreementId, VERSION_NEXT); + assertEq(preCurrentType, OFFER_TYPE_NEW, "RCA stored before cancel"); + assertEq(preNextType, OFFER_TYPE_UPDATE, "RCAU stored before cancel"); + + // Cancel the pre-acceptance RCA — one OfferCancelled event; pending RCAU survives + vm.expectEmit(address(_recurringCollector)); + emit IRecurringCollector.OfferCancelled(address(approver), agreementId, rcaHash); + vm.prank(address(approver)); + _recurringCollector.cancel(agreementId, rcaHash, SCOPE_PENDING); + + IRecurringCollector.AgreementData memory agreement = _recurringCollector.getAgreement(agreementId); + assertEq(agreement.activeTermsHash, bytes32(0), "activeTermsHash cleared"); + assertEq(agreement.pendingTermsHash, rcauHash, "pendingTermsHash survives RCA cancel"); + + (uint8 currentType, bytes memory currentData) = _recurringCollector.getAgreementOfferAt( + agreementId, + VERSION_CURRENT + ); + assertEq(currentType, OFFER_TYPE_NONE, "RCA offer deleted"); + assertEq(currentData.length, 0, "RCA data empty"); + + (uint8 nextType, bytes memory nextData) = _recurringCollector.getAgreementOfferAt(agreementId, VERSION_NEXT); + assertEq(nextType, OFFER_TYPE_UPDATE, "RCAU offer still retrievable"); + assertEq(keccak256(nextData), keccak256(abi.encode(rcau)), "RCAU data intact"); + } + + /// @notice Pre-acceptance RCA and pending RCAU can be cancelled in either order — + /// agreement.payer is a persistent field, so cancelling one doesn't un-authorize cancelling + /// the other. + function test_CancelPreAcceptance_EitherOrder() public { + MockAgreementOwner approver = new MockAgreementOwner(); + IRecurringCollector.RecurringCollectionAgreement memory rca = _makeRca(address(approver)); + bytes32 rcaHash = _recurringCollector.hashRCA(rca); + + vm.prank(address(approver)); + IAgreementCollector.AgreementDetails memory rcaDetails = _recurringCollector.offer( + OFFER_TYPE_NEW, + abi.encode(rca), + 0 + ); + bytes16 agreementId = rcaDetails.agreementId; + + IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau = _makeRcau(agreementId, rca, 1); + bytes32 rcauHash = _recurringCollector.hashRCAU(rcau); + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_UPDATE, abi.encode(rcau), 0); + + // Cancel the RCA first + vm.prank(address(approver)); + _recurringCollector.cancel(agreementId, rcaHash, SCOPE_PENDING); + + // Then cancel the pending RCAU — must succeed because agreement.payer is persistent + vm.expectEmit(address(_recurringCollector)); + emit IRecurringCollector.OfferCancelled(address(approver), agreementId, rcauHash); + vm.prank(address(approver)); + _recurringCollector.cancel(agreementId, rcauHash, SCOPE_PENDING); + + IRecurringCollector.AgreementData memory agreement = _recurringCollector.getAgreement(agreementId); + assertEq(agreement.activeTermsHash, bytes32(0), "active cleared"); + assertEq(agreement.pendingTermsHash, bytes32(0), "pending cleared"); + } + + /// @notice Pre-acceptance cancel with no pending RCAU still deletes the RCA offer and + /// emits a single OfferCancelled. + function test_CancelPreAcceptanceRca_NoPending_OnlyDeletesRca() public { + MockAgreementOwner approver = new MockAgreementOwner(); + IRecurringCollector.RecurringCollectionAgreement memory rca = _makeRca(address(approver)); + bytes32 rcaHash = _recurringCollector.hashRCA(rca); + + vm.prank(address(approver)); + IAgreementCollector.AgreementDetails memory details = _recurringCollector.offer( + OFFER_TYPE_NEW, + abi.encode(rca), + 0 + ); + bytes16 agreementId = details.agreementId; + + vm.expectEmit(address(_recurringCollector)); + emit IRecurringCollector.OfferCancelled(address(approver), agreementId, rcaHash); + vm.prank(address(approver)); + _recurringCollector.cancel(agreementId, rcaHash, SCOPE_PENDING); + + (uint8 currentType, ) = _recurringCollector.getAgreementOfferAt(agreementId, VERSION_CURRENT); + assertEq(currentType, OFFER_TYPE_NONE, "RCA offer deleted"); + assertEq(_recurringCollector.getAgreement(agreementId).activeTermsHash, bytes32(0), "activeTermsHash cleared"); + } + + // ────────────────────────────────────────────────────────────────────── + // OFFER_TYPE_NONE sentinel + // ────────────────────────────────────────────────────────────────────── + + /// @notice The offer-type sentinel values: OFFER_TYPE_NONE must be 0 so callers can distinguish + /// "no offer stored" (default mapping value) from OFFER_TYPE_NEW / OFFER_TYPE_UPDATE. + function test_OfferTypeConstants_NoneIsZero_OthersNonZero() public pure { + assertEq(OFFER_TYPE_NONE, uint8(0), "OFFER_TYPE_NONE must be 0"); + assertTrue(OFFER_TYPE_NEW != OFFER_TYPE_NONE, "OFFER_TYPE_NEW distinct from NONE"); + assertTrue(OFFER_TYPE_UPDATE != OFFER_TYPE_NONE, "OFFER_TYPE_UPDATE distinct from NONE"); + assertTrue(OFFER_TYPE_NEW != OFFER_TYPE_UPDATE, "NEW and UPDATE distinct"); + } + + /// @notice offer() rejects OFFER_TYPE_NONE as an offer type — the sentinel cannot be used to + /// create a stored offer, so getAgreementOfferAt's OFFER_TYPE_NONE return unambiguously means + /// "no offer stored". + function test_Offer_Revert_WhenOfferTypeIsNone() public { + MockAgreementOwner approver = new MockAgreementOwner(); + IRecurringCollector.RecurringCollectionAgreement memory rca = _makeRca(address(approver)); + bytes memory data = abi.encode(rca); + + vm.expectRevert( + abi.encodeWithSelector(IRecurringCollector.RecurringCollectorInvalidCollectData.selector, data) + ); + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_NONE, data, 0); + } + + /* solhint-enable graph/func-name-mixedcase */ +} diff --git a/packages/horizon/test/unit/payments/recurring-collector/returndataBomb.t.sol b/packages/horizon/test/unit/payments/recurring-collector/returndataBomb.t.sol new file mode 100644 index 000000000..3ef69f430 --- /dev/null +++ b/packages/horizon/test/unit/payments/recurring-collector/returndataBomb.t.sol @@ -0,0 +1,108 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.27; + +import { IRecurringCollector } from "@graphprotocol/interfaces/contracts/horizon/IRecurringCollector.sol"; +import { IGraphPayments } from "@graphprotocol/interfaces/contracts/horizon/IGraphPayments.sol"; +import { IAgreementOwner } from "@graphprotocol/interfaces/contracts/horizon/IAgreementOwner.sol"; +import { IProviderEligibility } from "@graphprotocol/interfaces/contracts/issuance/eligibility/IProviderEligibility.sol"; +import { OFFER_TYPE_NEW } from "@graphprotocol/interfaces/contracts/horizon/IAgreementCollector.sol"; +import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; + +import { RecurringCollectorSharedTest } from "./shared.t.sol"; + +/// @notice Payer that returns a configurable-size buffer from every callback. +/// Used to verify the collector caps returndata copy into its outer frame. +contract HugeReturnPayer is IAgreementOwner, IERC165 { + uint256 public returnBytes = 500_000; + + function setReturnBytes(uint256 size) external { + returnBytes = size; + } + + function supportsInterface(bytes4 interfaceId) external pure override returns (bool) { + return interfaceId == type(IERC165).interfaceId || interfaceId == type(IProviderEligibility).interfaceId; + } + + function beforeCollection(bytes16, uint256) external { + uint256 size = returnBytes; + // solhint-disable-next-line no-inline-assembly + assembly { + return(0, size) + } + } + + function afterCollection(bytes16, uint256) external { + uint256 size = returnBytes; + // solhint-disable-next-line no-inline-assembly + assembly { + return(0, size) + } + } + + /// @notice isEligible — first 32 bytes = 1 (eligible), remainder is memory-expansion padding. + fallback() external { + uint256 size = returnBytes; + // solhint-disable-next-line no-inline-assembly + assembly { + mstore(0, 1) + return(0, size) + } + } +} + +contract RecurringCollectorReturndataBombTest is RecurringCollectorSharedTest { + /* solhint-disable graph/func-name-mixedcase */ + + /// @notice All three payer callbacks return 500KB. With bounded retSize at each call site + /// the outer frame does not copy the returndata, so gas usage stays proportional to the + /// callbacks' own internal work. Without the bound, the outer frame incurs memory expansion + /// + RETURNDATACOPY for each 500KB payload, roughly doubling gas consumption. + function test_Collect_BoundsReturndataCopy_WhenPayerReturnsHuge() public { + HugeReturnPayer attacker = new HugeReturnPayer(); + attacker.setReturnBytes(500_000); + + IRecurringCollector.RecurringCollectionAgreement memory rca = _recurringCollectorHelper.sensibleRCA( + IRecurringCollector.RecurringCollectionAgreement({ + deadline: uint64(block.timestamp + 1 hours), + endsAt: uint64(block.timestamp + 365 days), + payer: address(attacker), + dataService: makeAddr("ds"), + serviceProvider: makeAddr("sp"), + maxInitialTokens: 100 ether, + maxOngoingTokensPerSecond: 1 ether, + minSecondsPerCollection: 600, + maxSecondsPerCollection: 3600, + conditions: 0, + nonce: 1, + metadata: "" + }) + ); + rca.conditions = 1; // CONDITION_ELIGIBILITY_CHECK — exercise the eligibility staticcall path + + vm.prank(address(attacker)); + _recurringCollector.offer(OFFER_TYPE_NEW, abi.encode(rca), 0); + _setupValidProvision(rca.serviceProvider, rca.dataService); + + vm.prank(rca.dataService); + bytes16 agreementId = _recurringCollector.accept(rca, ""); + + skip(rca.minSecondsPerCollection); + uint256 tokens = 1 ether; + bytes memory data = _generateCollectData(_generateCollectParams(rca, agreementId, bytes32("col1"), tokens, 0)); + + uint256 gasBefore = gasleft(); + vm.prank(rca.dataService); + uint256 collected = _recurringCollector.collect(IGraphPayments.PaymentTypes.IndexingFee, data); + uint256 gasUsed = gasBefore - gasleft(); + + assertEq(collected, tokens, "collect should succeed despite huge returndata"); + + // Bounded frame: base collect (~200k) plus three callbacks' internal 500KB expansion + // (~520k each) totals roughly 1.8M. Without the bound each callback additionally causes + // ~520k of outer-frame memory expansion plus the RETURNDATACOPY itself, pushing the + // total above 3.3M. A 2.5M ceiling cleanly separates the two cases. + assertLt(gasUsed, 2_500_000, "outer frame consumed unbounded payer returndata"); + } + + /* solhint-enable graph/func-name-mixedcase */ +} diff --git a/packages/horizon/test/unit/payments/recurring-collector/shared.t.sol b/packages/horizon/test/unit/payments/recurring-collector/shared.t.sol index 3e88525e9..2d90e7142 100644 --- a/packages/horizon/test/unit/payments/recurring-collector/shared.t.sol +++ b/packages/horizon/test/unit/payments/recurring-collector/shared.t.sol @@ -120,7 +120,6 @@ contract RecurringCollectorSharedTest is Test, Bounder { _rca.payer, _rca.serviceProvider, expectedAgreementId, - uint64(block.timestamp), _rca.endsAt, _rca.maxInitialTokens, _rca.maxOngoingTokensPerSecond, @@ -165,7 +164,6 @@ contract RecurringCollectorSharedTest is Test, Bounder { _rca.payer, _rca.serviceProvider, _agreementId, - uint64(block.timestamp), _by ); vm.prank(_rca.dataService); 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 be84dde2f..158157554 100644 --- a/packages/horizon/test/unit/payments/recurring-collector/update.t.sol +++ b/packages/horizon/test/unit/payments/recurring-collector/update.t.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.27; import { IRecurringCollector } from "@graphprotocol/interfaces/contracts/horizon/IRecurringCollector.sol"; +import { OFFER_TYPE_UPDATE } from "@graphprotocol/interfaces/contracts/horizon/IAgreementCollector.sol"; import { RecurringCollectorSharedTest } from "./shared.t.sol"; @@ -143,7 +144,6 @@ contract RecurringCollectorUpdateTest is RecurringCollectorSharedTest { acceptedRca.payer, acceptedRca.serviceProvider, rcau.agreementId, - uint64(block.timestamp), rcau.endsAt, rcau.maxInitialTokens, rcau.maxOngoingTokensPerSecond, @@ -154,11 +154,7 @@ contract RecurringCollectorUpdateTest is RecurringCollectorSharedTest { _recurringCollector.update(rcau, signature); IRecurringCollector.AgreementData memory agreement = _recurringCollector.getAgreement(agreementId); - assertEq(rcau.endsAt, agreement.endsAt); - assertEq(rcau.maxInitialTokens, agreement.maxInitialTokens); - assertEq(rcau.maxOngoingTokensPerSecond, agreement.maxOngoingTokensPerSecond); - assertEq(rcau.minSecondsPerCollection, agreement.minSecondsPerCollection); - assertEq(rcau.maxSecondsPerCollection, agreement.maxSecondsPerCollection); + assertEq(agreement.activeTermsHash, _recurringCollector.hashRCAU(rcau)); assertEq(rcau.nonce, agreement.updateNonce); } @@ -314,5 +310,84 @@ contract RecurringCollectorUpdateTest is RecurringCollectorSharedTest { assertEq(updatedAgreement2.updateNonce, 2); } + function test_Update_Idempotent_WhenAlreadyAtActiveHash(FuzzyTestUpdate calldata fuzzyTestUpdate) public { + ( + IRecurringCollector.RecurringCollectionAgreement memory acceptedRca, + , + uint256 signerKey, + bytes16 agreementId + ) = _sensibleAuthorizeAndAccept(fuzzyTestUpdate.fuzzyTestAccept); + + IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau = _recurringCollectorHelper.sensibleRCAU( + fuzzyTestUpdate.rcau + ); + rcau.agreementId = agreementId; + rcau.nonce = 1; + (, bytes memory signature) = _recurringCollectorHelper.generateSignedRCAU(rcau, signerKey); + + // First update consumes nonce 1 and sets activeTermsHash = hash(rcau). + vm.prank(acceptedRca.dataService); + _recurringCollector.update(rcau, signature); + + IRecurringCollector.AgreementData memory afterFirst = _recurringCollector.getAgreement(agreementId); + assertEq(afterFirst.updateNonce, 1, "nonce advanced to 1 after first update"); + + // Re-submitting the same RCAU is a no-op — nonce does NOT advance, no event, no revert. + vm.recordLogs(); + vm.prank(acceptedRca.dataService); + _recurringCollector.update(rcau, signature); + assertEq(vm.getRecordedLogs().length, 0, "no event emitted on idempotent re-update"); + + IRecurringCollector.AgreementData memory afterSecond = _recurringCollector.getAgreement(agreementId); + assertEq(afterSecond.updateNonce, 1, "nonce unchanged on idempotent re-update"); + assertEq(afterSecond.activeTermsHash, afterFirst.activeTermsHash, "activeTermsHash unchanged"); + } + + /// @notice Direct-apply update (no prior offer(UPDATE) that staged the RCAU as pending) writes + /// new terms via _validateAndStoreTerms, which must emit OfferStored. AgreementUpdated follows. + function test_Update_EmitsOfferStored_WhenDirectApplyFreshTerms(FuzzyTestUpdate calldata fuzzyTestUpdate) public { + ( + IRecurringCollector.RecurringCollectionAgreement memory acceptedRca, + , + uint256 signerKey, + bytes16 agreementId + ) = _sensibleAuthorizeAndAccept(fuzzyTestUpdate.fuzzyTestAccept); + + IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau = _recurringCollectorHelper.sensibleRCAU( + fuzzyTestUpdate.rcau + ); + rcau.agreementId = agreementId; + + ( + IRecurringCollector.RecurringCollectionAgreementUpdate memory signedRcau, + bytes memory signature + ) = _recurringCollectorHelper.generateSignedRCAUForAgreement(agreementId, rcau, signerKey); + bytes32 rcauHash = _recurringCollector.hashRCAU(signedRcau); + + // Pre-condition: no pending offer staged, so update() takes the direct-apply branch. + assertEq( + _recurringCollector.getAgreement(agreementId).pendingTermsHash, + bytes32(0), + "no pending before direct-apply" + ); + + vm.expectEmit(address(_recurringCollector)); + emit IRecurringCollector.OfferStored(agreementId, acceptedRca.payer, OFFER_TYPE_UPDATE, rcauHash); + vm.expectEmit(address(_recurringCollector)); + emit IRecurringCollector.AgreementUpdated( + acceptedRca.dataService, + acceptedRca.payer, + acceptedRca.serviceProvider, + agreementId, + signedRcau.endsAt, + signedRcau.maxInitialTokens, + signedRcau.maxOngoingTokensPerSecond, + signedRcau.minSecondsPerCollection, + signedRcau.maxSecondsPerCollection + ); + vm.prank(acceptedRca.dataService); + _recurringCollector.update(signedRcau, signature); + } + /* solhint-enable graph/func-name-mixedcase */ } diff --git a/packages/horizon/test/unit/payments/recurring-collector/updateUnsigned.t.sol b/packages/horizon/test/unit/payments/recurring-collector/updateUnsigned.t.sol index 45d05c55b..d91bb9a5c 100644 --- a/packages/horizon/test/unit/payments/recurring-collector/updateUnsigned.t.sol +++ b/packages/horizon/test/unit/payments/recurring-collector/updateUnsigned.t.sol @@ -87,7 +87,6 @@ contract RecurringCollectorUpdateUnsignedTest is RecurringCollectorSharedTest { rca.payer, rca.serviceProvider, agreementId, - uint64(block.timestamp), rcau.endsAt, rcau.maxInitialTokens, rcau.maxOngoingTokensPerSecond, @@ -99,11 +98,7 @@ contract RecurringCollectorUpdateUnsignedTest is RecurringCollectorSharedTest { _recurringCollector.update(rcau, ""); IRecurringCollector.AgreementData memory agreement = _recurringCollector.getAgreement(agreementId); - assertEq(rcau.endsAt, agreement.endsAt); - assertEq(rcau.maxInitialTokens, agreement.maxInitialTokens); - assertEq(rcau.maxOngoingTokensPerSecond, agreement.maxOngoingTokensPerSecond); - assertEq(rcau.minSecondsPerCollection, agreement.minSecondsPerCollection); - assertEq(rcau.maxSecondsPerCollection, agreement.maxSecondsPerCollection); + assertEq(agreement.activeTermsHash, _recurringCollector.hashRCAU(rcau)); assertEq(rcau.nonce, agreement.updateNonce); } @@ -218,20 +213,17 @@ contract RecurringCollectorUpdateUnsignedTest is RecurringCollectorSharedTest { IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau = _makeSimpleRCAU(agreementId, 1); - // Set the update deadline in the past + // Set the update deadline in the past — offer() now rejects expired deadlines rcau.deadline = uint64(block.timestamp - 1); - vm.prank(address(approver)); - _recurringCollector.offer(OFFER_TYPE_UPDATE, abi.encode(rcau), 0); - bytes memory expectedErr = abi.encodeWithSelector( IRecurringCollector.RecurringCollectorAgreementDeadlineElapsed.selector, block.timestamp, rcau.deadline ); vm.expectRevert(expectedErr); - vm.prank(rca.dataService); - _recurringCollector.update(rcau, ""); + vm.prank(address(approver)); + _recurringCollector.offer(OFFER_TYPE_UPDATE, abi.encode(rcau), 0); } /* solhint-enable graph/func-name-mixedcase */ diff --git a/packages/horizon/test/unit/payments/recurring-collector/upgradeScenario.t.sol b/packages/horizon/test/unit/payments/recurring-collector/upgradeScenario.t.sol index f65fe9464..82d2a1468 100644 --- a/packages/horizon/test/unit/payments/recurring-collector/upgradeScenario.t.sol +++ b/packages/horizon/test/unit/payments/recurring-collector/upgradeScenario.t.sol @@ -129,7 +129,7 @@ contract RecurringCollectorUpgradeScenarioTest is Test, Bounder { assertEq(v2Agreement.payer, payer, "payer lost"); assertEq(v2Agreement.serviceProvider, rca.serviceProvider, "serviceProvider lost"); assertEq(v2Agreement.dataService, rca.dataService, "dataService lost"); - assertEq(v2Agreement.maxOngoingTokensPerSecond, rca.maxOngoingTokensPerSecond, "terms lost"); + assertEq(v2Agreement.activeTermsHash, _recurringCollector.hashRCA(rca), "terms hash lost"); assertTrue(_recurringCollector.pauseGuardians(makeAddr("guardian")), "pause guardian lost"); } diff --git a/packages/horizon/test/unit/payments/recurring-collector/viewFunctions.t.sol b/packages/horizon/test/unit/payments/recurring-collector/viewFunctions.t.sol index 839cd146e..80445920b 100644 --- a/packages/horizon/test/unit/payments/recurring-collector/viewFunctions.t.sol +++ b/packages/horizon/test/unit/payments/recurring-collector/viewFunctions.t.sol @@ -15,10 +15,8 @@ contract RecurringCollectorViewFunctionsTest is RecurringCollectorSharedTest { function test_GetCollectionInfo_Accepted_AfterTime(FuzzyTestAccept calldata fuzzy) public { (, , , bytes16 agreementId) = _sensibleAuthorizeAndAccept(fuzzy); - IRecurringCollector.AgreementData memory agreement = _recurringCollector.getAgreement(agreementId); - - // Skip some time - skip(agreement.minSecondsPerCollection); + // Skip past the minimum collection window so collection is possible + skip(_recurringCollector.MIN_SECONDS_COLLECTION_WINDOW()); // Re-read agreement (timestamps don't change but view computes based on block.timestamp) (bool isCollectable, uint256 collectionSeconds, ) = _recurringCollector.getCollectionInfo(agreementId); @@ -129,22 +127,13 @@ contract RecurringCollectorViewFunctionsTest is RecurringCollectorSharedTest { assertEq(agreement.payer, rca.payer, "payer should match"); assertEq(agreement.dataService, rca.dataService, "dataService should match"); assertEq(agreement.serviceProvider, rca.serviceProvider, "serviceProvider should match"); - assertEq(agreement.endsAt, rca.endsAt, "endsAt should match"); - assertEq(agreement.minSecondsPerCollection, rca.minSecondsPerCollection, "minSeconds should match"); - assertEq(agreement.maxSecondsPerCollection, rca.maxSecondsPerCollection, "maxSeconds should match"); - assertEq(agreement.maxInitialTokens, rca.maxInitialTokens, "maxInitialTokens should match"); - assertEq( - agreement.maxOngoingTokensPerSecond, - rca.maxOngoingTokensPerSecond, - "maxOngoingTokensPerSecond should match" - ); assertEq( uint8(agreement.state), uint8(IRecurringCollector.AgreementState.Accepted), "state should be Accepted" ); assertTrue(agreement.acceptedAt > 0, "acceptedAt should be set"); - assertTrue(agreement.activeTermsHash != bytes32(0), "activeTermsHash should be set"); + assertEq(agreement.activeTermsHash, _recurringCollector.hashRCA(rca), "activeTermsHash should match RCA hash"); } /* solhint-enable graph/func-name-mixedcase */ diff --git a/packages/horizon/test/unit/utilities/Authorizable.t.sol b/packages/horizon/test/unit/utilities/Authorizable.t.sol index 18ed8df54..c9f47fcba 100644 --- a/packages/horizon/test/unit/utilities/Authorizable.t.sol +++ b/packages/horizon/test/unit/utilities/Authorizable.t.sol @@ -326,7 +326,11 @@ contract AuthorizableTest is Test, Bounder { authorizable.revokeAuthorizedSigner(signer); } - function test_IsAuthorized_Revert_WhenZero(address signer) public view { + function test_IsAuthorized_Revert_WhenZero(address signer) public { + // Subclasses (e.g. RecurringCollector) may treat specific addresses — notably + // the contract itself — as authorized regardless of the authorizer, so rely on + // assumeValidFuzzAddress to exclude those. + assumeValidFuzzAddress(signer); authHelper.assertNotAuthorized(address(0), signer); } } diff --git a/packages/interfaces/contracts/contracts/rewards/IRewardsManager.sol b/packages/interfaces/contracts/contracts/rewards/IRewardsManager.sol index 205bde73c..688c9469d 100644 --- a/packages/interfaces/contracts/contracts/rewards/IRewardsManager.sol +++ b/packages/interfaces/contracts/contracts/rewards/IRewardsManager.sol @@ -2,7 +2,6 @@ pragma solidity ^0.7.6 || ^0.8.0; -import { IIssuanceAllocationDistribution } from "../../issuance/allocate/IIssuanceAllocationDistribution.sol"; import { IRewardsIssuer } from "./IRewardsIssuer.sol"; /** @@ -179,13 +178,6 @@ interface IRewardsManager { */ function subgraphService() external view returns (IRewardsIssuer); - /** - * @notice Get the issuance allocator address - * @dev When set, this allocator controls issuance distribution instead of issuancePerBlock - * @return The issuance allocator contract (zero address if not set) - */ - function getIssuanceAllocator() external view returns (IIssuanceAllocationDistribution); - /** * @notice Get the reclaim address for a specific reason * @param reason The reclaim reason identifier diff --git a/packages/interfaces/contracts/horizon/IAgreementCollector.sol b/packages/interfaces/contracts/horizon/IAgreementCollector.sol index ee8bad086..11a3f9a78 100644 --- a/packages/interfaces/contracts/horizon/IAgreementCollector.sol +++ b/packages/interfaces/contracts/horizon/IAgreementCollector.sol @@ -3,51 +3,42 @@ pragma solidity ^0.8.22; import { IPaymentsCollector } from "./IPaymentsCollector.sol"; -// -- Agreement state flags -- -// REGISTERED, ACCEPTED are monotonic (once set, never cleared). -// All other flags are clearable — cleared when pending terms are accepted. +// -- State flags for AgreementDetails -- +// Describe the queried version in context of its agreement; returned by both +// offer() and getAgreementDetails(). See AgreementDetails.state NatSpec. -/// @dev Offer exists in storage +/// @dev Offer exists in storage. Implied by ACCEPTED. uint16 constant REGISTERED = 1; -/// @dev Provider accepted terms +/// @dev Provider accepted terms. Always returned with REGISTERED set (accepted terms were stored). uint16 constant ACCEPTED = 2; -/// @dev collectableUntil has been reduced, collection capped (clearable) +/// @dev The agreement's collection window has been truncated (e.g. by cancellation). +/// Paired with a BY_* flag identifying the origin. uint16 constant NOTICE_GIVEN = 4; -/// @dev Nothing to collect in current state (clearable — cleared on new terms promotion) +/// @dev Nothing to collect in current state uint16 constant SETTLED = 8; -// -- Who-initiated flags (clearable, meaningful when NOTICE_GIVEN is set) -- +// -- Who-initiated flags (meaningful when NOTICE_GIVEN is set) -- -/// @dev Notice given by payer +/// @dev NOTICE_GIVEN originated from the payer. uint16 constant BY_PAYER = 16; -/// @dev Notice given by provider (forfeit — immediate SETTLED) +/// @dev NOTICE_GIVEN originated from the service provider. uint16 constant BY_PROVIDER = 32; -/// @dev Notice given by data service -uint16 constant BY_DATA_SERVICE = 64; // -- Update-origin flag -- -/// @dev Terms originated from an RCAU (update), not the initial RCA. -/// Set on agreement state when active terms come from an accepted or pre-acceptance update. -/// ORed into returned state by getAgreementDetails for pending versions (index 1). +/// @dev This version's terms originated from an update, not the initial agreement offer. +/// Describes the version's provenance; set wherever the update-derived version is returned. uint16 constant UPDATE = 128; -// -- Togglable option flags (set via accept options parameter) -- - -/// @dev Provider opts in to automatic update on final collect -uint16 constant AUTO_UPDATE = 256; - -// -- Lifecycle flags (set by the collector during auto-update, clearable) -- - -/// @dev Active terms were promoted via auto-update (not explicit provider accept) -uint16 constant AUTO_UPDATED = 512; - // -- Offer type constants -- +/// @dev No stored offer — sentinel returned by {IAgreementCollector.getAgreementOfferAt} +/// when the requested version has no offer data. +uint8 constant OFFER_TYPE_NONE = 0; /// @dev Create a new agreement -uint8 constant OFFER_TYPE_NEW = 0; +uint8 constant OFFER_TYPE_NEW = 1; /// @dev Update an existing agreement -uint8 constant OFFER_TYPE_UPDATE = 1; +uint8 constant OFFER_TYPE_UPDATE = 2; // -- Cancel scope constants -- @@ -55,13 +46,21 @@ uint8 constant OFFER_TYPE_UPDATE = 1; uint8 constant SCOPE_ACTIVE = 1; /// @dev Cancel targets pending offers uint8 constant SCOPE_PENDING = 2; - -// -- Offer option constants (for unsigned offer path) -- - -/// @dev Reduce collectableUntil and set NOTICE_GIVEN | BY_PAYER on the agreement -uint16 constant WITH_NOTICE = 1; -/// @dev Revert if the targeted version has already been accepted -uint16 constant IF_NOT_ACCEPTED = 2; +/// @dev Cancel targets signed offers +uint8 constant SCOPE_SIGNED = 4; + +// -- Version indices (shared by getAgreementDetails and getAgreementOfferAt) -- +// +// Versions are enumerated starting at 0. Implementations may expose any number of versions; +// callers iterate until an empty result signals no further versions. These named aliases +// cover the two versions every collector is expected to expose. + +/// @dev The currently-active version: the accepted terms if the agreement is accepted, +/// otherwise the pre-acceptance offer (if any). Empty when no agreement or offer exists. +uint256 constant VERSION_CURRENT = 0; +/// @dev The next queued version: a pending update offer waiting to be accepted. +/// Empty when no queued update exists. +uint256 constant VERSION_NEXT = 1; /** * @title Base interface for agreement-based payment collectors @@ -80,12 +79,22 @@ interface IAgreementCollector is IPaymentsCollector { /** * @notice Agreement details: participants, version hash, and state flags. * Returned by {offer} and {getAgreementDetails}. + * + * The `state` field describes the version identified by `versionHash` in the + * context of its agreement. Flags that depend on the version (REGISTERED, + * ACCEPTED, UPDATE) are set only when they apply to that specific version; + * flags that describe the agreement-wide collectability of that version + * (NOTICE_GIVEN, BY_PAYER, BY_PROVIDER, SETTLED) reflect the current + * agreement state. Identical semantics whether returned by {offer} or + * {getAgreementDetails} — the returned flags always describe the queried + * version. + * * @param agreementId The 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 versionHash The EIP-712 hash of the terms at the requested version - * @param state Agreement state flags, with UPDATE set when applicable + * @param state State flags describing the queried version in context of its agreement */ // solhint-disable-next-line gas-struct-packing struct AgreementDetails { @@ -110,25 +119,39 @@ interface IAgreementCollector is IPaymentsCollector { /** * @notice Offer a new agreement or update an existing one. + * @dev Returns {AgreementDetails} for the just-stored offer. The `state` field + * describes that version in context of its agreement (see {AgreementDetails}): + * version-specific flags (REGISTERED, ACCEPTED, UPDATE) are set when they + * apply to the offered version; agreement-wide flags (NOTICE_GIVEN, BY_*, + * SETTLED) reflect current agreement state. * @param offerType The type of offer (OFFER_TYPE_NEW or OFFER_TYPE_UPDATE) * @param data ABI-encoded offer data - * @param options Bitmask of offer options + * @param options Bitmask reserved for implementation-specific options; pass 0 when none apply. + * No flags are defined at the interface level. * @return Agreement details including participants and version hash */ function offer(uint8 offerType, bytes calldata data, uint16 options) external returns (AgreementDetails memory); /** - * @notice Cancel an agreement or revoke a pending update, determined by termsHash. - * @param agreementId The agreement's ID. - * @param termsHash EIP-712 hash identifying which terms to cancel (active or pending). - * @param options Bitmask — SCOPE_ACTIVE (1) targets active terms, SCOPE_PENDING (2) targets pending offers. + * @notice Cancel an agreement, revoke a pending offer, or invalidate a signed offer. + * @dev Scopes can be combined. SCOPE_SIGNED is self-authenticating (keyed by msg.sender); + * SCOPE_PENDING and SCOPE_ACTIVE require payer authorization and no-op if nothing exists on-chain. + * @param agreementId The agreement's ID. For SCOPE_SIGNED, only blocks accept/update when + * the agreementId matches; passing bytes16(0) undoes a previous cancellation. + * @param termsHash EIP-712 hash identifying which terms to cancel. + * @param options Bitmask — SCOPE_ACTIVE (1) active terms, SCOPE_PENDING (2) pending offers, + * SCOPE_SIGNED (4) signed offers. */ function cancel(bytes16 agreementId, bytes32 termsHash, uint16 options) external; /** * @notice Get agreement details at a given version index. + * @dev Versions are enumerated from 0. VERSION_CURRENT is the active version (or + * pre-acceptance offer); VERSION_NEXT is the queued pending update, if any. Empty + * details are returned when no version exists at the requested index — callers can + * iterate versions until reaching an empty result. * @param agreementId The ID of the agreement - * @param index The zero-based version index + * @param index Version index (VERSION_CURRENT, VERSION_NEXT, or higher if the implementation supports more) * @return Agreement details including participants, version hash, and state flags */ function getAgreementDetails(bytes16 agreementId, uint256 index) external view returns (AgreementDetails memory); @@ -149,13 +172,15 @@ interface IAgreementCollector is IPaymentsCollector { function getMaxNextClaim(bytes16 agreementId) external view returns (uint256); /** - * @notice Original offer for a given version, enabling independent access and hash verification. - * @dev Returns the offer type (OFFER_TYPE_NEW or OFFER_TYPE_UPDATE) and the ABI-encoded - * original struct. Callers can decode and hash to verify the stored version hash. + * @notice Original offer data for a given version index, enabling independent access and hash verification. + * @dev Returns the offer type and the ABI-encoded original struct so callers can decode + * and rehash to verify the version hash returned by getAgreementDetails. Version semantics + * mirror getAgreementDetails, but empty data is returned when the version's offer was not + * stored (e.g. signed acceptance without a prior offer(), or overwritten by a later update). * @param agreementId The ID of the agreement - * @param index The zero-based version index - * @return offerType OFFER_TYPE_NEW (0) or OFFER_TYPE_UPDATE (1) - * @return offerData ABI-encoded original offer struct + * @param index Version index (VERSION_CURRENT, VERSION_NEXT, or higher if supported) + * @return offerType OFFER_TYPE_NEW, OFFER_TYPE_UPDATE, or OFFER_TYPE_NONE when no offer is stored + * @return offerData ABI-encoded original offer struct, or empty when offerType is OFFER_TYPE_NONE */ function getAgreementOfferAt( bytes16 agreementId, diff --git a/packages/interfaces/contracts/horizon/IRecurringCollector.sol b/packages/interfaces/contracts/horizon/IRecurringCollector.sol index 33501f940..1937d4a95 100644 --- a/packages/interfaces/contracts/horizon/IRecurringCollector.sol +++ b/packages/interfaces/contracts/horizon/IRecurringCollector.sol @@ -103,41 +103,31 @@ interface IRecurringCollector is IAuthorizable, IAgreementCollector { /** * @notice The data for an agreement * @dev This struct is used to store the data of an agreement in the contract. - * Fields are ordered for optimal storage packing (7 slots). + * Fields are ordered for optimal storage packing (5 slots). * @param dataService The address of the data service * @param acceptedAt The timestamp when the agreement was accepted - * @param minSecondsPerCollection The minimum amount of seconds that must pass between collections + * @param updateNonce The current nonce for updates (prevents replay attacks) * @param payer The address of the payer * @param lastCollectionAt The timestamp when the agreement was last collected at - * @param maxSecondsPerCollection The maximum seconds of service that can be collected in a single collection * @param serviceProvider The address of the service provider - * @param endsAt The timestamp when the agreement ends - * @param updateNonce The current nonce for updates (prevents replay attacks) - * @param maxInitialTokens The maximum amount of tokens that can be collected in the first collection - * on top of the amount allowed for subsequent collections - * @param maxOngoingTokensPerSecond The maximum amount of tokens that can be collected per second - * except for the first collection - * @param activeTermsHash EIP-712 hash of the currently active terms (RCA or RCAU) * @param canceledAt The timestamp when the agreement was canceled - * @param conditions Bitmask of payer-declared conditions * @param state The state of the agreement + * @param activeTermsHash EIP-712 hash of the current version. For accepted agreements this + * is the active terms (RCA or RCAU) hash. For pre-acceptance agreements this is the stored + * RCA offer hash. Use `state` to distinguish accepted from pre-acceptance. + * @param pendingTermsHash EIP-712 hash of the queued pending update (RCAU), or 0 if none. */ struct AgreementData { - address dataService; // 20 bytes ─┐ slot 0 (32/32) - uint64 acceptedAt; // 8 bytes ─┤ - uint32 minSecondsPerCollection; // 4 bytes ─┘ - address payer; // 20 bytes ─┐ slot 1 (32/32) - uint64 lastCollectionAt; // 8 bytes ─┤ - uint32 maxSecondsPerCollection; // 4 bytes ─┘ - address serviceProvider; // 20 bytes ─┐ slot 2 (32/32) - uint64 endsAt; // 8 bytes ─┤ - uint32 updateNonce; // 4 bytes ─┘ - uint256 maxInitialTokens; // 32 bytes ─── slot 3 - uint256 maxOngoingTokensPerSecond; // 32 bytes ─── slot 4 - bytes32 activeTermsHash; // 32 bytes ─── slot 5 - uint64 canceledAt; // 8 bytes ─┐ slot 6 (11/32) - uint16 conditions; // 2 bytes ─┤ - AgreementState state; // 1 byte ─┘ + address dataService; // 20 bytes ─┐ slot 0 (32/32) + uint64 acceptedAt; // 8 bytes ─┤ + uint32 updateNonce; // 4 bytes ─┘ + address payer; // 20 bytes ─┐ slot 1 (28/32) + uint64 lastCollectionAt; // 8 bytes ─┘ + address serviceProvider; // 20 bytes ─┐ slot 2 (29/32) + uint64 canceledAt; // 8 bytes ─┤ + AgreementState state; // 1 byte ─┘ + bytes32 activeTermsHash; // 32 bytes ── slot 3 + bytes32 pendingTermsHash; // 32 bytes ── slot 4 } /** @@ -164,7 +154,6 @@ interface IRecurringCollector is IAuthorizable, IAgreementCollector { * @param payer The address of the payer * @param serviceProvider The address of the service provider * @param agreementId The agreement ID - * @param acceptedAt The timestamp when the agreement was accepted * @param endsAt The timestamp when the agreement ends * @param maxInitialTokens The maximum amount of tokens that can be collected in the first collection * @param maxOngoingTokensPerSecond The maximum amount of tokens that can be collected per second @@ -176,7 +165,6 @@ interface IRecurringCollector is IAuthorizable, IAgreementCollector { address indexed payer, address indexed serviceProvider, bytes16 agreementId, - uint64 acceptedAt, uint64 endsAt, uint256 maxInitialTokens, uint256 maxOngoingTokensPerSecond, @@ -190,7 +178,6 @@ interface IRecurringCollector is IAuthorizable, IAgreementCollector { * @param payer The address of the payer * @param serviceProvider The address of the service provider * @param agreementId The agreement ID - * @param canceledAt The timestamp when the agreement was canceled * @param canceledBy The party that canceled the agreement */ event AgreementCanceled( @@ -198,7 +185,6 @@ interface IRecurringCollector is IAuthorizable, IAgreementCollector { address indexed payer, address indexed serviceProvider, bytes16 agreementId, - uint64 canceledAt, CancelAgreementBy canceledBy ); @@ -208,7 +194,6 @@ interface IRecurringCollector is IAuthorizable, IAgreementCollector { * @param payer The address of the payer * @param serviceProvider The address of the service provider * @param agreementId The agreement ID - * @param updatedAt The timestamp when the agreement was updated * @param endsAt The timestamp when the agreement ends * @param maxInitialTokens The maximum amount of tokens that can be collected in the first collection * @param maxOngoingTokensPerSecond The maximum amount of tokens that can be collected per second @@ -220,7 +205,6 @@ interface IRecurringCollector is IAuthorizable, IAgreementCollector { address indexed payer, address indexed serviceProvider, bytes16 agreementId, - uint64 updatedAt, uint64 endsAt, uint256 maxInitialTokens, uint256 maxOngoingTokensPerSecond, @@ -254,11 +238,6 @@ interface IRecurringCollector is IAuthorizable, IAgreementCollector { */ error RecurringCollectorAgreementNotFound(bytes16 agreementId); - /** - * @notice Thrown when accepting an agreement with a zero ID - */ - error RecurringCollectorAgreementIdZero(); - /** * @notice Thrown when interacting with an agreement not owned by the message sender * @param agreementId The agreement ID @@ -322,14 +301,14 @@ interface IRecurringCollector is IAuthorizable, IAgreementCollector { error RecurringCollectorAgreementAddressNotSet(); /** - * @notice Thrown when accepting or upgrading an agreement with an elapsed endsAt - * @param currentTimestamp The current timestamp + * @notice Thrown when an agreement's endsAt is not strictly after its deadline + * @param deadline The offer's acceptance deadline * @param endsAt The agreement end timestamp */ - error RecurringCollectorAgreementElapsedEndsAt(uint256 currentTimestamp, uint64 endsAt); + error RecurringCollectorAgreementEndsBeforeDeadline(uint64 deadline, uint64 endsAt); /** - * @notice Thrown when accepting or upgrading an agreement with an elapsed endsAt + * @notice Thrown when an agreement's collection window is below the minimum * @param allowedMinCollectionWindow The allowed minimum collection window * @param minSecondsPerCollection The minimum seconds per collection * @param maxSecondsPerCollection The maximum seconds per collection @@ -423,6 +402,13 @@ interface IRecurringCollector is IAuthorizable, IAgreementCollector { */ error RecurringCollectorPauseGuardianNoChange(address account, bool allowed); + /** + * @notice Thrown when accepting or updating with a hash that the signer cancelled via SCOPE_SIGNED + * @param signer The signer who cancelled the offer + * @param hash The cancelled EIP-712 hash + */ + error RecurringCollectorOfferCancelled(address signer, bytes32 hash); + /** * @notice Emitted when a pause guardian is set * @param account The address of the pause guardian @@ -450,6 +436,14 @@ interface IRecurringCollector is IAuthorizable, IAgreementCollector { */ event OfferStored(bytes16 indexed agreementId, address indexed payer, uint8 indexed offerType, bytes32 offerHash); + /** + * @notice Emitted when an offer is cancelled via SCOPE_SIGNED or SCOPE_PENDING + * @param caller The address that cancelled the offer (msg.sender) + * @param agreementId The agreement ID + * @param hash The EIP-712 hash that was cancelled + */ + event OfferCancelled(address indexed caller, bytes16 indexed agreementId, bytes32 hash); + /** * @notice Pauses the collector, blocking accept, update, collect, and cancel. * @dev Only callable by a pause guardian. Uses OpenZeppelin Pausable. diff --git a/packages/interfaces/contracts/issuance/agreement/IRecurringAgreements.sol b/packages/interfaces/contracts/issuance/agreement/IRecurringAgreements.sol index debbff6c0..1a19f3ef8 100644 --- a/packages/interfaces/contracts/issuance/agreement/IRecurringAgreements.sol +++ b/packages/interfaces/contracts/issuance/agreement/IRecurringAgreements.sol @@ -68,6 +68,13 @@ interface IRecurringAgreements { */ function getMinThawFraction() external view returns (uint8 fraction); + /** + * @notice Minimum residual escrow factor for cleanup. + * @dev Pairs with no agreements and escrow below 2^value are dropped from tracking. + * @return value The residual escrow decimals + */ + function getMinResidualEscrowFactor() external view returns (uint8 value); + /** * @notice Get the sum of maxNextClaim across all (collector, provider) pairs * @dev Populated lazily through normal operations. diff --git a/packages/interfaces/contracts/issuance/agreement/IRecurringEscrowManagement.sol b/packages/interfaces/contracts/issuance/agreement/IRecurringEscrowManagement.sol index f19bc108b..76bca5f62 100644 --- a/packages/interfaces/contracts/issuance/agreement/IRecurringEscrowManagement.sol +++ b/packages/interfaces/contracts/issuance/agreement/IRecurringEscrowManagement.sol @@ -76,6 +76,13 @@ interface IRecurringEscrowManagement { */ event MinThawFractionSet(uint8 oldFraction, uint8 newFraction); + /** + * @notice Emitted when the minimum residual escrow is changed + * @param oldValue The previous value + * @param newValue The new value + */ + event MinResidualEscrowFactorSet(uint8 oldValue, uint8 newValue); + // solhint-enable gas-indexed-events // -- Functions -- @@ -124,4 +131,20 @@ interface IRecurringEscrowManagement { * @param fraction The numerator over 256 for the dust threshold */ function setMinThawFraction(uint8 fraction) external; + + /** + * @notice Set the minimum residual escrow factor for pair tracking cleanup. + * @dev Requires OPERATOR_ROLE. When a (collector, provider) pair has no remaining agreements + * and the escrow balance is below 2^value, tracking is dropped because the residual is not worth + * the gas cost of further thaw/withdraw cycles. Funds remain in PaymentsEscrow but are no + * longer actively managed by RAM. Higher values drop tracking more aggressively. + * + * - 0: 2^0 = 1 wei (drop only at zero balance — effectively never drop) + * - 50: 2^50 ≈ 10^15 (0.001 GRT, default) + * - 60: 2^60 ≈ 10^18 (1 GRT) + * - 255: 2^255 (always drop when no agreements remain — effectively disables residual tracking) + * + * @param value The exponent (threshold = 2^value) + */ + function setMinResidualEscrowFactor(uint8 value) external; } diff --git a/packages/interfaces/contracts/issuance/allocate/IIssuanceTarget.sol b/packages/interfaces/contracts/issuance/allocate/IIssuanceTarget.sol index 90a311556..ed9f60b8f 100644 --- a/packages/interfaces/contracts/issuance/allocate/IIssuanceTarget.sol +++ b/packages/interfaces/contracts/issuance/allocate/IIssuanceTarget.sol @@ -2,6 +2,8 @@ pragma solidity ^0.7.6 || ^0.8.0; +import { IIssuanceAllocationDistribution } from "./IIssuanceAllocationDistribution.sol"; + /** * @title IIssuanceTarget * @author Edge & Node @@ -13,7 +15,10 @@ interface IIssuanceTarget { * @param oldIssuanceAllocator Old issuance allocator address * @param newIssuanceAllocator New issuance allocator address */ - event IssuanceAllocatorSet(address indexed oldIssuanceAllocator, address indexed newIssuanceAllocator); + event IssuanceAllocatorSet( + IIssuanceAllocationDistribution indexed oldIssuanceAllocator, + IIssuanceAllocationDistribution indexed newIssuanceAllocator + ); /// @notice Emitted before the issuance allocation changes event BeforeIssuanceAllocationChange(); @@ -27,11 +32,17 @@ interface IIssuanceTarget { */ function beforeIssuanceAllocationChange() external; + /** + * @notice Returns the current issuance allocator + * @return The issuance allocator contract (zero address if not set) + */ + function getIssuanceAllocator() external view returns (IIssuanceAllocationDistribution); + /** * @notice Sets the issuance allocator for this target * @dev This function facilitates upgrades by providing a standard way for targets * to change their allocator. Implementations can define their own access control. * @param newIssuanceAllocator Address of the issuance allocator */ - function setIssuanceAllocator(address newIssuanceAllocator) external; + function setIssuanceAllocator(IIssuanceAllocationDistribution newIssuanceAllocator) external; } diff --git a/packages/issuance/audits/PR1301/Graph_PR1301_v02.pdf b/packages/issuance/audits/PR1301/Graph_PR1301_v02.pdf new file mode 100644 index 000000000..e9512ec7e Binary files /dev/null and b/packages/issuance/audits/PR1301/Graph_PR1301_v02.pdf differ diff --git a/packages/issuance/audits/PR1301/README.md b/packages/issuance/audits/PR1301/README.md index 46695b14a..c8c0000c1 100644 --- a/packages/issuance/audits/PR1301/README.md +++ b/packages/issuance/audits/PR1301/README.md @@ -1,35 +1,53 @@ -# Trust Security Audit - PR #1301 +# Trust Security Audit - PR #1301 / #1312 **Auditor:** Trust Security **Period:** 2026-03-03 to 2026-03-19 **Commit:** 7405c9d5f73bce04734efb3f609b76d95ffb520e -**Report:** [Graph_PR1301_v01.pdf](Graph_PR1301_v01.pdf) +**Fix review commit:** 0bbb476f37f85d042927e84d8764fa58eb020ccf +**Report:** [Graph_PR1301_v02.pdf](Graph_PR1301_v02.pdf) ## Findings Summary -| ID | Title | Severity | -| ----------------------- | -------------------------------------------------------- | -------- | -| [TRST-H-1](TRST-H-1.md) | Malicious payer gas siphoning via 63/64 rule | High | -| [TRST-H-2](TRST-H-2.md) | Invalid supportsInterface() returndata escapes try/catch | High | -| [TRST-H-3](TRST-H-3.md) | Stale escrow snapshot causes perpetual revert loop | High | -| [TRST-H-4](TRST-H-4.md) | EOA payer can block collection via EIP-7702 | High | -| [TRST-M-1](TRST-M-1.md) | Micro-thaw griefing via permissionless depositTo() | Medium | -| [TRST-M-2](TRST-M-2.md) | tempJit fallback in beforeCollection() unreachable | Medium | -| [TRST-M-3](TRST-M-3.md) | Instant escrow mode degradation via agreement offer | Medium | -| [TRST-L-1](TRST-L-1.md) | Insufficient gas for afterCollection callback | Low | -| [TRST-L-2](TRST-L-2.md) | Pending update over-reserves escrow | Low | -| [TRST-L-3](TRST-L-3.md) | Unsafe approveAgreement behavior during pause | Low | -| [TRST-L-4](TRST-L-4.md) | Pair tracking removal blocked by 1 wei donation | Low | -| [TRST-L-5](TRST-L-5.md) | \_computeMaxFirstClaim overestimates near deadline | Low | +| ID | Title | Severity | Status | +| ------------------------- | -------------------------------------------------------- | -------- | ------------ | +| [TRST-H-1](TRST-H-1.md) | Malicious payer gas siphoning via 63/64 rule | High | Fixed | +| [TRST-H-2](TRST-H-2.md) | Invalid supportsInterface() returndata escapes try/catch | High | Fixed | +| [TRST-H-3](TRST-H-3.md) | Stale escrow snapshot causes perpetual revert loop | High | Fixed | +| [TRST-H-4](TRST-H-4.md) | EOA payer can block collection via EIP-7702 | High | Fixed | +| [TRST-M-1](TRST-M-1.md) | Micro-thaw griefing via permissionless depositTo() | Medium | Open | +| [TRST-M-2](TRST-M-2.md) | tempJit fallback in beforeCollection() unreachable | Medium | Fixed | +| [TRST-M-3](TRST-M-3.md) | Instant escrow mode degradation via agreement offer | Medium | Acknowledged | +| [TRST-M-4](TRST-M-4.md) | Returndata bombing via payer callbacks | Medium | Open | +| [TRST-M-5](TRST-M-5.md) | Perpetual thaw griefing via micro deposits | Medium | Open | +| [TRST-L-1](TRST-L-1.md) | Insufficient gas for afterCollection callback | Low | Fixed | +| [TRST-L-2](TRST-L-2.md) | Pending update over-reserves escrow | Low | Fixed | +| [TRST-L-3](TRST-L-3.md) | Unsafe approveAgreement behavior during pause | Low | Fixed | +| [TRST-L-4](TRST-L-4.md) | Pair tracking removal blocked by 1 wei donation | Low | Acknowledged | +| [TRST-L-5](TRST-L-5.md) | \_computeMaxFirstClaim overestimates near deadline | Low | Fixed | +| [TRST-L-6](TRST-L-6.md) | Update offer cleanup bypassed via planted offer | Low | Open | +| [TRST-L-7](TRST-L-7.md) | cancel() order sensitivity leaves RCAU offer unreachable | Low | Open | +| [TRST-L-8](TRST-L-8.md) | EOA payer signatures cannot be revoked before deadline | Low | Open | +| [TRST-L-9](TRST-L-9.md) | Callback gas precheck does not account for overhead | Low | Open | +| [TRST-L-10](TRST-L-10.md) | EIP-7702 payer code change enables callback gas griefing | Low | Open | +| [TRST-L-11](TRST-L-11.md) | Inaccurate state flags in getAgreementDetails() | Low | Open | ## Recommendations -| ID | Title | -| ----------------------- | ---------------------------------------------- | -| [TRST-R-1](TRST-R-1.md) | Avoid redeployment of RewardsEligibilityOracle | -| [TRST-R-2](TRST-R-2.md) | Improve stale documentation | -| [TRST-R-3](TRST-R-3.md) | Incorporate defensive coding best practices | -| [TRST-R-4](TRST-R-4.md) | Document critical assumptions in the RAM | +| ID | Title | +| ------------------------- | --------------------------------------------------------------- | +| [TRST-R-1](TRST-R-1.md) | Avoid redeployment of RewardsEligibilityOracle | +| [TRST-R-2](TRST-R-2.md) | Improve stale documentation | +| [TRST-R-3](TRST-R-3.md) | Incorporate defensive coding best practices | +| [TRST-R-4](TRST-R-4.md) | Document critical assumptions in the RAM | +| [TRST-R-5](TRST-R-5.md) | Ambiguous return value in getAgreementOfferAt() | +| [TRST-R-6](TRST-R-6.md) | Dead code guard in \_validateAndStoreUpdate() | +| [TRST-R-7](TRST-R-7.md) | Remove consumed offers in accept() and update() | +| [TRST-R-8](TRST-R-8.md) | Align pause documentation with callback behavior in the RAM | +| [TRST-R-9](TRST-R-9.md) | \_isAuthorized() override trusts itself for any authorizer | +| [TRST-R-10](TRST-R-10.md) | Document role-change semantics for existing agreements | +| [TRST-R-11](TRST-R-11.md) | Remove or implement unused state flags in IAgreementCollector | +| [TRST-R-12](TRST-R-12.md) | Document ACCEPTED state returned for cancelled agreements | +| [TRST-R-13](TRST-R-13.md) | Document reclaim reason change for stale allocation force-close | ## Centralization Risks diff --git a/packages/issuance/audits/PR1301/TRST-H-1.md b/packages/issuance/audits/PR1301/TRST-H-1.md index f250ee55c..7c15cd250 100644 --- a/packages/issuance/audits/PR1301/TRST-H-1.md +++ b/packages/issuance/audits/PR1301/TRST-H-1.md @@ -3,7 +3,7 @@ - **Severity:** High - **Category:** Gas-related issues - **Source:** RecurringCollector.sol -- **Status:** Open +- **Status:** Fixed ## Description @@ -19,7 +19,11 @@ Enforce a minimum gas reservation before each callback. Before calling `beforeCo ## Team Response -TBD +Fixed. + +## Mitigation Review + +Issue has been fixed as suggested. --- diff --git a/packages/issuance/audits/PR1301/TRST-H-2.md b/packages/issuance/audits/PR1301/TRST-H-2.md index 0f2acbffa..3f8eea841 100644 --- a/packages/issuance/audits/PR1301/TRST-H-2.md +++ b/packages/issuance/audits/PR1301/TRST-H-2.md @@ -3,7 +3,7 @@ - **Severity:** High - **Category:** Logical flaws - **Source:** RecurringCollector.sol -- **Status:** Open +- **Status:** Fixed ## Description @@ -19,7 +19,11 @@ Avoid receiving and decoding values from untrusted contract calls. This can be d ## Team Response -TBD +Fixed. + +## Mitigation Review + +Fixed. The affected code has been refactored, addressing the issue. --- diff --git a/packages/issuance/audits/PR1301/TRST-H-3.md b/packages/issuance/audits/PR1301/TRST-H-3.md index 5fac18493..66bddea4d 100644 --- a/packages/issuance/audits/PR1301/TRST-H-3.md +++ b/packages/issuance/audits/PR1301/TRST-H-3.md @@ -3,7 +3,7 @@ - **Severity:** High - **Category:** Logical flaws - **Source:** RecurringAgreementManager.sol -- **Status:** Open +- **Status:** Fixed ## Description @@ -21,7 +21,11 @@ Read the fresh escrow balance inside `_escrowMinMax()` when computing the defici ## Team Response -TBD +Fixed. + +## Mitigation Review + +The new code has a `_setEscrowSnap()` call before `_escrowMinMax()`, ensuring the snapshot is updated and fixing the root cause. --- diff --git a/packages/issuance/audits/PR1301/TRST-H-4.md b/packages/issuance/audits/PR1301/TRST-H-4.md index 80b4c4195..d9fa550bc 100644 --- a/packages/issuance/audits/PR1301/TRST-H-4.md +++ b/packages/issuance/audits/PR1301/TRST-H-4.md @@ -3,7 +3,7 @@ - **Severity:** High - **Category:** Type confusion - **Source:** RecurringCollector.sol -- **Status:** Open +- **Status:** Fixed ## Description @@ -21,8 +21,12 @@ Record whether the payer had code at agreement acceptance time by adding a bool ## Team Response -TBD +Fixed. + +## Mitigation Review + +Fixed under the assumption that a provider setting `CONDITION_ELIGIBILITY_CHECK` to true must trust the payer contract. The statement in the fix comment that "An EOA cannot pass this check, so an EOA cannot create an agreement with eligibility gating enabled" is inaccurate, because an EOA can always change its code back and forth via EIP-7702 to pass interface checks. The correct security boundary is that the provider trusts the payer contract when opting into eligibility, not that the payer cannot be an EOA. --- -Eligibility checks are now opt-in via the `CONDITION_ELIGIBILITY_CHECK` flag, set explicitly in the agreement terms. Providers agree to eligibility gating by accepting an agreement that includes this condition. When the flag is set, the payer must pass an ERC-165 `supportsInterface` check for `IProviderEligibility` at offer time. An EOA cannot pass this check, so an EOA cannot create an agreement with eligibility gating enabled. +Agreed; the security boundary is that a provider opts into `CONDITION_ELIGIBILITY_CHECK` to trust the payer contract. diff --git a/packages/issuance/audits/PR1301/TRST-L-1.md b/packages/issuance/audits/PR1301/TRST-L-1.md index 512e00e98..ed4cd9f11 100644 --- a/packages/issuance/audits/PR1301/TRST-L-1.md +++ b/packages/issuance/audits/PR1301/TRST-L-1.md @@ -3,7 +3,7 @@ - **Severity:** Low - **Category:** Time sensitivity flaw - **Source:** RecurringCollector.sol -- **Status:** Open +- **Status:** Fixed ## Description @@ -19,7 +19,11 @@ Enforce a minimum gas forwarding requirement for the `afterCollection()` callbac ## Team Response -TBD +Fixed. + +## Mitigation Review + +Fixed as suggested. --- diff --git a/packages/issuance/audits/PR1301/TRST-L-10.md b/packages/issuance/audits/PR1301/TRST-L-10.md new file mode 100644 index 000000000..919cda91d --- /dev/null +++ b/packages/issuance/audits/PR1301/TRST-L-10.md @@ -0,0 +1,26 @@ +# TRST-L-10: EIP-7702 payer code change enables callback gas griefing after acceptance + +- **Severity:** Low +- **Category:** Type confusion +- **Source:** RecurringCollector.sol +- **Status:** Open + +## Description + +Under EIP-7702, which is live on Ethereum mainnet and Arbitrum, an EOA can install arbitrary code via a delegation transaction. `_preCollectCallbacks()` and `_postCollectCallback()` dispatch the `beforeCollection()` and `afterCollection()` callbacks only when `payer.code.length != 0`. A payer who accepted an agreement as an EOA can later acquire code, and have the callbacks dispatched against delegated code that the service provider never considered at acceptance time. + +The callbacks are low level calls with a `MAX_PAYER_CALLBACK_GAS` budget, and they are vulnerable to the returndata bombing vector described in TRST-M-4, on top of the baseline call costs. Service providers estimate gas for `collect()` under the assumption that the payer is an EOA with no callbacks. If the payer is a contract at collection time, the provider's gas estimate may be insufficient and the transaction will revert with griefed gas. This is a distinct attack surface from TRST-H-4, which targeted the eligibility gate rather than the callback path. + +## Recommended Mitigation + +Use the introduced `CONDITION_ELIGIBILITY_CHECK` flag in place of the live `code.length` check in `_preCollectCallbacks()` and `_postCollectCallback()`. This freezes the contract-versus-EOA determination to the state the service provider observed at acceptance. + +## Team Response + +TBD + +--- + +Using `CONDITION_ELIGIBILITY_CHECK` for callback dispatch does not seem appropriate. The eligibility check is an agreement term, not a proxy for payer type and contract payers can legitimately offer agreements without this condition. The provider agreeing to the check requires greater trust in the payer. Gating callbacks on this flag would deny `beforeCollection`/`afterCollection` to contract payers for agreements without eligibility gating. + +With the returndata bombing fix (TRST-M-4), the gas impact of an EIP-7702 EOA gaining callbacks is bounded and predictable. We do not believe this as a significant attack vector. The `beforeCollection`/`afterCollection` callbacks are non-reverting and non-blocking. A payer adding code via EIP-7702 to better handle escrow reconciliation could be a valid use case and in the best interests of all parties. diff --git a/packages/issuance/audits/PR1301/TRST-L-11.md b/packages/issuance/audits/PR1301/TRST-L-11.md new file mode 100644 index 000000000..ad0771c7e --- /dev/null +++ b/packages/issuance/audits/PR1301/TRST-L-11.md @@ -0,0 +1,26 @@ +# TRST-L-11: Inaccurate state flags returned by getAgreementDetails() and \_offerUpdate() + +- **Severity:** Low +- **Category:** Logical flaws +- **Source:** RecurringCollector.sol +- **Status:** Open + +## Description + +The `IAgreementCollector` interface defines state bit flags including `ACCEPTED` and `UPDATE`, with the documented convention that `UPDATE` is ORed into the state returned by `getAgreementDetails()` for pending versions (index 1). Two deviations from the specification were observed. + +First, in `_offerUpdate()` (lines 417 to 455), when an update is offered against an already accepted agreement, the returned `AgreementDetails` sets state to `REGISTERED | UPDATE` without ORing `ACCEPTED`. Callers that inspect the returned state to determine whether the agreement is already live will misread the underlying agreement as not accepted. + +Second, in `getAgreementDetails()` (lines 500 to 528), the `UPDATE` bit is never ORed into the returned state for the pending version path. The interface documentation promises this behavior for pending versions, but the implementation returns `REGISTERED` or `ACCEPTED` without regard to whether an RCAU offer is pending. + +Neither deviation changes on-chain accounting, but integrators relying on the declared state semantics will receive misleading data. + +## Recommended Mitigation + +In `_offerUpdate()`, OR the `ACCEPTED` bit into state when the underlying agreement is in the Accepted state. In `getAgreementDetails()`, OR the `UPDATE` bit into the returned state when a pending RCAU offer exists for the agreement. + +## Team Response + +TBD + +--- diff --git a/packages/issuance/audits/PR1301/TRST-L-2.md b/packages/issuance/audits/PR1301/TRST-L-2.md index 3fd0d45e4..f3eee05c5 100644 --- a/packages/issuance/audits/PR1301/TRST-L-2.md +++ b/packages/issuance/audits/PR1301/TRST-L-2.md @@ -3,7 +3,7 @@ - **Severity:** Low - **Category:** Arithmetic issues - **Source:** RecurringAgreementManager.sol -- **Status:** Open +- **Status:** Fixed ## Description @@ -19,7 +19,11 @@ The `pendingMaxNextClaim` should be computed as stated above, then reduced by th ## Team Response -TBD +Fixed. + +## Mitigation Review + +Refactored so that at any point, the accurate worst-case collection is reflected. --- diff --git a/packages/issuance/audits/PR1301/TRST-L-3.md b/packages/issuance/audits/PR1301/TRST-L-3.md index ff8edd1a8..92a21e7e4 100644 --- a/packages/issuance/audits/PR1301/TRST-L-3.md +++ b/packages/issuance/audits/PR1301/TRST-L-3.md @@ -3,7 +3,7 @@ - **Severity:** Low - **Category:** Access control issues - **Source:** RecurringAgreementManager.sol -- **Status:** Open +- **Status:** Fixed ## Description @@ -19,7 +19,11 @@ Add a pause check to `approveAgreement()` that returns `bytes4(0)` when the cont ## Team Response -TBD +Fixed. + +## Mitigation Review + +Fixed. Underlying code has been refactored, addressing the issue. --- diff --git a/packages/issuance/audits/PR1301/TRST-L-4.md b/packages/issuance/audits/PR1301/TRST-L-4.md index 71ea33109..4df8bbef9 100644 --- a/packages/issuance/audits/PR1301/TRST-L-4.md +++ b/packages/issuance/audits/PR1301/TRST-L-4.md @@ -3,7 +3,7 @@ - **Severity:** Low - **Category:** Donation attacks - **Source:** RecurringAgreementManager.sol -- **Status:** Open +- **Status:** Acknowledged ## Description @@ -19,7 +19,7 @@ In `_reconcilePairTracking()`, base the removal decision on `pairAgreementCount` ## Team Response -TBD +Accepted limitation. Orphaned tracking entries do not affect correctness or funds safety. --- diff --git a/packages/issuance/audits/PR1301/TRST-L-5.md b/packages/issuance/audits/PR1301/TRST-L-5.md index 812ac5c35..2533503e0 100644 --- a/packages/issuance/audits/PR1301/TRST-L-5.md +++ b/packages/issuance/audits/PR1301/TRST-L-5.md @@ -3,7 +3,7 @@ - **Severity:** Low - **Category:** Logical flaw - **Source:** RecurringAgreementManager.sol -- **Status:** Open +- **Status:** Fixed ## Description @@ -19,7 +19,11 @@ Align `_computeMaxFirstClaim()` with the RecurringCollector's `getMaxNextClaim() ## Team Response -TBD +Fixed. + +## Mitigation Review + +Fixed. The RecurringCollector now calculates the effective window correctly. --- diff --git a/packages/issuance/audits/PR1301/TRST-L-6.md b/packages/issuance/audits/PR1301/TRST-L-6.md new file mode 100644 index 000000000..50b3bf72f --- /dev/null +++ b/packages/issuance/audits/PR1301/TRST-L-6.md @@ -0,0 +1,28 @@ +# TRST-L-6: Update offer cleanup bypassed via planted offer matching active terms + +- **Severity:** Low +- **Category:** Logical flaws +- **Source:** RecurringCollector.sol +- **Status:** Open + +## Description + +In `_validateAndStoreUpdate()` (lines 854-858), cleanup of stored offers after an update uses an if / else if chain keyed on the prior `activeTermsHash`. The first branch deletes a matching entry from `rcaOffers`; the second deletes a matching entry from `rcauOffers`. + +A payer who observes a pending update can call `offer()` with `OFFER_TYPE_NEW` and parameters that reproduce the agreement's currently active RCA terms. The resulting entry in `rcaOffers` hashes to the same `oldHash` value. When `update()` later reaches the cleanup block, the first branch matches and deletes the planted entry, and the else if branch that would have cleaned up the corresponding `rcauOffers` entry is skipped. The pending update offer is then orphaned in storage. + +The `updateNonce` check elsewhere in `_validateAndStoreUpdate()` prevents the orphaned RCAU from being re-accepted, so the issue does not translate to a direct economic exploit. However, it introduces a divergence between the documented invariant that replaced offers are cleaned up and the actual storage state, which could surface as a correctness issue in future features that rely on offer presence. + +## Recommended Mitigation + +Delete both `rcaOffers[agreementId]` and `rcauOffers[agreementId]` unconditionally at the end of `_validateAndStoreUpdate()`. After a successful update the agreement's active terms have changed and any pre-existing offer entries for the same `agreementId` are stale by definition. + +## Team Response + +TBD + +--- + +The described attack requires planting an RCA offer whose EIP-712 hash collides with the active `activeTermsHash`. Because `_hashRCA` and `_hashRCAU` use distinct type hashes (`EIP712_RCA_TYPEHASH` vs `EIP712_RCAU_TYPEHASH`), cross-type collisions require a keccak256 preimage collision? Same-type collisions require the payer to reproduce the exact RCA terms, which is not an attack (the payer authored those terms). + +(Cleanup handling will be improved in combination with the response to TRST-L-11.) diff --git a/packages/issuance/audits/PR1301/TRST-L-7.md b/packages/issuance/audits/PR1301/TRST-L-7.md new file mode 100644 index 000000000..5976cd463 --- /dev/null +++ b/packages/issuance/audits/PR1301/TRST-L-7.md @@ -0,0 +1,33 @@ +# TRST-L-7: The cancel() function order sensitivity leaves RCAU offer unreachable + +- **Severity:** Low +- **Category:** Time-sensitivity issues +- **Source:** RecurringCollector.sol +- **Status:** Open + +## Description + +When a payer has both a pending RCA offer and a pending RCAU offer for the same `agreementId` and neither has been accepted, the order of cancellations matters. The `cancel()` overload that takes a terms hash delegates authorization to `_requirePayer()` (lines 480-497), which first checks the accepted agreement's payer and then the stored `rcaOffers` entry's payer. It does not fall back to `rcauOffers`. + +If the payer first cancels the RCA offer under `SCOPE_PENDING`, the entry in `rcaOffers` is deleted. A subsequent attempt to cancel the RCAU offer then fails: `_requirePayer()` finds no accepted agreement and no RCA offer, and reverts with `RecurringCollectorAgreementNotFound`. The orphaned RCAU offer remains in storage and unreachable by the payer. If the same parameters are later re-used to offer a new RCA, the orphaned RCAU is associated with it. The `updateNonce` check prevents immediate acceptance of the stale RCAU, but the payer has lost the ability to clean up state they own. + +## Recommended Mitigation + +Extend `_requirePayer()` to also check `rcauOffers` for a payer match when neither an accepted agreement nor an RCA offer is present. Alternatively, enforce symmetric cleanup so that deleting an RCA offer under `SCOPE_PENDING` also deletes any `rcauOffers` entry with the same `agreementId`. + +## Team Response + +TBD + +--- + +Resolved by restructuring offer storage so cancellation authorization no longer depends on +the stored RCA offer. In the current design, `agreement.payer` is a persistent field set at +first registration (via `_registerAgreement`) and is not cleared on cancel. The authorization +helper (`_requirePayerIfExists`) reads `agreement.payer` directly rather than falling back +through stored offer entries. + +As a consequence, cancelling a pre-acceptance RCA offer and cancelling a pending RCAU offer +are fully independent operations that may be performed in either order. Neither path leaves +the other unreachable, because the persistent `agreement.payer` continues to authorize the +surviving offer. diff --git a/packages/issuance/audits/PR1301/TRST-L-8.md b/packages/issuance/audits/PR1301/TRST-L-8.md new file mode 100644 index 000000000..c85f413d0 --- /dev/null +++ b/packages/issuance/audits/PR1301/TRST-L-8.md @@ -0,0 +1,24 @@ +# TRST-L-8: EOA payer signatures cannot be revoked before deadline + +- **Severity:** Low +- **Category:** Functionality flaws +- **Source:** RecurringCollector.sol +- **Status:** Open + +## Description + +Payers approve agreements through two paths: an ECDSA signature consumed by `accept()` or `update()`, and a stored offer placed by a contract payer via `offer()` and consumed against the stored hash. Contract payers can revoke a pending offer by calling `cancel()` with `SCOPE_PENDING`, which deletes the matching entry from `rcaOffers` or `rcauOffers`. + +EOA payers have no equivalent revocation path. Once an RCA or RCAU has been signed, the signature is accepted by the collector at any time before the `deadline` field expires. A payer that wishes to cancel a signature-based offer before the deadline (for example, to renegotiate terms) has no mechanism to do so. The only remaining option to ensure no duplicate agreement risk is to wait out the deadline (and hope their unintended offer is not matched), or to revoke the signer via the Authorizable thawing and revocation flow, which affects all agreements authorized by that signer rather than an individual offer. + +## Recommended Mitigation + +Expose a `cancelSignature(bytes32 hash)` entry point that records the hash as invalidated on-chain, and have `_requireAuthorization()` reject any hash that has been invalidated. Alternatively, use a per-signer nonce that the payer can bump to invalidate all outstanding signatures for that signer. + +## Team Response + +TBD + +--- + +Added `SCOPE_SIGNED` flag to `cancel()`, giving EOA signers an on-chain revocation path like contract payers already have via `SCOPE_PENDING`. The signer calls `cancel(agreementId, termsHash, SCOPE_SIGNED)` which records `cancelledOffers[msg.sender][termsHash] = agreementId`. When `accept()` or `update()` later processes a signature, `_requireAuthorization` recovers the signer via ECDSA and rejects if the stored agreementId matches. Self-authenticating (keyed by signer address), idempotent, reversible (calling again with `bytes16(0)` undoes the cancellation), and combinable with other scopes. Also made `cancel` no-op when nothing exists on-chain instead of reverting. diff --git a/packages/issuance/audits/PR1301/TRST-L-9.md b/packages/issuance/audits/PR1301/TRST-L-9.md new file mode 100644 index 000000000..e98f66046 --- /dev/null +++ b/packages/issuance/audits/PR1301/TRST-L-9.md @@ -0,0 +1,33 @@ +# TRST-L-9: Callback gas precheck does not account for intermediate overhead + +- **Severity:** Low +- **Category:** Gas-related issues +- **Source:** RecurringCollector.sol +- **Status:** Open + +## Description + +Both `_preCollectCallbacks()` and `_postCollectCallback()` guard each payer callback with a precheck of the form `if (gasleft() < (MAX_PAYER_CALLBACK_GAS * 64) / 63) revert`. The intent is to ensure that `MAX_PAYER_CALLBACK_GAS` remains available to the callee after applying the EIP-150 63/64 rule. + +However, the precheck is performed before the CALL or STATICCALL opcode itself, and additional gas is consumed between the comparison and the opcode: local Solidity operations, stack and memory setup, calldata encoding, and the fixed cost of the CALL or STATICCALL instruction. The actual gas forwarded to the callee can fall below `MAX_PAYER_CALLBACK_GAS`. An honest callee may perform incorrect logic under the assumption of available gas. One can refer to Optimism's CrossDomainMessenger, which adds explicit buffer constants (`RELAY_GAS_CHECK_BUFFER` and `RELAY_CALL_OVERHEAD`) for this exact reason. + +## Recommended Mitigation + +Add explicit buffer constants to the precheck so that the comparison accounts for the CALL/STATICCALL cost and the intervening Solidity overhead. Size the buffer so that at least `MAX_PAYER_CALLBACK_GAS` is forwarded to the callee when the check passes. + +## Team Response + +TBD + +--- + +Added `CALLBACK_GAS_OVERHEAD = 3_000` constant. All three prechecks now use: + +```solidity +if (gasleft() < (MAX_PAYER_CALLBACK_GAS * 64) / 63 + CALLBACK_GAS_OVERHEAD) + revert RecurringCollectorInsufficientCallbackGas(); +``` + +Sized to cover the worst-case pre-opcode cost. The eligibility STATICCALL is the first access to the payer account on the collect path, so the EIP-2929 cold-account access cost (2_600) dominates; the remaining headroom covers `abi.encodeCall` and stack/memory setup. Subsequent `beforeCollection` / `afterCollection` calls hit the payer warm (100 gas access), so the buffer is generous there. + +Follows the Optimism buffer-constant pattern as suggested. diff --git a/packages/issuance/audits/PR1301/TRST-M-1.md b/packages/issuance/audits/PR1301/TRST-M-1.md index 6ff77952f..72927231d 100644 --- a/packages/issuance/audits/PR1301/TRST-M-1.md +++ b/packages/issuance/audits/PR1301/TRST-M-1.md @@ -23,8 +23,14 @@ Add a minimum thaw threshold in `_updateEscrow()`. Amounts below the threshold s ## Team Response -TBD +Fixed. + +## Mitigation Review + +The griefing path remains reachable. Before any agreement is offered, a 1 wei donation to the (collector, provider) escrow account, followed by a permissionless call to `_reconcilePairTracking()` reaches `_updateEscrow()` with min and max at zero, and the thaw threshold is also at zero. Any positive excess passes the `thawThreshold <= excess` check, causing an `adjustThaw(thawTarget = 1)`. The same sequence also occurs after the final collection of an agreement, when `sumMaxNextClaim` transitions to zero via `afterCollection()` -> `_reconcileAndUpdateEscrow()` -> `_reconcileAgreement()`. There should be a nominal, non-negligible minimum thaw amount on top of the fraction check, applied in both `_reconcileProviderEscrow()` and `_withdrawAndRebalance()`. When `escrowBasis` is JustInTime, override the nominal skip so that dust can still be thawed out for solvency. --- -Added configurable `minThawFraction` (uint8, proportion of 256, default 16 = 6.25%) that skips thaws when the excess above max is below `sumMaxNextClaim * fraction / 256` for the (collector, provider) pair. An attacker must now donate a meaningful fraction per griefing round, making such an attack both economically unattractive and less effective. +Added configurable `minThawFraction` (uint8, default 16 = 6.25% of `sumMaxNextClaim`) that skips thaws below threshold. + +The zero-threshold path when `sumMaxNextClaim = 0` is acknowledged. Timer resets do not occur (`evenIfTimerReset=false` rejects increases), so the vector is limited to postponing pair tracking cleanup via repeated dust deposits. Added `minResidualEscrowFactor` (uint8, default 50, threshold = 2^value ≈ 0.001 GRT for default): pairs with no agreements and escrow below threshold are dropped from tracking. Untracked pairs can still have escrow drained via blind thaw/withdraw on `reconcileProvider`. diff --git a/packages/issuance/audits/PR1301/TRST-M-2.md b/packages/issuance/audits/PR1301/TRST-M-2.md index 9fc633fa5..df5ca47c6 100644 --- a/packages/issuance/audits/PR1301/TRST-M-2.md +++ b/packages/issuance/audits/PR1301/TRST-M-2.md @@ -3,7 +3,7 @@ - **Severity:** Medium - **Category:** Logical flaw - **Source:** RecurringAgreementManager.sol -- **Status:** Open +- **Status:** Fixed ## Description @@ -19,7 +19,11 @@ The original intention cannot be truly fulfilled without major redesign of multi ## Team Response -TBD +Fixed. + +## Mitigation Review + +The new setup is schematically sound. Admin intervention to trigger JustInTime may still be required to satisfy requests when the system is in OnDemand but insufficient liquidity is being thawed or minted into the contract. --- diff --git a/packages/issuance/audits/PR1301/TRST-M-3.md b/packages/issuance/audits/PR1301/TRST-M-3.md index ea3c6f7da..7654bbe6c 100644 --- a/packages/issuance/audits/PR1301/TRST-M-3.md +++ b/packages/issuance/audits/PR1301/TRST-M-3.md @@ -3,7 +3,7 @@ - **Severity:** Medium - **Category:** Logical flaw - **Source:** RecurringAgreementManager.sol -- **Status:** Open +- **Status:** Acknowledged ## Description @@ -19,7 +19,7 @@ Add a separate configuration flag (e.g., `allowModeDegradation`) that must be ex ## Team Response -TBD +Acknowledged. The risk is documented, including the operator caution about pre-offer headroom checks. --- diff --git a/packages/issuance/audits/PR1301/TRST-M-4.md b/packages/issuance/audits/PR1301/TRST-M-4.md new file mode 100644 index 000000000..ad06eac0e --- /dev/null +++ b/packages/issuance/audits/PR1301/TRST-M-4.md @@ -0,0 +1,33 @@ +# TRST-M-4: Returndata bombing via payer callbacks in \_preCollectCallbacks and \_postCollectCallback + +- **Severity:** Medium +- **Category:** Gas-related issues +- **Source:** RecurringCollector.sol +- **Status:** Open + +## Description + +All three payer callbacks reachable from `_collect()` (the eligibility staticcall in `_preCollectCallbacks()` at line 633, the `beforeCollection()` call in the same function at line 646, and the `afterCollection()` call in `_postCollectCallback()` at line 666) use Solidity's default low-level call pattern, which copies the full returndata buffer into the caller's memory. Note that RETURNDATACOPY is emitted even when the returned bytes are discarded via the `(bool ok, )` tuple pattern. + +With a forwarded budget of `MAX_PAYER_CALLBACK_GAS` (1,500,000) per callback, a malicious payer can expand callee memory and return roughly 850 KB of data. The caller's RETURNDATACOPY and the associated memory expansion then consume approximately 1,500,000 gas in the `_collect()` frame for each callback. Across the three callbacks, a single `collect()` call can be forced to burn about 4,500,000 gas beyond the nominal callback budget. + +The impact is an inflated collection cost that is not reflected in off-chain gas estimates. This is gas griefing rather than a collection block, and gas costs remain manageable. + +## Recommended Mitigation + +Replace the affected high-level call sites with inline assembly that performs the call and bounds the amount of returndata copied. For the eligibility check, copy at most 32 bytes into scratch memory and read the result. For `beforeCollection()` and `afterCollection()`, copy zero bytes since the return value is unused. + +## Team Response + +TBD + +--- + +## Fix + +Replaced all three call sites with inline assembly that bounds returndata copy: + +- **Eligibility staticcall**: copies at most 32 bytes into scratch space (0x00), reads the `uint256` result from there. +- **beforeCollection / afterCollection**: copy 0 bytes (`retSize=0`), only the `bool success` from the CALL opcode is used. + +This prevents a malicious payer from forcing RETURNDATACOPY of ~850 KB per callback. diff --git a/packages/issuance/audits/PR1301/TRST-M-5.md b/packages/issuance/audits/PR1301/TRST-M-5.md new file mode 100644 index 000000000..155efa2be --- /dev/null +++ b/packages/issuance/audits/PR1301/TRST-M-5.md @@ -0,0 +1,28 @@ +# TRST-M-5: Perpetual thaw griefing via micro deposits in \_reconcileProviderEscrow + +- **Severity:** Medium +- **Category:** Griefing attacks +- **Source:** RecurringAgreementManager.sol +- **Status:** Open + +## Description + +The `_reconcileProviderEscrow()` and symmetrically `_withdrawAndRebalance()` functions compare the escrow excess against a fraction-based threshold derived from `sumMaxNextClaim`. The check is structured as `thawThreshold <= excess`, which permits a thaw whenever the cumulative excess is at least the threshold. Because the threshold is keyed on `sumMaxNextClaim` and not on the amount being added to `thawingTarget` in the current round, the check behaves like a one-time gate rather than a per-round qualifier. + +An attacker can grief the RAM in two phases. First, they make a single non-negligible donation via the permissionless `PaymentsEscrow.depositTo()` that pushes the escrow balance for a (collector, provider) pair above `initial_excess > thawThreshold`. This bootstrap round costs the attacker an amount on the order of the threshold and triggers the initial `adjustThaw()` call, starting the thaw timer with `thawingTarget = initial_excess`. Second, the attacker repeatedly donates 1 wei and triggers reconciliation. The bootstrap excess is still present, so `excess > thawThreshold` continues to hold. Each round passes the check, calls `adjustThaw()` with `thawingTarget` incremented by 1 wei, and resets the thaw timer. Legitimate larger thaws issued by the RAM while the griefing is active are blocked for the duration of the thawing period because the timer keeps resetting. + +The per-round cost to the attacker after the bootstrap is 1 wei plus gas. The griefing causes spurious thaws, consumes gas on every reconciliation, and interacts with `PaymentsEscrow.adjustThaw()` timer semantics to indefinitely delay legitimate thaws for the targeted pair. + +## Recommended Mitigation + +Gate the check on the incremental amount being added to `thawingTarget` in the current round rather than on the cumulative excess over the maximum. A round should only pass the threshold check when the new delta to `thawingTarget` is non-trivial. Combine this with an absolute nominal minimum thaw amount applied in both `_reconcileProviderEscrow()` and `_withdrawAndRebalance()` so that sub-nominal dust increments cannot reset the thaw timer even after the bootstrap. + +## Team Response + +TBD + +--- + +RAM always calls `adjustThaw(..., evenIfTimerReset=false)`. When a thaw is already active, any increase to `thawingTarget` that would change `thawEndTimestamp` is silently rejected by PaymentsEscrow so the timer is never reset. The "bootstrap + repeated 1 wei" attack does not work as described? + +The actual vector is narrower: indefinite postponement of pair tracking cleanup when `sumMaxNextClaim = 0`. Addressed by `minResidualEscrowFactor` in the fix for TRST-M-1. diff --git a/packages/issuance/audits/PR1301/TRST-R-10.md b/packages/issuance/audits/PR1301/TRST-R-10.md new file mode 100644 index 000000000..e1d200ba7 --- /dev/null +++ b/packages/issuance/audits/PR1301/TRST-R-10.md @@ -0,0 +1,11 @@ +# TRST-R-10: Document role-change semantics for existing agreements + +- **Severity:** Recommendation + +## Description + +Changes to `DATA_SERVICE_ROLE` and `COLLECTOR_ROLE` on the RecurringAgreementManager do not affect agreements that have already been offered or accepted through the previously authorized addresses. This is by design (revoking a role should not invalidate settled obligations), but the behavior is not documented. Record this invariant in the RAM documentation so that operators and integrators understand the effect of role changes. + +--- + +Documented in the `RecurringAgreementManager` contract header: role changes are not retroactive — revoking `COLLECTOR_ROLE` or `DATA_SERVICE_ROLE` does not invalidate tracked agreements, which continue to reconcile to orderly settlement. Role checks gate only new `offerAgreement` calls and discovery inside `_reconcileAgreement`. diff --git a/packages/issuance/audits/PR1301/TRST-R-11.md b/packages/issuance/audits/PR1301/TRST-R-11.md new file mode 100644 index 000000000..d200e7468 --- /dev/null +++ b/packages/issuance/audits/PR1301/TRST-R-11.md @@ -0,0 +1,15 @@ +# TRST-R-11: Remove or implement unused state flags in IAgreementCollector + +- **Severity:** Recommendation + +## Description + +`IAgreementCollector` defines state flag constants that are not currently used in the RecurringCollector implementation, including `NOTICE_GIVEN`, `SETTLED`, `BY_PAYER`, `BY_PROVIDER`, `BY_DATA_SERVICE`, `AUTO_UPDATE`, and `AUTO_UPDATED`. Unused public interface constants are a source of confusion for integrators, who may code against documented semantics that the implementation does not honor. Either remove the unused flags from the interface, or implement the behaviors they describe in the collector. + +--- + +Removed usused flags: `AUTO_UPDATE`, `AUTO_UPDATED`, `BY_DATA_SERVICE`, `WITH_NOTICE` and `IF_NOT_ACCEPTED` are dropped from the interface. + +NatSpec updated for remaining flags with new semantics. + +In RecurringCollector `NOTICE_GIVEN`, `SETTLED`, `BY_PAYER`, `BY_PROVIDER` are now set by `getAgreementDetails` to describe cancel origin and collectability (see TRST-R-12 fix). diff --git a/packages/issuance/audits/PR1301/TRST-R-12.md b/packages/issuance/audits/PR1301/TRST-R-12.md new file mode 100644 index 000000000..030c0272d --- /dev/null +++ b/packages/issuance/audits/PR1301/TRST-R-12.md @@ -0,0 +1,17 @@ +# TRST-R-12: Document ACCEPTED state returned for cancelled agreements + +- **Severity:** Recommendation + +## Description + +In `getAgreementDetails()`, any agreement whose state is not `AgreementState.NotAccepted` is reported with state flag `ACCEPTED`. This includes agreements that have been cancelled (`CanceledByPayer` or `CanceledByServiceProvider`). Integrators inspecting the returned state cannot distinguish cancelled agreements from live ones without reading separate storage. Document this behavior in the interface, or extend the state bitmask with a `CANCELED` flag and return it for the non-active terminal states. + +--- + +Reusing the existing interface flags instead of adding a `CANCELED` flag. `getAgreementDetails` now composes cancel and collectability information: + +- `NOTICE_GIVEN` — set on cancelled agreements (collection window truncated). +- `BY_PAYER` / `BY_PROVIDER` — paired with `NOTICE_GIVEN` to identify the cancel origin. +- `SETTLED` — set when nothing currently claimable. Covers provider-cancelled agreements (immediately non-collectable), fully-collected agreements, payer-cancelled agreements past their canceledAt window. + +`ACCEPTED` is also narrowed: it is now only set on the active-slot version (`VERSION_CURRENT`) of agreements past `NotAccepted`, so pending updates (`VERSION_NEXT`) no longer report `ACCEPTED`. Integrators distinguish cancelled-vs-live by `NOTICE_GIVEN`, and stop-collecting-now via `SETTLED`. See the TRST-R-11 fix for the accompanying flag cleanup. diff --git a/packages/issuance/audits/PR1301/TRST-R-13.md b/packages/issuance/audits/PR1301/TRST-R-13.md new file mode 100644 index 000000000..cefb73ec0 --- /dev/null +++ b/packages/issuance/audits/PR1301/TRST-R-13.md @@ -0,0 +1,11 @@ +# TRST-R-13: Document reclaim reason change for stale allocation force-close + +- **Severity:** Recommendation + +## Description + +Before the PR's refactor, `forceCloseStaleAllocation()` closed the allocation via `_closeAllocation()` and caused a reclaim with reason `CLOSE_ALLOCATION`. Post refactor, the force close path goes through `_resizeAllocation(allocationId, 0, ...)`, which triggers a reclaim with reason `STALE_POI` instead. The reclaim still occurs, but the reason code exposed to reclaim address configuration changes. Document this change so that operators are able to prepare accordingly and have funding paths line up with intention. + +--- + +Noted. The previous `CLOSE_ALLOCATION` reclaim behavior for this path has not shipped to production, so there is no live operator configuration to migrate. `STALE_POI` is the correct reason for the post-refactor semantics (the allocation is stale; it stays open as stakeless rather than closing). diff --git a/packages/issuance/audits/PR1301/TRST-R-3.md b/packages/issuance/audits/PR1301/TRST-R-3.md index d3fa90130..0e012a072 100644 --- a/packages/issuance/audits/PR1301/TRST-R-3.md +++ b/packages/issuance/audits/PR1301/TRST-R-3.md @@ -5,3 +5,7 @@ ## Description In the RAM's `cancelAgreement()` function, the agreement state is required to not be not accepted. However, the logic could be more specific and require the agreement to be Accepted - rejecting previously cancelled agreements. There is no impact because corresponding checks in the RecurringCollector would deny such cancels, but it remains as a best practice. + +--- + +Fixed. The RAM's `cancelAgreement()` was refactored into a pass-through to `collector.cancel()`, which requires `agreement.state == AgreementState.Accepted` before proceeding. The defensive guard now lives in the single authoritative location for agreement state. diff --git a/packages/issuance/audits/PR1301/TRST-R-4.md b/packages/issuance/audits/PR1301/TRST-R-4.md index 6e40e6682..7947adbdc 100644 --- a/packages/issuance/audits/PR1301/TRST-R-4.md +++ b/packages/issuance/audits/PR1301/TRST-R-4.md @@ -5,3 +5,7 @@ ## Description The `approveAgreement()` view checks if the agreement hash is valid, however it offers no replay protection for repeated agreement approvals. This attack vector is only stopped at the RecurringCollector as it checks the agreement does not exist and maintains unidirectional transitions from the agreement Accepted state. For future collectors this may not be the case, necessitating clear documentation of the assumption. + +--- + +Documented in the `RecurringAgreementManager` contract header (collector-trust section): collectors own agreement uniqueness, replay protection, and state transitions; RAM does not re-check them. diff --git a/packages/issuance/audits/PR1301/TRST-R-5.md b/packages/issuance/audits/PR1301/TRST-R-5.md new file mode 100644 index 000000000..0db3ff607 --- /dev/null +++ b/packages/issuance/audits/PR1301/TRST-R-5.md @@ -0,0 +1,11 @@ +# TRST-R-5: Ambiguous return value in getAgreementOfferAt() + +- **Severity:** Recommendation + +## Description + +`getAgreementOfferAt()` returns `(uint8 offerType, bytes memory offerData)`. The offer type constant `OFFER_TYPE_NEW` is defined as 0, which is also the default Solidity return value when no stored offer exists for the given `agreementId` and index. A caller receiving `offerType == 0` cannot distinguish between a stored new-type offer existing and no offer existing. Consider redefining offer type constants with 1-indexed values, or adding an explicit `bool found` return parameter. + +--- + +Using non-zero offer type constants as suggested: `OFFER_TYPE_NEW = 1`, `OFFER_TYPE_UPDATE = 2`. The zero value is declared explicitly as `OFFER_TYPE_NONE` so the "no stored offer" sentinel is part of the interface rather than a NatSpec-only convention. diff --git a/packages/issuance/audits/PR1301/TRST-R-6.md b/packages/issuance/audits/PR1301/TRST-R-6.md new file mode 100644 index 000000000..76460062d --- /dev/null +++ b/packages/issuance/audits/PR1301/TRST-R-6.md @@ -0,0 +1,11 @@ +# TRST-R-6: Dead code guard in \_validateAndStoreUpdate() + +- **Severity:** Recommendation + +## Description + +In `_validateAndStoreUpdate()` (line 855), the guard `if (oldHash != bytes32(0))` is unreachable as a false branch. Only agreements in the Accepted state may be updated, and every accepted agreement has a non-zero `activeTermsHash` written during `accept()` or a prior `update()`. The guard can be removed or converted into an invariant comment documenting this assumption. + +--- + +Fixed. Removed the dead `if (oldHash != bytes32(0))` guard. The offer cleanup is now unconditional with an inline comment noting the invariant. diff --git a/packages/issuance/audits/PR1301/TRST-R-7.md b/packages/issuance/audits/PR1301/TRST-R-7.md new file mode 100644 index 000000000..65f7ae98c --- /dev/null +++ b/packages/issuance/audits/PR1301/TRST-R-7.md @@ -0,0 +1,13 @@ +# TRST-R-7: Remove consumed offers in accept() and update() + +- **Severity:** Recommendation + +## Description + +After `accept()` or `update()` consumes a stored offer, the corresponding entry in `rcaOffers` or `rcauOffers` becomes stale. Currently only `_validateAndStoreUpdate()` cleans up the previously active offer by looking up the old `activeTermsHash`; the offer whose terms were just accepted is not deleted. This is a storage hygiene concern: stale offer entries remain in storage indefinitely until explicitly replaced or matched by a future update. Consider deleting the consumed offer entry inside `accept()` and `update()` after it has been applied. + +--- + +Keeping consumed offers in storage is by design — offer data (including metadata, nonce, deadline) remains accessible on-chain via `getAgreementOfferAt()` until the terms are obsolete. Stale entries are cleaned up by `_validateAndStoreUpdate()` on the next update, overwritten by a new `offer()`, or removed by `cancel()`. Eagerly deleting on consumption would lose data that callers may still want to inspect. + +(Cleanup handling will be improved in combination with the response to TRST-L-11.) diff --git a/packages/issuance/audits/PR1301/TRST-R-8.md b/packages/issuance/audits/PR1301/TRST-R-8.md new file mode 100644 index 000000000..821e84823 --- /dev/null +++ b/packages/issuance/audits/PR1301/TRST-R-8.md @@ -0,0 +1,11 @@ +# TRST-R-8: Align pause documentation with callback behavior in the RAM + +- **Severity:** Recommendation + +## Description + +The RecurringAgreementManager documentation header states that pausing the contract "stops all permissionless escrow management". In practice, the `whenNotPaused` modifier also applies to `beforeCollection()` and `afterCollection()`, so pause also halts the callback path used during `collect()`. Update the documentation to reflect that callbacks are affected, or narrow the modifier application so that behavior matches the prose. + +--- + +Updated in the `RecurringAgreementManager` contract header: pause is described as blocking permissionless state changes "including collection callbacks and reconciliation", with a cross-reference to the existing cross-contract note describing the resulting escrow-accounting drift. diff --git a/packages/issuance/audits/PR1301/TRST-R-9.md b/packages/issuance/audits/PR1301/TRST-R-9.md new file mode 100644 index 000000000..efa601a43 --- /dev/null +++ b/packages/issuance/audits/PR1301/TRST-R-9.md @@ -0,0 +1,11 @@ +# TRST-R-9: \_isAuthorized() override in RecurringCollector trusts itself for any authorizer + +- **Severity:** Recommendation + +## Description + +The `_isAuthorized(address authorizer, address signer)` override in RecurringCollector returns true whenever `signer == address(this)`, regardless of `authorizer`. This enables RecurringCollector to call `dataService.cancelIndexingAgreementByPayer()` on the payer's behalf. The semantics are safe in the current integration with SubgraphService, but they widen the trust surface: any future consumer that relies on `RecurringCollector.isAuthorized()` for access control will grant access when the signer is the collector itself. Consider tightening the override to scope trust to specific callers, or explicitly document the integration contract so it is not misapplied by future consumers. + +--- + +Added a `@custom:security` note at the `RecurringCollector` contract header: self-authorization requires the collector itself to perform the appropriate authorization check before any external call. diff --git a/packages/issuance/contracts/agreement/RecurringAgreementManager.sol b/packages/issuance/contracts/agreement/RecurringAgreementManager.sol index 881208eed..4993ba3fe 100644 --- a/packages/issuance/contracts/agreement/RecurringAgreementManager.sol +++ b/packages/issuance/contracts/agreement/RecurringAgreementManager.sol @@ -54,8 +54,15 @@ import { ReentrancyGuardTransient } from "@openzeppelin/contracts/utils/Reentran * and {cancelAgreement} call collectors directly. Discovery calls `getAgreementDetails`; * reconciliation calls `getMaxNextClaim` — these return values drive escrow accounting. * A broken or malicious collector can cause reconciliation to revert; use - * {forceRemoveAgreement} as an operator escape hatch. Once tracked, reconciliation proceeds - * even if COLLECTOR_ROLE is later revoked, ensuring orderly settlement. + * {forceRemoveAgreement} as an operator escape hatch. + * + * Collectors own agreement uniqueness, replay protection, and state transitions; this + * contract does not re-check them. + * + * Role changes are not retroactive. Revoking COLLECTOR_ROLE or DATA_SERVICE_ROLE does not + * invalidate agreements that were offered or accepted while the roles were held. Once + * tracked, reconciliation proceeds to orderly settlement. Role changes only gate *new* + * {offerAgreement} calls and discovery inside {_reconcileAgreement}. * * {offerAgreement} and {cancelAgreement} forward to the collector then reconcile locally. * The collector does not callback to `msg.sender`, so these methods own the full call @@ -77,7 +84,8 @@ import { ReentrancyGuardTransient } from "@openzeppelin/contracts/utils/Reentran * Escalation ladder (targeted → full stop): * 1. {emergencyRevokeRole} — disable a specific actor (operator, collector, guardian) * 2. {emergencyClearEligibilityOracle} — fail-open if oracle blocks collections - * 3. Pause this contract — stops all permissionless escrow management + * 3. Pause this contract — blocks permissionless state changes, including collection + * callbacks and reconciliation (see cross-contract note above) * 4. Pause RecurringCollector — stops all collections and state changes * 5. Pause both — full halt * @@ -143,7 +151,11 @@ contract RecurringAgreementManager is /** * @notice Per-(collector, provider) pair tracking data * @param sumMaxNextClaim Sum of maxNextClaim for all agreements in this pair - * @param escrowSnap Last known escrow balance (for snapshot diff) + * @param escrowSnap Snapshot of escrow balance at the last _setEscrowSnap call. + * Input to totalEscrowDeficit accounting, not a guarantee of the live balance — it can + * drift between reconciliations (e.g. after beforeCollection's JIT deposit) until the + * next _reconcileProviderEscrow resyncs it. Read the live balance via _fetchEscrowAccount + * when actual solvency matters. * @param agreements Set of agreement IDs for this pair (stored as bytes32 for EnumerableSet) */ struct CollectorProviderData { @@ -176,8 +188,9 @@ contract RecurringAgreementManager is /// @notice Total unfunded escrow: sum of max(0, sumMaxNextClaim[c][p] - escrowSnap[c][p]) uint256 totalEscrowDeficit; /// @notice The issuance allocator that mints GRT to this contract (20 bytes) - /// @dev Packed slot (28/32 bytes): issuanceAllocator (20) + ensuredIncomingDistributedToBlock (4) + - /// escrowBasis (1) + minOnDemandBasisThreshold (1) + minFullBasisMargin (1) + minThawFraction (1). + /// @dev Packed slot (29/32 bytes): issuanceAllocator (20) + ensuredIncomingDistributedToBlock (4) + + /// escrowBasis (1) + minOnDemandBasisThreshold (1) + minFullBasisMargin (1) + minThawFraction (1) + + /// minResidualEscrowFactor (1). /// All read together in _reconcileProviderEscrow / beforeCollection. IIssuanceAllocationDistribution issuanceAllocator; /// @notice Block number when _ensureIncomingDistributionToCurrentBlock last ran @@ -194,6 +207,13 @@ contract RecurringAgreementManager is /// per (collector, provider) pair is skipped as operationally insignificant. /// Governance-configured. uint8 minThawFraction; + /// @notice Minimum residual escrow factor: when a (collector, provider) pair has no agreements + /// and the escrow balance is below 2^value, tracking is dropped; the residual is not worth + /// the gas cost of further thaw/withdraw cycles. Funds remain in PaymentsEscrow but are no + /// longer actively managed by RAM. Higher values drop more aggressively: + /// 0 = drop only at zero balance (effectively never drop); 255 = always drop when no + /// agreements remain. Governance-configured. Default 50 ≈ 0.001 GRT. + uint8 minResidualEscrowFactor; /// @notice Optional oracle for checking payment eligibility of service providers (20/32 bytes in slot) IProviderEligibility providerEligibilityOracle; } @@ -231,6 +251,7 @@ contract RecurringAgreementManager is $.minOnDemandBasisThreshold = 128; $.minFullBasisMargin = 16; $.minThawFraction = 16; + $.minResidualEscrowFactor = 50; // 2^50 ≈ 10^15 ≈ 0.001 GRT } // -- ERC165 -- @@ -254,6 +275,11 @@ contract RecurringAgreementManager is /// @inheritdoc IIssuanceTarget function beforeIssuanceAllocationChange() external virtual override {} + /// @inheritdoc IIssuanceTarget + function getIssuanceAllocator() external view virtual override returns (IIssuanceAllocationDistribution) { + return _getStorage().issuanceAllocator; + } + /// @inheritdoc IIssuanceTarget /// @dev The allocator is expected to call distributeIssuance() (bringing distribution up to /// the current block) before any configuration change. As a result, the same-block dedup in @@ -262,21 +288,23 @@ contract RecurringAgreementManager is /// in a standalone transaction to avoid interleaving with collection in the same block. /// Even if interleaved, the only effect is a one-block lag before the new allocator's /// distribution is picked up — corrected automatically on the next block. - function setIssuanceAllocator(address newIssuanceAllocator) external virtual override onlyRole(GOVERNOR_ROLE) { + function setIssuanceAllocator( + IIssuanceAllocationDistribution newIssuanceAllocator + ) external virtual override onlyRole(GOVERNOR_ROLE) { RecurringAgreementManagerStorage storage $ = _getStorage(); - if (address($.issuanceAllocator) == newIssuanceAllocator) return; + if (address($.issuanceAllocator) == address(newIssuanceAllocator)) return; - if (newIssuanceAllocator != address(0)) + if (address(newIssuanceAllocator) != address(0)) require( ERC165Checker.supportsInterface( - newIssuanceAllocator, + address(newIssuanceAllocator), type(IIssuanceAllocationDistribution).interfaceId ), - InvalidIssuanceAllocator(newIssuanceAllocator) + InvalidIssuanceAllocator(address(newIssuanceAllocator)) ); - emit IssuanceAllocatorSet(address($.issuanceAllocator), newIssuanceAllocator); - $.issuanceAllocator = IIssuanceAllocationDistribution(newIssuanceAllocator); + emit IssuanceAllocatorSet($.issuanceAllocator, newIssuanceAllocator); + $.issuanceAllocator = newIssuanceAllocator; } // -- IAgreementOwner -- @@ -435,6 +463,16 @@ contract RecurringAgreementManager is emit MinThawFractionSet(oldFraction, fraction); } + /// @inheritdoc IRecurringEscrowManagement + function setMinResidualEscrowFactor(uint8 value) external onlyRole(OPERATOR_ROLE) { + RecurringAgreementManagerStorage storage $ = _getStorage(); + if ($.minResidualEscrowFactor == value) return; + + uint8 oldValue = $.minResidualEscrowFactor; + $.minResidualEscrowFactor = value; + emit MinResidualEscrowFactorSet(oldValue, value); + } + // -- IProviderEligibilityManagement -- /// @inheritdoc IProviderEligibilityManagement @@ -542,6 +580,11 @@ contract RecurringAgreementManager is return _getStorage().minThawFraction; } + /// @inheritdoc IRecurringAgreements + function getMinResidualEscrowFactor() external view returns (uint8) { + return _getStorage().minResidualEscrowFactor; + } + /// @inheritdoc IRecurringAgreements function getCollectorCount() external view returns (uint256) { return _getStorage().collectorSet.length(); @@ -683,9 +726,17 @@ contract RecurringAgreementManager is } /** - * @notice Reconcile escrow then remove (collector, provider) tracking if fully drained. - * @dev Calls {_reconcileProviderEscrow} to withdraw completed thaws, then removes the pair from - * tracking only when both agreement count and escrowSnap are zero. + * @notice Reconcile escrow then remove (collector, provider) tracking if below residual threshold. + * @dev For tracked pairs (in providerSet): runs {_reconcileProviderEscrow}, then drops tracking + * when no agreements remain and escrow balance is strictly below the residual threshold. + * For untracked pairs: performs a blind drain (withdraw matured thaw, thaw remainder) without + * re-creating tracking state. + * + * The residual threshold = 2^minResidualEscrowFactor. Below this, the residual is not worth + * the gas cost of further thaw/withdraw cycles, so tracking is dropped. Funds remain in + * PaymentsEscrow, just no longer actively managed by RAM. A subsequent {_offerAgreement} + * for the same pair will re-add tracking naturally. + * * Cascades to remove the collector when it has no remaining providers. * @param $ The storage reference * @param collector The collector contract address @@ -698,11 +749,22 @@ contract RecurringAgreementManager is address collector, address provider ) private returns (bool tracked) { + if (!$.collectors[collector].providerSet.contains(provider)) { + // Not tracked — blind drain without re-creating tracking state. + _drainUntracked(collector, provider); + return false; + } + _reconcileProviderEscrow($, collector, provider); CollectorProviderData storage cpd = $.collectors[collector].providers[provider]; - if (cpd.agreements.length() != 0 || cpd.escrowSnap != 0) tracked = true; - else if ($.collectors[collector].providerSet.remove(provider)) { + // Drop tracking when no agreements and escrow is below residual threshold. + // Funds remain in PaymentsEscrow; deficit contribution is already 0 (sumMaxNextClaim == 0). + // Read real balance (escrowSnap is already cleared when sumMaxNextClaim == 0). + tracked = + cpd.agreements.length() != 0 || + ((uint256(1) << $.minResidualEscrowFactor) <= _fetchEscrowAccount(collector, provider).balance); + if (!tracked && $.collectors[collector].providerSet.remove(provider)) { emit ProviderRemoved(collector, provider); if ($.collectors[collector].providerSet.length() == 0) { // Provider agreement count will already be zero at this point. @@ -712,6 +774,24 @@ contract RecurringAgreementManager is } } + /** + * @notice Blind drain for an untracked (collector, provider) escrow pair. + * @dev Withdraws matured thaw if any, then starts a new thaw for remaining balance. + * Does not read or write any RAM tracking state. Only acts when no thaw is active + * (after withdraw or if none was started), so thaw() is safe — no timer to reset. + * @param collector The collector contract address + * @param provider Service provider address + */ + function _drainUntracked(address collector, address provider) private { + IPaymentsEscrow.EscrowAccount memory account = _fetchEscrowAccount(collector, provider); + if (0 < account.tokensThawing && account.thawEndTimestamp < block.timestamp) { + PAYMENTS_ESCROW.withdraw(collector, provider); + account = _fetchEscrowAccount(collector, provider); + } + if (account.tokensThawing == 0 && 0 < account.balance) + PAYMENTS_ESCROW.thaw(collector, provider, account.balance); + } + /** * @notice The sole mutation point for agreement.maxNextClaim and all derived totals. * @dev ALL writes to agreement.maxNextClaim, sumMaxNextClaim, sumMaxNextClaimAll, and @@ -928,7 +1008,8 @@ contract RecurringAgreementManager is address provider ) private { uint256 oldEscrow = cpd.escrowSnap; - uint256 newEscrow = _fetchEscrowAccount(collector, provider).balance; + // No need to track escrow when no claims remain (deficit is 0 regardless). + uint256 newEscrow = cpd.sumMaxNextClaim != 0 ? _fetchEscrowAccount(collector, provider).balance : 0; if (oldEscrow == newEscrow) return; uint256 oldDeficit = _providerEscrowDeficit(cpd); diff --git a/packages/issuance/contracts/allocate/DirectAllocation.sol b/packages/issuance/contracts/allocate/DirectAllocation.sol index 91f153b5e..9df058eca 100644 --- a/packages/issuance/contracts/allocate/DirectAllocation.sol +++ b/packages/issuance/contracts/allocate/DirectAllocation.sol @@ -2,6 +2,9 @@ pragma solidity ^0.8.27; +import { ERC165Checker } from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol"; + +import { IIssuanceAllocationDistribution } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceAllocationDistribution.sol"; import { IIssuanceTarget } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceTarget.sol"; import { ISendTokens } from "@graphprotocol/interfaces/contracts/issuance/allocate/ISendTokens.sol"; import { BaseUpgradeable } from "../common/BaseUpgradeable.sol"; @@ -24,6 +27,36 @@ import { ERC165Upgradeable } from "@openzeppelin/contracts-upgradeable/utils/int * @custom:security-contact Please email security+contracts@thegraph.com if you find any bugs. We might have an active bug bounty program. */ contract DirectAllocation is BaseUpgradeable, IIssuanceTarget, ISendTokens { + // -- Namespaced Storage -- + + /// @notice ERC-7201 storage location for DirectAllocation + bytes32 private constant DIRECT_ALLOCATION_STORAGE_LOCATION = + // solhint-disable-next-line gas-small-strings + keccak256(abi.encode(uint256(keccak256("graphprotocol.storage.DirectAllocation")) - 1)) & + ~bytes32(uint256(0xff)); + + /// @notice Main storage structure for DirectAllocation using ERC-7201 namespaced storage + /// @param issuanceAllocator The issuance allocator that distributes tokens to this contract + /// @custom:storage-location erc7201:graphprotocol.storage.DirectAllocation + struct DirectAllocationData { + IIssuanceAllocationDistribution issuanceAllocator; + } + + /** + * @notice Returns the storage struct for DirectAllocation + * @return $ contract storage + */ + function _getDirectAllocationStorage() private pure returns (DirectAllocationData storage $) { + // solhint-disable-previous-line use-natspec + // Solhint does not support $ return variable in natspec + + bytes32 slot = DIRECT_ALLOCATION_STORAGE_LOCATION; + // solhint-disable-next-line no-inline-assembly + assembly { + $.slot := slot + } + } + // -- Custom Errors -- /// @notice Thrown when token transfer fails @@ -31,6 +64,10 @@ contract DirectAllocation is BaseUpgradeable, IIssuanceTarget, ISendTokens { /// @param amount The amount of tokens that failed to transfer error SendTokensFailed(address to, uint256 amount); + /// @notice Thrown when the issuance allocator does not support IIssuanceAllocationDistribution + /// @param allocator The rejected allocator address + error InvalidIssuanceAllocator(address allocator); + // -- Events -- /// @notice Emitted when tokens are sent @@ -89,9 +126,28 @@ contract DirectAllocation is BaseUpgradeable, IIssuanceTarget, ISendTokens { */ function beforeIssuanceAllocationChange() external virtual override {} - /** - * @dev No-op for DirectAllocation; issuanceAllocator is not stored. - * @inheritdoc IIssuanceTarget - */ - function setIssuanceAllocator(address issuanceAllocator) external virtual override onlyRole(GOVERNOR_ROLE) {} + /// @inheritdoc IIssuanceTarget + function getIssuanceAllocator() external view virtual override returns (IIssuanceAllocationDistribution) { + return _getDirectAllocationStorage().issuanceAllocator; + } + + /// @inheritdoc IIssuanceTarget + function setIssuanceAllocator( + IIssuanceAllocationDistribution newIssuanceAllocator + ) external virtual override onlyRole(GOVERNOR_ROLE) { + DirectAllocationData storage $ = _getDirectAllocationStorage(); + if (address(newIssuanceAllocator) == address($.issuanceAllocator)) return; + + if (address(newIssuanceAllocator) != address(0)) + require( + ERC165Checker.supportsInterface( + address(newIssuanceAllocator), + type(IIssuanceAllocationDistribution).interfaceId + ), + InvalidIssuanceAllocator(address(newIssuanceAllocator)) + ); + + emit IssuanceAllocatorSet($.issuanceAllocator, newIssuanceAllocator); + $.issuanceAllocator = newIssuanceAllocator; + } } diff --git a/packages/issuance/contracts/test/allocate/MockNotificationTracker.sol b/packages/issuance/contracts/test/allocate/MockNotificationTracker.sol index a33212282..2b5fb5aec 100644 --- a/packages/issuance/contracts/test/allocate/MockNotificationTracker.sol +++ b/packages/issuance/contracts/test/allocate/MockNotificationTracker.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.24; +import { IIssuanceAllocationDistribution } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceAllocationDistribution.sol"; import { IIssuanceTarget } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceTarget.sol"; import { ERC165 } from "@openzeppelin/contracts/utils/introspection/ERC165.sol"; @@ -30,7 +31,12 @@ contract MockNotificationTracker is IIssuanceTarget, ERC165 { } /// @inheritdoc IIssuanceTarget - function setIssuanceAllocator(address _issuanceAllocator) external pure override {} + function getIssuanceAllocator() external pure override returns (IIssuanceAllocationDistribution) { + return IIssuanceAllocationDistribution(address(0)); + } + + /// @inheritdoc IIssuanceTarget + function setIssuanceAllocator(IIssuanceAllocationDistribution _issuanceAllocator) external pure override {} /// @inheritdoc ERC165 function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) { diff --git a/packages/issuance/contracts/test/allocate/MockReentrantTarget.sol b/packages/issuance/contracts/test/allocate/MockReentrantTarget.sol index 484648805..ffa4e5aae 100644 --- a/packages/issuance/contracts/test/allocate/MockReentrantTarget.sol +++ b/packages/issuance/contracts/test/allocate/MockReentrantTarget.sol @@ -85,8 +85,13 @@ contract MockReentrantTarget is IIssuanceTarget, ERC165 { } /// @inheritdoc IIssuanceTarget - function setIssuanceAllocator(address _issuanceAllocator) external override { - issuanceAllocator = _issuanceAllocator; + function getIssuanceAllocator() external view override returns (IIssuanceAllocationDistribution) { + return IIssuanceAllocationDistribution(issuanceAllocator); + } + + /// @inheritdoc IIssuanceTarget + function setIssuanceAllocator(IIssuanceAllocationDistribution _issuanceAllocator) external override { + issuanceAllocator = address(_issuanceAllocator); } /// @inheritdoc ERC165 diff --git a/packages/issuance/contracts/test/allocate/MockRevertingTarget.sol b/packages/issuance/contracts/test/allocate/MockRevertingTarget.sol index 27522e5a4..eb0ec1734 100644 --- a/packages/issuance/contracts/test/allocate/MockRevertingTarget.sol +++ b/packages/issuance/contracts/test/allocate/MockRevertingTarget.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.24; +import { IIssuanceAllocationDistribution } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceAllocationDistribution.sol"; import { IIssuanceTarget } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceTarget.sol"; import { ERC165 } from "@openzeppelin/contracts/utils/introspection/ERC165.sol"; @@ -23,7 +24,14 @@ contract MockRevertingTarget is IIssuanceTarget, ERC165 { /** * @inheritdoc IIssuanceTarget */ - function setIssuanceAllocator(address _issuanceAllocator) external pure override { + function getIssuanceAllocator() external pure override returns (IIssuanceAllocationDistribution) { + return IIssuanceAllocationDistribution(address(0)); + } + + /** + * @inheritdoc IIssuanceTarget + */ + function setIssuanceAllocator(IIssuanceAllocationDistribution _issuanceAllocator) external pure override { // No-op } diff --git a/packages/issuance/contracts/test/allocate/MockSimpleTarget.sol b/packages/issuance/contracts/test/allocate/MockSimpleTarget.sol index 311e1f03c..fddaed78b 100644 --- a/packages/issuance/contracts/test/allocate/MockSimpleTarget.sol +++ b/packages/issuance/contracts/test/allocate/MockSimpleTarget.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.24; +import { IIssuanceAllocationDistribution } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceAllocationDistribution.sol"; import { IIssuanceTarget } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceTarget.sol"; import { ERC165 } from "@openzeppelin/contracts/utils/introspection/ERC165.sol"; @@ -15,7 +16,12 @@ contract MockSimpleTarget is IIssuanceTarget, ERC165 { function beforeIssuanceAllocationChange() external pure override {} /// @inheritdoc IIssuanceTarget - function setIssuanceAllocator(address _issuanceAllocator) external pure override {} + function getIssuanceAllocator() external pure override returns (IIssuanceAllocationDistribution) { + return IIssuanceAllocationDistribution(address(0)); + } + + /// @inheritdoc IIssuanceTarget + function setIssuanceAllocator(IIssuanceAllocationDistribution _issuanceAllocator) external pure override {} /// @inheritdoc ERC165 function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) { diff --git a/packages/issuance/test/unit/agreement-manager/approver.t.sol b/packages/issuance/test/unit/agreement-manager/approver.t.sol index f38db6a7c..488b74729 100644 --- a/packages/issuance/test/unit/agreement-manager/approver.t.sol +++ b/packages/issuance/test/unit/agreement-manager/approver.t.sol @@ -8,6 +8,7 @@ import { IRecurringEscrowManagement } from "@graphprotocol/interfaces/contracts/ import { IProviderEligibilityManagement } from "@graphprotocol/interfaces/contracts/issuance/eligibility/IProviderEligibilityManagement.sol"; import { IRecurringAgreements } from "@graphprotocol/interfaces/contracts/issuance/agreement/IRecurringAgreements.sol"; import { IIssuanceTarget } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceTarget.sol"; +import { IIssuanceAllocationDistribution } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceAllocationDistribution.sol"; import { IAgreementCollector, OFFER_TYPE_NEW @@ -57,13 +58,13 @@ contract RecurringAgreementManagerApproverTest is RecurringAgreementManagerShare MockIssuanceAllocator alloc = new MockIssuanceAllocator(token, address(agreementManager)); vm.expectRevert(); vm.prank(nonGovernor); - agreementManager.setIssuanceAllocator(address(alloc)); + agreementManager.setIssuanceAllocator(IIssuanceAllocationDistribution(address(alloc))); } function test_SetIssuanceAllocator_Governor() public { MockIssuanceAllocator alloc = new MockIssuanceAllocator(token, address(agreementManager)); vm.prank(governor); - agreementManager.setIssuanceAllocator(address(alloc)); + agreementManager.setIssuanceAllocator(IIssuanceAllocationDistribution(address(alloc))); } // -- View Function Tests -- diff --git a/packages/issuance/test/unit/agreement-manager/branchCoverage.t.sol b/packages/issuance/test/unit/agreement-manager/branchCoverage.t.sol index 2b7db27a4..458e76347 100644 --- a/packages/issuance/test/unit/agreement-manager/branchCoverage.t.sol +++ b/packages/issuance/test/unit/agreement-manager/branchCoverage.t.sol @@ -7,6 +7,7 @@ import { IAgreementCollector } from "@graphprotocol/interfaces/contracts/horizon import { IRecurringCollector } from "@graphprotocol/interfaces/contracts/horizon/IRecurringCollector.sol"; import { IAgreementCollector } from "@graphprotocol/interfaces/contracts/horizon/IAgreementCollector.sol"; import { IRecurringAgreementManagement } from "@graphprotocol/interfaces/contracts/issuance/agreement/IRecurringAgreementManagement.sol"; +import { IIssuanceAllocationDistribution } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceAllocationDistribution.sol"; import { IAgreementCollector } from "@graphprotocol/interfaces/contracts/horizon/IAgreementCollector.sol"; import { IAgreementCollector } from "@graphprotocol/interfaces/contracts/horizon/IAgreementCollector.sol"; @@ -15,6 +16,7 @@ import { IAgreementCollector } from "@graphprotocol/interfaces/contracts/horizon import { RecurringAgreementManagerSharedTest } from "./shared.t.sol"; import { IAgreementCollector } from "@graphprotocol/interfaces/contracts/horizon/IAgreementCollector.sol"; import { MockRecurringCollector } from "./mocks/MockRecurringCollector.sol"; +import { MockIssuanceAllocator } from "./mocks/MockIssuanceAllocator.sol"; /// @notice Targeted tests for uncovered branches in RecurringAgreementManager. contract RecurringAgreementManagerBranchCoverageTest is RecurringAgreementManagerSharedTest { @@ -36,7 +38,7 @@ contract RecurringAgreementManagerBranchCoverageTest is RecurringAgreementManage address(recurringCollector) ) ); - agreementManager.setIssuanceAllocator(address(recurringCollector)); + agreementManager.setIssuanceAllocator(IIssuanceAllocationDistribution(address(recurringCollector))); } /// @notice Setting allocator to an EOA (no code) also fails ERC165 check. @@ -44,7 +46,7 @@ contract RecurringAgreementManagerBranchCoverageTest is RecurringAgreementManage address eoa = makeAddr("randomEOA"); vm.prank(governor); vm.expectRevert(abi.encodeWithSelector(RecurringAgreementManager.InvalidIssuanceAllocator.selector, eoa)); - agreementManager.setIssuanceAllocator(eoa); + agreementManager.setIssuanceAllocator(IIssuanceAllocationDistribution(eoa)); } // ══════════════════════════════════════════════════════════════════════ @@ -219,6 +221,52 @@ contract RecurringAgreementManagerBranchCoverageTest is RecurringAgreementManage // _withdrawAndRebalance — deposit deficit branch (L854/857–862) // ══════════════════════════════════════════════════════════════════════ + // ══════════════════════════════════════════════════════════════════════ + // getIssuanceAllocator — view getter (L281-282) + // ══════════════════════════════════════════════════════════════════════ + + /// @notice getIssuanceAllocator returns the configured allocator and the + /// zero default prior to setIssuanceAllocator. + function test_GetIssuanceAllocator_ReturnsConfiguredValue() public { + assertEq(address(agreementManager.getIssuanceAllocator()), address(0), "Default allocator must be zero"); + + MockIssuanceAllocator allocator = new MockIssuanceAllocator(token, address(agreementManager)); + vm.prank(governor); + agreementManager.setIssuanceAllocator(allocator); + + assertEq( + address(agreementManager.getIssuanceAllocator()), + address(allocator), + "Configured allocator must be returned" + ); + } + + // ══════════════════════════════════════════════════════════════════════ + // offerAgreement — collector returns zero agreementId (L361) + // ══════════════════════════════════════════════════════════════════════ + + /// @notice A conformant collector must return a non-zero agreementId; RAM + /// enforces this invariant with AgreementIdZero. + function test_OfferAgreement_Revert_AgreementIdZero() public { + ZeroIdCollector rogue = new ZeroIdCollector(dataService, address(agreementManager), indexer); + vm.prank(governor); + agreementManager.grantRole(COLLECTOR_ROLE, address(rogue)); + + // Payload content is irrelevant — the mock returns a zero agreementId unconditionally. + IRecurringCollector.RecurringCollectionAgreement memory rca = _makeRCA( + 100 ether, + 1 ether, + 60, + 3600, + uint64(block.timestamp + 365 days) + ); + + token.mint(address(agreementManager), 1_000_000 ether); + vm.prank(operator); + vm.expectRevert(IRecurringAgreementManagement.AgreementIdZero.selector); + agreementManager.offerAgreement(IAgreementCollector(address(rogue)), OFFER_TYPE_NEW, abi.encode(rca)); + } + /// @notice When escrow balance drops below min (after collection), reconcile deposits the deficit. function test_WithdrawAndRebalance_DepositDeficit() public { // Offer agreement in Full mode — escrow gets fully funded @@ -268,3 +316,28 @@ contract RecurringAgreementManagerBranchCoverageTest is RecurringAgreementManage /* solhint-enable graph/func-name-mixedcase */ } + +/// @notice Minimal collector stub that returns a zero agreementId with valid +/// payer/dataService/serviceProvider, used to exercise RAM's AgreementIdZero guard. +contract ZeroIdCollector { + address private immutable _dataService; + address private immutable _payer; + address private immutable _serviceProvider; + + constructor(address dataService_, address payer_, address serviceProvider_) { + _dataService = dataService_; + _payer = payer_; + _serviceProvider = serviceProvider_; + } + + function offer( + uint8 /* offerType */, + bytes calldata /* data */, + uint16 /* options */ + ) external view returns (IAgreementCollector.AgreementDetails memory details) { + details.agreementId = bytes16(0); + details.payer = _payer; + details.dataService = _dataService; + details.serviceProvider = _serviceProvider; + } +} diff --git a/packages/issuance/test/unit/agreement-manager/callbackGas.t.sol b/packages/issuance/test/unit/agreement-manager/callbackGas.t.sol index e4870924f..efe2abce6 100644 --- a/packages/issuance/test/unit/agreement-manager/callbackGas.t.sol +++ b/packages/issuance/test/unit/agreement-manager/callbackGas.t.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.27; import { IRecurringCollector } from "@graphprotocol/interfaces/contracts/horizon/IRecurringCollector.sol"; +import { IIssuanceAllocationDistribution } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceAllocationDistribution.sol"; import { RecurringAgreementManagerSharedTest } from "./shared.t.sol"; import { MockIssuanceAllocator } from "./mocks/MockIssuanceAllocator.sol"; @@ -36,7 +37,7 @@ contract RecurringAgreementManagerCallbackGasTest is RecurringAgreementManagerSh vm.label(address(mockAllocator), "MockIssuanceAllocator"); vm.prank(governor); - agreementManager.setIssuanceAllocator(address(mockAllocator)); + agreementManager.setIssuanceAllocator(IIssuanceAllocationDistribution(address(mockAllocator))); } // ==================== beforeCollection gas ==================== diff --git a/packages/issuance/test/unit/agreement-manager/cascadeCleanup.t.sol b/packages/issuance/test/unit/agreement-manager/cascadeCleanup.t.sol index eeffa61e1..b9d058c6c 100644 --- a/packages/issuance/test/unit/agreement-manager/cascadeCleanup.t.sol +++ b/packages/issuance/test/unit/agreement-manager/cascadeCleanup.t.sol @@ -193,6 +193,12 @@ contract RecurringAgreementManagerCascadeCleanupTest is RecurringAgreementManage assertEq(agreementManager.getCollectorCount(), 0); assertEq(agreementManager.getProviderCount(IAgreementCollector(address(recurringCollector))), 0); + // Storage fully released: escrowSnap cleared when sumMaxNextClaim reached 0 + assertEq( + agreementManager.getEscrowSnap(IAgreementCollector(address(recurringCollector)), indexer), + 0, + "escrowSnap should be cleared after pair drop" + ); } function test_Cascade_ReconcileLastProvider_CollectorCleanedUp_OtherCollectorRemains() public { @@ -271,6 +277,11 @@ contract RecurringAgreementManagerCascadeCleanupTest is RecurringAgreementManage vm.warp(block.timestamp + paymentsEscrow.THAWING_PERIOD() + 1); agreementManager.reconcileProvider(IAgreementCollector(address(recurringCollector)), indexer); assertEq(agreementManager.getCollectorCount(), 0); + assertEq( + agreementManager.getEscrowSnap(IAgreementCollector(address(recurringCollector)), indexer), + 0, + "escrowSnap clean before re-add" + ); // Re-add — sets repopulate (IRecurringCollector.RecurringCollectionAgreement memory rca2, ) = _makeRCAForCollector(recurringCollector, 2); diff --git a/packages/issuance/test/unit/agreement-manager/ensureDistributed.t.sol b/packages/issuance/test/unit/agreement-manager/ensureDistributed.t.sol index d84782d37..ec9542977 100644 --- a/packages/issuance/test/unit/agreement-manager/ensureDistributed.t.sol +++ b/packages/issuance/test/unit/agreement-manager/ensureDistributed.t.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.27; import { Vm } from "forge-std/Vm.sol"; import { IIssuanceTarget } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceTarget.sol"; +import { IIssuanceAllocationDistribution } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceAllocationDistribution.sol"; import { IRecurringCollector } from "@graphprotocol/interfaces/contracts/horizon/IRecurringCollector.sol"; import { RecurringAgreementManager } from "contracts/agreement/RecurringAgreementManager.sol"; @@ -23,7 +24,7 @@ contract RecurringAgreementManagerEnsureDistributedTest is RecurringAgreementMan vm.label(address(mockAllocator), "MockIssuanceAllocator"); vm.prank(governor); - agreementManager.setIssuanceAllocator(address(mockAllocator)); + agreementManager.setIssuanceAllocator(IIssuanceAllocationDistribution(address(mockAllocator))); } // ==================== setIssuanceAllocator ==================== @@ -33,26 +34,26 @@ contract RecurringAgreementManagerEnsureDistributedTest is RecurringAgreementMan vm.prank(governor); vm.expectEmit(address(agreementManager)); - emit IIssuanceTarget.IssuanceAllocatorSet(address(mockAllocator), address(newAllocator)); - agreementManager.setIssuanceAllocator(address(newAllocator)); + emit IIssuanceTarget.IssuanceAllocatorSet(IIssuanceAllocationDistribution(address(mockAllocator)), IIssuanceAllocationDistribution(address(newAllocator))); + agreementManager.setIssuanceAllocator(IIssuanceAllocationDistribution(address(newAllocator))); } function test_SetIssuanceAllocator_Revert_WhenNotGovernor() public { vm.prank(operator); vm.expectRevert(); - agreementManager.setIssuanceAllocator(address(mockAllocator)); + agreementManager.setIssuanceAllocator(IIssuanceAllocationDistribution(address(mockAllocator))); } function test_SetIssuanceAllocator_CanSetToZero() public { vm.prank(governor); - agreementManager.setIssuanceAllocator(address(0)); + agreementManager.setIssuanceAllocator(IIssuanceAllocationDistribution(address(0))); // Should not revert — _ensureIncomingDistributionToCurrentBlock is a no-op with zero address } function test_SetIssuanceAllocator_NoopWhenUnchanged() public { vm.prank(governor); vm.recordLogs(); - agreementManager.setIssuanceAllocator(address(mockAllocator)); + agreementManager.setIssuanceAllocator(IIssuanceAllocationDistribution(address(mockAllocator))); Vm.Log[] memory logs = vm.getRecordedLogs(); assertEq(logs.length, 0, "should not emit when address unchanged"); } @@ -201,7 +202,7 @@ contract RecurringAgreementManagerEnsureDistributedTest is RecurringAgreementMan function test_EnsureDistributed_NoopWhenAllocatorNotSet() public { // Clear allocator vm.prank(governor); - agreementManager.setIssuanceAllocator(address(0)); + agreementManager.setIssuanceAllocator(IIssuanceAllocationDistribution(address(0))); (IRecurringCollector.RecurringCollectionAgreement memory rca, ) = _makeRCAWithId( 100 ether, @@ -309,14 +310,14 @@ contract RecurringAgreementManagerEnsureDistributedTest is RecurringAgreementMan vm.expectRevert( abi.encodeWithSelector(RecurringAgreementManager.InvalidIssuanceAllocator.selector, notAllocator) ); - agreementManager.setIssuanceAllocator(notAllocator); + agreementManager.setIssuanceAllocator(IIssuanceAllocationDistribution(notAllocator)); } function test_SetIssuanceAllocator_Revert_WhenEOA() public { address eoa = makeAddr("eoa"); vm.prank(governor); vm.expectRevert(abi.encodeWithSelector(RecurringAgreementManager.InvalidIssuanceAllocator.selector, eoa)); - agreementManager.setIssuanceAllocator(eoa); + agreementManager.setIssuanceAllocator(IIssuanceAllocationDistribution(eoa)); } // ==================== setIssuanceAllocator switches allocator ==================== @@ -334,7 +335,7 @@ contract RecurringAgreementManagerEnsureDistributedTest is RecurringAgreementMan // Switch allocator MockIssuanceAllocator newAllocator = new MockIssuanceAllocator(token, address(agreementManager)); vm.prank(governor); - agreementManager.setIssuanceAllocator(address(newAllocator)); + agreementManager.setIssuanceAllocator(IIssuanceAllocationDistribution(address(newAllocator))); // Next block: new allocator should be called via _updateEscrow vm.roll(block.number + 1); diff --git a/packages/issuance/test/unit/agreement-manager/residualEscrow.t.sol b/packages/issuance/test/unit/agreement-manager/residualEscrow.t.sol new file mode 100644 index 000000000..c96003e67 --- /dev/null +++ b/packages/issuance/test/unit/agreement-manager/residualEscrow.t.sol @@ -0,0 +1,280 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.27; + +import { IAgreementCollector } from "@graphprotocol/interfaces/contracts/horizon/IAgreementCollector.sol"; +import { IPaymentsEscrow } from "@graphprotocol/interfaces/contracts/horizon/IPaymentsEscrow.sol"; +import { IRecurringCollector } from "@graphprotocol/interfaces/contracts/horizon/IRecurringCollector.sol"; +import { IRecurringAgreementManagement } from "@graphprotocol/interfaces/contracts/issuance/agreement/IRecurringAgreementManagement.sol"; +import { IRecurringEscrowManagement } from "@graphprotocol/interfaces/contracts/issuance/agreement/IRecurringEscrowManagement.sol"; + +import { RecurringAgreementManagerSharedTest } from "./shared.t.sol"; + +/// @notice Tests for minResidualEscrowFactor — residual escrow threshold for pair cleanup. +contract RecurringAgreementManagerResidualEscrowTest is RecurringAgreementManagerSharedTest { + /* solhint-disable graph/func-name-mixedcase */ + + // -- Helpers -- + + /// @notice Create an agreement, cancel it, and advance past the thaw period so escrow is withdrawable. + function _createAndCancelAgreement() + private + returns (bytes16 agreementId, IRecurringCollector.RecurringCollectionAgreement memory rca) + { + (rca, ) = _makeRCAWithId(100 ether, 1 ether, 3600, uint64(block.timestamp + 365 days)); + agreementId = _offerAgreement(rca); + + _setAgreementCanceledBySP(agreementId, rca); + agreementManager.reconcileAgreement(IAgreementCollector(address(recurringCollector)), agreementId); + } + + /// @notice Inject dust directly into escrow (simulates external depositTo by attacker). + function _injectDust(uint256 amount) private { + (uint256 bal, uint256 thawing, uint256 thawEnd) = paymentsEscrow.escrowAccounts( + address(agreementManager), + address(recurringCollector), + indexer + ); + // Mint backing tokens to the escrow so withdraw can transfer them + token.mint(address(paymentsEscrow), amount); + paymentsEscrow.setAccount( + address(agreementManager), + address(recurringCollector), + indexer, + bal + amount, + thawing, + thawEnd + ); + } + + // -- Tests: residual threshold drops tracking -- + + function test_ResidualEscrow_DropsTrackingBelowThreshold() public { + // Default factor = 50, threshold = 2^50 ≈ 1.1e15 + _createAndCancelAgreement(); + + // Advance past thaw period so escrow can be withdrawn + vm.warp(block.timestamp + 1 days + 1); + + // reconcileProvider: withdraws full balance, dust is zero, pair is dropped + bool tracked = agreementManager.reconcileProvider(IAgreementCollector(address(recurringCollector)), indexer); + assertFalse(tracked, "pair should be dropped when escrow is zero"); + assertEq( + agreementManager.getProviderCount(IAgreementCollector(address(recurringCollector))), + 0, + "provider should be removed from set" + ); + assertEq(agreementManager.getCollectorCount(), 0, "collector should be removed from set"); + } + + function test_ResidualEscrow_KeepsTrackingAboveThreshold() public { + _createAndCancelAgreement(); + + // Inject balance well above threshold (2^50 ≈ 1.1e15) + vm.warp(block.timestamp + 1 days + 1); + _injectDust(1 ether); + + bool tracked = agreementManager.reconcileProvider(IAgreementCollector(address(recurringCollector)), indexer); + assertTrue(tracked, "pair should remain tracked when escrow exceeds threshold"); + } + + function test_ResidualEscrow_DustGriefingDropsTracking() public { + _createAndCancelAgreement(); + + // Advance past thaw, then inject 1 wei (simulates attacker depositTo) + vm.warp(block.timestamp + 1 days + 1); + _injectDust(1); + + // reconcileProvider: withdraws matured thaw, 1 wei remains, + // 1 wei < 2^50 threshold → pair is dropped + bool tracked = agreementManager.reconcileProvider(IAgreementCollector(address(recurringCollector)), indexer); + assertFalse(tracked, "dust should not prevent cleanup"); + } + + // -- Tests: blind drain for untracked pairs -- + + function test_ResidualEscrow_BlindDrainUntrackedPair() public { + _createAndCancelAgreement(); + + // Drop tracking first + vm.warp(block.timestamp + 1 days + 1); + agreementManager.reconcileProvider(IAgreementCollector(address(recurringCollector)), indexer); + assertEq(agreementManager.getProviderCount(IAgreementCollector(address(recurringCollector))), 0); + + // Inject dust into the now-untracked escrow + _injectDust(100); + + // reconcileProvider on untracked pair: blind drain starts thaw + bool tracked = agreementManager.reconcileProvider(IAgreementCollector(address(recurringCollector)), indexer); + assertFalse(tracked, "untracked pair should stay untracked"); + + // Escrow should now be thawing + (uint256 bal, uint256 thawing, ) = paymentsEscrow.escrowAccounts( + address(agreementManager), + address(recurringCollector), + indexer + ); + assertEq(thawing, bal, "full balance should be thawing"); + } + + function test_ResidualEscrow_BlindDrainWithdrawsMaturedThaw() public { + _createAndCancelAgreement(); + + // Drop tracking + vm.warp(block.timestamp + 1 days + 1); + agreementManager.reconcileProvider(IAgreementCollector(address(recurringCollector)), indexer); + + // Inject dust, start thaw via blind drain + _injectDust(100); + agreementManager.reconcileProvider(IAgreementCollector(address(recurringCollector)), indexer); + + // Read the thaw end timestamp and advance past it + (, , uint256 thawEnd) = paymentsEscrow.escrowAccounts( + address(agreementManager), + address(recurringCollector), + indexer + ); + vm.warp(thawEnd + 1); + + uint256 balBefore = token.balanceOf(address(agreementManager)); + agreementManager.reconcileProvider(IAgreementCollector(address(recurringCollector)), indexer); + uint256 balAfter = token.balanceOf(address(agreementManager)); + + assertEq(balAfter - balBefore, 100, "dust should be withdrawn to agreement manager"); + } + + function test_ResidualEscrow_BlindDrainNoopMidThaw() public { + _createAndCancelAgreement(); + + // Drop tracking + vm.warp(block.timestamp + 1 days + 1); + agreementManager.reconcileProvider(IAgreementCollector(address(recurringCollector)), indexer); + + // Inject dust, start thaw + _injectDust(100); + agreementManager.reconcileProvider(IAgreementCollector(address(recurringCollector)), indexer); + + // Inject more dust mid-thaw — blind drain should NOT reset the timer + _injectDust(50); + + (, , uint256 thawEndBefore) = paymentsEscrow.escrowAccounts( + address(agreementManager), + address(recurringCollector), + indexer + ); + + agreementManager.reconcileProvider(IAgreementCollector(address(recurringCollector)), indexer); + + (, uint256 thawingAfter, uint256 thawEndAfter) = paymentsEscrow.escrowAccounts( + address(agreementManager), + address(recurringCollector), + indexer + ); + + // Timer should not have reset (evenIfTimerReset=false) + assertEq(thawEndAfter, thawEndBefore, "thaw timer should not reset on blind drain mid-thaw"); + // Only the original 100 should be thawing, not 150 + assertEq(thawingAfter, 100, "thaw amount should not increase mid-thaw"); + } + + // -- Tests: re-entry after drop restores tracking -- + + function test_ResidualEscrow_ReentryRestoresTracking() public { + _createAndCancelAgreement(); + + // Drop tracking + vm.warp(block.timestamp + 1 days + 1); + agreementManager.reconcileProvider(IAgreementCollector(address(recurringCollector)), indexer); + assertEq(agreementManager.getCollectorCount(), 0, "collector should be removed"); + + // New agreement for the same (collector, provider) pair + IRecurringCollector.RecurringCollectionAgreement memory rca2 = _makeRCA( + 50 ether, + 0.5 ether, + 60, + 3600, + uint64(block.timestamp + 365 days) + ); + rca2.nonce = 2; + _offerAgreement(rca2); + + // Tracking should be restored + assertEq( + agreementManager.getProviderCount(IAgreementCollector(address(recurringCollector))), + 1, + "provider should be re-tracked" + ); + assertEq(agreementManager.getCollectorCount(), 1, "collector should be re-tracked"); + } + + function test_ResidualEscrow_ReentryWithStaleSnapCorrects() public { + _createAndCancelAgreement(); + + // Inject extra balance, then drop tracking — snap records the inflated balance + _injectDust(500); + vm.warp(block.timestamp + 1 days + 1); + agreementManager.reconcileProvider(IAgreementCollector(address(recurringCollector)), indexer); + + // Escrow still has some balance (the dust that was below threshold or leftover) + // Now create new agreement — snap should be corrected from real balance + IRecurringCollector.RecurringCollectionAgreement memory rca2 = _makeRCA( + 50 ether, + 0.5 ether, + 60, + 3600, + uint64(block.timestamp + 365 days) + ); + rca2.nonce = 2; + _offerAgreement(rca2); + + // The system should work normally — no stale snap causing issues + // Verify escrow is funded correctly for the new agreement + (uint256 bal, , ) = paymentsEscrow.escrowAccounts( + address(agreementManager), + address(recurringCollector), + indexer + ); + uint256 expectedMaxClaim = 0.5 ether * 3600 + 50 ether; + assertEq(bal, expectedMaxClaim, "escrow should be funded for new agreement (snap corrected)"); + } + + // -- Tests: setter -- + + function test_ResidualEscrow_SetFactor() public { + assertEq(agreementManager.getMinResidualEscrowFactor(), 50, "default should be 50"); + + vm.prank(operator); + agreementManager.setMinResidualEscrowFactor(60); + assertEq(agreementManager.getMinResidualEscrowFactor(), 60); + } + + function test_ResidualEscrow_SetFactor_SameValueNoop() public { + vm.prank(operator); + // Should not emit event + vm.recordLogs(); + agreementManager.setMinResidualEscrowFactor(50); + assertEq(vm.getRecordedLogs().length, 0, "no event on same value"); + } + + function test_ResidualEscrow_SetFactor_EmitsEvent() public { + vm.expectEmit(address(agreementManager)); + emit IRecurringEscrowManagement.MinResidualEscrowFactorSet(50, 100); + + vm.prank(operator); + agreementManager.setMinResidualEscrowFactor(100); + } + + function test_ResidualEscrow_SetFactor_ZeroDisables() public { + _createAndCancelAgreement(); + + vm.prank(operator); + agreementManager.setMinResidualEscrowFactor(0); + + // With factor=0, threshold = 2^0 = 1, only drops at zero balance + // Inject 1 wei — should keep tracking + vm.warp(block.timestamp + 1 days + 1); + _injectDust(1); + + bool tracked = agreementManager.reconcileProvider(IAgreementCollector(address(recurringCollector)), indexer); + assertTrue(tracked, "factor=0 means threshold=1, 1 wei should keep tracking"); + } +} diff --git a/packages/issuance/test/unit/allocator/distribution.t.sol b/packages/issuance/test/unit/allocator/distribution.t.sol index fb94737de..196317dcf 100644 --- a/packages/issuance/test/unit/allocator/distribution.t.sol +++ b/packages/issuance/test/unit/allocator/distribution.t.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.27; import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; import { IIssuanceTarget } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceTarget.sol"; +import { IIssuanceAllocationDistribution } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceAllocationDistribution.sol"; import { TargetIssuancePerBlock, DistributionState, @@ -487,7 +488,7 @@ contract IssuanceAllocatorDistributionTest is IssuanceAllocatorSharedTest { _setIssuanceRate(ISSUANCE_PER_BLOCK); // Set up reentrant target - reentrantTarget.setIssuanceAllocator(address(allocator)); + reentrantTarget.setIssuanceAllocator(IIssuanceAllocationDistribution(address(allocator))); reentrantTarget.setReentrantAction(MockReentrantTarget.ReentrantAction.SetTargetAllocation1Param); // Adding the target should fail due to reentrancy in notification callback diff --git a/packages/issuance/test/unit/allocator/interfaceIdStability.t.sol b/packages/issuance/test/unit/allocator/interfaceIdStability.t.sol index 463416bbd..aee42df80 100644 --- a/packages/issuance/test/unit/allocator/interfaceIdStability.t.sol +++ b/packages/issuance/test/unit/allocator/interfaceIdStability.t.sol @@ -40,7 +40,7 @@ contract AllocateInterfaceIdStabilityTest is Test { // -- DirectAllocation / shared interfaces -- function test_InterfaceId_IIssuanceTarget() public pure { - assertEq(type(IIssuanceTarget).interfaceId, bytes4(0xaee4dc43)); + assertEq(type(IIssuanceTarget).interfaceId, bytes4(0x19f6601a)); } function test_InterfaceId_ISendTokens() public pure { diff --git a/packages/issuance/test/unit/direct-allocation/DirectAllocation.t.sol b/packages/issuance/test/unit/direct-allocation/DirectAllocation.t.sol index 112126a38..d76204091 100644 --- a/packages/issuance/test/unit/direct-allocation/DirectAllocation.t.sol +++ b/packages/issuance/test/unit/direct-allocation/DirectAllocation.t.sol @@ -8,6 +8,7 @@ import { IAccessControl } from "@openzeppelin/contracts/access/IAccessControl.so import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; import { TransparentUpgradeableProxy } from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol"; +import { IIssuanceAllocationDistribution } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceAllocationDistribution.sol"; import { IIssuanceTarget } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceTarget.sol"; import { ISendTokens } from "@graphprotocol/interfaces/contracts/issuance/allocate/ISendTokens.sol"; @@ -15,6 +16,26 @@ import { BaseUpgradeable } from "../../../contracts/common/BaseUpgradeable.sol"; import { IGraphToken } from "../../../contracts/common/IGraphToken.sol"; import { DirectAllocation } from "../../../contracts/allocate/DirectAllocation.sol"; import { MockGraphToken } from "../mocks/MockGraphToken.sol"; +import { TargetIssuancePerBlock } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceAllocatorTypes.sol"; + +/// @notice Minimal IIssuanceAllocationDistribution stub that advertises the interface via ERC-165. +/// Used to exercise DirectAllocation's ERC-165 acceptance path without pulling in heavier +/// allocator mocks from other test trees. +contract StubIssuanceAllocator is IIssuanceAllocationDistribution, IERC165 { + function distributeIssuance() external pure override returns (uint256) { + return 0; + } + + function getTargetIssuancePerBlock(address) external pure override returns (TargetIssuancePerBlock memory) { + return TargetIssuancePerBlock(0, 0, 0, 0); + } + + function supportsInterface(bytes4 interfaceId) external pure override returns (bool) { + return + interfaceId == type(IIssuanceAllocationDistribution).interfaceId || + interfaceId == type(IERC165).interfaceId; + } +} /// @notice Tests for DirectAllocation contract. contract DirectAllocationTest is Test { @@ -133,15 +154,81 @@ contract DirectAllocationTest is Test { directAlloc.beforeIssuanceAllocationChange(); } - function test_SetIssuanceAllocator_NoOp() public { + function test_GetIssuanceAllocator_InitiallyZero() public view { + assertEq(address(directAlloc.getIssuanceAllocator()), address(0)); + } + + function test_SetIssuanceAllocator_UpdatesGetter() public { + StubIssuanceAllocator allocator = new StubIssuanceAllocator(); + vm.prank(governor); + directAlloc.setIssuanceAllocator(allocator); + assertEq(address(directAlloc.getIssuanceAllocator()), address(allocator)); + } + + function test_SetIssuanceAllocator_EmitsEvent() public { + StubIssuanceAllocator allocator = new StubIssuanceAllocator(); + vm.prank(governor); + vm.expectEmit(address(directAlloc)); + emit IIssuanceTarget.IssuanceAllocatorSet(IIssuanceAllocationDistribution(address(0)), allocator); + directAlloc.setIssuanceAllocator(allocator); + } + + function test_SetIssuanceAllocator_EmitsEventWithOldValue() public { + StubIssuanceAllocator first = new StubIssuanceAllocator(); + StubIssuanceAllocator second = new StubIssuanceAllocator(); + vm.prank(governor); + directAlloc.setIssuanceAllocator(first); + + vm.prank(governor); + vm.expectEmit(address(directAlloc)); + emit IIssuanceTarget.IssuanceAllocatorSet(first, second); + directAlloc.setIssuanceAllocator(second); + } + + function test_SetIssuanceAllocator_SkipsWhenSameValue() public { + StubIssuanceAllocator allocator = new StubIssuanceAllocator(); + vm.prank(governor); + directAlloc.setIssuanceAllocator(allocator); + + vm.prank(governor); + vm.recordLogs(); + directAlloc.setIssuanceAllocator(allocator); + assertEq(vm.getRecordedLogs().length, 0); + } + + function test_SetIssuanceAllocator_AllowsZeroAddress() public { + // Zero-address bypasses the ERC165 check — clearing the allocator is always legal. + StubIssuanceAllocator allocator = new StubIssuanceAllocator(); + vm.prank(governor); + directAlloc.setIssuanceAllocator(allocator); + + vm.prank(governor); + directAlloc.setIssuanceAllocator(IIssuanceAllocationDistribution(address(0))); + assertEq(address(directAlloc.getIssuanceAllocator()), address(0)); + } + + /// @notice An EOA (no code) fails the ERC-165 interface probe and must be rejected. Prevents + /// governance from accidentally wiring up a non-contract as the allocator. + function test_Revert_SetIssuanceAllocator_WhenEOA() public { + address eoa = makeAddr("eoa"); + vm.prank(governor); + vm.expectRevert(abi.encodeWithSelector(DirectAllocation.InvalidIssuanceAllocator.selector, eoa)); + directAlloc.setIssuanceAllocator(IIssuanceAllocationDistribution(eoa)); + } + + /// @notice A contract that does not implement IIssuanceAllocationDistribution must be rejected. + /// Uses the MockGraphToken fixture — it has code but doesn't advertise the allocator interface. + function test_Revert_SetIssuanceAllocator_WhenWrongInterface() public { vm.prank(governor); - directAlloc.setIssuanceAllocator(makeAddr("allocator")); + vm.expectRevert(abi.encodeWithSelector(DirectAllocation.InvalidIssuanceAllocator.selector, address(token))); + directAlloc.setIssuanceAllocator(IIssuanceAllocationDistribution(address(token))); } function test_Revert_SetIssuanceAllocator_NonGovernor() public { + StubIssuanceAllocator allocator = new StubIssuanceAllocator(); vm.expectRevert(); vm.prank(unauthorized); - directAlloc.setIssuanceAllocator(makeAddr("allocator")); + directAlloc.setIssuanceAllocator(allocator); } // ==================== ERC-165 Interface Support ==================== diff --git a/packages/subgraph-service/contracts/libraries/IndexingAgreement.sol b/packages/subgraph-service/contracts/libraries/IndexingAgreement.sol index 1aa2b9677..8516334f4 100644 --- a/packages/subgraph-service/contracts/libraries/IndexingAgreement.sol +++ b/packages/subgraph-service/contracts/libraries/IndexingAgreement.sol @@ -218,12 +218,6 @@ library IndexingAgreement { bytes32 allocationDeploymentId ); - /** - * @notice Thrown when the agreement is already accepted - * @param agreementId The agreement ID - */ - error IndexingAgreementAlreadyAccepted(bytes16 agreementId); - /** * @notice Thrown when an allocation already has an active agreement * @param allocationId The allocation ID @@ -310,42 +304,48 @@ library IndexingAgreement { IIndexingAgreement.State storage agreement = self.agreements[agreementId]; - require(agreement.allocationId == address(0), IndexingAgreementAlreadyAccepted(agreementId)); - - require( - allocation.subgraphDeploymentId == metadata.subgraphDeploymentId, - IndexingAgreementDeploymentIdMismatch( - metadata.subgraphDeploymentId, + // Accept is idempotent for the same allocation, and supports moving + // the agreement to a different allocation. The collector's accept handles state + // validity (reverts if the agreement is cancelled, no-ops if already accepted). + if (agreement.allocationId != allocationId) { + require( + allocation.subgraphDeploymentId == metadata.subgraphDeploymentId, + IndexingAgreementDeploymentIdMismatch( + metadata.subgraphDeploymentId, + allocationId, + allocation.subgraphDeploymentId + ) + ); + + // Ensure that an allocation can only have one active indexing agreement + require( + self.allocationToActiveAgreementId[allocationId] == bytes16(0), + AllocationAlreadyHasIndexingAgreement(allocationId) + ); + + if (agreement.allocationId != address(0)) delete self.allocationToActiveAgreementId[agreement.allocationId]; + agreement.allocationId = allocationId; + + self.allocationToActiveAgreementId[allocationId] = agreementId; + + agreement.version = metadata.version; + + require( + metadata.version == IIndexingAgreement.IndexingAgreementVersion.V1, + IndexingAgreementInvalidVersion(metadata.version) + ); + _setTermsV1(self, agreementId, metadata.terms, rca.maxOngoingTokensPerSecond); + + emit IndexingAgreementAccepted( + rca.serviceProvider, + rca.payer, + agreementId, allocationId, - allocation.subgraphDeploymentId - ) - ); - - // Ensure that an allocation can only have one active indexing agreement - require( - self.allocationToActiveAgreementId[allocationId] == bytes16(0), - AllocationAlreadyHasIndexingAgreement(allocationId) - ); - self.allocationToActiveAgreementId[allocationId] = agreementId; - - agreement.version = metadata.version; - agreement.allocationId = allocationId; - - require( - metadata.version == IIndexingAgreement.IndexingAgreementVersion.V1, - IndexingAgreementInvalidVersion(metadata.version) - ); - _setTermsV1(self, agreementId, metadata.terms, rca.maxOngoingTokensPerSecond); - - emit IndexingAgreementAccepted( - rca.serviceProvider, - rca.payer, - agreementId, - allocationId, - metadata.subgraphDeploymentId, - metadata.version, - metadata.terms - ); + metadata.subgraphDeploymentId, + metadata.version, + metadata.terms + ); + } require( _directory().recurringCollector().accept(rca, authData) == agreementId, @@ -380,12 +380,18 @@ library IndexingAgreement { bytes calldata authData ) external { IIndexingAgreement.AgreementWrapper memory wrapper = _get(self, rcau.agreementId); - require(_isActive(wrapper), IndexingAgreementNotActive(rcau.agreementId)); + // SS gate: only checks that this is an SS-managed, tracked agreement. Collector is the + // state authority — it reverts if the agreement cannot actually accept an update. + require(_isValid(wrapper), IndexingAgreementNotActive(rcau.agreementId)); require( wrapper.collectorAgreement.serviceProvider == indexer, IndexingAgreementNotAuthorized(rcau.agreementId, indexer) ); + // Idempotent: this RCAU is already the active version — both SS terms and collector state + // are in sync because both are written together on the original update. + if (wrapper.collectorAgreement.activeTermsHash == _directory().recurringCollector().hashRCAU(rcau)) return; + UpdateIndexingAgreementMetadata memory metadata = IndexingAgreementDecoder.decodeRCAUMetadata(rcau.metadata); require( @@ -396,7 +402,7 @@ library IndexingAgreement { metadata.version == IIndexingAgreement.IndexingAgreementVersion.V1, IndexingAgreementInvalidVersion(metadata.version) ); - _setTermsV1(self, rcau.agreementId, metadata.terms, wrapper.collectorAgreement.maxOngoingTokensPerSecond); + _setTermsV1(self, rcau.agreementId, metadata.terms, rcau.maxOngoingTokensPerSecond); emit IndexingAgreementUpdated({ indexer: wrapper.collectorAgreement.serviceProvider, diff --git a/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/accept.t.sol b/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/accept.t.sol index 1d2e2b9fb..77be7c67d 100644 --- a/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/accept.t.sol +++ b/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/accept.t.sol @@ -5,6 +5,7 @@ import { PausableUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/P import { ProvisionManager } from "@graphprotocol/horizon/contracts/data-service/utilities/ProvisionManager.sol"; import { IRecurringCollector } from "@graphprotocol/interfaces/contracts/horizon/IRecurringCollector.sol"; import { IAllocation } from "@graphprotocol/interfaces/contracts/subgraph-service/internal/IAllocation.sol"; +import { IIndexingAgreement } from "@graphprotocol/interfaces/contracts/subgraph-service/internal/IIndexingAgreement.sol"; import { IndexingAgreement } from "../../../../contracts/libraries/IndexingAgreement.sol"; import { IndexingAgreementDecoder } from "../../../../contracts/libraries/IndexingAgreementDecoder.sol"; @@ -231,7 +232,9 @@ contract SubgraphServiceIndexingAgreementAcceptTest is SubgraphServiceIndexingAg subgraphService.acceptIndexingAgreement(indexerState.allocationId, unacceptableRca, signature); } - function test_SubgraphService_AcceptIndexingAgreement_Revert_WhenAgreementAlreadyAccepted(Seed memory seed) public { + function test_SubgraphService_AcceptIndexingAgreement_Idempotent_WhenAlreadyAcceptedSameAllocation( + Seed memory seed + ) public { Context storage ctx = _newCtx(seed); IndexerState memory indexerState = _withIndexer(ctx); ( @@ -239,19 +242,20 @@ contract SubgraphServiceIndexingAgreementAcceptTest is SubgraphServiceIndexingAg bytes16 agreementId ) = _withAcceptedIndexingAgreement(ctx, indexerState); - // Re-sign for the re-accept attempt (the original signature was consumed) + // Re-sign for the re-accept (the original signature was consumed) (, bytes memory signature) = _recurringCollectorHelper.generateSignedRCA( acceptedRca, ctx.payer.signerPrivateKey ); - bytes memory expectedErr = abi.encodeWithSelector( - IndexingAgreement.IndexingAgreementAlreadyAccepted.selector, - agreementId - ); - vm.expectRevert(expectedErr); + // Re-accepting the same RCA on the same allocation is a no-op. resetPrank(ctx.indexers[0].addr); - subgraphService.acceptIndexingAgreement(ctx.indexers[0].allocationId, acceptedRca, signature); + bytes16 returnedId = subgraphService.acceptIndexingAgreement( + ctx.indexers[0].allocationId, + acceptedRca, + signature + ); + assertEq(returnedId, agreementId); } function test_SubgraphService_AcceptIndexingAgreement_Revert_WhenAgreementAlreadyAllocated( @@ -384,5 +388,148 @@ contract SubgraphServiceIndexingAgreementAcceptTest is SubgraphServiceIndexingAg resetPrank(indexerState.addr); subgraphService.acceptIndexingAgreement(indexerState.allocationId, acceptableRca, signature); } + + function test_SubgraphService_AcceptIndexingAgreement_Rebinds_WhenDifferentAllocation( + Seed memory seed, + uint256 secondAllocationKey + ) public { + Context storage ctx = _newCtx(seed); + IndexerState memory indexerState = _withIndexer(ctx); + ( + IRecurringCollector.RecurringCollectionAgreement memory acceptedRca, + bytes16 agreementId + ) = _withAcceptedIndexingAgreement(ctx, indexerState); + + // Agreement is now bound to the first allocation. + IIndexingAgreement.AgreementWrapper memory before = subgraphService.getIndexingAgreement(agreementId); + assertEq(before.agreement.allocationId, indexerState.allocationId, "starts bound to first allocation"); + + // Derive a second allocation for the same indexer + same subgraph deployment. The first + // allocation already consumed the indexer's provision, so top up first. + uint256 extraTokens = 10_000_000 ether; + deal({ token: address(token), to: indexerState.addr, give: extraTokens }); + resetPrank(indexerState.addr); + _addToProvision(indexerState.addr, extraTokens); + + secondAllocationKey = boundKey(secondAllocationKey); + address secondAllocationId = vm.addr(secondAllocationKey); + vm.assume(secondAllocationId != indexerState.allocationId); + vm.assume(ctx.allocations[secondAllocationId] == address(0)); + ctx.allocations[secondAllocationId] = indexerState.addr; + + bytes memory allocData = _createSubgraphAllocationData( + indexerState.addr, + indexerState.subgraphDeploymentId, + secondAllocationKey, + extraTokens + ); + _startService(indexerState.addr, allocData); + + // Re-sign the same RCA (original signature was consumed on first accept). + (, bytes memory signature) = _recurringCollectorHelper.generateSignedRCA( + acceptedRca, + ctx.payer.signerPrivateKey + ); + + // Re-accepting the same agreement on the new allocation rebinds it: + // event is re-emitted, agreement.allocationId updates, old allocation's active-agreement + // mapping is cleared. Collector's accept() is a no-op (already Accepted). + IndexingAgreement.AcceptIndexingAgreementMetadata memory metadata = abi.decode( + acceptedRca.metadata, + (IndexingAgreement.AcceptIndexingAgreementMetadata) + ); + vm.expectEmit(address(subgraphService)); + emit IndexingAgreement.IndexingAgreementAccepted( + acceptedRca.serviceProvider, + acceptedRca.payer, + agreementId, + secondAllocationId, + metadata.subgraphDeploymentId, + metadata.version, + metadata.terms + ); + resetPrank(indexerState.addr); + bytes16 returnedId = subgraphService.acceptIndexingAgreement(secondAllocationId, acceptedRca, signature); + assertEq(returnedId, agreementId, "rebind returns same agreementId"); + + IIndexingAgreement.AgreementWrapper memory rebound = subgraphService.getIndexingAgreement(agreementId); + assertEq(rebound.agreement.allocationId, secondAllocationId, "rebound to second allocation"); + assertEq( + uint8(rebound.collectorAgreement.state), + uint8(IRecurringCollector.AgreementState.Accepted), + "collector state still Accepted after rebind" + ); + + // Closing the OLD allocation must not cancel the agreement — the agreement no longer + // points to it. onCloseAllocation's allocationToActiveAgreementId lookup should return 0. + resetPrank(indexerState.addr); + subgraphService.stopService(indexerState.addr, abi.encode(indexerState.allocationId)); + + IIndexingAgreement.AgreementWrapper memory afterOldClose = subgraphService.getIndexingAgreement(agreementId); + assertEq( + uint8(afterOldClose.collectorAgreement.state), + uint8(IRecurringCollector.AgreementState.Accepted), + "closing old allocation leaves agreement intact" + ); + assertEq(afterOldClose.agreement.allocationId, secondAllocationId, "still bound to second allocation"); + } + + /// @notice Rebinding an already-accepted agreement to a new allocation must still succeed after + /// the original RCA's acceptance deadline has elapsed. The collector's idempotent short-circuit + /// runs before the deadline check — same-hash re-accept is a no-op and does not consume the + /// signature's lifetime. Without this, indexers could not move agreements across allocations + /// after the typically-short RCA acceptance window closes. + function test_SubgraphService_AcceptIndexingAgreement_Rebinds_AfterRcaDeadline( + Seed memory seed, + uint256 secondAllocationKey + ) public { + Context storage ctx = _newCtx(seed); + IndexerState memory indexerState = _withIndexer(ctx); + ( + IRecurringCollector.RecurringCollectionAgreement memory acceptedRca, + bytes16 agreementId + ) = _withAcceptedIndexingAgreement(ctx, indexerState); + + // Top up provision and allocate a second allocation on the same subgraph deployment. + uint256 extraTokens = 10_000_000 ether; + deal({ token: address(token), to: indexerState.addr, give: extraTokens }); + resetPrank(indexerState.addr); + _addToProvision(indexerState.addr, extraTokens); + + secondAllocationKey = boundKey(secondAllocationKey); + address secondAllocationId = vm.addr(secondAllocationKey); + vm.assume(secondAllocationId != indexerState.allocationId); + vm.assume(ctx.allocations[secondAllocationId] == address(0)); + ctx.allocations[secondAllocationId] = indexerState.addr; + + bytes memory allocData = _createSubgraphAllocationData( + indexerState.addr, + indexerState.subgraphDeploymentId, + secondAllocationKey, + extraTokens + ); + _startService(indexerState.addr, allocData); + + // Warp past the RCA's acceptance deadline. A fresh accept would now revert with + // RecurringCollectorAgreementDeadlineElapsed — the rebind must take the idempotent path. + vm.warp(uint256(acceptedRca.deadline) + 1); + + (, bytes memory signature) = _recurringCollectorHelper.generateSignedRCA( + acceptedRca, + ctx.payer.signerPrivateKey + ); + + resetPrank(indexerState.addr); + bytes16 returnedId = subgraphService.acceptIndexingAgreement(secondAllocationId, acceptedRca, signature); + assertEq(returnedId, agreementId, "rebind after deadline returns same agreementId"); + + IIndexingAgreement.AgreementWrapper memory rebound = subgraphService.getIndexingAgreement(agreementId); + assertEq(rebound.agreement.allocationId, secondAllocationId, "rebound to second allocation after deadline"); + assertEq( + uint8(rebound.collectorAgreement.state), + uint8(IRecurringCollector.AgreementState.Accepted), + "collector state still Accepted after post-deadline rebind" + ); + } /* solhint-enable graph/func-name-mixedcase */ } diff --git a/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/cancel.t.sol b/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/cancel.t.sol index 0b5463cd4..3a8d0340f 100644 --- a/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/cancel.t.sol +++ b/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/cancel.t.sol @@ -250,9 +250,7 @@ contract SubgraphServiceIndexingAgreementCancelTest is SubgraphServiceIndexingAg // solhint-disable-next-line graph/func-name-mixedcase /// @notice An indexer whose provision drops below minimum should still be able /// to cancel their indexing agreement. Cancel is an exit path. - function test_SubgraphService_CancelIndexingAgreement_OK_WhenProvisionBelowMinimum( - Seed memory seed - ) public { + function test_SubgraphService_CancelIndexingAgreement_OK_WhenProvisionBelowMinimum(Seed memory seed) public { Context storage ctx = _newCtx(seed); IndexerState memory indexerState = _withIndexer(ctx); ( diff --git a/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/integration.t.sol b/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/integration.t.sol index 609a91b46..45f31e527 100644 --- a/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/integration.t.sol +++ b/packages/subgraph-service/test/unit/subgraphService/indexing-agreement/integration.t.sol @@ -139,7 +139,6 @@ contract SubgraphServiceIndexingAgreementIntegrationTest is SubgraphServiceIndex acceptedRca.payer, acceptedRca.serviceProvider, agreementId, - uint64(block.timestamp), IRecurringCollector.CancelAgreementBy.Payer ); 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 cd35f4aa0..c4d84d705 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 @@ -294,13 +294,14 @@ contract SubgraphServiceIndexingAgreementSharedTest is SubgraphServiceTest, Boun _rca.deadline, _rca.nonce ); + rcau = _recurringCollectorHelper.sensibleRCAU(rcau); rcau.metadata = _encodeUpdateIndexingAgreementMetadataV1( _newUpdateIndexingAgreementMetadataV1( - bound(_ctx.ctxInternal.seed.termsV1.tokensPerSecond, 0, _rca.maxOngoingTokensPerSecond), + bound(_ctx.ctxInternal.seed.termsV1.tokensPerSecond, 0, rcau.maxOngoingTokensPerSecond), _ctx.ctxInternal.seed.termsV1.tokensPerEntityPerSecond ) ); - return _recurringCollectorHelper.sensibleRCAU(rcau); + return rcau; } function _requireIndexer(Context storage _ctx, address _indexer) internal view returns (IndexerState memory) { @@ -448,10 +449,5 @@ contract SubgraphServiceIndexingAgreementSharedTest is SubgraphServiceTest, Boun assertEq(_expected.dataService, _actual.collectorAgreement.dataService); assertEq(_expected.payer, _actual.collectorAgreement.payer); assertEq(_expected.serviceProvider, _actual.collectorAgreement.serviceProvider); - assertEq(_expected.endsAt, _actual.collectorAgreement.endsAt); - assertEq(_expected.maxInitialTokens, _actual.collectorAgreement.maxInitialTokens); - assertEq(_expected.maxOngoingTokensPerSecond, _actual.collectorAgreement.maxOngoingTokensPerSecond); - assertEq(_expected.minSecondsPerCollection, _actual.collectorAgreement.minSecondsPerCollection); - assertEq(_expected.maxSecondsPerCollection, _actual.collectorAgreement.maxSecondsPerCollection); } } 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 321c26df0..dcd6bf32f 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 @@ -166,10 +166,10 @@ contract SubgraphServiceIndexingAgreementUpgradeTest is SubgraphServiceIndexingA indexerState ); - // Create update with tokensPerSecond exceeding the RCA's maxOngoingTokensPerSecond - uint256 excessiveTokensPerSecond = acceptedRca.maxOngoingTokensPerSecond + 1; + // Create update with tokensPerSecond exceeding the RCAU's maxOngoingTokensPerSecond IRecurringCollector.RecurringCollectionAgreementUpdate memory rcau = _generateAcceptableRecurringCollectionAgreementUpdate(ctx, acceptedRca); + uint256 excessiveTokensPerSecond = rcau.maxOngoingTokensPerSecond + 1; rcau.metadata = _encodeUpdateIndexingAgreementMetadataV1( IndexingAgreement.UpdateIndexingAgreementMetadata({ version: IIndexingAgreement.IndexingAgreementVersion.V1, @@ -190,7 +190,7 @@ contract SubgraphServiceIndexingAgreementUpgradeTest is SubgraphServiceIndexingA bytes memory expectedErr = abi.encodeWithSelector( IndexingAgreement.IndexingAgreementInvalidTerms.selector, excessiveTokensPerSecond, - acceptedRca.maxOngoingTokensPerSecond + rcau.maxOngoingTokensPerSecond ); vm.expectRevert(expectedErr); resetPrank(indexerState.addr); @@ -227,5 +227,29 @@ contract SubgraphServiceIndexingAgreementUpgradeTest is SubgraphServiceIndexingA resetPrank(indexerState.addr); subgraphService.updateIndexingAgreement(indexerState.addr, acceptableRcau, authData); } + + function test_SubgraphService_UpdateIndexingAgreement_Idempotent_WhenAlreadyAtActiveHash(Seed memory seed) public { + Context storage ctx = _newCtx(seed); + IndexerState memory indexerState = _withIndexer(ctx); + (IRecurringCollector.RecurringCollectionAgreement memory acceptedRca, ) = _withAcceptedIndexingAgreement( + ctx, + indexerState + ); + ( + IRecurringCollector.RecurringCollectionAgreementUpdate memory acceptableRcau, + bytes memory authData + ) = _generateAcceptableSignedRCAU(ctx, acceptedRca); + + // First update sets activeTermsHash = hash(rcau) on the collector and applies SS terms. + resetPrank(indexerState.addr); + subgraphService.updateIndexingAgreement(indexerState.addr, acceptableRcau, authData); + + // Re-submitting the same RCAU is a no-op at the SS layer: + // the hash match short-circuits before re-emitting or re-writing terms. + vm.recordLogs(); + resetPrank(indexerState.addr); + subgraphService.updateIndexingAgreement(indexerState.addr, acceptableRcau, authData); + assertEq(vm.getRecordedLogs().length, 0, "no event emitted on idempotent re-update"); + } /* solhint-enable graph/func-name-mixedcase */ } diff --git a/packages/testing/test/harness/FullStackHarness.t.sol b/packages/testing/test/harness/FullStackHarness.t.sol index 842ebe1a1..d095804f0 100644 --- a/packages/testing/test/harness/FullStackHarness.t.sol +++ b/packages/testing/test/harness/FullStackHarness.t.sol @@ -27,6 +27,7 @@ import { OFFER_TYPE_NEW } from "@graphprotocol/interfaces/contracts/horizon/IAgreementCollector.sol"; import { IIssuanceTarget } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceTarget.sol"; +import { IIssuanceAllocationDistribution } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceAllocationDistribution.sol"; import { IGraphToken as IssuanceIGraphToken } from "issuance/common/IGraphToken.sol"; import { IIndexingAgreement } from "@graphprotocol/interfaces/contracts/subgraph-service/internal/IIndexingAgreement.sol"; import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; @@ -291,7 +292,7 @@ abstract contract FullStackHarness is Test { ram.grantRole(OPERATOR_ROLE, operator); ram.grantRole(DATA_SERVICE_ROLE, address(subgraphService)); ram.grantRole(COLLECTOR_ROLE, address(recurringCollector)); - ram.setIssuanceAllocator(address(issuanceAllocator)); + ram.setIssuanceAllocator(IIssuanceAllocationDistribution(address(issuanceAllocator))); issuanceAllocator.setIssuancePerBlock(1 ether); issuanceAllocator.setTargetAllocation(IIssuanceTarget(address(ram)), 1 ether); diff --git a/packages/testing/test/harness/RealStackHarness.t.sol b/packages/testing/test/harness/RealStackHarness.t.sol index db99ace6c..1d7cf6bcd 100644 --- a/packages/testing/test/harness/RealStackHarness.t.sol +++ b/packages/testing/test/harness/RealStackHarness.t.sol @@ -9,6 +9,7 @@ import { RecurringCollector } from "horizon/payments/collectors/RecurringCollect import { IssuanceAllocator } from "issuance/allocate/IssuanceAllocator.sol"; import { RecurringAgreementManager } from "issuance/agreement/RecurringAgreementManager.sol"; import { IIssuanceTarget } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceTarget.sol"; +import { IIssuanceAllocationDistribution } from "@graphprotocol/interfaces/contracts/issuance/allocate/IIssuanceAllocationDistribution.sol"; // Use the issuance IGraphToken for RAM/allocator (IERC20 + mint) import { IGraphToken as IssuanceIGraphToken } from "issuance/common/IGraphToken.sol"; @@ -123,7 +124,7 @@ abstract contract RealStackHarness is Test { ram.grantRole(OPERATOR_ROLE, operator); ram.grantRole(DATA_SERVICE_ROLE, dataService); ram.grantRole(COLLECTOR_ROLE, address(recurringCollector)); - ram.setIssuanceAllocator(address(issuanceAllocator)); + ram.setIssuanceAllocator(IIssuanceAllocationDistribution(address(issuanceAllocator))); // Configure allocator: set total issuance rate, then allocate to RAM issuanceAllocator.setIssuancePerBlock(1 ether); issuanceAllocator.setTargetAllocation(IIssuanceTarget(address(ram)), 1 ether); diff --git a/packages/testing/test/integration/AgreementLifecycleAdvanced.t.sol b/packages/testing/test/integration/AgreementLifecycleAdvanced.t.sol index d20a8e347..95f91f1a0 100644 --- a/packages/testing/test/integration/AgreementLifecycleAdvanced.t.sol +++ b/packages/testing/test/integration/AgreementLifecycleAdvanced.t.sol @@ -600,11 +600,93 @@ contract AgreementLifecycleAdvancedTest is FullStackHarness { ); } + // ═══════════════════════════════════════════════════════════════════ + // Scenario 15: Rebind after cancellation — collector state authority + // ═══════════════════════════════════════════════════════════════════ + + /// @notice Cancellation is terminal at the collector. The SubgraphService rebind path must + /// defer to that authority: an attempt to rebind a cancelled agreement onto a fresh allocation + /// must revert, leaving both the collector state and SS state untouched. Exercises the full + /// offer → accept → cancel → open-second-allocation → rebind-attempt flow end-to-end with the + /// real contract stack. + function test_Scenario15_RebindAfterCancellation_Reverts() public { + IndexingAgreement.IndexingAgreementTermsV1 memory terms = IndexingAgreement.IndexingAgreementTermsV1({ + tokensPerSecond: 0.5 ether, + tokensPerEntityPerSecond: 0 + }); + IRecurringCollector.RecurringCollectionAgreement memory rca = _buildRCA(indexer, 0, 1 ether, 3600, terms); + bytes16 agreementId = _offerAndAccept(indexer, rca); + + // Cancel via the indexer path — CanceledByServiceProvider. + vm.prank(indexer.addr); + subgraphService.cancelIndexingAgreement(indexer.addr, agreementId); + assertEq( + uint8(recurringCollector.getAgreement(agreementId).state), + uint8(IRecurringCollector.AgreementState.CanceledByServiceProvider), + "precondition: cancelled at collector" + ); + + // Open a second allocation on the same subgraph deployment. + (address secondAllocationId, address cancelRebindTarget) = _openSecondAllocationForIndexer( + indexer, + "cancel-rebind-alloc" + ); + assertEq(cancelRebindTarget, indexer.addr, "indexer owns the new allocation"); + + // Attempt rebind to the new allocation. SS would stage the bookkeeping, but collector + // rejects (state != NotAccepted), reverting the whole tx. Both layers stay clean. + vm.prank(indexer.addr); + vm.expectRevert( + abi.encodeWithSelector( + IRecurringCollector.RecurringCollectorAgreementIncorrectState.selector, + agreementId, + IRecurringCollector.AgreementState.CanceledByServiceProvider + ) + ); + subgraphService.acceptIndexingAgreement(secondAllocationId, rca, ""); + + // Post-revert: agreement still cancelled at collector, still bound to old allocation in SS. + assertEq( + uint8(recurringCollector.getAgreement(agreementId).state), + uint8(IRecurringCollector.AgreementState.CanceledByServiceProvider), + "collector state unchanged" + ); + IIndexingAgreement.AgreementWrapper memory wrapper = subgraphService.getIndexingAgreement(agreementId); + assertEq(wrapper.agreement.allocationId, indexer.allocationId, "SS still bound to original allocation"); + } + // ── Helpers ── function _getHardcodedPoiMetadata() internal view returns (bytes memory) { return abi.encode(block.number, bytes32("PUBLIC_POI1"), uint8(0), uint8(0), uint256(0)); } + + /// @notice Top up the indexer's provision and open a second allocation on the same + /// subgraph deployment. Returns the new allocation's id plus the indexer that owns it + /// (both for readability and to let callers assert ownership in a single expression). + function _openSecondAllocationForIndexer( + IndexerSetup memory _indexer, + string memory _label + ) internal returns (address allocationId, address owner) { + uint256 extraTokens = MINIMUM_PROVISION_TOKENS; + _addProvisionTokens(_indexer, extraTokens); + + uint256 allocationKey; + (allocationId, allocationKey) = makeAddrAndKey(_label); + + bytes32 digest = subgraphService.encodeAllocationProof(_indexer.addr, allocationId); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(allocationKey, digest); + bytes memory allocationData = abi.encode( + _indexer.subgraphDeploymentId, + extraTokens, + allocationId, + abi.encodePacked(r, s, v) + ); + vm.prank(_indexer.addr); + subgraphService.startService(_indexer.addr, allocationData); + + owner = _indexer.addr; + } } /// @notice Mock eligibility oracle for testing