Indexing payments management audit fix#1325
Open
RembrandtK wants to merge 47 commits intoindexing-payments-management-auditfrom
Open
Indexing payments management audit fix#1325RembrandtK wants to merge 47 commits intoindexing-payments-management-auditfrom
RembrandtK wants to merge 47 commits intoindexing-payments-management-auditfrom
Conversation
Update existing findings with status, team responses, and mitigation reviews from the v02 audit report. Add 17 new findings (M-4, M-5, L-6 through L-11, R-5 through R-13) and the v02 PDF.
The gasleft() prechecks only accounted for EIP-150's 63/64 rule but not the gas consumed between the check and the CALL opcode. Add a CALLBACK_GAS_OVERHEAD constant (3,000 gas) to all three prechecks so at least MAX_PAYER_CALLBACK_GAS is forwarded to the callee.
Replace Solidity low-level calls with inline assembly to bound returndata copy: eligibility staticcall copies at most 32 bytes, beforeCollection/afterCollection copy zero bytes. Prevents a malicious payer from forcing ~4.5M gas overhead via returndata bombing.
CONDITION_ELIGIBILITY_CHECK is an agreement term, not a proxy for payer type — contract payers can legitimately offer agreements without it, and gating callback dispatch on the flag would deny beforeCollection / afterCollection to those payers. With the M-4 returndata-bombing fix in place, the gas impact of an EIP-7702 EOA acquiring callbacks is bounded and predictable, and the callbacks themselves are non-reverting and non-blocking.
…-1, TRST-M-5) Add minResidualEscrowFactor (uint8, default 50 → threshold 2^50 wei ≈ 0.001 GRT) to RecurringAgreementManager. When a (collector, provider) pair has no remaining agreements and the escrow balance is below the threshold, tracking is dropped — the residual is not worth the gas cost of further thaw/withdraw cycles. For untracked pairs, reconcileProvider performs a blind drain (withdraw matured thaw, thaw remainder) without re-creating tracking state. New agreements for the same pair re-add tracking naturally via _offerAgreement.
TRST-L-6: planted-offer-matching-active-terms cleanup bypass — rejected because cross-type EIP-712 collisions are computationally infeasible and same-type 'collisions' require the payer to reproduce their own terms, which is not an attack. TRST-R-7: eagerly delete consumed offers — rejected because offer data (metadata, nonce, deadline) is intentionally kept accessible via getAgreementOfferAt() until obsolete.
The RAM's cancelAgreement is now a pass-through to collector.cancel(), which requires agreement.state == AgreementState.Accepted. The defensive guard the recommendation asks for already lives in the single authoritative location for agreement state; no further change required.
_validateAndStoreUpdate's `if (oldHash != bytes32(0))` branch was unreachable — every Accepted agreement has a non-zero activeTermsHash written during accept() or a prior update(). Dropped the guard; the offer cleanup is now unconditional with an inline comment noting the invariant.
…nel (TRST-R-5) getAgreementOfferAt callers could not distinguish a stored OFFER_TYPE_NEW (value 0) from the zero default returned when no offer exists. Make the offer type flags non-zero (NEW=1, UPDATE=2), reserve 0 as the named OFFER_TYPE_NONE sentinel, and use it at the no-offer return site.
…en flag NatSpec (TRST-R-11) Remove AUTO_UPDATE, AUTO_UPDATED, and BY_DATA_SERVICE from IAgreementCollector: none had an implementation path or in-tree consumer (RecurringCollector's cancel vocabulary is Payer / ServiceProvider only; there is no auto-update feature). Remaining flags' NatSpec tightened to describe the semantics they now carry after R-12. Also drop WITH_NOTICE and IF_NOT_ACCEPTED: declared on the unsigned offer path but never referenced — offer() ignores its options parameter. Parameter NatSpec now describes the bitmask as reserved for implementation-specific use.
The v02 mitigation review corrected the security-boundary framing in our fix comment: an EIP-7702 EOA can toggle code on and off across calls, so "an EOA cannot pass the interface check" is not a durable guarantee. The correct boundary is that a provider opting into CONDITION_ELIGIBILITY_CHECK is trusting the payer contract. Recorded the acknowledgement in the team response — no code change required, since the gate already depends on the provider's opt-in.
STALE_POI is the correct reason for the resize-based stale-allocation path (allocation stays open as stakeless, not closed). The previous CLOSE_ALLOCATION behavior never shipped to production, so there is no operator configuration to migrate.
RAM trusts collectors to enforce agreement uniqueness and state transitions. Future collectors must implement their own replay protection on acceptance.
Revoking COLLECTOR_ROLE or DATA_SERVICE_ROLE does not invalidate tracked agreements; reconciliation proceeds to orderly settlement. Role checks gate only new offerAgreement calls and discovery inside _reconcileAgreement.
…T-R-8) Escalation ladder item 3 now refers to the existing cross-contract note so the prose matches the whenNotPaused scope on beforeCollection and afterCollection.
…R-9) RC overrides _isAuthorized to return true when signer == address(this), so RC itself must perform the appropriate authorization check before any external call it initiates.
…tale agreement rate IndexingAgreement.update() validated new indexing terms against wrapper.collectorAgreement.maxOngoingTokensPerSecond (the current agreement's rate) instead of rcau.maxOngoingTokensPerSecond (the update's rate). If the RCAU decreased the rate, indexing terms exceeding the new cap would be accepted. accept() already validates against rca.maxOngoingTokensPerSecond — this makes update() consistent.
…stants Assorted small refactors and interface tweaks that prepare for follow-on changes without changing behavior: - extract _rcaIdAndHash helper (agreement ID + RCA hash used together) - default _getMaxNextClaimScoped scope to both (active | pending) on 0 - drop redundant isSigned param from _requireAuthorization - drop redundant timestamp from agreement lifecycle events - single-line AgreementCanceled emit - add VERSION_CURRENT/VERSION_NEXT constants and clarify state flag NatSpec in IAgreementCollector
…ation The (window params + eligibility + overflow) triple was duplicated in _validateAndStoreAgreement and _validateAndStoreUpdate. Extract into _requireValidTerms. No behaviour change.
…ment Move the state flip (acceptedAt, state=Accepted) and AgreementAccepted event from _validateAndStoreAgreement into accept() inline. Use rca.* for the event instead of re-reading from storage. The function now only validates and registers (identity + terms).
Move nonce check, nonce write, and AgreementUpdated event from _validateAndStoreUpdate into update() inline. Use rcau.* for event fields. The function now only validates terms and writes them to storage; update() handles lifecycle (nonce, event).
…nding Defer state authority to the collector and align SS-side semantics for duplicate calls and re-acceptance with a different allocation: - update(): _isValid replaces _isActive; an activeTermsHash match short-circuits the SS-side event and terms re-write. - accept(): same-allocation re-accept is an idempotent no-op at the SS layer; different-allocation re-accept rebinds the agreement by clearing the old allocationToActiveAgreementId link and establishing the new one. Enables moving an active agreement to a new allocation when the original is closed.
…isons Preparatory cleanup: - Hoist `solhint-disable gas-strict-inequalities` to file level; drop per-block/per-line fences and flip `deadline >= block.timestamp` callsites to the idiomatic `block.timestamp <= deadline`.
…stamp Collection-window and duration checks now use the offer's acceptance deadline as the reference point instead of `block.timestamp`, making validation time-independent: if terms pass here they remain valid for any acceptance on or before `deadline`. Callers still enforce `block.timestamp <= deadline` at the acceptance entry point. - `_requireValidCollectionWindowParams` takes a `_deadline` parameter and becomes `pure`. `_endsAt > block.timestamp` becomes `_deadline < _endsAt`; `_endsAt - block.timestamp >= min + WINDOW` becomes `min + WINDOW <= _endsAt - _deadline`. - `_requireValidTerms` propagates `_deadline` to the window check. - Accept/update call sites pass the RCA/RCAU deadline. - Interface: replace `RecurringCollectorAgreementElapsedEndsAt` with `RecurringCollectorAgreementEndsBeforeDeadline(deadline, endsAt)`. Prerequisite for hash-keyed terms storage, where a single stored hash must remain validatable without re-checking against wall clock on every read.
Preparatory step for TRST-L-11 (per-version semantics) and TRST-L-8 (SCOPE_SIGNED cancel) — minimizes thrash in those commits. Not the final correct implementation: index is passed through but only VERSION_CURRENT and VERSION_NEXT are distinguished, getAgreementOfferAt still uses OFFER_TYPE_* indexing, and _versionHashAt still keys VERSION_CURRENT off agreement.state because activeTermsHash is not yet persisted pre-acceptance (lands in TRST-L-7). - offer() routes through _getAgreementDetails(id, versionHash, index) using tuple-returning _offerNew/_offerUpdate (id, versionHash, index). The offer path supplies the hash it just produced; the helper avoids re-reading storage to recompute it. - _versionHashAt resolves the offer hash for the requested version: pre-acceptance CURRENT reads rcaOffers; post-acceptance CURRENT reads agreement.activeTermsHash; NEXT reads rcauOffers but skips when the stored RCAU is already the active version. - getAgreementDetails(id, index) looks up the hash via _versionHashAt and forwards to _getAgreementDetails. The helper returns empty when versionHash is zero, treating "no version exists" uniformly across both call sites. State semantics preserved: REGISTERED for pre-acceptance current, ACCEPTED for post-acceptance current, REGISTERED|UPDATE for any pending RCAU.
…on (TRST-L-7)
Persist agreement.payer (and dataService/serviceProvider) at offer time
rather than waiting until accept(). _requirePayer is replaced by an
inline payer check at the cancel() call site now that agreement.payer
is the reliable authority — no more fallback decoding of stored RCA
data on every cancel.
Persistent agreement.payer makes cancelling a pre-acceptance RCA offer
and cancelling a pending RCAU offer independent operations that may be
performed in either order. Neither path leaves the other unreachable.
_offerUpdate also simplifies: it reads agreement.payer/dataService/
serviceProvider directly (set by _offerNew) rather than decoding the
stored RCA on every update offer. State guard relaxes to accept
{NotAccepted, Accepted} so update offers work post-acceptance.
cancel(by) clears any pending RCAU offer at cancellation time —
pendingHash != activeTermsHash means the pending offer is now stale
and can be reaped.
offer() hoists the msg.sender == details.payer authorization out of
both _offerNew and _offerUpdate now that details.payer is reliably
populated by either path.
accept() now stores the RCA offer idempotently (when not already
present) so accept-without-prior-offer paths leave the same on-chain
trail. update() does the same for RCAU storage.
Align re-accept, re-update, and cancel-on-nothing semantics so duplicate calls with the same signed terms are no-ops rather than reverts, and cancel against a nonexistent agreement is a silent no-op. - accept(): short-circuits when state == Accepted and the stored activeTermsHash already equals the incoming RCA hash. Re-accepting the same signed RCA is a no-op (skips deadline + auth). Cancelled agreements still revert — re-accept of a cancelled agreement is never valid. The state == NotAccepted require is dropped: the short-circuit handles re-accept-same, and _requireAuthorization handles re-accept-different (signature won't match a different hash). - update(): short-circuits when activeTermsHash already equals the RCAU hash, skipping deadline and authorization checks on the idempotent path. - cancel(): when no agreement or stored offer exists (agreement.payer == 0) the call returns silently instead of reverting with RecurringCollectorAgreementNotFound. Cancel against nothing is a no-op — same idempotent spirit. Built on top of TRST-L-7's persistent agreement.payer.
…ions Emit OfferCancelled when cancel() with SCOPE_PENDING deletes a stored RCA or RCAU offer entry. Provides off-chain observability of offer cancellations symmetric to OfferStored. The same event is also emitted by SCOPE_SIGNED cancellations (added in the TRST-L-8 commit on top of this one).
…-11) Honor the index parameter in getAgreementDetails (previously ignored) and in getAgreementOfferAt (previously used OFFER_TYPE_* values). Per-version flag composition (the queried version, not the underlying agreement): - VERSION_CURRENT: REGISTERED for pre-acceptance offer; REGISTERED | ACCEPTED for accepted active terms; UPDATE additionally set when active terms came from update() (proxy: agreement.updateNonce > 0). Pre-acceptance reads identity from agreement storage (persistent payer from TRST-L-7). - VERSION_NEXT: REGISTERED | UPDATE when a pending RCAU exists, else empty. - index >= 2: empty struct. getAgreementOfferAt mirrored: VERSION_CURRENT returns the active offer (matched by activeTermsHash, RCA pre-update or RCAU post-update); VERSION_NEXT returns the pending RCAU when distinct from the active hash. _offerUpdate's pending result still returns REGISTERED | UPDATE without ACCEPTED. The queried version is the just-stored RCAU, which is not itself accepted (per-version semantics). The auditor's recommendation to OR ACCEPTED is rejected on this point and noted in the audit response.
…(TRST-R-12) Populate state flags beyond REGISTERED/ACCEPTED/UPDATE so agreement-scoped views distinguish cancelled from live and signal when nothing is currently claimable: - NOTICE_GIVEN + BY_PAYER / BY_PROVIDER — cancelled agreement, origin identified by the BY_* flag. - SETTLED — _getMaxNextClaimScoped(agreementId, 0) returns zero, meaning no tokens are claimable under either active or pending scope. Covers provider-cancelled agreements (immediately non-collectable), fully-collected agreements, and payer-cancelled agreements past their canceledAt window.
…n (TRST-L-8) Give EOA signers an on-chain revocation path via cancel(agreementId, termsHash, SCOPE_SIGNED). Records cancelledOffers[msg.sender][termsHash] = agreementId; _requireAuthorization rejects when the stored agreementId matches. Self-authenticating, idempotent, reversible (bytes16(0) undoes), and combinable with SCOPE_PENDING/SCOPE_ACTIVE. Builds on the version-indexed storage and idempotent cancel semantics from the preceding L-11 refactor: SCOPE_SIGNED is added as a new branch at the top of cancel() alongside the existing SCOPE_PENDING / SCOPE_ACTIVE handling, and the cancelledOffers lookup slots into _requireAuthorization's signed branch.
Every issuance target should expose its allocator. Add getIssuanceAllocator() returning IIssuanceAllocationDistribution to IIssuanceTarget. Implement in RecurringAgreementManager (reads from storage), DirectAllocation (stores and returns), and RewardsManager (existing impl, moved from IRewardsManager to IIssuanceTarget). Also change IIssuanceTarget.setIssuanceAllocator parameter from address to IIssuanceAllocationDistribution for compile-time type safety.
Replace _requirePayerToSupportEligibilityCheck in _offerNew/_offerUpdate with _requireValidTerms, so deadline/endsAt/min-max collection seconds and ongoing-rate are validated when the offer is registered, not only when the offer is accepted. Pre-acceptance views (getMaxNextClaim, getAgreementDetails) read terms from the stored RCA bytes, so an offer with malformed terms could otherwise be advertised — and surface non-zero pending caps — until accept() rejected it.
_getMaxNextClaimScoped read offer deadlines incorrectly: - pre-acceptance used `block.timestamp < rca.deadline`, excluding the boundary; aligned with offer/accept which use `<=`. - SCOPE_PENDING had no deadline check at all — an expired pending RCAU still contributed to maxClaim. - SCOPE_PENDING also fired when the stored RCAU offer was already the active version (post-update), double-counting it against SCOPE_ACTIVE; skip when rcauOffer.offerHash == activeTermsHash.
The CanceledByServiceProvider early-return at the top of _getMaxNextClaim is fully subsumed by the next check (state must be Accepted or CanceledByPayer). Drop the redundant first check; the comprehensive guard catches CanceledByServiceProvider and any future non-collectable state.
Indexing payments management audit fix
🚨 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 #1325 +/- ##
======================================================================
+ Coverage 90.86% 91.16% +0.30%
======================================================================
Files 81 81
Lines 5274 5342 +68
Branches 949 975 +26
======================================================================
+ Hits 4792 4870 +78
+ Misses 453 449 -4
+ Partials 29 23 -6
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-fix-reduced
RembrandtK
changed the base branch from
indexing-payments-management-audit-fix-reduced
to
indexing-payments-management-audit
…0 headroom Drops the auto-generated public getters for MIN_SECONDS_COLLECTION_WINDOW, CONDITION_ELIGIBILITY_CHECK, EIP712_RCA_TYPEHASH, and EIP712_RCAU_TYPEHASH (~50 bytes each) so the contract stays under the EIP-170 24576-byte limit when later additions land. Tests that read these via the public ABI switch to the literal value with a naming comment. Mirrors the dropped constants and EIP-712 typestrings under @graphprotocol/toolshed/core/recurring-collector so off-chain agents constructing offers have a typed source of truth.
…callback opt-in Replaces the live `payer.code.length != 0` callback dispatch in _preCollectCallbacks/_postCollectCallback with a stored condition flag. An offer that sets CONDITION_AGREEMENT_OWNER is only acceptable when the payer declares ERC-165 support for IAgreementOwner; the flag is then read at collection time, so callback dispatch is frozen to acceptance and unaffected by post-acceptance code changes such as EIP-7702 delegation swaps. This closes the gas-estimator griefing vector where an EOA payer could attach delegation between estimation and execution and cause collect() to revert. Consolidates the two interface-support errors into a single RecurringCollectorPayerDoesNotSupportInterface(payer, interfaceId). Renames _requirePayerToSupportEligibilityCheck to _requirePayerInterfaceSupport and folds both ERC-165 checks into it. Test coverage: - offer(NEW) reverts when CONDITION_AGREEMENT_OWNER is set on a payer that does not declare IAgreementOwner via ERC-165. - offer(UPDATE) re-validates ERC-165 support when an RCAU adds the flag to an already-accepted agreement, and reverts if the payer doesn't declare it. - collect skips both beforeCollection and afterCollection when the flag is unset, even with a contract payer — proves dispatch gates on the stored flag, not live payer.code.length. Addresses TRST-L-10.
…g lows for v03
v03 of the Trust Security report withdraws v02 TRST-M-5 (perpetual thaw
griefing via micro deposits) and v02 TRST-L-6 (update offer cleanup
bypassed via planted offer), and renumbers the remaining lows from
L-7..L-11 down to L-6..L-10.
This commit performs the content side and parks each renamed file
under TRST-L-{old}-{new}.md so the next commit can move each file to
its final v03 path with no cascade and no path conflicts:
- delete v02 TRST-M-5.md and v02 TRST-L-6.md
- update the title line of TRST-L-7..L-11 to the new v03 number
- rename TRST-L-{old}.md -> TRST-L-{old}-{new}.md
Each rename here is a self-contained delete+add pair, so git's default
rename detection picks them up at >=96% similarity without -B.
…nal paths
Pure path rename of the five lows parked in the previous commit to
their final TRST-L-{new}.md paths. Each file's content is byte-
identical across the rename, so git's default detection sees all five
at 100% similarity (no -B required, no cascade).
Adds the v03 report PDF and aligns md responses with the auditor's text. - README: add 2nd fix-review commit, point at v03 PDF, refresh status column, drop M-5/L-6 rows (withdrawn) and add R-14 row, plus a footer noting the dropped findings and where their concerns were addressed. - TRST-M-1, M-4, L-6..L-10: status flipped to Fixed (M-1 main finding Acknowledged); v02 local response promoted into the auditor's "Team Response" section as the auditor incorporated it verbatim; new "Mitigation Review" section added with the v03 verdict. - TRST-R-14: new recommendation about avoiding magic numbers (`getAgreementDetails(0)` should use VERSION_CURRENT). No code changes; the contract fixes for these findings already landed in earlier commits.
…GNED (TRST-L-7) The v03 mitigation review on TRST-L-7 asks for it to be clarified that when a payer is represented by a separate signer (Authorizable), the signer — not the payer — must call cancel() with SCOPE_SIGNED for the revocation to take effect against subsequent accept/update. Updated the IAgreementCollector NatSpec on cancel() to spell out the per-scope caller requirement, including that combining SCOPE_SIGNED with SCOPE_PENDING / SCOPE_ACTIVE only makes sense when msg.sender is both payer and signer. Mirrored the clarification in the RecurringCollector implementation comment so the dev-doc and inline comment agree.
… safety margin (TRST-L-8) The v03 mitigation review on TRST-L-8 asks for tests that ensure the defined CALLBACK_GAS_OVERHEAD constant stays sufficient as the code evolves. Existing tests cover the lower bound (precheck reverts under tight gas) but do not assert the upper bound — that when the precheck passes, the EIP-150 cap still forwards at least MAX_PAYER_CALLBACK_GAS to the callee. Added recordedBeforeCollectionGasleft / recordedAfterCollectionGasleft to MockAgreementOwner, sampled at the first opcode of each callback, and a regression test asserting both stay within 500 gas of MAX. The current overhead constant lands the recorded value ~300 below MAX (function dispatch overhead), leaving ~200 gas of headroom against the 500 tolerance — so an edit that adds pre-CALL Solidity overhead trips the alarm before CALLBACK_GAS_OVERHEAD becomes outright insufficient.
…GIBILITY_CHECK (TRST-L-9) The v03 mitigation review on TRST-L-9 asks for it to be documented that turning CONDITION_ELIGIBILITY_CHECK on against an EOA payer is considered fully trusting that payer, since EIP-7702 lets the EOA attach a contract that returns false from isEligible() to block collections at any time. The existing comment treated the flag as a plain opt-in feature without naming the EOA-via-7702 caveat. Expanded the NatSpec on the constant declaration so the trust boundary is visible at the same site readers consult when deciding whether to opt into the flag for a given payer.
…Details (TRST-R-14) The v03 report's TRST-R-14 flags _getAgreementProvider passing 0 as the index argument to IAgreementCollector.getAgreementDetails: that 0 is no longer a placeholder, it now selects VERSION_CURRENT at the collector, and the named constant communicates intent and survives any future remapping of version indices. Imports VERSION_CURRENT from the interface and substitutes it at the single call site.
RembrandtK
changed the title
- subgraph-service: ScopedCancelActive integration test leaked the payer prank into post-cancel view-reads via resetPrank (startPrank). When fuzz picked rca.payer == SubgraphService's auto-deployed proxy admin (0x8816d8...), the verification reads on subgraphService reverted with ProxyDeniedAdminAccess. Switched to single-shot vm.prank for the cancel call and added a deterministic regression test using the failing seed. - horizon: TRST-L-8's MAX_PAYER_CALLBACK_GAS margin test asserts the production gas overhead, but `forge coverage` disables the optimizer, legitimately growing the dispatch δ past tolerance. Skip under FOUNDRY_COVERAGE=1 (set by the package's coverage scripts); direct `forge coverage` invocations still fail loudly. - foundry.toml (all 4 packages): exclude block-timestamp lint — pure noise across this codebase.
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
1 participant
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.
Audit response follow-ups on top of indexing-payments-management-audit-fix-reduced, addressing the v02 Trust report (TRST-M-4/5, L-6 through L-11, R-3 through R-13).
Recurring Collector
Recurring Agreement Manager
Subgraph Service
Interfaces