Pending indexing payments and post-Horizon contract changes by RembrandtK · Pull Request #1288 · graphprotocol/contracts

RembrandtK · 2026-02-23T12:57:27Z

Baseline of pending contract changes that need re-audit after rebasing to main.

For post-Horizon code cleanup to be valid these assumptions about the network must be true:

There are no more legacy allocations open. This can be guaranteed by force closing allocations, which can be done permisionlessly 28 days after Horizon go live.
All service provider legacy unstake operations must be fully thawed. This needs to be monitored but it's also guaranteed as long as the transition period is longer than 28 days.

(Pending merge of #1291 to remove a previous assumption.)

This PR is likely to be split into two, and presented for audit processing in the following order (commits indicative but might need some adjusting):

Indexing payments (81ac02f)
Escrow management #1290
Post-Horizon code cleanup (remaining commits on this PR combined with feat: add back legacy allocation id collision check #1291).

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>

…face

Prevents stack-too-deep compiler error when running coverage without optimizer, caused by RecurringCollector contract complexity.

Add forge-lint disable comments for RCA/POI acronym casing in function and variable names. Remove unused Allocation and IDataServiceFees imports. Replace unsafe bytes32("poi") cast with keccak256("poi").

…service coverage - Revert strict inequality conversions (> x-1, < x+1) back to (>=, <=) and suppress gas-strict-inequalities with solhint comments instead - Suppress gas-small-strings, function-max-lines solhint warnings - Add --ir-minimum to subgraph-service forge coverage to fix stack too deep - Change getCollectionInfo param from memory to calldata - Add @author natspec tag

…overage

…-merge Merges 5 new audit-fix commits from origin/issuance-audit: - TRST-R-1: reclaim pending rewards on stale allocation resize - TRST-R-2: improve reward documentation accuracy - TRST-R-3: remove redundant subgraphAllocatedTokens check - Rename NO_ALLOCATION to NO_ALLOCATED_TOKENS - Document minimumSubgraphSignal retroactive application Conflict resolution: - AllocationManager.sol: kept thin wrapper, passed maxPOIStaleness to AllocationHandler.resizeAllocation() - AllocationHandler.sol: ported stale reclaim logic (isStale check, reclaimRewards, clearPendingRewards) into resizeAllocation() - resize.t.sol: kept AllocationHandler import, added IAllocation import for new test assertions Auto-merged (verified): - MockRewardsManager.sol: realistic calcRewards/reclaimRewards - SubgraphService.t.sol: pending rewards assertion fix

Solhint already has func-name-mixedcase and var-name-mixedcase turned off. Align forge-lint by excluding these rules in horizon and subgraph-service to avoid redundant inline suppressions.

Cover pause/unpause access control, setPauseGuardian, and no-change revert cases.

Remove LibFixedMath, Denominations, DataServiceRescuable and its interface. None are imported by any source code in horizon or subgraph-service.

MathUtils only provided min() to horizon/subgraph-service code. The other two functions were unused. Switch to OZ's Math.min which is already a dependency, and delete the custom library.

Prevents forge lint from scanning into test/node_modules or other directories with incompatible dependencies or symlink loops.

- Add contract-level natspec to StakeClaims, IndexingAgreement, IndexingAgreementDecoder, IndexingAgreementDecoderRaw - Suppress gas-indexed-events on StakeClaims events and asm-keccak256 note - Move forge-lint comments before natspec blocks to fix natspec detection - Use block-style solhint-disable for function-max-lines suppression - Change struct params to calldata in AllocationHandler and IndexingAgreement - Remove orphaned natspec and unnecessary forge-lint comments

- Reorder AllocateParams struct to pack _delegationRatio (uint32) with _indexer (address) into a single 32-byte slot - Add forge-lint unsafe-typecast annotations in RecurringCollector with safety rationale for uint32/uint64 casts bounded by require checks

openzeppelin-code · 2026-02-23T13:16:13Z

Pending indexing payments and post-Horizon contract changes

Generated at commit: eb839002ad8448b16300284e49263f201dd09851

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	3 5 0 14 37 59
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

For more details view the full report in OpenZeppelin Code Inspector

codecov · 2026-02-23T13:29:12Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.80%. Comparing base (ada3155) to head (eb83900).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1288      +/-   ##
==========================================
- Coverage   86.95%   86.80%   -0.16%     
==========================================
  Files          46       46              
  Lines        2492     2485       -7     
  Branches      746      743       -3     
==========================================
- Hits         2167     2157      -10     
- Misses        325      328       +3

Flag	Coverage Δ
unittests	`86.80% <100.00%> (-0.16%)`	⬇️

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.

tmigone · 2026-02-23T19:27:35Z

packages/subgraph-service/contracts/SubgraphServiceStorage.sol

    mapping(address indexer => address destination) public override paymentsDestination;
+
+    /// @notice The cut data service takes from indexing fee payments. In PPM.
+    uint256 public indexingFeesCut;


This should be in SubgraphServiceV2Storage no?

Yes, I thought about doing that but since we wanted minimum changes and the audit was done without SubgraphServiceV2Storage I left it unchanged.

tmigone · 2026-02-23T19:34:15Z

Post-horizon clean up additions taken from #1261 and #1271 look good.

Worth keeping this in context for this upgrade:

In order for these changes to be valid there are some assumptions about the network state that must hold:

There are no more legacy allocations open. This can be guaranteed by force closing allocations, which can be done permisionlessly 28 days after Horizon go live.

Legacy allocation ids have been fully migrated to the SubgraphService. This is a privileged account operation that will be executed once Horizon is live and legacy allocations can no longer be created.

All service provider legacy unstake operations must be fully thawed. This needs to be monitored but it's also guaranteed as long as the transition period is longer than 28 days.

Related to 2), we actually need to revert that change, I'll do it shortly in a separate PR.

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

tmigone · 2026-02-23T20:23:34Z

#1291 addresses the changes mentioned in my previous comment, it also eliminates one of the assumptions:

In order for these changes to be valid there are some assumptions about the network state that must hold:

There are no more legacy allocations open. This can be guaranteed by force closing allocations, which can be done permisionlessly 28 days after Horizon go live.

All service provider legacy unstake operations must be fully thawed. This needs to be monitored but it's also guaranteed as long as the transition period is longer than 28 days.

feat: add back legacy allocation id collision check

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>

Maikol and others added 20 commits February 9, 2026 19:42

chore: update pragma versions and centralize import paths for horizon

3c1a2f1

refactor: extract IndexingAgreement types to IIndexingAgreement inter…

26c6c44

…face

chore: rename test constants to UPPER_SNAKE_CASE and update imports

9742dd8

fix: update expected errors and assertions in failing tests

fa57fbc

fix: add --ir-minimum flag to forge coverage command

89c7010

Prevents stack-too-deep compiler error when running coverage without optimizer, caused by RecurringCollector contract complexity.

fix: resolve forge lint warnings in subgraph-service

4450e9c

Add forge-lint disable comments for RCA/POI acronym casing in function and variable names. Remove unused Allocation and IDataServiceFees imports. Replace unsafe bytes32("poi") cast with keccak256("poi").

fix: change agreements mapping to internal to fix stack-too-deep in c…

78b778d

…overage

Merge branch 'main' into tmigone/horizon-cleanup-audit-fixes

90a119a

chore: disable mixed-case lint rules in forge-lint

80831bc

Solhint already has func-name-mixedcase and var-name-mixedcase turned off. Align forge-lint by excluding these rules in horizon and subgraph-service to avoid redundant inline suppressions.

test: add unit tests for DataServicePausableUpgradeable

ff23f01

Cover pause/unpause access control, setPauseGuardian, and no-change revert cases.

chore: remove unused libraries and extensions

a7869e4

Remove LibFixedMath, Denominations, DataServiceRescuable and its interface. None are imported by any source code in horizon or subgraph-service.

refactor: replace MathUtils.min with OpenZeppelin Math.min

fc4e9e0

MathUtils only provided min() to horizon/subgraph-service code. The other two functions were unused. Switch to OZ's Math.min which is already a dependency, and delete the custom library.

Merge branch 'mde/dips-issuance-merge' into rem-baseline-merge

a9b500a

Merge branch 'rem-horizon-cleanup-merge' into rem-baseline-merge

59062ee

fix: scope forge lint to contracts/ directory

597a68b

Prevents forge lint from scanning into test/node_modules or other directories with incompatible dependencies or symlink loops.

fixup! Merge branch 'rem-horizon-cleanup-merge' into rem-baseline-merge

5b9463f

tmigone reviewed Feb 23, 2026

View reviewed changes

Maikol and others added 3 commits February 23, 2026 17:14

chore: add dips audit review doc

c071455

feat: add back legacy allocation id collision check

edf40dc

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

chore: natspec

b558574

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

RembrandtK and others added 2 commits February 24, 2026 09:27

fixup! chore: add dips audit review doc

fa6bd73

Merge pull request #1291 from graphprotocol/tmigone/add-legacy-id

eb83900

feat: add back legacy allocation id collision check

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Pending indexing payments and post-Horizon contract changes#1288

Pending indexing payments and post-Horizon contract changes#1288
RembrandtK wants to merge 66 commits intomainfrom
rem-baseline-merge

RembrandtK commented Feb 23, 2026 •

edited

Loading

Uh oh!

openzeppelin-code bot commented Feb 23, 2026 •

edited

Loading

Uh oh!

codecov bot commented Feb 23, 2026 •

edited

Loading

Uh oh!

tmigone Feb 23, 2026

Uh oh!

Maikol Feb 23, 2026

Uh oh!

tmigone commented Feb 23, 2026 •

edited

Loading

Uh oh!

tmigone commented Feb 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

Conversation

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

Uh oh!

Uh oh!

openzeppelin-code bot commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pending indexing payments and post-Horizon contract changes

🚨 Report Summary

Uh oh!

codecov bot commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tmigone Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Maikol Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

tmigone commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmigone commented Feb 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RembrandtK commented Feb 23, 2026 •

edited

Loading

openzeppelin-code bot commented Feb 23, 2026 •

edited

Loading

codecov bot commented Feb 23, 2026 •

edited

Loading

tmigone commented Feb 23, 2026 •

edited

Loading