From 061ac224a8e18e880c6cbffbc4b725b7c386292a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Tue, 29 Apr 2025 14:02:43 -0300 Subject: [PATCH 1/2] fix: ensure getRewards and takeRewards return 0 for closed allos (OZ L-11) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- .../contracts/rewards/IRewardsIssuer.sol | 3 ++ .../contracts/rewards/RewardsManager.sol | 29 +++++++++++++------ .../contracts/staking/IStakingBase.sol | 2 +- .../contracts/contracts/staking/Staking.sol | 18 +++++++++--- packages/contracts/scripts/build | 6 ++-- .../test/unit/rewards/rewards.test.ts | 25 ++++++++++++++++ .../staking/HorizonStakingExtension.sol | 11 +++---- .../contracts/SubgraphService.sol | 3 +- .../test/mocks/MockRewardsManager.sol | 8 +++-- 9 files changed, 80 insertions(+), 25 deletions(-) diff --git a/packages/contracts/contracts/rewards/IRewardsIssuer.sol b/packages/contracts/contracts/rewards/IRewardsIssuer.sol index 9da18b92b..2a18b6e3c 100644 --- a/packages/contracts/contracts/rewards/IRewardsIssuer.sol +++ b/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, diff --git a/packages/contracts/contracts/rewards/RewardsManager.sol b/packages/contracts/contracts/rewards/RewardsManager.sol index 1517775f9..54dbead81 100644 --- a/packages/contracts/contracts/rewards/RewardsManager.sol +++ b/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,6 +327,7 @@ contract RewardsManager is RewardsManagerV5Storage, GraphUpgradeable, IRewardsMa ); ( + bool isActive, , bytes32 subgraphDeploymentId, uint256 tokens, @@ -333,6 +335,10 @@ contract RewardsManager is RewardsManagerV5Storage, GraphUpgradeable, IRewardsMa 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); diff --git a/packages/contracts/contracts/staking/IStakingBase.sol b/packages/contracts/contracts/staking/IStakingBase.sol index f01ca4326..18c421ef1 100644 --- a/packages/contracts/contracts/staking/IStakingBase.sol +++ b/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 diff --git a/packages/contracts/contracts/staking/Staking.sol b/packages/contracts/contracts/staking/Staking.sol index eaafdee0c..e3751f187 100644 --- a/packages/contracts/contracts/staking/Staking.sol +++ b/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,8 +819,6 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M require(isIndexerOrOperator, "!auth"); } - // Close the allocation - __allocations[_allocationID].closedAtEpoch = alloc.closedAtEpoch; // -- Rewards Distribution -- @@ -836,6 +843,9 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M ); } + // Close the allocation + __allocations[_allocationID].closedAtEpoch = alloc.closedAtEpoch; + emit AllocationClosed( alloc.indexer, alloc.subgraphDeploymentID, diff --git a/packages/contracts/scripts/build b/packages/contracts/scripts/build index 06b9823c1..b034b0892 100755 --- a/packages/contracts/scripts/build +++ b/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 \ No newline at end of file +mv dist/contracts/build/types dist/types \ No newline at end of file diff --git a/packages/contracts/test/unit/rewards/rewards.test.ts b/packages/contracts/test/unit/rewards/rewards.test.ts index fe5ac75fb..50d598e6e 100644 --- a/packages/contracts/test/unit/rewards/rewards.test.ts +++ b/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))) + }) }) }) diff --git a/packages/horizon/contracts/staking/HorizonStakingExtension.sol b/packages/horizon/contracts/staking/HorizonStakingExtension.sol index 12327217c..2b82559a5 100644 --- a/packages/horizon/contracts/staking/HorizonStakingExtension.sol +++ b/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,9 @@ contract HorizonStakingExtension is HorizonStakingBase, IHorizonStakingExtension alloc.tokens; } + // Close the allocation + __DEPRECATED_allocations[_allocationID].closedAtEpoch = alloc.closedAtEpoch; + emit AllocationClosed( alloc.indexer, alloc.subgraphDeploymentID, diff --git a/packages/subgraph-service/contracts/SubgraphService.sol b/packages/subgraph-service/contracts/SubgraphService.sol index 55d75f60f..f0d0ec25d 100644 --- a/packages/subgraph-service/contracts/SubgraphService.sol +++ b/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, diff --git a/packages/subgraph-service/test/mocks/MockRewardsManager.sol b/packages/subgraph-service/test/mocks/MockRewardsManager.sol index dd94d6a2c..a87c9c06a 100644 --- a/packages/subgraph-service/test/mocks/MockRewardsManager.sol +++ b/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); From e05352770d9e00d78e9490c657dc603fd3c69c45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Fri, 2 May 2025 12:19:19 -0300 Subject: [PATCH 2/2] chore: document reentrancy risk vector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- packages/contracts/contracts/staking/Staking.sol | 3 ++- packages/horizon/contracts/staking/HorizonStakingExtension.sol | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/contracts/contracts/staking/Staking.sol b/packages/contracts/contracts/staking/Staking.sol index e3751f187..f4c09d79f 100644 --- a/packages/contracts/contracts/staking/Staking.sol +++ b/packages/contracts/contracts/staking/Staking.sol @@ -819,7 +819,6 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M require(isIndexerOrOperator, "!auth"); } - // -- Rewards Distribution -- // Process non-zero-allocation rewards tracking @@ -844,6 +843,8 @@ 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( diff --git a/packages/horizon/contracts/staking/HorizonStakingExtension.sol b/packages/horizon/contracts/staking/HorizonStakingExtension.sol index 2b82559a5..bb51ac433 100644 --- a/packages/horizon/contracts/staking/HorizonStakingExtension.sol +++ b/packages/horizon/contracts/staking/HorizonStakingExtension.sol @@ -376,6 +376,8 @@ contract HorizonStakingExtension is HorizonStakingBase, IHorizonStakingExtension } // 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(