Indexing payments management audit fixed rebased by RembrandtK · Pull Request #1331 · graphprotocol/contracts

RembrandtK · 2026-05-05T15:53:12Z

Rebase of #1301 (indexing-payments-management-audit) and #1325 (indexing-payments-management-audit-fix-2-light) onto current main.

For subsequent fixes see: #1334

Commits being compared

ORIG=indexing-payments-management-audit-pre-rebase   # pre-rebase tag
REB=2292e6ae8                                        # the rebase result
OLD_BASE=ddee12b11                                   # where ORIG branched off main
NEW_BASE=52b5356d3                                   # where REB branched off main

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:

git diff "$ORIG" "$REB" --name-status -- '*.sol'
# Output: M       packages/token-distribution/contracts/GraphTokenLockWallet.sol

This contract change was separately audited and is not in scope of this audit.

For reference, audit scope files (with exclusion added for token-distribution):

git diff $NEW_BASE $REB --name-status --diff-filter=d -- \
    '*.sol' ':!*/test*' ':!*.t.sol' ':!*Helper.sol' ':!*/mock*' ':!*/toolshed/*' \
    ':!packages/token-distribution'

Equivalence checks

# Sanity: the rebase moved ORIG from OLD_BASE to NEW_BASE.
git merge-base "$ORIG" "$NEW_BASE"                     # = OLD_BASE
git merge-base --is-ancestor "$OLD_BASE" "$NEW_BASE"   # exits 0

# The drift between ORIG and REB is exactly main's drift since OLD_BASE, nothing else changed.
diff <(git diff "$ORIG" "$REB") <(git diff "$OLD_BASE" "$NEW_BASE")
# expected: empty

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>

…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.

openzeppelin-code · 2026-05-05T16:11:13Z

Indexing payments management audit fixed rebased

Generated at commit: 675d39716add4192e0e65ecbd92c5a27f050d26a

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	3 5 0 14 38 60
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

For more details view the full report in OpenZeppelin Code Inspector

- subgraph-service: ScopedCancelActive integration test leaked the payer prank into post-cancel view-reads via resetPrank (startPrank). When fuzz picked rca.payer == SubgraphService's auto-deployed proxy admin (0x8816d8...), the verification reads on subgraphService reverted with ProxyDeniedAdminAccess. Switched to single-shot vm.prank for the cancel call and added a deterministic regression test using the failing seed. - horizon: TRST-L-8's MAX_PAYER_CALLBACK_GAS margin test asserts the production gas overhead, but `forge coverage` disables the optimizer, legitimately growing the dispatch δ past tolerance. Skip under FOUNDRY_COVERAGE=1 (set by the package's coverage scripts); direct `forge coverage` invocations still fail loudly. - foundry.toml (all 4 packages): exclude block-timestamp lint — pure noise across this codebase.

socket-security · 2026-05-06T10:48:45Z

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	@types/json5@2.2.0
	@nomiclabs/hardhat-etherscan@3.1.8
	arbos-precompiles@1.0.2
	axios@1.12.2
	@openzeppelin/foundry-upgrades@0.4.0
	@rocketh/export@0.17.16
	@graphql-yoga/plugin-persisted-operations@3.15.2
	@rocketh/read-execute@0.17.8
	@rocketh/deploy@0.17.8
	@tenderly/hardhat-tenderly@1.11.0
	@rocketh/verifier@0.17.16
	@types/glob@8.1.0
	@rocketh/core@0.17.8
	@rocketh/proxy@0.17.12
	@types/sinon-chai@3.2.12
	@typescript-eslint/parser@8.53.1
	@rocketh/node@0.17.16
	@rocketh/doc@0.17.16
	@tenderly/api-client@1.1.0
	@rocketh/diamond@0.17.11
	@nomicfoundation/hardhat-foundry@1.2.0
	@nomicfoundation/hardhat-toolbox@4.0.0
	@types/chai@4.3.20
	@wagmi/cli@2.5.1
	@types/mocha@10.0.10
	@types/mocha@9.1.1
	@nomicfoundation/ignition-core@0.15.13
	@nomiclabs/hardhat-waffle@2.0.6
	@nomiclabs/hardhat-ethers@2.2.3
	@typescript-eslint/eslint-plugin@8.53.1
	@types/node@20.19.14
	@openzeppelin/contracts@3.4.2
See 21 more rows in the dashboard

View full report

socket-security · 2026-05-06T10:48:47Z

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.

Action	Severity	Alert (click "▶" to expand/collapse)
Warn		Critical CVE: Babel vulnerable to arbitrary code execution when compiling specifically crafted malicious code in npm `babel-traverse` CVE: GHSA-67hx-6x53-jw92 Babel vulnerable to arbitrary code execution when compiling specifically crafted malicious code (CRITICAL) Affected versions: >= 0 Patched version: No patched versions From: pnpm-lock.yaml → `npm/babel-traverse@6.26.0` ℹ Read more on: This package \| This alert \| What is a critical CVE? Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at `support@socket.dev`. Suggestion: Remove or replace dependencies that include known critical CVEs. Consumers can use dependency overrides or npm audit fix --force to remove vulnerable dependencies. Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment `@SocketSecurity ignore npm/babel-traverse@6.26.0`. You can also ignore all packages with `@SocketSecurity ignore-all`. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.
Warn		Obfuscated code: npm `@react-native/debugger-frontend` is 96.0% likely obfuscated Confidence: 0.96 Location: Package overview From: pnpm-lock.yaml → `npm/@react-native/debugger-frontend@0.81.4` ℹ Read more on: This package \| This alert \| What is obfuscated code? Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at `support@socket.dev`. Suggestion: Packages should not obfuscate their code. Consider not using packages with obfuscated code. Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment `@SocketSecurity ignore npm/@react-native/debugger-frontend@0.81.4`. You can also ignore all packages with `@SocketSecurity ignore-all`. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.
Warn		Obfuscated code: npm `buffer` is 96.0% likely obfuscated Confidence: 0.96 Location: Package overview From: pnpm-lock.yaml → `npm/@openzeppelin/hardhat-upgrades@1.28.0` → `npm/@openzeppelin/foundry-upgrades@0.4.0` → `npm/buffer@4.9.2` ℹ Read more on: This package \| This alert \| What is obfuscated code? Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at `support@socket.dev`. Suggestion: Packages should not obfuscate their code. Consider not using packages with obfuscated code. Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment `@SocketSecurity ignore npm/buffer@4.9.2`. You can also ignore all packages with `@SocketSecurity ignore-all`. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

After the audit branch was rebased onto current main as indexing-payments-management-audit-fixed-rebased, the linear chain was re-signed and the SHAs cited throughout the report no longer resolve. Anchor the pre-rebase tip at tag indexing-payments-management-audit-pre-rebase and provide the mapping plus a one-line equivalence check.

codecov · 2026-05-06T11:46:30Z

Codecov Report

❌ Patch coverage is 99.06890% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.16%. Comparing base (52b5356) to head (675d397).

Files with missing lines	Patch %	Lines
...ntracts/payments/collectors/RecurringCollector.sol	99.20%	3 Missing ⚠️
...ges/contracts/contracts/rewards/RewardsManager.sol	96.42%	1 Missing ⚠️
...n/contracts/data-service/libraries/StakeClaims.sol	96.15%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1331      +/-   ##
==========================================
+ Coverage   88.62%   91.16%   +2.54%     
==========================================
  Files          75       81       +6     
  Lines        4615     5342     +727     
  Branches      823      975     +152     
==========================================
+ Hits         4090     4870     +780     
+ Misses        504      449      -55     
- Partials       21       23       +2

Flag	Coverage Δ
unittests	`91.16% <99.06%> (+2.54%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign-off commit bbec75e. v04 addressed recommendations only; no new findings or status changes versus v03.

Maikol · 2026-05-06T19:07:35Z

        address graphTokenGateway,
-        address graphProxyAdmin,
-        address graphCuration
+        address graphProxyAdmin


this is a breaking change for the event, I don't think it's being used by the network subgraph but maybe we'll want to update the abi eventually so it's not broken.

@tmigone do you want to answer? (From commit 392047c I think.)

definitely not used in the network subgraph or by anyone, i think it's safe

Maikol · 2026-05-06T19:22:06Z

+     * @param allocationId The id of the allocation
+     * @param subgraphDeploymentId The id of the subgraph deployment
+     */
+    event LegacyAllocationMigrated(


I believe this is no longer being used so we could delete. Maybe leave for a post-post-horizon cleanup 😅

LIkely. I am not sure if it was ever used. @matiasedgeandnode? (From commit a7fb875.)

this was never used indeed. added with horizon to migrate legacy allo ids from staking to subgraph service but we never executed on it and have since decided to remove with clean up.
its likely it got re-added with matias' commit but we don't need it.

Maikol · 2026-05-06T20:12:05Z

+ * - getRewards
+ * These functions may overestimate the actual rewards due to changes in the total supply
+ * until the actual takeRewards function is called.
+ * custom:security-contact Please email security+contracts@ thegraph.com (remove space) if you find any bugs. We might have an active bug bounty program.


security+contracts@ thegraph.com (remove space)

why can't we have the proper email without the space? Also, shouldn't this be:

@custom:security-contact

?

A work-around due to limitation of pragma version compiler; 0.76 does not support that natspec form (introduced in 0.8.2).

BTW not introduced by this branch, already on main?

Maikol · 2026-05-06T20:31:13Z

+     * @notice Accept a Recurring Collection Agreement.
+     * @dev Caller must be the data service the RCA was issued to.
+     * If `signature` is non-empty: checks `rca.deadline >= block.timestamp` and verifies the ECDSA signature.
+     * If `signature` is empty: the payer must be a contract implementing {IAgreementOwner.approveAgreement}


This function doesn't exist for IAgreementOwner

Yes, good point. :) It was changed, and even then the comment was out of date. Did a sweep of a few related stale doc issues, committed them in #1334.

…eClaimsClaimNotFound revert Closes the two real coverage gaps from PR #1331: - RewardsManager.supportsInterface(IProviderEligibilityManagement.interfaceId) was the only OR-clause never asserted true. - StakeClaims.processStakeClaim's claim-not-found require was never exercised; reachable only via library misuse, so test directly through a thin harness with empty mappings.

Maikol · 2026-05-07T13:32:31Z

+     * States:
+     * - Null = indexer == address(0)
+     * - Active = not Null && tokens > 0
+     * - Closed = Active && closedAtEpoch != 0


nit: this looks wrong. Maybe the intention was to replace not Null && tokens > 0 with Active referencing the previous definition for active but the result feels strange to me.

Maikol · 2026-05-07T13:35:01Z

+     * @dev Possible states a legacy allocation can be.
+     * States:
+     * - Null = indexer == address(0)
+     * - Active = not Null && tokens > 0


Implementation doesn't check that tokens > 0, we can change this to

not NULL && closedAtEpoch == 0

matiasedgeandnode and others added 30 commits June 11, 2025 14:08

feat: implement Indexing Agreements

a7fb875

test: Add extra tests for Indexing Agreements

1651716

fix: [TRST-H-1] IndexingAgreement.collect() on CanceledByPayer

b53ca01

fix: Remove PaymentType constraint from RecurringCollector

da42fb4

fix: [TRST-H-2] Only agreement owner can collect indexing fee

7695c9e

fix: [TRST-H-3] collect() checks provision

8048c4c

fix: [TRST-L-3] Add deterministic agreement ID

aac9f8b

fix: [TRST-L-6] Proper agreement version check

e3d2787

document: [TRST-L-7] update()

308d6e6

fix: [TRST-L-9] Cancel agreement if over-allocated

17b794e

fix: [TRST-R-1] minor fixes

6d9a18c

fix: [TRST-R-4] CEI violation

58b7a28

fix: [TRST-R-5] Terms validation

5f732ac

fix: [TRST-R-6] Configurable indexing fees cut

b492251

fix: [TRST-M-2] improve _getCollectionInfo()

0e469be

feat(interfaces): remove staking extension from interfaces

f65877e

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

chore: more interface removal

714b32c

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

feat: clean up post horizon on horizon contracts

392047c

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

test: update horizon tests after cleanup

28f2661

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

fix: more cleanup on horizon, interfaces and toolshed packages

1d2b76d

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

fix: contracts changes

ac103d7

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

feat: remove stuff from subgraph service contracts

c541c8d

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

chore: remove dead code

abcafaa

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

Merge branch 'main' into tmigone/horizon-cleanup

eb59951

test(contract): hack outdated test suite to pass

a2abc90

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

test: fix vm.assume too many rejections

bee3e79

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

RembrandtK added 4 commits May 5, 2026 16:39

RembrandtK self-assigned this May 5, 2026

RembrandtK added audited GIP post-horizon labels May 6, 2026

RembrandtK marked this pull request as ready for review May 6, 2026 12:02

RembrandtK mentioned this pull request May 6, 2026

Deployment preparation for testnet-dips #1326

Closed

docs(audit): add Trust Security PR1331 v04 fix-review report

675d397

Sign-off commit bbec75e. v04 addressed recommendations only; no new findings or status changes versus v03.

This was referenced May 6, 2026

Indexing payments management audit #1301

Closed

Indexing payments management audit fix #1325

Closed

RembrandtK requested review from Maikol, MoonBoi9001, pcarranzav and tmigone May 6, 2026 13:27

RembrandtK mentioned this pull request May 6, 2026

Indexing payments with issuance merge #1283

Closed

Maikol reviewed May 6, 2026

View reviewed changes

RembrandtK mentioned this pull request May 8, 2026

Indexing payments management post audit fixes #1334

Open

Maikol approved these changes May 8, 2026

View reviewed changes

RembrandtK merged commit 675d397 into main May 9, 2026
7 checks passed

RembrandtK deleted the indexing-payments-management-audit-fixed-rebased branch May 9, 2026 10:24

Conversation

RembrandtK commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Commits being compared

Drift introduced by main since the original merge base

Equivalence checks

Uh oh!

openzeppelin-code Bot commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Indexing payments management audit fixed rebased

🚨 Report Summary

Uh oh!

socket-security Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

socket-security Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RembrandtK commented May 5, 2026 •

edited

Loading

openzeppelin-code Bot commented May 5, 2026 •

edited

Loading

socket-security Bot commented May 6, 2026 •

edited

Loading

socket-security Bot commented May 6, 2026 •

edited

Loading

codecov Bot commented May 6, 2026 •

edited

Loading