Post indexing payments audit#1338
Merged
Merged
Conversation
…ment registries Exposes the RecurringCollector contract across interfaces and toolshed so downstream DIPs consumers can build directly against it: - Re-export IRecurringCollector from @graphprotocol/interfaces under both the bare name and the conventional toolshed alias (RecurringCollector), matching the pattern used for other contracts. - Add toolshed/src/core/recurring-collector.ts (DIPs helpers, including conditions in RCA decoder) and a covering test. - Update toolshed deployment registries for issuance and REO contracts.
The set of packages exported by @graphprotocol/address-book was duplicated
three times: the exports map in package.json, FILES_TO_COPY in
copy-addresses-for-publish.js, and SYMLINKS_TO_RESTORE in restore-symlinks.js.
Drift had already crept in -- issuance was referenced everywhere but
src/issuance/ was missing on disk, which would break prepublishOnly.
Collapse to a single SOURCES array in scripts/sources.js consumed by both
scripts. Use a subpath pattern in package.json exports
("./*/addresses.json": "./src/*/addresses.json") so the exports map no
longer enumerates names.
Both scripts now mkdir src/<name>/ recursively and use rmSync({ force: true })
in place of the existsSync/unlinkSync dance, so a missing dir or stale entry
is handled silently. The copy step ends with a drift check that catches
src/<name>/ dirs not listed in SOURCES (stale leftovers from removed entries).
Verified end-to-end: pnpm pack produces a tarball containing real files
under src/{horizon,issuance,subgraph-service}/addresses.json, and
restore-symlinks puts the symlinks back at the original relative targets.
…an env fallback)
Three small workspace-root fixes needed to make the deployment package
and the legacy contracts test suite coexist:
- patches/rocketh@0.17.13.patch re-enables the deployScript skip()
hook. Upstream 0.17.13 has the skip path commented out, so any
deploy script defining `func.skip` was executed unconditionally;
the deployment package relies on skip() to short-circuit no-op
steps. Patch restores the call and removes the commented-out
scaffolding.
- pnpm packageExtensions for @nomiclabs/hardhat-waffle. Under pnpm
strict mode the package's missing peer declarations on
@ethereum-waffle/{chai,provider} cause hardhat-waffle to fail to
resolve them. The legacy contracts test suite still imports
hardhat-waffle, so declare the deps via packageExtensions rather
than upgrading the test suite off waffle.
- ARBISCAN_API_KEY env fallback in packages/contracts/hardhat.config.ts.
Hardhat keystore (`vars`) is the documented path, but CI and local
scripts often only set the environment variable; fall back to
process.env so verification works in both contexts.
The rest of the workspace had moved to ethers v6 and the @nomicfoundation/hardhat-* plugin generation; data-edge was the last holdout on ethers v5, @nomiclabs/hardhat-*, ethereum-waffle, and the @ethersproject/* sub-packages. The mismatch produced peer-dependency clashes that broke local network deploys whenever data-edge was pulled into the same Hardhat run as the rest of the workspace. Safe to do as a straight bump: - The package's deployed contracts only need recompilation to match a later toolchain; any new deployment naturally targets the newer versions, so there is no historical compatibility to preserve. - The migration is mechanical (ethers v6 API renames, BigNumber → bigint, chai-matchers move under @NomicFoundation, etc.) and stays contained to hardhat.config.ts, tasks, and tests — no contract code is touched. Also drops dead testnet entries (ropsten, rinkeby, kovan, arbitrum-goerli) and unused dependencies (tenderly, ethereum-waffle, ethlint, husky/lint-staged) on the way through.
…aims/IndexingAgreement libraries via deployImplementation Add recurringCollectorAddress as a module parameter and pass it into the SubgraphService constructor alongside the existing collector / curation / dispute-manager addresses, and link the StakeClaims, AllocationHandler, IndexingAgreementDecoder(Raw), and IndexingAgreement libraries. Uses the deployImplementation() helper from @graphprotocol/horizon/ignition rather than m.contract() — matches the pattern every other core module already uses (HorizonStaking, GraphPayments, PaymentsEscrow, RecurringCollector, RewardsManager, Curation). SubgraphService was the odd one out.
Align localNetwork config across horizon and subgraph-service so addresses match the deploy modules' hardcoded ACCOUNT1 convention. Previously caused OwnableUnauthorizedAccount reverts mid-batch when run-scripts signed with ACCOUNT1_SECRET. Covers: - horizon localNetwork migrate/protocol governor → ACCOUNT1 - horizon localNetwork pauseGuardian and subgraphAvailabilityOracle - subgraph-service localNetwork migrate/protocol governor → ACCOUNT1
Introduces packages/deployment with the deploy/ scripts, network configs, ABI generation, rocketh-based orchestration, and supporting tasks for the GIP-0088 testnet upgrade. Core infra: - governor registered as a rocketh named account so local/test signing works without explicit private-key plumbing - localNetwork tweaks to HorizonStaking ignition module - tag-deployment task annotation cleaned up (no commit-sha noise) Also includes: - drive RM revertOnIneligible from network config through ResolvedSettings into the upgrade governance batch; remove vestigial RecurringCollector block from network configs - gate sync rocketh-record seeding on artifact verification. When syncing a non-proxy address-book entry that has no rocketh deployment record, seed rocketh's record from the local artifact's bytecode only when the artifact's hash matches the address book's stored hash. Otherwise leave the 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. Extract the gate into a pure shouldSeedRocketh helper and pin its truth table. - cross-package config reconciliation test that catches drift between per-network Ignition config files in horizon/ and subgraph-service/: sibling agreement, localNetwork all-files $global agreement, same-package cross-prefix sub-object agreement, and mnemonic-index correctness for accounts referenced in localNetwork configs. - sync RewardsManager in mock REO integrate path (otherwise stale on subsequent runs); namespace mock REO governance-tx label.
Introduces a `*.todo.md` sidecar convention for capturing pending edits next to the contract source they target. Tracks deferred items so anyone working on the contract sees the pending list without having to touch the contract just to record an intent. Initial entries cover four files: - IHorizonStakingTypes.sol — LegacyAllocationState.Active NatSpec drift from PR #1331 nits. - BaseUpgradeable.sol — unused MILLION public constant; ABI-impact note on removal. - AllocationHandler.sol — legacy-allo migration block (~L170) confirmed unused; deferred to a post-horizon cleanup pass. - RecurringCollector.sol — EIP-170 bytecode-headroom refactor candidates, max-next-claim consolidation, the two remaining unreachable defensive branches (with disposition direction), the offer-keyed-terms storage restructure sketch, and pointers to the three preserved L-7 alternative tags (audit-l7-alt-{cascade-delete,independent-cancel,document-only}).
Extends the root lint:json prettier glob and the lint-staged config to cover *.json5 alongside *.json, then runs prettier across the 17 ignition / deployment-config files in the repo to baseline against the standard prettier style (unquoted keys, single quotes, trailing commas). The cross-package config-reconciliation test (14 cases) still passes — content is unchanged, only quote style + trailing commas shift. Going forward, the json5 ignition configs go through the same pre-commit prettier pass as everything else, so trailing-comma drift, key-quote drift, and bracketing irregularities get caught passively instead of accumulating in hand-edited config files.
…ules
Six of the eight horizon deploy modules hardcoded
`const governor = m.getAccount(1)` for transferOwnership targets, while
the subgraph-service side and all migrate modules read
`m.getParameter('governor')` from config. The dual convention required
anyone touching the system to know which one applied where, and it left
horizon's protocol.<network>.json5 configs silently missing a governor
field where SS's were not — exactly the gap that hid the SS-side
localNetwork governor drift fixed earlier on this branch.
Unify on `m.getParameter('governor')` for the six modules where
governor is only used as a target argument:
- core/{GraphPayments,PaymentsEscrow,RecurringCollector}
- periphery/{Controller,GNS,GraphProxyAdmin}
Two remaining call sites still need `m.getAccount(1)` because the
`from:` option on `m.call` only accepts an AccountRuntimeValue
(returned by `m.getAccount`) and not the ModuleParameterRuntimeValue
that `m.getParameter` returns:
- deploy.ts — Controller / GraphProxyAdmin acceptOwnership
- periphery/GraphToken.ts — L2GraphToken acceptOwnership
In both files the kept `m.getAccount(1)` carries a comment recording
the constraint and the expectation that configs set the 'governor'
parameter to the index-1 address of the deploying mnemonic.
Adds the `governor` field to the two horizon protocol configs that
didn't have one (matching the corresponding SS-side values):
- protocol.localNetwork.json5 → 0x70997970…79C8 (test mnemonic index 1)
- protocol.default.json5 → 0xFFcf8FDE…409f0 (legacy default mnemonic
index 1, matching SS-side protocol.default.json5)
All 14 cross-package config-reconciliation tests still pass, plus all
574 horizon, 290 subgraph-service, and 101 deployment unit/foundry
tests. Production deploys (arbitrumOne, arbitrumSepolia) go through
migrate which already used `m.getParameter('governor')`, so no
production behaviour changes.
Bumps the workspace solhint catalog entry within the major and refreshes the lockfile. All eight packages (contracts, horizon, issuance, subgraph-service, interfaces, data-edge, token-distribution, root) pick up the new version through the catalog. Also restructures the per-package .solhint.json files. Previously each used `"extends": ["solhint:recommended", "./../../.solhint.json"]`, which under solhint 6.2.1's reduceRight-based extend composition let solhint:recommended's rules win over the root override and re-enabled `compiler-version` (which the root config sets to `off` to accommodate the workspace's mix of pragmas — legacy interfaces on `^0.7.6 || ^0.8.0`, horizon/issuance/subgraph-service on `^0.8.27`, data-edge on `^0.8.12`). Switch each package to a single-string extends pointing at the root config: `"extends": "./../../.solhint.json"`. The root config itself extends solhint:recommended, so the rule chain is equivalent — but the single-indirection form propagates root overrides correctly under 6.2.1. After the bump + config fix, `pnpm lint:sol` runs clean across all packages: zero errors, zero warnings beyond the pre-existing pattern set. No contract source files were modified (verified — solhint --fix touched none).
This was referenced May 11, 2026
Post indexing payments audit
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1338 +/- ##
========================================
Coverage 91.16% 91.16%
========================================
Files 81 81
Lines 5318 5318
Branches 969 1128 +159
========================================
Hits 4848 4848
Misses 448 448
Partials 22 22
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:
|
…rings IssuanceAllocator.todo.md sidecar captures pending NatSpec fixes: - _advanceSelfMintingBlock and distributeIssuance docstrings still describe the pre-Self-Minting-Accumulation behaviour. - _distributeIssuance and _distributePendingIssuance both call _advanceSelfMintingBlock; one is redundant. Reword "selfMintingOffset == 0" framing on the test_DistributePendingIssuance_NoOffset_* tests to "fresh catch-up case": unconditional accumulation makes the literal phrasing inaccurate for the elapsed period inside the call.
Keep Graph_PR1334_v05.pdf as the canonical Trust Security audit report. Remove the v01–v04 intermediate fix-round PDFs, the per-finding TRST-*.md extracts, and the directory README that indexed them — all superseded by v05.
…om PR1301/ Adds YYYY-MM-DD_ prefix (date the report is signed on the cover page) and flattens packages/issuance/audits/PR1301/ — that sub- directory now contained a single PDF after the intermediate markdown was dropped. Renames: - Graph_PR1242_1243_v02.pdf → 2025-11-17_… - Graph_EligibilityOracle_v02.pdf → 2025-12-13_… - Graph_IssuanceAllocator_v03.pdf → 2025-12-28_… - Graph_PR1279_v02.pdf → 2026-02-15_… - PR1301/Graph_PR1334_v05.pdf → 2026-05-09_…
Dockerfile bakes `pnpm install --frozen-lockfile` + `pnpm build` into a
node:24-bookworm-slim base with Foundry. No entrypoint — consumers layer
their own (local-network's graph-contracts wrapper sets the deploy script).
Workflow builds multi-arch (linux/amd64 on ubuntu-24.04, linux/arm64 on
ubuntu-24.04-arm), pushes by per-platform digest, then merges to a manifest
list. Triggers on push to main, deployment/**, and v* tags. Tag scheme:
:<branch>, :sha-<short>, :<git-tag>.
docker-compose.yml + justfile add `just build-image` for local builds tagged
`ghcr.io/graphprotocol/contracts:${CONTRACTS_TAG:-local}`.
Maikol
reviewed
May 13, 2026
| @@ -0,0 +1,183 @@ | |||
| import assert from 'node:assert/strict' | |||
Member
There was a problem hiding this comment.
I don't think this test file is being run by any script, this package doesn't have unit tests so it doesn't have anything setup.
| IGraphProxyAdmin as GraphProxyAdmin, | ||
| IGraphTallyCollectorToolshed as GraphTallyCollector, | ||
| IHorizonStakingToolshed as HorizonStaking, | ||
| IRecurringCollector, |
Member
There was a problem hiding this comment.
We're exporting this twice, probably can remove this line and keep IRecurringCollector as RecurringCollector to match other contracts.
| } | ||
|
|
||
| /// @notice ERC165 interface detection. | ||
| /// @dev Supports IRewardsEligibility (0x66e305fd) and inherited interfaces. |
Member
There was a problem hiding this comment.
minor: this confused me a bit because I couldn't find the interface but there's some leftover artifacts:
packages/interfaces/artifacts/contracts/issuance/eligibility/IRewardsEligibility.sol/...
packages/contracts/artifacts/@graphprotocol/interfaces/contracts/issuance/eligibility/IRewardsEligibility.sol/...
| "DirectAllocation_Implementation": { | ||
| "address": "0xd5de0951759b8306226fa370a9ecca40a31aa2d3", | ||
| "deployment": { | ||
| "txHash": "", |
| * @param env - Rocketh environment (used to get chain ID from provider) | ||
| * @param update - Deployment update details | ||
| */ | ||
| updateHorizonAddressBook: async (env: Environment, update: DeploymentUpdate) => { |
Member
There was a problem hiding this comment.
nit: we could merge this and updateIssuanceAddressBook to avoid duplicated code.
| const configPath = resolve(__dirname, '..', 'config', `${networkName}.json5`) | ||
| try { | ||
| const raw = readFileSync(configPath, 'utf-8') | ||
| return JSON.parse(stripComments(raw)) as DeploymentConfigFile |
Member
There was a problem hiding this comment.
should we use JSON5.parse here since configs are json5 files? Not sure the exact behavior at the moment since we're passing an invalid json file to the parser.
|
|
||
| if (contract.addressBook === 'horizon') { | ||
| await graphUtils.updateHorizonAddressBook(env, update) | ||
| } else { |
Member
There was a problem hiding this comment.
I'm guessing we never call this with subgraphService address book? Maybe better to be explicit just in case?
- C1: drop orphan toolshed test - C2: dedupe IRecurringCollector interfaces export - C3: fix MockRewardsEligibilityOracle NatSpec (IProviderEligibility) - C4: gate DirectAllocation_Implementation metadata write on a real txHash; backfill addresses.json - C5+C7: dedupe updateXAddressBook; add subgraph-service route - C6: parse deployment .json5 via JSON5.parse Introduces buildDeploymentMetadata, getAddressBookForType, and graph.updateAddressBookForContract so the fixes are systemic.
Replace the ad-hoc addressBook-type ternaries across 7 sites with getAddressBookForType (and graph.updateAddressBookForContract for the deploy-standalone update path). Fixes a latent gap where deploy-standalone silently no-op'd subgraph-service contracts, and throws on missing shared-impl metadata in upgrade-implementation instead of writing an empty sentinel.
Deploy-time only recorded implementationDeployment; the proxy's own deployment record was left to sync's backfill. Add proxyDeployment to DeploymentUpdate and capture it in both deployProxyWithOwnImpl and deployProxyWithSharedImpl.
Library resolution was added to computeBytecodeHash but only the new-code sites adopted the linkReferences + resolver args. deployProxyWithOwnImpl and sync-utils' backfill paths kept calling the 1-arg form, diverging from checkShouldSync's artifact-side hash for library-using contracts. Introduce computeArtifactBytecodeHash + tryComputeArtifactBytecodeHash; route every deploy-time and sync callsite through them. Side-touch: replace verify-contract's private getAddressBook switch with getAddressBookForType.
The minimal setPendingImplementation landed alongside setPendingImplementationWithMetadata but was never wired up. Drop it and update doc references.
sync-utils had its own near-duplicates of two helpers from elsewhere in the package. Export tryLoadArtifactFromSource (parallel to loadArtifactFromSource) and toBlockNumber; route sync-utils' three blockNumber sites and seven artifact-load sites through them.
Five comments/docs introduced during GIP-0088 work named the 'rem-local-network' setup as if it were the project's canonical local network, and one TODO pointed at a /sync/notes/ path that isn't part of the repo. Both are personal dev infrastructure that doesn't belong in committed source. Reword in terms of the actual network (chainId 1337, Graph local network) and drop the external note pointer; the surrounding text stands on its own.
A forge crash leaves a multi-hundred-MB ELF core file (mode 0600) at `packages/<pkg>/core`. It's already in .gitignore (`**/core`), but without a matching .dockerignore entry BuildKit tries to send it into the build context and fails on the xattr probe — restrictive mode and large size both unhelpful. Anchor the pattern at `packages/*/core` so it matches only the dump file at the package root, not directories named `core` deeper in the tree (notably `packages/toolshed/src/core/`, toolshed's main entry).
Touched-content changes since the previous publish: - @graphprotocol/interfaces 0.7.1-dips.0 → 0.7.2 Removed the duplicate `IRecurringCollector` export from horizon types (kept only the I-prefix-stripped alias). Dropping the -dips prerelease qualifier on the way to a clean publish. - @graphprotocol/issuance 1.0.0 → 1.0.1 Backfilled DirectAllocation_Implementation deployment metadata (txHash + blockNumber) in arbitrumSepolia addresses.json. Mock NatSpec touch is cosmetic. - @graphprotocol/address-book 1.2.0 → 1.2.1 Re-publishes packages/issuance/addresses.json via symlink, so the same content change applies. - @graphprotocol/toolshed 1.2.1-dips.2 → 1.2.2 No published artefact change in this PR (only a test file deletion that was never in the npm bundle); bump drops the -dips qualifier alongside interfaces.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
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.
Follow-ups on top of
indexing-payments-management-audit-fix-3. 16 commits across deployment infrastructure, dips-publish prep, address-book, root tooling, and config-convention cleanup. No contract logic changes — pragma/source files untouched outside of the data-edge ethers-v6 toolchain refresh.What's in it
GIP-0088 deployment infrastructure (
8baf424b3)New
packages/deploymentwith deploy scripts, network configs, rocketh-based orchestration, and supporting tasks. Bundles the RMrevertOnIneligibleconfig wiring, the syncrocketh-recordartifact-verification gate (withshouldSeedRockethtruth-table tests), the cross-package config-reconciliation test, and the mock REO sync/label fixes.Audit-fix-3 dips-publish prep
dc6f9fb3ainterfaces re-export ofIRecurringCollector; toolshedrecurring-collector.tshelpers + DIPs decoder conditions; deployment registries for issuance and REO.b034dbc5fMockRewardsEligibilityOracletest mock.807c03332SubgraphService now wiresRecurringCollector+StakeClaims/IndexingAgreementlibraries viadeployImplementation().76546e1b7version bumpsinterfaces 0.7.1-dips.0/toolshed 1.2.1-dips.2.Address book
38828bc8dconsolidate publish-source list (collapses three duplicated package enumerations into oneSOURCESarray); adds the missingsrc/issuance/addresses.jsonsymlink.247b49d621.2.0 changelog + publishing guide.87d5abbb3arbitrumSepolia address + deployment-metadata refresh.Tooling
d1495a660root tooling compat:rocketh@0.17.13patch (re-enablesdeployScript.skip()), pnpmpackageExtensionsfor@nomiclabs/hardhat-wafflepeer deps,ARBISCAN_API_KEYenv fallback.a9682e183data-edge: ethers v6 +@nomicfoundation/hardhat-*plugins (resolves peer-dep clashes on local-network deploys).f7c9e99f5json5 files inlint:jsonglob + baseline prettier pass across 17 ignition/deployment configs.25052413fsolhint catalog^6.0.3→^6.2.1, simplifies per-package.solhint.jsonextends to a single string (fixescompiler-versionoverride propagation under 6.2.1).Config / convention cleanup
16e7db9b4localNetwork governor/pauseGuardian/SAO alignment to ACCOUNT1 across horizon and subgraph-service configs.d8c79aceahorizon ignition modules drivegovernorfromm.getParameter('governor')(6 of 8 modules; 2 keepm.getAccount(1)forfrom:signer because ignition'sfrom:only acceptsAccountRuntimeValue). Addsgovernorfield to horizon'sprotocol.{default,localNetwork}.json5.Documentation
56ec1e9fbREO testing plans (baseline, indexer test guide, rewards-conditions, subgraph-denial, mainnet/testnet details, support tooling) anddocs/RewardsBehaviourChanges.md.0bf5d9cbeintroduces a*.todo.mdsidecar convention for capturing pending edits next to contract source. Initial entries:IHorizonStakingTypesNatSpec drift,BaseUpgradeableunusedMILLION,AllocationHandlerlegacy-allo cleanup,RecurringCollectorbytecode-headroom / max-next-claim / unreachable-defensive-branches / offer-keyed-terms-storage + pointers to preserved L-7 alternative tags.