Indexing payments management audit fix hybrid reduced scope#1312
Indexing payments management audit fix hybrid reduced scope#1312RembrandtK wants to merge 21 commits intoindexing-payments-management-auditfrom
Conversation
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}.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
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).
RembrandtK
force-pushed
the
indexing-payments-management-audit-fix-reduced
branch
from
5ec30ff to
bbe0195
Compare
Indexing payments management audit fix hybrid reduced scope
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## indexing-payments-management-audit #1312 +/- ##
======================================================================
+ Coverage 90.29% 90.86% +0.56%
======================================================================
Files 81 81
Lines 5130 5274 +144
Branches 1084 949 -135
======================================================================
+ Hits 4632 4792 +160
+ Misses 465 453 -12
+ Partials 33 29 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
RembrandtK
changed the base branch from
main
to
indexing-payments-management-audit
…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
1 participant
Filter for files in audit scope:
Audit findings are individually tracked in packages/issuance/audits/PR1301/. Responses (after the --- separator) are added in relevant commits.
Trust Model: callback hardening & explicit eligibility
RecurringCollector.sol,IAgreementCollector.sol(new),IAgreementOwner.solTRST-H-1,H-2,H-4: Payer callbacks are now gas bounded and cannot block or revert the collect path. RAM no longer needs an approval callback because RC has advance approval.TRST-L-1,SR-4: Eligibility checking promoted to an agreement term; both parties know and agree up front, no surprise gating after acceptance.TRST-L-2,L-5: Offers committed on-chain before acceptance; terms verified against stored hashes, alongside the existing RCA framework.Escrow Management: threshold/margin model, JIT removed
TRST-M-2,M-3: More robust threshold and margin based degradation.TRST-H-3: re-read escrow balance before acting (stale snapshot fix).TRST-M-1: Minimum thaw fraction prevents dust-thaw griefing.Operational Hardening: pause & emergency controls
TRST-L-3: Pause mechanism on RecurringCollector (upgradeable).Acknowledged without code changes (TRST-CR-1/3, L-4, R-1, SR-1/2/3)
Incidental
RecurringAgreementManager.sol): Storage moves from flat to per-collector keying since agreement IDs cannot be relied on to be globally unique. Mixed with switching to RC-reported max-next-claim for tracking lifetime.SubgraphService.sol,AllocationHandler.sol,IndexingAgreement.sol,SubgraphServiceStorage.sol): Resize-to-zero instead of force-close for allocations with active agreements; optionally (configuration controlled) block closing allocations with active agreements.