Indexing payments management audit#1301
Open
RembrandtK wants to merge 79 commits intomainfrom
Open
Conversation
Fixes TRST-M-1 audit finding: Wrong TYPEHASH string is used for agreement updates, limiting functionality. * Fixed EIP712_RCAU_TYPEHASH to use correct uint64 types for deadline and endsAt fields (was incorrectly using uint256) * This prevents signature verification failures for RecurringCollectionAgreementUpdate
Fixes TRST-M-2 audit finding: Collection for an elapsed or canceled agreement could be wrong due to temporal calculation inconsistencies between IndexingAgreement and RecurringCollector layers. * Replace isCollectable() with getCollectionInfo() that returns both collectability and duration * Make RecurringCollector the single source of truth for temporal logic * Update IndexingAgreement to call getCollectionInfo() once and pass duration to _tokensToCollect()
Fixes signature replay attack vulnerability where old signed RecurringCollectionAgreementUpdate messages could be replayed to revert agreements to previous terms. ## Changes - Add `nonce` field to RecurringCollectionAgreementUpdate struct (uint32) - Add `updateNonce` field to AgreementData struct to track current nonce - Add nonce validation in RecurringCollector.update() to ensure sequential updates - Update EIP712_RCAU_TYPEHASH to include nonce field - Add comprehensive tests for nonce validation and replay attack prevention - Add RecurringCollectorInvalidUpdateNonce error for invalid nonce attempts ## Implementation Details - Nonces start at 0 when agreement is accepted - Each update must use current nonce + 1 - Nonce is incremented after successful update - Uses uint32 for gas optimization (supports 4B+ updates per agreement) - Single source of truth: nonce stored in AgreementData struct
Implements slippage protection mechanism to prevent silent token loss during rate-limited collections in RecurringCollector agreements. The implementation uses type(uint256).max convention to disable slippage checks, providing users full control over acceptable token loss during rate limiting. Resolves audit finding TRST-L-5: "RecurringCollector silently reduces collected tokens without user consent"
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Articulates trust assumptions between the five core actors (payer, collector, data service, receiver, escrow) in the Graph Horizon payments protocol, with implementation-specific details for RecurringCollector, SubgraphService, and RAM.
Add packages/testing with real PaymentsEscrow, RecurringCollector, IssuanceAllocator, and RecurringAgreementManager deployed together. Gas results confirm 21-27x headroom on callback budgets. Improve test coverage across all packages: - horizon: RecurringCollector 86%→100% lines, 76%→100% functions - subgraph-service: 92.86%→94.76% branches - issuance: restore 100% coverage, remove dead getPageBytes16 Fix coverage scripts to print summary tables to stdout.
Trust Security audit (2026-03-03 to 2026-03-19) findings extracted from Graph_PR1301_v01.pdf into PR1301/ directory with README index. Includes auditor-supplied staleSnap PoC test.
Replace tempJit/fullReserveMargin with two configurable parameters: minOnDemandBasisThreshold (default 128) and minFullBasisMargin (default 16). Effective basis limited based on spare balance relative to sumMaxNextClaimAll using strict <. Delete staleSnap.t.sol (PoC scenario no longer applicable after threshold-based degradation replaces tempJit).
…ST-M-1) 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.
Add revertOnIneligible flag to RewardsManager. When true, ineligible indexers cause takeRewards to revert (blocking POI presentation and preserving rewards for future collection). When false (default), ineligible indexers have rewards reclaimed but takeRewards succeeds.
Over-allocated and stale allocations are now resized to zero tokens instead of being force-closed. This keeps the allocation open so that any bound indexing agreement remains active. The allocation stays as a stakeless (altruistic) allocation rather than being deleted. Changes: - AllocationHandler.presentPOI: call _resizeAllocation(0) instead of _closeAllocation when over-allocated - SubgraphService.closeStaleAllocation: resize to zero instead of _onCloseAllocation + _closeAllocation - SubgraphService._collectIndexingRewards: remove _onCloseAllocation call after over-allocation (allocation stays open) - Extract _resizeAllocation internal helper from resizeAllocation
Add a governor-controlled guard that prevents closing an allocation when it has an active indexing agreement. When enabled, stopService reverts with SubgraphServiceAllocationHasActiveAgreement instead of auto-canceling the agreement. - Add SubgraphServiceV2Storage with blockClosingAllocationWithActiveAgreement - Add setter/getter and BlockClosingAllocationWithActiveAgreementSet event - IndexingAgreement.onCloseAllocation: revert if active when guard enabled, otherwise cancel as ServiceProvider (forceClosed param removed since closeStaleAllocation now resizes instead of closing)
…terms Add checked-arithmetic overflow guard on accept and update paths to prevent agreements whose maxOngoingTokensPerSecond × maxSecondsPerCollection product would overflow during collection, with 1024× headroom margin.
…ancel (TRST-L-2, L-5) Payers pre-store RCA/RCAU offers on-chain; acceptance verifies the stored hash instead of calling back to the payer contract. This eliminates the callback-based approval flow and its associated trust assumptions. Escrow reservation uses max(active, pending) instead of additive current + pending, so only the larger of the two term sets is reserved (L-2). The claim formula caps the collection window at min(remaining time, maxSecondsPerCollection), eliminating overestimates for agreements near their deadline (L-5). Scoped getMaxNextClaim lets callers query active terms, pending offers, or both. Payer-initiated cancel targets specific terms by hash and scope (ACTIVE/PENDING), with a guard preventing deletion of offers whose terms are already active.
…TRST-H-1, H-2, H-4, L-1, SR-4) Payer callbacks (beforeCollection, afterCollection, isEligible) now use gas-capped low-level call/staticcall instead of try/catch, preventing gas siphoning via the 63/64 rule (H-1) and caller-side ABI decode reverts from malformed returndata (H-2). A gasleft() guard before each callback reverts when insufficient gas remains (L-1). Failed callbacks emit PayerCallbackFailed for off-chain monitoring (SR-4). Eligibility checking is now opt-in via a conditions bitmask on RCA/RCAU. CONDITION_ELIGIBILITY_CHECK gates the IProviderEligibility staticcall at collection time. At acceptance, _requireEligibilityCapability validates ERC-165 support — an EOA without code cannot set this flag, closing the EIP-7702 attack vector where an EOA later acquires code to block collection (H-4).
Replace getCollectors, getCollectorProviders, getProviderAgreements (and their paginated overloads), getCollectorProviderCount, getProviderAgreementCount, getPairAgreementCount, and getTotalAgreementCount with indexed accessors: getCollectorAt, getProviderCount/At, getAgreementCount/At, and getEscrowSnap. Remove EnumerableSetUtil import and using statements. The getPage and getPageBytes16 helpers inline substantial library code at each call site — six call sites pushed RAM over the 24576-byte Spurious Dragon deployable bytecode limit. Also switches view param types from IRecurringCollector to IAgreementCollector and renames getSumMaxNextClaimAll to getSumMaxNextClaim to match the updated IRecurringAgreements interface. Does not compile: new accessors reference future hierarchical storage ($.collectors[c].providers[p], $.collectorSet) introduced in a later commit.
…atch Add IEmergencyRoleControl implementation allowing pause guardians to revoke any non-governor role as a fast-response emergency measure. Governor role is excluded to prevent locking out governance. Also adds emergencyClearEligibilityOracle for fail-open when the oracle is broken, extracts _setProviderEligibilityOracle private helper to share between the governor setter and emergency clear, and removes the afterAgreementStateChange stub (obsolete for thin pass-through).
…ntCollector pass-throughs offerAgreement now calls collector.offer() with opaque offer data instead of constructing RCA hashes and storage directly. cancelAgreement calls collector.cancel() with a version hash and options bitmask instead of routing through the data service. Both forward to the collector then reconcile locally — the collector owns agreement state, RAM just caches maxNextClaim for escrow accounting. Does not compile: new function bodies reference _reconcileAgreement with a collector parameter that is introduced in a later commit.
…d revokeOffer These functions are subsumed by the IAgreementCollector offer/cancel interface: updates are offered via offerAgreement with an update offer type, and revocations use cancelAgreement targeting the relevant terms hash. The collector now owns offer/update lifecycle state. Also add forceRemoveAgreement.
Replace flat storage mappings with hierarchical CollectorData and
CollectorProviderData structs. Agreements are now namespaced under their
collector ($.collectors[c].agreements[id]) instead of a global mapping,
preventing cross-collector ID collisions.
Add _getAgreementProvider for lazy discovery: on first encounter, validates
collector role, reads agreement details, checks payer/dataService, and
registers in tracking sets. Subsequent accesses use the cached provider.
Rewrite internal functions to use the hierarchical storage:
- _reconcileAgreement: discover-then-reconcile flow
- _removeAgreement: replaces _deleteAgreement
- _reconcileProvider: replaces _reconcilePairTracking
- _setAgreementMaxNextClaim: takes CollectorProviderData, reads old value
- _escrowMinMax: takes sumMaxNextClaim value parameter
- _providerEscrowDeficit: takes CollectorProviderData
- _reconcileProviderEscrow: replaces _updateEscrow, uses cpd
- _setEscrowSnap: takes CollectorProviderData
Add forceRemoveAgreement operator escape hatch. Update beforeCollection,
afterCollection, reconcileAgreement, and reconcileProvider to use the
new collector-parameterised internal functions.
Reduces AgreementInfo to {provider, maxNextClaim}.
Convert RecurringCollector to TransparentUpgradeableProxy pattern with ERC-7201 namespaced storage. Authorizable storage also converted to ERC-7201 for proxy compatibility.
Add pause guardian pattern gating accept, update, collect, cancel, and offer behind whenNotPaused. This provides a middle layer between the RAM-level pause (agreement lifecycle only) and the Controller-level nuclear pause (all escrow operations protocol-wide). The previous approveAgreement pause-bypass vector no longer exists since callback- based approval was replaced by stored-hash authorization (L-3).
…celIndexingAgreement cancelIndexingAgreement (introduced in a7fb875) required a valid provision check (onlyValidProvision, later refactored to enforceService with VALID_PROVISION | REGISTERED). Cancel should remain available regardless of provision state. Combined with blockClosingAllocationWithActiveAgreement (b124656), an indexer whose provision drops below minimum is stuck in a catch-22: VALID_PROVISION blocks cancel, and the active agreement blocks closing the allocation. REGISTERED is not relevant to cancelling on accepted agreement either. Changed to enforceService(indexer, DEFAULT). IndexingAgreement.cancel() validates the caller and pause check remains. Tests added (not all related to this issue): - Cross-package integration lifecycle - Cancel with below-minimum provision - Horizon coverage gaps
Contributor
Author
This was referenced Apr 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Filter for files in audit scope:
bf6d4cb Indexing payments (merge)
Previously audited (2025-06-Indexing-Payments.pdf) implementation of indexing agreements for the Graph Protocol's recurring payment system. Adds RecurringCollector, IndexingAgreement library, and integrates indexing fee collection into SubgraphService and DisputeManager.
28edcd7 Post-Horizon migration removal of transition code (merge)
Remove HorizonStakingExtension, ExponentialRebates, LibFixedMath, MathUtils, and all transition-period guard logic. Simplify HorizonStaking to direct implementation without delegatecall split. Clean up RewardsManager multi-issuer model to single subgraphService. Remove legacy allocation/dispute code from SubgraphService.
f4451f1 feat: add back legacy allocation id collision check
fa99514 fix: cap maxSecondsPerCollection instead of reverting
c6836a7 fix: enforce temporal validation on zero-token collections and remove zero-POI special case
8efaec9 feat: add adjustThaw to PaymentsEscrow
3f1578c refactor: rename IRewardsEligibility to IProviderEligibility
(For the purpose of this audit note that the interfaces being changed are not in production.)
d20bc84 feat: contract approver model for RecurringCollector accept/update
ec72360 feat: IDataServiceAgreements interface and SubgraphService integration
89def3d feat: enumerable indexer tracking for REO and issuance constructor cleanup
(Do not need storage compatibility for delpoying an upgrade.)
843fdd2 feat: RecurringAgreementManager with lifecycle, escrow funding, and agreement updates