Indexing payments management audit fix 2 light#1325
Open
RembrandtK wants to merge 37 commits intoindexing-payments-management-audit-fix-reducedfrom
Open
Indexing payments management audit fix 2 light#1325RembrandtK wants to merge 37 commits intoindexing-payments-management-audit-fix-reducedfrom
RembrandtK wants to merge 37 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.
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 2 light
🚨 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 @@
## main #1325 +/- ##
==========================================
+ Coverage 88.55% 91.15% +2.59%
==========================================
Files 75 81 +6
Lines 4615 5337 +722
Branches 981 973 -8
==========================================
+ Hits 4087 4865 +778
+ Misses 507 449 -58
- Partials 21 23 +2
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
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