fix: prevent over-allocation double-collection in presentPOI auto-downsize

packages/contracts-test/tests/unit/rewards/rewards-signal-allocation-update.test.ts

@@ -13,17 +13,16 @@ import { NetworkFixture } from '../lib/fixtures'
 const { HashZero } = constants
 
 /**
- * Test for the signal/allocation update accounting bug fix.
+ * Invariant: signal/allocation update accounting.
  *
- * The bug: When `onSubgraphSignalUpdate()` is called before `onSubgraphAllocationUpdate()`
- * in the SAME BLOCK, the per-signal delta is zero but rewards tracked in `accRewardsForSubgraph`
- * are never distributed to allocations. This causes rewards to be "bricked".
+ * When `onSubgraphSignalUpdate()` runs before `onSubgraphAllocationUpdate()` in the SAME BLOCK,
+ * the per-signal delta is zero. Rewards already tracked in `accRewardsForSubgraph` must still be
+ * distributed to allocations via the snapshot delta
+ * (`accRewardsForSubgraph - accRewardsForSubgraphSnapshot`), rather than relying on the per-signal
+ * delta alone. Distribution must never depend on the ordering of these two calls within a block.
  *
- * The fix: Use the snapshot delta (accRewardsForSubgraph - accRewardsForSubgraphSnapshot) instead
- * of only relying on the per-signal delta for calculating new rewards.
- *
- * IMPORTANT: These tests use evm_setAutomine to batch transactions into the same block,
- * which is necessary to reproduce the bug condition where per-signal delta = 0.
+ * IMPORTANT: These tests use evm_setAutomine to batch transactions into one block so the
+ * per-signal delta is zero, exercising the snapshot-delta path.
  */
 describe('Rewards: Signal and Allocation Update Accounting', () => {
   const graph = hre.graph()
@@ -141,11 +140,11 @@ describe('Rewards: Signal and Allocation Update Accounting', () => {
       // Get final state
       const subgraphAfterAllocation = await rewardsManager.subgraphs(subgraphDeploymentID)
 
-      // THE FIX: accRewardsPerAllocatedToken should be updated even though per-signal delta was 0
-      // With the bug, this would remain unchanged because newRewards=0 caused early return
+      // accRewardsPerAllocatedToken must advance via the snapshot delta even when the per-signal
+      // delta is zero, so accumulated rewards reach allocations regardless of update ordering.
       expect(subgraphAfterAllocation.accRewardsPerAllocatedToken).to.be.gt(
         accRewardsPerAllocatedTokenBefore,
-        'accRewardsPerAllocatedToken should increase (BUG: was not updated when signal update preceded allocation update)',
+        'accRewardsPerAllocatedToken should increase when a signal update precedes an allocation update in the same block',
       )
 
       // Verify snapshot consistency
@@ -196,12 +195,12 @@ describe('Rewards: Signal and Allocation Update Accounting', () => {
       // Get stored state
       const subgraph = await rewardsManager.subgraphs(subgraphDeploymentID)
 
-      // THE BUG: With the original buggy code, accRewardsPerAllocatedToken would remain at 0
-      // because newRewards from per-signal delta is 0, causing early return.
-      // THE FIX: accRewardsPerAllocatedToken should be updated to reflect the accumulated rewards
+      // Even when the per-signal delta is zero, accRewardsPerAllocatedToken must reflect the
+      // rewards accumulated in accRewardsForSubgraph via the snapshot delta, so they are
+      // distributed to allocations rather than stranded.
       expect(subgraph.accRewardsPerAllocatedToken).to.be.gt(
         0,
-        'accRewardsPerAllocatedToken should be non-zero (BUG: rewards were bricked)',
+        'accRewardsPerAllocatedToken should be non-zero so accumulated rewards are distributed, not stranded',
       )
 
       // Verify view function and stored state are consistent
@@ -297,7 +296,8 @@ describe('Rewards: Signal and Allocation Update Accounting', () => {
         'accRewardsPerAllocatedToken should not increase when denied',
       )
 
-      // THE FIX: accRewardsForSubgraphSnapshot should be updated to prevent re-reclaiming
+      // accRewardsForSubgraphSnapshot must advance in the reclaim path so the same rewards
+      // cannot be reclaimed again on a later update.
       expect(subgraphAfter.accRewardsForSubgraphSnapshot).to.be.gte(
         subgraphBefore.accRewardsForSubgraphSnapshot,
         'accRewardsForSubgraphSnapshot should be updated in reclaim path',
@@ -368,12 +368,12 @@ describe('Rewards: Signal and Allocation Update Accounting', () => {
       await helpers.mine(100)
 
       // Call onSubgraphSignalUpdate (simulates curator action)
-      // With Option B fix: rewards should be reclaimed immediately
+      // For a denied subgraph, rewards are reclaimed immediately rather than accumulated.
       const tx = await rewardsManager.connect(governor).onSubgraphSignalUpdate(subgraphDeploymentID)
       const receipt = await tx.wait()
       const afterSignalUpdate = await rewardsManager.subgraphs(subgraphDeploymentID)
 
-      // With Option B: accRewardsForSubgraph should NOT change for denied subgraphs
+      // For a denied subgraph, accRewardsForSubgraph must NOT change
       // (rewards are reclaimed directly, not stored)
       expect(afterSignalUpdate.accRewardsForSubgraph).to.equal(
         afterDenial.accRewardsForSubgraph,
@@ -429,7 +429,8 @@ describe('Rewards: Signal and Allocation Update Accounting', () => {
       const rewardsDuringDenial = await rewardsManager.getAccRewardsForSubgraph(subgraphDeploymentID)
       expect(rewardsDuringDenial).to.equal(rewardsAtDenial, 'View should not increase during denial')
 
-      // Call signal update (with bug, this would NOT reclaim, causing view to jump on next allocation update)
+      // A signal update on a denied subgraph reclaims the accumulated rewards, so the view stays
+      // stable and does not jump on the next allocation update.
       // Configure reclaim address so rewards are reclaimed
       const SUBGRAPH_DENIED = hre.ethers.utils.id('SUBGRAPH_DENIED')
       await rewardsManager.connect(governor).setReclaimAddress(SUBGRAPH_DENIED, governor.address)
@@ -513,7 +514,7 @@ describe('Rewards: Signal and Allocation Update Accounting', () => {
     it('should maintain accounting invariant across mixed updates (with same-block scenarios)', async function () {
       await setupSubgraphWithAllocation()
 
-      // Sequence of operations that could trigger the bug
+      // Sequence exercising the same-block signal/allocation ordering
       await helpers.mine(25)
 
       // First: signal update followed by allocation update in SAME BLOCK

packages/contracts-test/tests/unit/rewards/rewards-snapshot-inversion.test.ts

@@ -20,17 +20,20 @@ const { HashZero } = constants
  *   S = accRewardsForSubgraphSnapshot (stored snapshot, set at allocation updates)
  *   P = rewardsSinceSignalSnapshot    (pending rewards since last signal snapshot)
  *
- * After a proxy upgrade, subgraphs whose last pre-upgrade interaction was
- * `onSubgraphAllocationUpdate` have A < S. The old code set S from a view function
- * (storage + pending) while leaving A at its stored value, so S leads and A lags.
- * The original code's `A.sub(S).add(P)` reverts on the intermediate `A - S`.
+ * For affected subgraphs, on-chain storage can hold an inverted snapshot state
+ * where A < S: the snapshot S leads (it was written from a view value of
+ * storage + pending) while the accumulator A lags at its stored value.
  *
- * The fix: Rearrange to `A.add(P).sub(S)` — add P first, then subtract S.
- * Since P covers T1→now and the gap S - A covers T1→T2, and now >= T2,
- * we have S - A <= P, so S <= A + P always holds. No clamping needed.
+ * Invariant the reward math must uphold: with an inverted state (A < S), the
+ * computation of A + P - S must never underflow. It does so by adding pending
+ * first and subtracting the snapshot last — `A.add(P).sub(S)` — so the
+ * intermediate `A + P` stays non-negative. Since P covers T1→now and the gap
+ * S - A covers T1→T2, and now >= T2, we have S - A <= P, so S <= A + P always
+ * holds; no clamping is needed. Subtracting the gap S - A discards rewards
+ * already distributed, preventing double-counting.
  *
- * These tests use `hardhat_setStorageAt` to directly create the inverted storage state
- * that exists on-chain for affected subgraphs.
+ * These tests use `hardhat_setStorageAt` to construct the inverted storage state
+ * directly so the invariant can be exercised for affected subgraphs.
  */
 describe('Rewards: Snapshot Inversion', () => {
   const graph = hre.graph()
@@ -202,8 +205,9 @@ describe('Rewards: Snapshot Inversion', () => {
       // Advance enough blocks so P > gap. At ~200 GRT/block, 50 blocks ≈ 10,000 GRT > 7,000.
       await helpers.mine(50)
 
-      // Old code: A.sub(S).add(P) reverts on intermediate A - S when A < S.
-      // Fix: A.add(P).sub(S) adds P first, so A + P >= S always holds.
+      // With an inverted state (A < S), the update must not revert: pending P is
+      // added before the snapshot S is subtracted (A.add(P).sub(S)), so the
+      // intermediate A + P stays non-negative and A + P >= S always holds.
       await expect(rewardsManager.connect(governor).onSubgraphSignalUpdate(subgraphDeploymentID)).to.not.be.reverted
     })
 
@@ -229,11 +233,11 @@ describe('Rewards: Snapshot Inversion', () => {
       // First call with inverted state
       await rewardsManager.connect(governor).onSubgraphSignalUpdate(subgraphDeploymentID)
 
-      // After the fix processes the inverted state, snapshots should be synced
+      // After processing the inverted state, snapshots should be synced
       const after = await rewardsManager.subgraphs(subgraphDeploymentID)
       expect(after.accRewardsForSubgraphSnapshot).to.equal(
         after.accRewardsForSubgraph,
-        'snapshot should equal accumulated after fix processes inverted state',
+        'snapshot should equal accumulated after processing inverted state',
       )
 
       // Subsequent calls should work normally
@@ -272,10 +276,10 @@ describe('Rewards: Snapshot Inversion', () => {
       expect(perAllocBefore).to.be.lt(after.accRewardsPerAllocatedToken, 'should distribute rewards: 0 < (A + P) - S')
 
       // The distributed amount should be less than total new rewards (P)
-      // because the gap represents already-distributed rewards from the old code
+      // because the gap represents already-distributed rewards
       // Undistributed = (A + P) - S = P - gap (since S = A + gap)
       // If P ≈ 2000 GRT and gap = 500 GRT, undistributed ≈ 1500 GRT
-      // Without the gap subtraction, it would have been P ≈ 2000 GRT (double-counting)
+      // Subtracting the gap is what keeps this from being P ≈ 2000 GRT (double-counting)
 
       // Verify snapshots are synced
       expect(after.accRewardsForSubgraphSnapshot).to.equal(after.accRewardsForSubgraph)
@@ -350,7 +354,7 @@ describe('Rewards: Snapshot Inversion', () => {
   })
 
   describe('normal operation (no inversion)', function () {
-    it('should produce identical results when A == S (post-fix steady state)', async function () {
+    it('should produce identical results when A == S (synced snapshot steady state)', async function () {
       await setupSubgraphWithAllocation()
 
       // Ensure snapshots are synced (normal state)

packages/issuance/test/unit/agreement-manager/cancelWithPendingUpdate.t.sol

@@ -14,12 +14,12 @@ import { RecurringAgreementManagerSharedTest } from "./shared.t.sol";
 contract RecurringAgreementManagerCancelWithPendingUpdateTest is RecurringAgreementManagerSharedTest {
     /* solhint-disable graph/func-name-mixedcase */
 
-    /// @notice Demonstrates the bug: when an accepted agreement with a pending (unapplied)
-    /// update is canceled, the pendingUpdateMaxNextClaim escrow is NOT freed during
-    /// cancelAgreement. The escrow remains locked until the agreement is fully drained
-    /// and deleted, even though the update can never be accepted (collector rejects
-    /// updates on non-Accepted agreements).
-    function test_CancelAgreement_PendingUpdateEscrowNotFreed() public {
+    /// @notice Verifies that a pending (unapplied) update on a canceled agreement does not
+    /// inflate sumMaxNextClaim. When an accepted agreement with a pending update is canceled,
+    /// the pendingUpdateMaxNextClaim escrow must be freed: the update can never be applied
+    /// (the collector rejects updates on non-Accepted agreements), so only the base claim
+    /// counts toward reserved escrow.
+    function test_CancelAgreement_PendingUpdateEscrowFreed() public {
         // 1. Offer and accept an agreement
         (IRecurringCollector.RecurringCollectionAgreement memory rca, ) = _makeRCAWithId(
             100 ether,
@@ -68,16 +68,16 @@ contract RecurringAgreementManagerCancelWithPendingUpdateTest is RecurringAgreem
         );
         assertTrue(exists, "agreement should still exist (has remaining claims)");
 
-        // 4. BUG: The pending update can never be accepted (collector rejects updates on
-        // canceled agreements), yet pendingUpdateMaxNextClaim is still reserved.
+        // 4. The pending update can never be applied — the collector rejects updates on
+        // canceled agreements — so its escrow must not count toward sumMaxNextClaim.
         uint256 sumAfterCancel = agreementManager.getSumMaxNextClaim(_collector(), indexer);
 
-        // The pending escrow should have been freed (zeroed) since the update is dead.
-        // sumMaxNextClaim should only include the base claim, not the dead pending update.
+        // The pending escrow must be freed (zeroed) since the update can no longer be applied.
+        // sumMaxNextClaim must include only the base claim, not the unappliable pending update.
         assertEq(
             sumAfterCancel,
             agreementManager.getAgreementMaxNextClaim(IAgreementCollector(address(recurringCollector)), agreementId),
-            "BUG: sumMaxNextClaim should only include the base claim, not the dead pending update"
+            "sumMaxNextClaim must include only the base claim, not the unappliable pending update"
         );
     }
 

packages/issuance/test/unit/agreement-manager/fundingModes.t.sol

@@ -236,10 +236,12 @@ contract RecurringAgreementManagerFundingModesTest is RecurringAgreementManagerS
         uint256 maxClaim2 = 2 ether * 7200 + 200 ether;
 
         // indexer is fully deposited (undeposited = 0), indexer2 has full deficit (undeposited = maxClaim2)
-        // totalEscrowDeficit must be maxClaim2, NOT 0 (the old buggy sumMaxNextClaim - totalInEscrow approach
-        // would compute sumMaxNextClaim = maxClaim1 + maxClaim2, totalInEscrow = maxClaim1,
-        // deficit = maxClaim2 — which happens to be correct here, but would be wrong if indexer
-        // were over-deposited and the excess masked indexer2's deficit)
+        // totalEscrowDeficit must be computed per-provider: it sums each provider's own undeposited
+        // shortfall, so here it equals indexer2's full deficit (maxClaim2) and nothing from indexer.
+        // Per-provider computation is required as a permanent invariant: a naive
+        // sum(maxNextClaim) - totalInEscrow would let one provider's surplus offset another's
+        // shortfall, so if indexer were over-deposited its excess would mask indexer2's deficit and
+        // understate the total. The per-provider sum avoids that by never netting surplus against deficit.
         assertEq(agreementManager.getTotalEscrowDeficit(), maxClaim2, "Undeposited = indexer2's full deficit");
 
         // Verify per-provider escrow state

packages/subgraph-service/contracts/libraries/AllocationHandler.sol

@@ -374,7 +374,7 @@ library AllocationHandler {
         // Scoped for stack management
         {
             (uint256 tokensIndexerRewards, uint256 tokensDelegationRewards) = _distributeIndexingRewards(
-                allocation,
+                allocation.indexer,
                 rewardsCollected,
                 params
             );
@@ -409,7 +409,6 @@ library AllocationHandler {
                 params.graphStaking,
                 params.graphRewardsManager,
                 params._allocationId,
-                allocation,
                 0,
                 params._delegationRatio,
                 params.maxPOIStaleness
@@ -502,7 +501,6 @@ library AllocationHandler {
             graphStaking,
             graphRewardsManager,
             _allocationId,
-            allocation,
             _tokens,
             _delegationRatio,
             _maxPOIStaleness
@@ -518,7 +516,6 @@ library AllocationHandler {
      * @param graphStaking The staking contract
      * @param graphRewardsManager The rewards manager contract
      * @param _allocationId The allocation ID to resize
-     * @param allocation The current allocation state
      * @param _tokens The new token amount for the allocation
      * @param _delegationRatio The delegation ratio for provision tracking
      * @param _maxPOIStaleness The maximum POI staleness threshold
@@ -530,11 +527,12 @@ library AllocationHandler {
         IHorizonStaking graphStaking,
         IRewardsManager graphRewardsManager,
         address _allocationId,
-        IAllocation.State memory allocation,
         uint256 _tokens,
         uint32 _delegationRatio,
         uint256 _maxPOIStaleness
     ) internal {
+        IAllocation.State memory allocation = _allocations.get(_allocationId);
+
         // Update provision tracker
         uint256 oldTokens = allocation.tokens;
         if (_tokens > oldTokens) {
@@ -656,41 +654,41 @@ library AllocationHandler {
 
     /**
      * @notice Distributes indexing rewards to delegators and indexer
-     * @param _allocation The allocation state
+     * @param _indexer The indexer that owns the allocation
      * @param _rewardsCollected Total rewards to distribute
      * @param _params The present params containing staking, token, and destination info
      * @return tokensIndexerRewards Amount sent to indexer
      * @return tokensDelegationRewards Amount sent to delegation pool
      */
     function _distributeIndexingRewards(
-        IAllocation.State memory _allocation,
+        address _indexer,
         uint256 _rewardsCollected,
         PresentParams memory _params
     ) private returns (uint256 tokensIndexerRewards, uint256 tokensDelegationRewards) {
         if (_rewardsCollected == 0) return (0, 0);
 
         // Calculate and distribute delegator share
         uint256 delegatorCut = _params.graphStaking.getDelegationFeeCut(
-            _allocation.indexer,
+            _indexer,
             _params.dataService,
             IGraphPayments.PaymentTypes.IndexingRewards
         );
         IHorizonStakingTypes.DelegationPool memory pool = _params.graphStaking.getDelegationPool(
-            _allocation.indexer,
+            _indexer,
             _params.dataService
         );
         tokensDelegationRewards = pool.shares > 0 ? _rewardsCollected.mulPPM(delegatorCut) : 0;
         if (tokensDelegationRewards > 0) {
             _params.graphToken.approve(address(_params.graphStaking), tokensDelegationRewards);
-            _params.graphStaking.addToDelegationPool(_allocation.indexer, _params.dataService, tokensDelegationRewards);
+            _params.graphStaking.addToDelegationPool(_indexer, _params.dataService, tokensDelegationRewards);
         }
 
         // Distribute indexer share
         tokensIndexerRewards = _rewardsCollected - tokensDelegationRewards;
         if (tokensIndexerRewards > 0) {
             if (_params._paymentsDestination == address(0)) {
                 _params.graphToken.approve(address(_params.graphStaking), tokensIndexerRewards);
-                _params.graphStaking.stakeToProvision(_allocation.indexer, _params.dataService, tokensIndexerRewards);
+                _params.graphStaking.stakeToProvision(_indexer, _params.dataService, tokensIndexerRewards);
             } else {
                 _params.graphToken.pushTokens(_params._paymentsDestination, tokensIndexerRewards);
             }