Indexing payments management audit fix

docs/PaymentsTrustModel.md

@@ -68,7 +68,7 @@ RecurringCollector adds payer callbacks when the payer is a contract:
 <───┘
 ```
 
-- **`isEligible`**: hard `require` — contract payer can block collection for ineligible receivers. Only called when `0 < tokensToCollect`.
+- **`isEligible`**: fail-open gate — only an explicit return of `0` blocks collection; call failures (reverts, malformed data) are ignored to prevent a buggy payer from griefing the receiver. Only called when `0 < tokensToCollect`.
 - **`beforeCollection`**: try-catch — allows payer to top up escrow (RAM uses this for JIT deposits), but cannot block (though a malicious contract payer could consume excessive gas). Only called when `0 < tokensToCollect`.
 - **`afterCollection`**: try-catch — allows payer to reconcile state post-collection, cannot block (same gas exhaustion caveat). Called even when `tokensToCollect == 0` (zero-token collections still trigger reconciliation).
 
@@ -99,7 +99,7 @@ RecurringCollector adds payer callbacks when the payer is a contract:
 Caveats on effective escrow (contract payers introduce additional trust requirements — see caveat 3):
 
 1. **Thawing reduces effective balance** — a payer can initiate a thaw; once the thaw period completes, those tokens are withdrawable. The receiver should account for the thawing period and any in-progress thaws when assessing available escrow.
-2. **Cancellation freezes the collection window** at `canceledAt` — the receiver can still collect for the period up to cancellation (with `minSecondsPerCollection` bypassed), but no further.
+2. **Cancellation freezes the collection window** at `collectableUntil` — the receiver can still collect for the period up to cancellation; `minSecondsPerCollection` is enforced during the notice period and bypassed only after `collectableUntil` is reached.
 3. **Contract payers can block** — if the payer is a contract that implements `IProviderEligibility`, it can deny collection via `isEligible` (see [RecurringCollector Extensions](#recurringcollector-extensions)).
 
 **Mitigation**: The thawing period provides a window for the receiver to collect before funds are withdrawn. The escrow balance and thaw state are publicly visible on-chain.

packages/contracts-test/tests/unit/rewards/rewards-config.test.ts

@@ -274,5 +274,47 @@ describe('Rewards - Configuration', () => {
         expect(await rewardsManager.minimumSubgraphSignal()).eq(newMinimumSignal)
       })
     })
+
+    describe('revertOnIneligible', function () {
+      it('should reject setRevertOnIneligible if unauthorized', async function () {
+        const tx = rewardsManager.connect(indexer1).setRevertOnIneligible(true)
+        await expect(tx).revertedWith('Only Controller governor')
+      })
+
+      it('should set revertOnIneligible to true', async function () {
+        const tx = rewardsManager.connect(governor).setRevertOnIneligible(true)
+        await expect(tx).emit(rewardsManager, 'ParameterUpdated').withArgs('revertOnIneligible')
+        expect(await rewardsManager.getRevertOnIneligible()).eq(true)
+      })
+
+      it('should set revertOnIneligible to false', async function () {
+        // First set to true
+        await rewardsManager.connect(governor).setRevertOnIneligible(true)
+
+        // Then set back to false
+        const tx = rewardsManager.connect(governor).setRevertOnIneligible(false)
+        await expect(tx).emit(rewardsManager, 'ParameterUpdated').withArgs('revertOnIneligible')
+        expect(await rewardsManager.getRevertOnIneligible()).eq(false)
+      })
+
+      it('should be a no-op when setting same value (false to false)', async function () {
+        // Default is false
+        expect(await rewardsManager.getRevertOnIneligible()).eq(false)
+
+        const tx = rewardsManager.connect(governor).setRevertOnIneligible(false)
+        await expect(tx).to.not.emit(rewardsManager, 'ParameterUpdated')
+
+        expect(await rewardsManager.getRevertOnIneligible()).eq(false)
+      })
+
+      it('should be a no-op when setting same value (true to true)', async function () {
+        await rewardsManager.connect(governor).setRevertOnIneligible(true)
+
+        const tx = rewardsManager.connect(governor).setRevertOnIneligible(true)
+        await expect(tx).to.not.emit(rewardsManager, 'ParameterUpdated')
+
+        expect(await rewardsManager.getRevertOnIneligible()).eq(true)
+      })
+    })
   })
 })

packages/contracts-test/tests/unit/rewards/rewards-eligibility-oracle.test.ts

@@ -533,6 +533,97 @@ describe('Rewards - Eligibility Oracle', () => {
       expectApproxEq(event.args[2], expectedIndexingRewards, 'rewards amount')
     })
 
+    it('should revert for ineligible indexer when revertOnIneligible is true', async function () {
+      // Setup REO that denies indexer1
+      const MockRewardsEligibilityOracleFactory = await hre.ethers.getContractFactory(
+        'contracts/tests/MockRewardsEligibilityOracle.sol:MockRewardsEligibilityOracle',
+      )
+      const mockOracle = await MockRewardsEligibilityOracleFactory.deploy(false) // Deny
+      await mockOracle.deployed()
+      await rewardsManager.connect(governor).setProviderEligibilityOracle(mockOracle.address)
+
+      // Enable revert on ineligible
+      await rewardsManager.connect(governor).setRevertOnIneligible(true)
+
+      // Align with the epoch boundary
+      await helpers.mineEpoch(epochManager)
+
+      // Setup allocation
+      await setupIndexerAllocation()
+
+      // Jump to next epoch
+      await helpers.mineEpoch(epochManager)
+
+      // Close allocation - should revert because indexer is ineligible
+      const tx = staking.connect(indexer1).closeAllocation(allocationID1, randomHexBytes())
+      await expect(tx).revertedWith('Indexer not eligible for rewards')
+    })
+
+    it('should not revert for eligible indexer when revertOnIneligible is true', async function () {
+      // Setup REO that allows indexer1
+      const MockRewardsEligibilityOracleFactory = await hre.ethers.getContractFactory(
+        'contracts/tests/MockRewardsEligibilityOracle.sol:MockRewardsEligibilityOracle',
+      )
+      const mockOracle = await MockRewardsEligibilityOracleFactory.deploy(true) // Allow
+      await mockOracle.deployed()
+      await rewardsManager.connect(governor).setProviderEligibilityOracle(mockOracle.address)
+
+      // Enable revert on ineligible
+      await rewardsManager.connect(governor).setRevertOnIneligible(true)
+
+      // Align with the epoch boundary
+      await helpers.mineEpoch(epochManager)
+
+      // Setup allocation
+      await setupIndexerAllocation()
+
+      // Jump to next epoch
+      await helpers.mineEpoch(epochManager)
+
+      // Close allocation - should succeed (indexer is eligible)
+      const tx = staking.connect(indexer1).closeAllocation(allocationID1, randomHexBytes())
+      await expect(tx).emit(rewardsManager, 'HorizonRewardsAssigned')
+    })
+
+    it('should reclaim (not revert) for ineligible indexer when revertOnIneligible is false', async function () {
+      // Setup REO that denies indexer1
+      const MockRewardsEligibilityOracleFactory = await hre.ethers.getContractFactory(
+        'contracts/tests/MockRewardsEligibilityOracle.sol:MockRewardsEligibilityOracle',
+      )
+      const mockOracle = await MockRewardsEligibilityOracleFactory.deploy(false) // Deny
+      await mockOracle.deployed()
+      await rewardsManager.connect(governor).setProviderEligibilityOracle(mockOracle.address)
+
+      // Ensure revertOnIneligible is false (default)
+      expect(await rewardsManager.getRevertOnIneligible()).eq(false)
+
+      // Align with the epoch boundary
+      await helpers.mineEpoch(epochManager)
+
+      // Setup allocation
+      await setupIndexerAllocation()
+
+      // Jump to next epoch
+      await helpers.mineEpoch(epochManager)
+
+      // Close allocation - should succeed but deny rewards
+      const tx = await staking.connect(indexer1).closeAllocation(allocationID1, randomHexBytes())
+      const receipt = await tx.wait()
+
+      // Should emit RewardsDeniedDueToEligibility (not revert)
+      const rewardsDeniedEvents = receipt.logs
+        .map((log) => {
+          try {
+            return rewardsManager.interface.parseLog(log)
+          } catch {
+            return null
+          }
+        })
+        .filter((event) => event?.name === 'RewardsDeniedDueToEligibility')
+
+      expect(rewardsDeniedEvents.length).to.equal(1, 'RewardsDeniedDueToEligibility event not found')
+    })
+
     it('should verify event structure differences between denial mechanisms', async function () {
       // Test 1: Denylist denial - event WITHOUT amount
       // Create allocation FIRST, then deny (so there are pre-denial rewards to deny)

packages/contracts-test/tests/unit/rewards/rewards-interface.test.ts

@@ -58,7 +58,7 @@ describe('RewardsManager interfaces', () => {
     })
 
     it('IRewardsManager should have stable interface ID', () => {
-      expect(IRewardsManager__factory.interfaceId).to.equal('0x7e0447a1')
+      expect(IRewardsManager__factory.interfaceId).to.equal('0x337b092e')
     })
   })
 

packages/contracts/contracts/rewards/RewardsManager.sol

@@ -265,6 +265,14 @@ contract RewardsManager is
         }
     }
 
+    /// @inheritdoc IRewardsManager
+    function setRevertOnIneligible(bool _revertOnIneligible) external override onlyGovernor {
+        if (revertOnIneligible != _revertOnIneligible) {
+            revertOnIneligible = _revertOnIneligible;
+            emit ParameterUpdated("revertOnIneligible");
+        }
+    }
+
     // -- Denylist --
 
     /**
@@ -344,6 +352,11 @@ contract RewardsManager is
         return rewardsEligibilityOracle;
     }
 
+    /// @inheritdoc IRewardsManager
+    function getRevertOnIneligible() external view override returns (bool) {
+        return revertOnIneligible;
+    }
+
     /// @inheritdoc IRewardsManager
     function getNewRewardsPerSignal() public view override returns (uint256 claimablePerSignal) {
         (claimablePerSignal, ) = _getNewRewardsPerSignal();
@@ -772,6 +785,11 @@ contract RewardsManager is
         bool isDeniedSubgraph = isDenied(subgraphDeploymentID);
         bool isIneligible = address(rewardsEligibilityOracle) != address(0) &&
             !rewardsEligibilityOracle.isEligible(indexer);
+
+        // When configured to revert, block collection so rewards remain claimable if
+        // the indexer becomes eligible and collects before the allocation goes stale.
+        require(!isIneligible || !revertOnIneligible, "Indexer not eligible for rewards");
+
         if (!isDeniedSubgraph && !isIneligible) return false;
 
         if (isDeniedSubgraph) emit RewardsDenied(indexer, allocationID);

packages/contracts/contracts/rewards/RewardsManagerStorage.sol

@@ -117,4 +117,9 @@ abstract contract RewardsManagerV6Storage is RewardsManagerV5Storage {
     /// @dev Default fallback address for reclaiming rewards when no reason-specific address is configured.
     /// Zero address means rewards are dropped (not minted) if no specific reclaim address matches.
     address internal defaultReclaimAddress;
+
+    /// @dev When true, ineligible indexers cause takeRewards to revert (blocking POI presentation
+    /// and allowing allocations to go stale). When false (default), ineligible indexers have
+    /// rewards reclaimed but takeRewards succeeds (returning 0).
+    bool internal revertOnIneligible;
 }