fix: ensure getRewards and takeRewards return 0 for closed allos (L-11)

packages/contracts/contracts/rewards/IRewardsIssuer.sol

@@ -5,7 +5,9 @@ pragma solidity ^0.7.6 || 0.8.27;
 interface IRewardsIssuer {
     /**
      * @dev Get allocation data to calculate rewards issuance
+     * 
      * @param allocationId The allocation Id
+     * @return isActive Whether the allocation is active or not
      * @return indexer The indexer address
      * @return subgraphDeploymentId Subgraph deployment id for the allocation
      * @return tokens Amount of allocated tokens
@@ -18,6 +20,7 @@ interface IRewardsIssuer {
         external
         view
         returns (
+            bool isActive,
             address indexer,
             bytes32 subgraphDeploymentId,
             uint256 tokens,

packages/contracts/contracts/rewards/RewardsManager.sol

@@ -316,6 +316,7 @@ contract RewardsManager is RewardsManagerV5Storage, GraphUpgradeable, IRewardsMa
     /**
      * @dev Calculate current rewards for a given allocation on demand.
      * The allocation could be a legacy allocation or a new subgraph service allocation.
+     * Returns 0 if the allocation is not active.
      * @param _allocationID Allocation
      * @return Rewards amount for an allocation
      */
@@ -326,13 +327,18 @@ contract RewardsManager is RewardsManagerV5Storage, GraphUpgradeable, IRewardsMa
         );
 
         (
+            bool isActive,
             ,
             bytes32 subgraphDeploymentId,
             uint256 tokens,
             uint256 alloAccRewardsPerAllocatedToken,
             uint256 accRewardsPending
         ) = IRewardsIssuer(_rewardsIssuer).getAllocationData(_allocationID);
 
+        if (!isActive) {
+            return 0;
+        }
+
         (uint256 accRewardsPerAllocatedToken, ) = getAccRewardsPerAllocatedToken(subgraphDeploymentId);
         return
             accRewardsPending.add(_calcRewards(tokens, alloAccRewardsPerAllocatedToken, accRewardsPerAllocatedToken));
@@ -372,6 +378,7 @@ contract RewardsManager is RewardsManagerV5Storage, GraphUpgradeable, IRewardsMa
      * This function can only be called by an authorized rewards issuer which are
      * the staking contract (for legacy allocations), and the subgraph service (for new allocations).
      * This function will mint the necessary tokens to reward based on the inflation calculation.
+     * Mints 0 tokens if the allocation is not active.
      * @param _allocationID Allocation
      * @return Assigned rewards amount
      */
@@ -383,6 +390,7 @@ contract RewardsManager is RewardsManagerV5Storage, GraphUpgradeable, IRewardsMa
         );
 
         (
+            bool isActive,
             address indexer,
             bytes32 subgraphDeploymentID,
             uint256 tokens,
@@ -398,15 +406,18 @@ contract RewardsManager is RewardsManagerV5Storage, GraphUpgradeable, IRewardsMa
             return 0;
         }
 
-        // Calculate rewards accrued by this allocation
-        uint256 rewards = accRewardsPending.add(
-            _calcRewards(tokens, accRewardsPerAllocatedToken, updatedAccRewardsPerAllocatedToken)
-        );
-        if (rewards > 0) {
-            // Mint directly to rewards issuer for the reward amount
-            // The rewards issuer contract will do bookkeeping of the reward and
-            // assign in proportion to each stakeholder incentive
-            graphToken().mint(rewardsIssuer, rewards);
+        uint256 rewards = 0;
+        if (isActive) {
+            // Calculate rewards accrued by this allocation
+            rewards = accRewardsPending.add(
+                _calcRewards(tokens, accRewardsPerAllocatedToken, updatedAccRewardsPerAllocatedToken)
+            );
+            if (rewards > 0) {
+                // Mint directly to rewards issuer for the reward amount
+                // The rewards issuer contract will do bookkeeping of the reward and
+                // assign in proportion to each stakeholder incentive
+                graphToken().mint(rewardsIssuer, rewards);
+            }
         }
 
         emit HorizonRewardsAssigned(indexer, _allocationID, rewards);

packages/contracts/contracts/staking/IStakingBase.sol

@@ -368,7 +368,7 @@ interface IStakingBase is IStakingData {
      */
     function getAllocationData(
         address _allocationID
-    ) external view returns (address, bytes32, uint256, uint256, uint256);
+    ) external view returns (bool, address, bytes32, uint256, uint256, uint256);
 
     /**
      * @dev New function to get the allocation active status for the rewards manager

packages/contracts/contracts/staking/Staking.sol

@@ -479,9 +479,18 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M
      */
     function getAllocationData(
         address _allocationID
-    ) external view override returns (address, bytes32, uint256, uint256, uint256) {
+    ) external view override returns (bool, address, bytes32, uint256, uint256, uint256) {
         Allocation memory alloc = __allocations[_allocationID];
-        return (alloc.indexer, alloc.subgraphDeploymentID, alloc.tokens, alloc.accRewardsPerAllocatedToken, 0);
+        bool isActive = _getAllocationState(_allocationID) == AllocationState.Active;
+
+        return (
+            isActive,
+            alloc.indexer,
+            alloc.subgraphDeploymentID,
+            alloc.tokens,
+            alloc.accRewardsPerAllocatedToken,
+            0
+        );
     }
 
     /**
@@ -810,9 +819,6 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M
             require(isIndexerOrOperator, "!auth");
         }
 
-        // Close the allocation
-        __allocations[_allocationID].closedAtEpoch = alloc.closedAtEpoch;
-
         // -- Rewards Distribution --
 
         // Process non-zero-allocation rewards tracking
@@ -836,6 +842,11 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M
             );
         }
 
+        // Close the allocation
+        // Note that this breaks CEI pattern. We update after the rewards distribution logic as it expects the allocation
+        // to still be active. There shouldn't be reentrancy risk here as all internal calls are to trusted contracts.
+        __allocations[_allocationID].closedAtEpoch = alloc.closedAtEpoch;
+
         emit AllocationClosed(
             alloc.indexer,
             alloc.subgraphDeploymentID,

packages/contracts/scripts/build

@@ -10,8 +10,8 @@ yarn compile
 tsc
 
 # Copy types and abis to distribution folder
-cp -R build/types/* dist/build/types
-cp -R build/abis/ dist/abis
+cp -R build/types/* dist/contracts/build/types
+cp -R build/abis/ dist/contracts/build/abis
 
 # Move compiled types ts
-mv dist/build/types dist/types
+mv dist/contracts/build/types dist/types

packages/contracts/test/unit/rewards/rewards.test.ts

@@ -21,6 +21,7 @@ import {
   toGRT,
 } from '@graphprotocol/sdk'
 import type { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'
+import { impersonateAccount, setBalance } from '../../../../sdk/src/helpers'
 
 const MAX_PPM = 1000000
 
@@ -838,6 +839,30 @@ describe('Rewards', () => {
           .emit(rewardsManager, 'RewardsDenied')
           .withArgs(indexer1.address, allocationID1)
       })
+
+      it('should not distribute rewards if allocation is not active', async function () {
+        // Setup
+        await setupIndexerAllocation()
+
+        // Jump and close allocation
+        await helpers.mineEpoch(epochManager)
+        await staking.connect(indexer1).closeAllocation(allocationID1, randomHexBytes())
+
+        // Jump some more
+        await helpers.mineEpoch(epochManager, 10)
+
+        // Impersonate staking contract
+        const impersonatedStaking = await impersonateAccount(staking.address)
+        await setBalance(staking.address, toGRT('1000000'))
+
+        // Distribute rewards
+        const tx = await rewardsManager.connect(impersonatedStaking).takeRewards(allocationID1)
+        const receipt = await tx.wait()
+        const event = rewardsManager.interface.parseLog(receipt.logs[0]).args
+        expect(event.indexer).eq(indexer1.address)
+        expect(event.allocationID).eq(allocationID1)
+        expect(toRound(event.amount)).eq(toRound(BigNumber.from(0)))
+      })
     })
   })
 

packages/horizon/contracts/staking/HorizonStakingExtension.sol

@@ -223,9 +223,10 @@ contract HorizonStakingExtension is HorizonStakingBase, IHorizonStakingExtension
     /// @inheritdoc IRewardsIssuer
     function getAllocationData(
         address allocationID
-    ) external view override returns (address, bytes32, uint256, uint256, uint256) {
+    ) external view override returns (bool, address, bytes32, uint256, uint256, uint256) {
         Allocation memory allo = __DEPRECATED_allocations[allocationID];
-        return (allo.indexer, allo.subgraphDeploymentID, allo.tokens, allo.accRewardsPerAllocatedToken, 0);
+        bool isActive = _getAllocationState(allocationID) == AllocationState.Active;
+        return (isActive, allo.indexer, allo.subgraphDeploymentID, allo.tokens, allo.accRewardsPerAllocatedToken, 0);
     }
 
     /// @inheritdoc IHorizonStakingExtension
@@ -351,9 +352,6 @@ contract HorizonStakingExtension is HorizonStakingBase, IHorizonStakingExtension
             require(isIndexerOrOperator, "!auth");
         }
 
-        // Close the allocation
-        __DEPRECATED_allocations[_allocationID].closedAtEpoch = alloc.closedAtEpoch;
-
         // -- Rewards Distribution --
 
         // Process non-zero-allocation rewards tracking
@@ -377,6 +375,11 @@ contract HorizonStakingExtension is HorizonStakingBase, IHorizonStakingExtension
                 alloc.tokens;
         }
 
+        // Close the allocation
+        // Note that this breaks CEI pattern. We update after the rewards distribution logic as it expects the allocation
+        // to still be active. There shouldn't be reentrancy risk here as all internal calls are to trusted contracts.
+        __DEPRECATED_allocations[_allocationID].closedAtEpoch = alloc.closedAtEpoch;
+
         emit AllocationClosed(
             alloc.indexer,
             alloc.subgraphDeploymentID,

packages/subgraph-service/contracts/SubgraphService.sol

@@ -382,9 +382,10 @@ contract SubgraphService is
     /// @inheritdoc IRewardsIssuer
     function getAllocationData(
         address allocationId
-    ) external view override returns (address, bytes32, uint256, uint256, uint256) {
+    ) external view override returns (bool, address, bytes32, uint256, uint256, uint256) {
         Allocation.State memory allo = _allocations[allocationId];
         return (
+            allo.isOpen(),
             allo.indexer,
             allo.subgraphDeploymentId,
             allo.tokens,

packages/subgraph-service/test/mocks/MockRewardsManager.sol

@@ -14,7 +14,7 @@ interface IRewardsIssuer {
     )
         external
         view
-        returns (address indexer, bytes32 subgraphDeploymentId, uint256 tokens, uint256 accRewardsPerAllocatedToken);
+        returns (bool isActive, address indexer, bytes32 subgraphDeploymentId, uint256 tokens, uint256 accRewardsPerAllocatedToken);
 }
 
 contract MockRewardsManager is IRewardsManager {
@@ -71,10 +71,14 @@ contract MockRewardsManager is IRewardsManager {
 
     function takeRewards(address _allocationID) external returns (uint256) {
         address rewardsIssuer = msg.sender;
-        (, , uint256 tokens, uint256 accRewardsPerAllocatedToken) = IRewardsIssuer(rewardsIssuer).getAllocationData(
+        (bool isActive, , , uint256 tokens, uint256 accRewardsPerAllocatedToken) = IRewardsIssuer(rewardsIssuer).getAllocationData(
             _allocationID
         );
 
+        if (!isActive) {
+            return 0;
+        }   
+
         uint256 accRewardsPerTokens = tokens.mulPPM(rewardsPerSignal);
         uint256 rewards = accRewardsPerTokens - accRewardsPerAllocatedToken;
         token.mint(rewardsIssuer, rewards);