Deployment preparation for testnet-dips#1326
Draft
RembrandtK wants to merge 166 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>
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.
Bump from 0.6.6 → 0.7.0-test.0 to validate the OIDC trusted-publishing flow end-to-end. Prerelease tag (-test.0) keeps it off the latest dist-tag and makes it easy to discard if the test reveals issues. Real 0.7.0 release will follow once trusted publishing is verified; 0.6.6 → 0.7.0 reflects the breaking changes accumulated since 0.6.6 (deleted IDataServiceFees/Rescuable/IHorizonStakingExtension, renamed IRewardsEligibility → IProviderEligibility, reworked IRewardsManager).
npm's trusted-publishing flow cross-checks package.json's repository.url against the source repo recorded in the Sigstore provenance attestation. Without this field, publish fails at the registry side after provenance has already been signed: Failed to validate repository information: package.json: "repository.url" is "", expected to match "https://github.com/graphprotocol/contracts" from provenance Mirrors the shape used in @graphprotocol/contracts; adds directory for proper monorepo source linking.
Single source of truth for the Node version. Local fnm reads .nvmrc and auto-switches; CI setup-node reads the same file via node-version-file. Corepack picks pnpm from the packageManager field, so the explicit `corepack prepare pnpm@X.Y.Z --activate` step is no longer needed. - Add .nvmrc (24) - Add .npmrc (engine-strict=true) so pnpm refuses wrong Node/pnpm - package.json: add engines (node ^24, pnpm ^10.28) - .github/actions/setup/action.yml: switch to node-version-file: '.nvmrc', drop the redundant Corepack prepare step
npm's trusted-publishing flow cross-checks package.json's repository.url against the source repo recorded in the Sigstore provenance attestation. Without this field, publish fails at the registry side after provenance has already been signed: Failed to validate repository information: package.json: "repository.url" is "", expected to match "https://github.com/graphprotocol/contracts" from provenance Mirrors the shape applied to @graphprotocol/interfaces in a61ed9a; adds directory for proper monorepo source linking.
…tion Mirrors the fix applied to interfaces (a61ed9a) and toolshed (9ba6366a3). Without this field, OIDC trusted-publishing will fail at the registry's provenance verification step: Failed to validate repository information: package.json: "repository.url" is "", expected to match "https://github.com/graphprotocol/contracts" from provenance Preemptive — address-book has not been republished since the OIDC switch, but the workflow lists it as a choice.
@graphprotocol/sdk was extracted to its own repo in 32ef00c ("chore: move sdk to own repo, update versions"); packages/sdk/ no longer exists in this monorepo. Dispatching package=sdk would fail at the Read package info step trying to require a non-existent package.json.
…orepo source linking Drop the .git suffix from repository.url and add directory: packages/contracts to match the shape used in interfaces (a61ed9a), toolshed (9ba6366a3), and address-book (c3e219ba1). The .git suffix is normalized away by npm's provenance verification, so the previous shape would likely have worked, but this keeps the four publishable packages consistent and gives npmjs.com a correct subfolder link.
- Add IRecurringCollector to GraphHorizonContracts interface - Re-export IRecurringCollector from interfaces main entrypoint - Add encodeCollectIndexingFeesData() helper for indexing fee collection - Add decoders: decodeSignedRCA, decodeAcceptIndexingAgreementMetadata, decodeIndexingAgreementTermsV1 - Add round-trip tests for all decoders and encoder
The audit-branch RecurringCollectionAgreement struct added a uint16 conditions field at position 9 (between maxSecondsPerCollection and nonce). The toolshed decoder tuple was missing it, so decodeSignedRCA read 10 fields against 11 ABI-encoded fields: nonce read what was actually conditions, metadata read the wrong offset, decode threw. The indexer-agent relies on this decoder to read pending RCA proposals the indexer-service persists. Without the fix, every proposal logs "Failed to decode pending RCA proposal" and is skipped, so the agent never calls acceptIndexingAgreement on-chain and DIPs agreements expire in dipper's DB. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Re-publish needed because 1.2.1-dips.0 from audit-fix-2 was missing the recurring-collector module (decodeSignedRCA, decodeAcceptIndexingAgreementMetadata, decodeIndexingAgreementTermsV1, SignedRCA type). Cherry-picked from mb9/dips-local-testing-fixes: - 25c0b88 feat: add RecurringCollector type and DIPs helpers to toolshed - 3e739a3 fix(toolshed): add conditions field to RCA decoder tuple These decoders are imported by indexer-common's pending-rca-consumer and types modules; without them indexer-agent's yarn prepare fails.
…les' ACCOUNT1 Phase 1 deploy modules hardcode m.getAccount(1) (ACCOUNT1) when transferring ProxyAdmin ownership for new proxies, but migrate.localNetwork.json5 declared ACCOUNT0 as governor. New ProxyAdmins minted by the GIP-0088 upgrade phase therefore landed with ACCOUNT0 as owner, while local-network's issuance.run.sh signs upgrade txs with GOVERNOR_KEY=ACCOUNT1_SECRET, causing OwnableUnauthorizedAccount reverts mid-batch. Align migrate config with the deploy modules and the run-script.
…oy modules' ACCOUNT1
deploy:protocol Phase 1.3 (Deploy2Module → DisputeManager.ts /
SubgraphService.ts) transfers the freshly-minted ProxyAdmins to
m.getParameter('governor'), which on localNetwork resolved to
ACCOUNT0. Local-network's issuance.run.sh signs subsequent
upgradeAndCall txs with GOVERNOR_KEY=ACCOUNT1_SECRET, so the
DisputeManager / SubgraphService PA upgrades reverted with
OwnableUnauthorizedAccount.
Bumping the SS-side protocol-config governor to ACCOUNT1 brings
local-network into alignment with both the run-script and the
m.getAccount(1) convention horizon's deploy modules already use.
Production migrate paths (migrate.<network>.json5) are unaffected.
…izon sibling Mirror the earlier horizon migrate-config bump on the SS side so both migrate.localNetwork.json5 files use ACCOUNT1 as governor. Not load-bearing for any current flow (the migrate path isn't run on local-network), but keeps the two sibling files in sync and consistent with the m.getAccount(1) convention used by horizon's deploy modules.
…lityOracle addresses The header comment claims addresses are derived from the hardhat default mnemonic, but the values were copied from protocol.default.json5 which uses the older 'myth like bonus' mnemonic. Replace with the correct hardhat-default-mnemonic addresses at the stated indices. Latent — both values are passed to setPauseGuardian / setSubgraph- AvailabilityOracle on local-network, but no current local-network test exercises pause or availability-oracle functionality, so the mismatch hadn't surfaced. Caught while building the cross-package config reconciliation test.
Static unit test that catches drift between per-network Ignition
config files in packages/horizon/ignition/configs/ and
packages/subgraph-service/ignition/configs/.
Four checks:
1. Cross-package sibling agreement. For each (prefix, network) pair
where both packages have a file (e.g. both migrate.arbitrumOne.json5),
every overlapping non-empty $global field must match.
2. localNetwork all-files $global agreement. For localNetwork
specifically (one stack, one governor) every $global field
meaningfully declared in more than one of the four
{horizon,subgraph-service}/{migrate,protocol}.localNetwork.json5
files must match across all of them. Stricter than #1 — catches
same-package cross-prefix drift.
3. localNetwork same-package cross-prefix sub-object agreement. Each
package's per-contract config blocks (e.g. "DisputeManager": { ... },
"RecurringCollector": { ... }) must agree leaf-by-leaf between
migrate and protocol. Catches drift in fields like eip712Name /
eip712Version (signature verification breakers) and disputePeriod /
disputeDeposit parameters. Restricted to localNetwork because for
other networks (notably default) migrate and protocol are
intentionally different templates.
4. localNetwork mnemonic-index correctness. Lines like
"governor": "0x70997970…", // index 1
must have an address that derives from the hardhat default mnemonic
at the stated BIP44 index. Catches copy-paste mistakes where the
value updates but the comment doesn't, or vice versa.
Verified to fail on the SS protocol governor drift the test was
designed against: reverting governor in
subgraph-service/ignition/configs/protocol.localNetwork.json5 to
ACCOUNT0 produces a clean three-way mismatch report from check #2.
Adds json5 as a direct devDep (was transitively available via
toolshed but not declarable that way under pnpm strict mode).
…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.
…nt/testnet-dips/2024-04-28/audit-fix-2 # Conflicts: # packages/toolshed/src/core/recurring-collector.ts
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
6 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.