Deployment preparation for testnet-dips#1326
Draft
RembrandtK wants to merge 144 commits intomainfrom
Draft
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>
…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.
…24/audit-fix-2 Bring in GIP-0088 deployment infrastructure from reo-deployment-3 onto the indexing-payments-management-audit-fix-2-light contract source so a scratch testnet deployment can be made from this branch. Conflict resolution policy: production contracts and their tests stay as per indexing-payments-management-audit-fix-2-light (HEAD); deployment infra comes from reo-deployment-3. Conflicts resolved: - packages/deployment/lib/abis.ts -> reo-deployment-3 (generated ABI infra) - packages/horizon/test/unit/payments/recurring-collector/accept.t.sol -> -light - packages/horizon/test/unit/utilities/Authorizable.t.sol -> -light - packages/issuance/contracts/allocate/DirectAllocation.sol -> -light (ERC165 check) - packages/issuance/test/unit/agreement-manager/ensureDistributed.t.sol -> -light - packages/issuance/test/unit/direct-allocation/DirectAllocation.t.sol -> -light
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Deployment preparation for testnet-dips
🚨 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 #1326 +/- ##
==========================================
+ 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:
|
eligibility_revert.ts now reads RewardsManager.revertOnIneligible from config/<network>.json5, defaulting to true (the expected target for all deployments). Idempotency check compares on-chain to desired instead of skipping when already true. Status check (getRewardsManagerChecks) compares the on-chain value against the same config-resolved desired value and shows ✓/✗ instead of a neutral info line, surfacing config-vs-chain drift in deploy:status. Extracts loadDeploymentConfigForChain(chainId) — a silent, sync, chainId-keyed loader — so the status task (which has no Environment) can read config too. The async env-based loadDeploymentConfig delegates to it. Drops the now-redundant RewardsManager block from all three network configs (arbitrumOne, arbitrumSepolia, localNetwork); the field remains in the DeploymentConfig type as an escape hatch.
…work configs The empty RecurringCollector block carried only a comment explaining why pause guardian is not config-driven — context that mattered when the per-network config was first introduced but is no longer load-bearing. The RecurringCollector entry in DeploymentConfig stays so eip712Name and revokeSignerThawingPeriod can be set per-network if a future deployment needs them; today both fall through to script defaults.
Replace per-call defaulting (config.X?.Y ?? defaultY scattered across deploy scripts and status checks) with a single getResolvedSettings(chainId) that returns a fully-resolved settings object. Defaults live in one DEFAULT_SETTINGS constant; consumers read concrete fields from a non-optional type. Eliminates the duplicated default for RewardsManager.revertOnIneligible (was in eligibility_revert.ts and status-detail.ts) and the same latent duplication risk for IssuanceAllocator and RecurringCollector fields. Removes the env-based loadDeploymentConfig and the chainId-only loadDeploymentConfigForChain in favour of getResolvedSettings(chainId) plus a thin getResolvedSettingsForEnv(env) convenience wrapper. Drops the per-call "Loaded config from..." log lines, which fired multiple times per deploy run with no useful signal. Per-network targeting is preserved — chainId remains the lookup key, networks override only what they need. Defaults are uniform across networks; if a network needs a different value, it states so explicitly.
…ance batch Adds checkRMRevertOnIneligible precondition helper. 04_upgrade.ts now queues setRevertOnIneligible(<config>) into the governance batch when on-chain doesn't match config — gated on RM upgraded, alongside the existing setDefaultReclaimAddress block. upgrade/10_status.ts surfaces mismatch as a deferred issue alongside Reclaim, so the Next-step hint correctly accounts for it. No new goal or activation tag — revertOnIneligible is RM-side governance config that rides the existing upgrade governance batch, same shape as setDefaultReclaimAddress. The desired value flows from getResolvedSettings into deploy, status, verification, and per-component display.
…ation
When syncing a non-proxy address-book entry that has no rocketh deployment
record, sync was unconditionally seeding rocketh's record from the local
artifact's bytecode. If the artifact had drifted from on-chain (source
recompiled since the impl was deployed), this masked the change: rocketh's
native bytecode comparison then matched its (just-seeded) record against the
artifact and returned newlyDeployed=false. The address book never advanced,
proxy scripts saw no impl-address change, and shared-impl proxies
(DefaultAllocation, ReclaimedRewards) ended up with code-changed status but
no pendingImplementation set — so the upgrade orchestrator skipped them.
Mirror the gate the proxy path already uses for ${name}_Implementation:
seed only when the local artifact's bytecode hash matches the address
book's stored hash. When it doesn't, leave rocketh's record absent so the
next deployFn sees no prior bytecode and produces newlyDeployed=true,
propagating the new impl address through to proxy pending-upgrade detection
naturally.
Scope of the gate:
- Only registered registry names — proxy sync recurses with synthetic names
(e.g. RewardsManager_Implementation) that aren't real address-book
entries; the proxy path has its own hashMatches gate before recursing.
- Only non-prerequisites — externally-deployed contracts (L2GraphToken)
are never run through deployFn, so the dedup-masking concern doesn't
apply and they still need an env record for downstream reads.
Also replace the silent fall-through in deployProxyContract's shared-impl
branch with a throw when env.getOrNull(sharedImplementation.name) returns
null. With the sync fix in place, missing implDep is no longer a routine
state masking drift; it's a real signal that the impl deploy script didn't
run or didn't produce a record.
Extract the inline gate from syncContract's non-proxy seed path into a
pure shouldSeedRocketh helper and pin its truth table.
The helper already had three known failure modes during this fix's
development: it had to let unregistered synthetic names through (proxy
sync recurses with `${name}_Implementation` names), let prerequisites
through (L2GraphToken-style external deployments), and skip the seed
only on a *verified* mismatch — not whenever any signal is missing.
Each of those is a regression test: getting the gate wrong on any of
them broke a real deploy run.
Consolidates publish.yml improvements (address-book choice, Read package info, Tag release) from reo-deployment branches plus new interfaces and toolshed package choices and a dry_run boolean input so auth/packaging can be verified without burning a version.
Replace GRAPHPROTOCOL_NPM_TOKEN with GitHub OIDC (id-token: write) and pnpm --provenance so the workflow mints a short-lived credential and attaches a SLSA build attestation. Each target package needs its own Trusted Publisher entry on npmjs.com (owner: graphprotocol, repo: contracts, workflow: publish.yml); verify per-package via a dry_run dispatch before a real publish. Pattern follows graphprotocol/graph-node#6460. contents: write is retained so the existing Tag release step can push the annotated tag.
pnpm publish delegates registry auth to the underlying npm CLI, which needs to be >= 11.5.1 to exchange the GitHub OIDC token for an npm publish credential. The shared setup action brings Node 22 which ships an older npm; install the latest npm globally in the publish job before pnpm publish runs. Pattern follows eslint/config-inspector#174.
The repo's root package.json declares pnpm@10.28.0 as the packageManager, but the shared setup action's corepack prepare step pinned the older 10.17.0. Resolve the drift by matching the declared version. Minor-version bump within pnpm 10.x; affects lint, build-test, verifydeployed, and publish workflows that share this setup.
Reproducibility: pinning avoids surprise behavior changes or Node engine bumps from a future npm "latest". Bump the pin intentionally; only constraint is npm >= 11.5.1 for OIDC trusted publishing.
- actions/setup-node: v4 → v6, node-version 22 → 24 - actions/checkout: v3/v4 → v6 - actions/upload-artifact: v3 → v7 (v3 was sunset by GitHub) - actions/download-artifact: v3 → v8 (v3 was sunset by GitHub) - actions/github-script: v7 → v9 - codecov/codecov-action: v3 → v6 - Add description: field to setup composite action (schema) Fixes the npm install -g npm@11.13.0 cross-major self-upgrade crash seen on Node 22 — Node 24 ships npm 11.x, so this is now an 11→11 swap. Also clears the Node 20 runtime deprecation warning on JS actions. cache: 'pnpm' on setup-node@v6 verified: v6's "limit automatic caching to npm" change only affects auto-detection when cache is unset; explicit cache: 'pnpm' is unaffected.
Node 24 ships npm 11.x bundled (24.15.0 → npm 11.12.1), which already satisfies the OIDC trusted-publishing requirement of npm >= 11.5.1. The explicit `npm install -g npm@11.13.0` step was a Node 22 workaround and is now redundant. Pinning the Node version effectively pins npm; if stricter pinning is ever needed it can be reintroduced.
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
4 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.
Working branch. Expected to be dropped in favour of rebased versions when contract audit completes and/or other changes are merged to main.