From 914ff3324121f890c343ac93018206fde0c71322 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Thu, 21 Jul 2022 11:16:02 +0200 Subject: [PATCH 1/4] test: add a test for underflow in rewards when min signal changes --- test/rewards/rewards.test.ts | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/test/rewards/rewards.test.ts b/test/rewards/rewards.test.ts index 2c9021d76..961c4ec3f 100644 --- a/test/rewards/rewards.test.ts +++ b/test/rewards/rewards.test.ts @@ -636,6 +636,41 @@ describe('Rewards', () => { expect(toRound(afterTokenSupply)).eq(toRound(expectedTokenSupply)) }) + it('does not revert with an underflow if the minimum signal changes', async function () { + // Align with the epoch boundary + await advanceToNextEpoch(epochManager) + // Setup + await setupIndexerAllocation() + + await rewardsManager.connect(governor.signer).setMinimumSubgraphSignal(toGRT(14000)) + + // Jump + await advanceToNextEpoch(epochManager) + + // Close allocation. At this point rewards should be collected for that indexer + const tx = staking.connect(indexer1.signer).closeAllocation(allocationID1, randomHexBytes()) + await expect(tx) + .emit(rewardsManager, 'RewardsAssigned') + .withArgs(indexer1.address, allocationID1, await epochManager.currentEpoch(), toBN(0)) + }) + + it('does not revert if signal was already under minimum', async function () { + await rewardsManager.connect(governor.signer).setMinimumSubgraphSignal(toGRT(2000)) + // Align with the epoch boundary + await advanceToNextEpoch(epochManager) + // Setup + await setupIndexerAllocation() + + // Jump + await advanceToNextEpoch(epochManager) + // Close allocation. At this point rewards should be collected for that indexer + const tx = staking.connect(indexer1.signer).closeAllocation(allocationID1, randomHexBytes()) + + await expect(tx) + .emit(rewardsManager, 'RewardsAssigned') + .withArgs(indexer1.address, allocationID1, await epochManager.currentEpoch(), toBN(0)) + }) + it('should distribute rewards on closed allocation and send to destination', async function () { const destinationAddress = randomHexBytes(20) await staking.connect(indexer1.signer).setRewardsDestination(destinationAddress) From 8a844a14acd7d82b6cc14f2377238f5c65ee0abf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Thu, 21 Jul 2022 11:32:20 +0200 Subject: [PATCH 2/4] test: also test the case where signal change happens after allocation --- test/rewards/rewards.test.ts | 41 ++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/test/rewards/rewards.test.ts b/test/rewards/rewards.test.ts index 961c4ec3f..23aee0dfa 100644 --- a/test/rewards/rewards.test.ts +++ b/test/rewards/rewards.test.ts @@ -537,6 +537,29 @@ describe('Rewards', () => { ) } + async function setupIndexerAllocationSignalingAfter() { + // Setup + await epochManager.setEpochLength(10) + + // Allocate + const tokensToAllocate = toGRT('12500') + await staking.connect(indexer1.signer).stake(tokensToAllocate) + await staking + .connect(indexer1.signer) + .allocateFrom( + indexer1.address, + subgraphDeploymentID1, + tokensToAllocate, + allocationID1, + metadata, + await channelKey1.generateProof(indexer1.address), + ) + + // Update total signalled + const signalled1 = toGRT('1500') + await curation.connect(curator1.signer).mint(subgraphDeploymentID1, signalled1, 0) + } + async function setupIndexerAllocationWithDelegation( tokensToDelegate: BigNumber, delegationParams: DelegationParameters, @@ -654,6 +677,24 @@ describe('Rewards', () => { .withArgs(indexer1.address, allocationID1, await epochManager.currentEpoch(), toBN(0)) }) + it('does not revert with an underflow if the minimum signal changes, and signal came after allocation', async function () { + // Align with the epoch boundary + await advanceToNextEpoch(epochManager) + // Setup + await setupIndexerAllocationSignalingAfter() + + await rewardsManager.connect(governor.signer).setMinimumSubgraphSignal(toGRT(14000)) + + // Jump + await advanceToNextEpoch(epochManager) + + // Close allocation. At this point rewards should be collected for that indexer + const tx = staking.connect(indexer1.signer).closeAllocation(allocationID1, randomHexBytes()) + await expect(tx) + .emit(rewardsManager, 'RewardsAssigned') + .withArgs(indexer1.address, allocationID1, await epochManager.currentEpoch(), toBN(0)) + }) + it('does not revert if signal was already under minimum', async function () { await rewardsManager.connect(governor.signer).setMinimumSubgraphSignal(toGRT(2000)) // Align with the epoch boundary From 9ac3b0754ad9bce2619291b64c2fe0de0211fad3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Carranza=20V=C3=A9lez?= Date: Thu, 21 Jul 2022 11:22:36 +0200 Subject: [PATCH 3/4] fix: prevent underflow when subgraphs go under the minimum signal threshold --- contracts/rewards/RewardsManager.sol | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/contracts/rewards/RewardsManager.sol b/contracts/rewards/RewardsManager.sol index 6d8f1be74..0c68adf49 100644 --- a/contracts/rewards/RewardsManager.sol +++ b/contracts/rewards/RewardsManager.sol @@ -281,9 +281,12 @@ contract RewardsManager is RewardsManagerV3Storage, GraphUpgradeable, IRewardsMa Subgraph storage subgraph = subgraphs[_subgraphDeploymentID]; uint256 accRewardsForSubgraph = getAccRewardsForSubgraph(_subgraphDeploymentID); - uint256 newRewardsForSubgraph = accRewardsForSubgraph.sub( - subgraph.accRewardsForSubgraphSnapshot - ); + uint256 newRewardsForSubgraph; + if (accRewardsForSubgraph > subgraph.accRewardsForSubgraphSnapshot) { + newRewardsForSubgraph = accRewardsForSubgraph.sub( + subgraph.accRewardsForSubgraphSnapshot + ); + } uint256 subgraphAllocatedTokens = staking().getSubgraphAllocatedTokens( _subgraphDeploymentID From 0ec8778f51a20501f32852114a743bcd6b58f7b2 Mon Sep 17 00:00:00 2001 From: Ariel Barmat Date: Tue, 2 Aug 2022 15:14:53 -0300 Subject: [PATCH 4/4] chore: use an existing function to calculate diff or zero if underflow --- contracts/rewards/RewardsManager.sol | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/contracts/rewards/RewardsManager.sol b/contracts/rewards/RewardsManager.sol index 0c68adf49..6d2e78965 100644 --- a/contracts/rewards/RewardsManager.sol +++ b/contracts/rewards/RewardsManager.sol @@ -6,6 +6,7 @@ pragma abicoder v2; import "@openzeppelin/contracts/math/SafeMath.sol"; import "../upgrades/GraphUpgradeable.sol"; +import "../staking/libs/MathUtils.sol"; import "./RewardsManagerStorage.sol"; import "./IRewardsManager.sol"; @@ -281,12 +282,10 @@ contract RewardsManager is RewardsManagerV3Storage, GraphUpgradeable, IRewardsMa Subgraph storage subgraph = subgraphs[_subgraphDeploymentID]; uint256 accRewardsForSubgraph = getAccRewardsForSubgraph(_subgraphDeploymentID); - uint256 newRewardsForSubgraph; - if (accRewardsForSubgraph > subgraph.accRewardsForSubgraphSnapshot) { - newRewardsForSubgraph = accRewardsForSubgraph.sub( - subgraph.accRewardsForSubgraphSnapshot - ); - } + uint256 newRewardsForSubgraph = MathUtils.diffOrZero( + accRewardsForSubgraph, + subgraph.accRewardsForSubgraphSnapshot + ); uint256 subgraphAllocatedTokens = staking().getSubgraphAllocatedTokens( _subgraphDeploymentID