DRAFT - all audit fixes

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/data-service/extensions/DataServiceFees.sol

@@ -63,6 +63,9 @@ abstract contract DataServiceFees is DataService, DataServiceFeesV1Storage, IDat
     /**
      * @notice Releases expired stake claims for a service provider.
      * @dev This function can be overriden and/or disabled.
+     * @dev Note that the list is traversed by creation date not by releasableAt date. Traversing will stop
+     * when the first stake claim that is not yet expired is found even if later stake claims have expired. This
+     * could happen if stake claims are genereted with different unlock periods.
      * @dev Emits a {StakeClaimsReleased} event, and a {StakeClaimReleased} event for each claim released.
      * @param _serviceProvider The address of the service provider
      * @param _numClaimsToRelease Amount of stake claims to process. If 0, all stake claims are processed.
@@ -85,7 +88,7 @@ abstract contract DataServiceFees is DataService, DataServiceFeesV1Storage, IDat
      * @dev This function is used as a callback in the stake claims linked list traversal.
      * @param _claimId The id of the stake claim
      * @param _acc The accumulator for the stake claims being processed
-     * @return Wether the stake claim is still locked, indicating that the traversal should continue or stop.
+     * @return Whether the stake claim is still locked, indicating that the traversal should continue or stop.
      * @return The updated accumulator data
      */
     function _processStakeClaim(bytes32 _claimId, bytes memory _acc) private returns (bool, bytes memory) {

packages/horizon/contracts/data-service/extensions/DataServiceFeesStorage.sol

@@ -11,6 +11,7 @@ import { LinkedList } from "../../libraries/LinkedList.sol";
  * bugs. We may have an active bug bounty program.
  */
 abstract contract DataServiceFeesV1Storage {
+    /// @notice The amount of tokens locked in stake claims for each service provider
     mapping(address serviceProvider => uint256 tokens) public feesProvisionTracker;
 
     /// @notice List of all locked stake claims to be released to service providers

packages/horizon/contracts/data-service/extensions/DataServicePausable.sol

@@ -32,12 +32,12 @@ abstract contract DataServicePausable is Pausable, DataService, IDataServicePaus
     }
 
     /// @inheritdoc IDataServicePausable
-    function pause() external override onlyPauseGuardian whenNotPaused {
+    function pause() external override onlyPauseGuardian {
         _pause();
     }
 
     /// @inheritdoc IDataServicePausable
-    function unpause() external override onlyPauseGuardian whenPaused {
+    function unpause() external override onlyPauseGuardian {
         _unpause();
     }
 

packages/horizon/contracts/data-service/extensions/DataServicePausableUpgradeable.sol

@@ -31,12 +31,12 @@ abstract contract DataServicePausableUpgradeable is PausableUpgradeable, DataSer
     }
 
     /// @inheritdoc IDataServicePausable
-    function pause() external override onlyPauseGuardian whenNotPaused {
+    function pause() external override onlyPauseGuardian {
         _pause();
     }
 
     /// @inheritdoc IDataServicePausable
-    function unpause() external override onlyPauseGuardian whenPaused {
+    function unpause() external override onlyPauseGuardian {
         _unpause();
     }
 

packages/horizon/contracts/data-service/utilities/ProvisionManager.sol

@@ -19,6 +19,10 @@ import { ProvisionManagerV1Storage } from "./ProvisionManagerStorage.sol";
  * The parameters are:
  * - Provision parameters (thawing period and verifier cut)
  * - Provision tokens
+ *
+ * Note that default values for all provision parameters provide the most permissive configuration, it's
+ * highly recommended to override them at the data service level.
+ *
  * @custom:security-contact Please email [email protected] if you find any
  * bugs. We may have an active bug bounty program.
  */
@@ -43,6 +47,9 @@ abstract contract ProvisionManager is Initializable, GraphDirectory, ProvisionMa
     /// @notice The default maximum provision tokens.
     uint256 internal constant DEFAULT_MAX_PROVISION_TOKENS = type(uint256).max;
 
+    /// @notice The default delegation ratio.
+    uint32 internal constant DEFAULT_DELEGATION_RATIO = type(uint32).max;
+
     /**
      * @notice Emitted when the provision tokens range is set.
      * @param min The minimum allowed value for the provision tokens.
@@ -138,15 +145,14 @@ abstract contract ProvisionManager is Initializable, GraphDirectory, ProvisionMa
         _setProvisionTokensRange(DEFAULT_MIN_PROVISION_TOKENS, DEFAULT_MAX_PROVISION_TOKENS);
         _setVerifierCutRange(DEFAULT_MIN_VERIFIER_CUT, DEFAULT_MAX_VERIFIER_CUT);
         _setThawingPeriodRange(DEFAULT_MIN_THAWING_PERIOD, DEFAULT_MAX_THAWING_PERIOD);
+        _setDelegationRatio(DEFAULT_DELEGATION_RATIO);
     }
 
     /**
      * @notice Verifies and accepts the provision parameters of a service provider in
      * the {HorizonStaking} contract.
      * @dev Checks the pending provision parameters, not the current ones.
      *
-     * Emits a {ProvisionPendingParametersAccepted} event.
-     *
      * @param _serviceProvider The address of the service provider.
      */
     function _acceptProvisionParameters(address _serviceProvider) internal {

packages/horizon/contracts/interfaces/IAuthorizable.sol

@@ -100,7 +100,7 @@ interface IAuthorizable {
      * - `proof` must be a valid signature from the signer being authorized
      *
      * Emits a {SignerAuthorized} event
-     * @param signer The addres of the signer
+     * @param signer The address of the signer
      * @param proofDeadline The deadline for the proof provided by the signer
      * @param proof The proof provided by the signer to be authorized by the authorizer
      * consists of (chain id, verifying contract address, domain, proof deadline, authorizer address)
@@ -141,7 +141,8 @@ interface IAuthorizable {
     function revokeAuthorizedSigner(address signer) external;
 
     /**
-     * @notice Returns the timestamp at which the thawing period ends for a signer
+     * @notice Returns the timestamp at which the thawing period ends for a signer.
+     * Returns 0 if the signer is not thawing.
      * @param signer The address of the signer
      * @return The timestamp at which the thawing period ends
      */

packages/horizon/contracts/interfaces/internal/IHorizonStakingBase.sol

@@ -169,7 +169,11 @@ interface IHorizonStakingBase {
     ) external view returns (LinkedList.List memory);
 
     /**
-     * @notice Gets the amount of thawed tokens for a given provision.
+     * @notice Gets the amount of thawed tokens that can be releasedfor a given provision.
+     * @dev Note that the value returned by this function does not return the total amount of thawed tokens
+     * but only those that can be released. If thaw requests are created with different thawing periods it's
+     * possible that an unexpired thaw request temporarily blocks the release of other ones that have already
+     * expired. This function will stop counting when it encounters the first thaw request that is not yet expired.
      * @param thawRequestType The type of thaw request.
      * @param serviceProvider The address of the service provider.
      * @param verifier The address of the verifier.

packages/horizon/contracts/interfaces/internal/IHorizonStakingExtension.sol

@@ -140,6 +140,10 @@ interface IHorizonStakingExtension is IRewardsIssuer {
 
     /**
      * @notice Slash the indexer stake. Delegated tokens are not subject to slashing.
+     * Note that depending on the state of the indexer's stake, the slashed amount might be smaller than the
+     * requested slash amount. This can happen if the indexer has moved a significant part of their stake to
+     * a provision. Any outstanding slashing amount should be settled using Horizon's slash function
+     * {IHorizonStaking.slash}.
      * @dev Can only be called by the slasher role.
      * @param indexer Address of indexer to slash
      * @param tokens Amount of tokens to slash from the indexer stake