Indexing payments management audit fix 2#1323
Draft
RembrandtK wants to merge 31 commits intoindexing-payments-management-audit-fix-reducedfrom
Draft
Indexing payments management audit fix 2#1323RembrandtK wants to merge 31 commits intoindexing-payments-management-audit-fix-reducedfrom
RembrandtK wants to merge 31 commits intoindexing-payments-management-audit-fix-reducedfrom
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.
Indexing payments management audit fix 2
🚨 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-fix-reduced #1323 +/- ##
==================================================================================
+ Coverage 90.86% 91.04% +0.18%
==================================================================================
Files 81 81
Lines 5274 5317 +43
Branches 949 964 +15
==================================================================================
+ Hits 4792 4841 +49
+ Misses 453 451 -2
+ Partials 29 25 -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:
|
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.
RembrandtK
force-pushed
the
indexing-payments-management-audit-fix-2
branch
2 times, most recently
from
dfc4201 to
d288545
Compare
RembrandtK
force-pushed
the
indexing-payments-management-audit-fix-2
branch
from
d288545 to
e2c9297
Compare
…-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.
Assorted small refactors and interface tweaks that set the stage for the hash-keyed terms work 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
…reement to accept()
…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).
Align re-accept, re-update, and cancel-on-nothing semantics across RecurringCollector and SubgraphService so duplicate calls with the same signed terms are no-ops rather than reverts, cancel against a nonexistent agreement is a silent no-op, and an accepted agreement can be moved to a different allocation. RecurringCollector - accept(): short-circuits on Accepted state after _validateAndStoreAgreement, so re-accepting the same signed RCA is a no-op. Cancelled agreements still revert — re-accept of a cancelled agreement is never valid. - update(): short-circuits when activeTermsHash already equals the RCAU hash, skipping deadline and authorization checks on the idempotent path. - cancel(): _requirePayer returns silently (instead of reverting with RecurringCollectorAgreementNotFound) when no agreement or stored offer exists. Cancel against nothing is a no-op — same idempotent spirit. SubgraphService (IndexingAgreement library) - update(): defers state authority to the collector — _isValid replaces _isActive, and 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.
…to offer() Move the require(msg.sender == payer) from both _offerNew and _offerUpdate into offer() as a post-check on details.payer. Single site for the payer authorization requirement.
Preparatory cleanup ahead of the hash-keyed storage restructure: - 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`. - Reject expired offers at `offer()` time in `_offerNew`/`_offerUpdate` rather than deferring to `accept`/`update`. Previously an expired offer would be stored and only fail at acceptance. No storage or interface changes.
…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.
…(TRST-L-11) Restructure agreement storage around decoded terms keyed by EIP-712 hash. All internal reads access terms[hash] uniformly — no abi.decode, no offerType discrimination in read paths. Storage changes: - rcaOffers/rcauOffers → terms[hash] (single mapping, decoded fields) - AgreementData slimmed to identity + lifecycle + hash pointers (5 slots, was 7). Terms fields removed; getAgreement() returns the slim struct. - AgreementData.pendingTermsHash added for queued RCAU offers. Architecture: - _storeTerms: validates + stores + links (active or pending). Single write gate — every stored term is validated (invariant). - _termsFromRCA/_termsFromRCAU: type → AgreementTerms extraction. - _registerNew: composite for NEW path (register identity + store + link). Shared by offer(NEW) and accept(). Idempotent. - _activeClaimWindow + _maxClaimForTerms: replace 3-way state dispatch in max-claim calculation. Interface additions: - OfferCancelled event for SCOPE_PENDING cancellations. - AgreementData.pendingTermsHash.
…(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.
RembrandtK
force-pushed
the
indexing-payments-management-audit-fix-2
branch
from
e2c9297 to
bf35f19
Compare
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.
RembrandtK
force-pushed
the
indexing-payments-management-audit-fix-2
branch
from
bf35f19 to
a9c2737
Compare
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
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.
WIP: Preparing audit fix response