Indexing payments management audit fixed rebased#1331
Draft
RembrandtK wants to merge 125 commits intomainfrom
Draft
Indexing payments management audit fixed rebased#1331RembrandtK wants to merge 125 commits intomainfrom
RembrandtK wants to merge 125 commits intomainfrom
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>
…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.
…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.
Indexing payments management audit fixed rebased
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
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.
Rebase of #1301 (
indexing-payments-management-audit) and #1325 (indexing-payments-management-audit-fix-2-light) onto currentmain.Commits being compared
Output:
Drift introduced by main since the original merge base
23 files. Zero file-level overlap with the audit-branch surface, no behavioural interaction with anything under audit. Single substantive production-contract change:
This contract change was separately audited and is not in scope of this audit.
Equivalence checks