delegation:

Delegators stake can get soft-locked.

Add a validations in the _delegate function to check whether the indexer to whom the tokens are going to be delegated to has tokens currently staked in the protocol.

If a delegator tries to send tokens by using an _indexer value as a parameter to one without stake the transaction will revert.

This avoids the delegators sending funds to and address by mistake and needing to way the unbonding period to recover from that error.

Author: abarmat

contracts/staking/Staking.sol

@@ -517,10 +517,7 @@ contract Staking is StakingV1Storage, GraphUpgradeable, IStaking {
         require(_tokens > 0, "Cannot stake zero tokens");
 
         // Transfer tokens to stake from caller to this contract
-        require(
-            graphToken().transferFrom(msg.sender, address(this), _tokens),
-            "Cannot transfer tokens to stake"
-        );
+        require(graphToken().transferFrom(msg.sender, address(this), _tokens), "!transfer");
 
         // Stake the transferred tokens
         _stake(_indexer, _tokens);
@@ -557,7 +554,7 @@ contract Staking is StakingV1Storage, GraphUpgradeable, IStaking {
         require(tokensToWithdraw > 0, "No tokens available to withdraw");
 
         // Return tokens to the indexer
-        require(graphToken().transfer(indexer, tokensToWithdraw), "Cannot transfer tokens");
+        require(graphToken().transfer(indexer, tokensToWithdraw), "!transfer");
 
         emit StakeWithdrawn(indexer, tokensToWithdraw);
     }
@@ -611,7 +608,7 @@ contract Staking is StakingV1Storage, GraphUpgradeable, IStaking {
 
         // Give the beneficiary a reward for slashing
         if (_reward > 0) {
-            require(graphToken().transfer(_beneficiary, _reward), "Error sending dispute reward");
+            require(graphToken().transfer(_beneficiary, _reward), "!transfer");
         }
 
         emit StakeSlashed(_indexer, _tokens, _reward, _beneficiary);
@@ -626,10 +623,7 @@ contract Staking is StakingV1Storage, GraphUpgradeable, IStaking {
         address delegator = msg.sender;
 
         // Transfer tokens to delegate to this contract
-        require(
-            graphToken().transferFrom(delegator, address(this), _tokens),
-            "Cannot transfer tokens to stake"
-        );
+        require(graphToken().transferFrom(delegator, address(this), _tokens), "!transfer");
 
         // Update state
         _delegate(delegator, _indexer, _tokens);
@@ -677,7 +671,7 @@ contract Staking is StakingV1Storage, GraphUpgradeable, IStaking {
             _delegate(delegator, _newIndexer, tokensToWithdraw);
         } else {
             // Return tokens to the delegator
-            require(graphToken().transfer(delegator, tokensToWithdraw), "Cannot transfer tokens");
+            require(graphToken().transfer(delegator, tokensToWithdraw), "!transfer");
         }
     }
 
@@ -782,10 +776,7 @@ contract Staking is StakingV1Storage, GraphUpgradeable, IStaking {
         // require(alloc.assetHolder == msg.sender, "caller is not authorized");
 
         // Transfer tokens to collect from the authorized sender
-        require(
-            graphToken().transferFrom(msg.sender, address(this), _tokens),
-            "Cannot transfer tokens to collect"
-        );
+        require(graphToken().transferFrom(msg.sender, address(this), _tokens), "!transfer");
 
         _collect(_allocationID, msg.sender, _tokens);
     }
@@ -843,10 +834,7 @@ contract Staking is StakingV1Storage, GraphUpgradeable, IStaking {
                 _stake(alloc.indexer, tokensToClaim);
             } else {
                 // Transfer funds back to the indexer
-                require(
-                    graphToken().transfer(alloc.indexer, tokensToClaim),
-                    "Cannot transfer tokens"
-                );
+                require(graphToken().transfer(alloc.indexer, tokensToClaim), "!transfer");
             }
         }
 
@@ -1105,10 +1093,12 @@ contract Staking is StakingV1Storage, GraphUpgradeable, IStaking {
         address _indexer,
         uint256 _tokens
     ) internal returns (uint256) {
-        // Can only delegate a non-zero amount of tokens
+        // Only delegate a non-zero amount of tokens
         require(_tokens > 0, "Cannot delegate zero tokens");
-        // Can only delegate to non-empty address
+        // Only delegate to non-empty address
         require(_indexer != address(0), "Cannot delegate to empty address");
+        // Only delegate to staked indexer
+        require(stakes[_indexer].hasTokens(), "Cannot delegate to non-staked indexer");
 
         // Get the delegation pool of the indexer
         DelegationPool storage pool = delegationPools[_indexer];

package.json

@@ -32,6 +32,7 @@
     "@types/yargs": "^15.0.5",
     "@typescript-eslint/eslint-plugin": "^3.10.1",
     "@typescript-eslint/parser": "^3.10.1",
+    "bignumber.js": "^9.0.0",
     "buidler-gas-reporter": "^0.1.3",
     "chai": "^4.2.0",
     "cli-table": "^0.3.1",

test/rewards/rewards.test.ts

@@ -1,5 +1,6 @@
 import { expect } from 'chai'
 import { constants, BigNumber } from 'ethers'
+import { BigNumber as BN } from 'bignumber.js'
 
 import { NetworkFixture } from '../lib/fixtures'
 
@@ -25,8 +26,7 @@ const MAX_PPM = 1000000
 
 const { HashZero, WeiPerEther } = constants
 
-const toFloat = (n: BigNumber) => parseFloat(formatGRT(n))
-const toRound = (n: BigNumber) => Math.round(toFloat(n))
+const toRound = (n: BigNumber) => formatGRT(n).split('.')[0]
 
 describe('Rewards', () => {
   let delegator: Account
@@ -55,9 +55,11 @@ describe('Rewards', () => {
   const ISSUANCE_RATE_PER_BLOCK = toBN('1012272234429039270') // % increase every block
 
   // Core formula that gets accumulated rewards per signal for a period of time
-  const getRewardsPerSignal = (p: BigNumber, r: BigNumber, t: BigNumber, s: BigNumber): number => {
-    if (!toFloat(s)) return 0
-    return (toRound(p) * toFloat(r) ** t.toNumber() - toFloat(p)) / toFloat(s)
+  const getRewardsPerSignal = (p: BN, r: BN, t: BN, s: BN): string => {
+    if (s.eq(0)) {
+      return '0'
+    }
+    return p.times(r.pow(t)).minus(p).div(s).toPrecision(18).toString()
   }
 
   // Tracks the accumulated rewards as totalSignalled or supply changes across snapshots
@@ -74,7 +76,7 @@ describe('Rewards', () => {
     }
 
     async snapshot() {
-      this.accumulated = this.accumulated.add(await this.accruedGRT())
+      this.accumulated = this.accumulated.add(await this.accrued())
       this.totalSupply = await grt.totalSupply()
       this.totalSignalled = await grt.balanceOf(curation.address)
       this.lastUpdatedBlock = await latestBlock()
@@ -88,21 +90,13 @@ describe('Rewards', () => {
 
     async accrued() {
       const nBlocks = await this.elapsedBlocks()
-      return getRewardsPerSignal(
-        this.totalSupply,
-        ISSUANCE_RATE_PER_BLOCK,
-        nBlocks,
-        this.totalSignalled,
+      const n = getRewardsPerSignal(
+        new BN(this.totalSupply.toString()),
+        new BN(ISSUANCE_RATE_PER_BLOCK.toString()).div(1e18),
+        new BN(nBlocks.toString()),
+        new BN(this.totalSignalled.toString()),
       )
-    }
-
-    async accruedGRT() {
-      const n = await this.accrued()
-      return toGRT(n.toString())
-    }
-
-    async accruedRounded() {
-      return Math.round(await this.accrued())
+      return toGRT(n)
     }
   }
 
@@ -119,10 +113,10 @@ describe('Rewards', () => {
     // Contract calculation
     const contractAccrued = await rewardsManager.getNewRewardsPerSignal()
     // Local calculation
-    const expectedAccrued = await tracker.accruedRounded()
+    const expectedAccrued = await tracker.accrued()
 
     // Check
-    expect(expectedAccrued).eq(toRound(contractAccrued))
+    expect(toRound(expectedAccrued)).eq(toRound(contractAccrued))
     return expectedAccrued
   }
 
@@ -268,8 +262,8 @@ describe('Rewards', () => {
       const contractAccrued = await rewardsManager.accRewardsPerSignal()
 
       // Check
-      const expectedAccrued = await tracker.accruedRounded()
-      expect(expectedAccrued).eq(toRound(contractAccrued))
+      const expectedAccrued = await tracker.accrued()
+      expect(toRound(expectedAccrued)).eq(toRound(contractAccrued))
     })
 
     it('update the accumulated rewards per signal state after many blocks', async function () {
@@ -286,8 +280,8 @@ describe('Rewards', () => {
       const contractAccrued = await rewardsManager.accRewardsPerSignal()
 
       // Check
-      const expectedAccrued = await tracker.accruedRounded()
-      expect(expectedAccrued).eq(toRound(contractAccrued))
+      const expectedAccrued = await tracker.accrued()
+      expect(toRound(expectedAccrued)).eq(toRound(contractAccrued))
     })
   })
 
@@ -350,13 +344,13 @@ describe('Rewards', () => {
       // Check
       const contractRewardsSG1 = (await rewardsManager.subgraphs(subgraphDeploymentID1))
         .accRewardsForSubgraph
-      const rewardsPerSignal1 = await tracker1.accruedGRT()
+      const rewardsPerSignal1 = await tracker1.accrued()
       const expectedRewardsSG1 = rewardsPerSignal1.mul(signalled1).div(WeiPerEther)
       expect(toRound(expectedRewardsSG1)).eq(toRound(contractRewardsSG1))
 
       const contractAccrued = await rewardsManager.accRewardsPerSignal()
-      const expectedAccrued = await tracker1.accruedRounded()
-      expect(expectedAccrued).eq(toRound(contractAccrued))
+      const expectedAccrued = await tracker1.accrued()
+      expect(toRound(expectedAccrued)).eq(toRound(contractAccrued))
 
       const contractBlockUpdated = await rewardsManager.accRewardsPerSignalLastBlockUpdated()
       const expectedBlockUpdated = await latestBlock()
@@ -426,8 +420,8 @@ describe('Rewards', () => {
       // Prepare expected results
       // NOTE: calculated the expected result manually as the above code has 1 off block difference
       // replace with a RewardsManagerMock
-      const expectedSubgraphRewards = 891695471
-      const expectedRewardsAT = 51572
+      const expectedSubgraphRewards = toGRT('891695470')
+      const expectedRewardsAT = toGRT('51571')
 
       // Update
       await rewardsManager.onSubgraphAllocationUpdate(subgraphDeploymentID1)
@@ -439,8 +433,8 @@ describe('Rewards', () => {
       )
       const contractRewardsAT = subgraph.accRewardsPerAllocatedToken
 
-      expect(expectedSubgraphRewards).eq(toRound(contractSubgraphRewards))
-      expect(expectedRewardsAT).eq(toRound(contractRewardsAT))
+      expect(toRound(expectedSubgraphRewards)).eq(toRound(contractSubgraphRewards))
+      expect(toRound(expectedRewardsAT)).eq(toRound(contractRewardsAT))
     })
   })
 
@@ -486,37 +480,59 @@ describe('Rewards', () => {
       queryFeeCut: BigNumber
       cooldownBlocks: number
     }
+    async function setupIndexerAllocation() {
+      // Setup
+      await epochManager.setEpochLength(10)
+
+      // Update total signalled
+      const signalled1 = toGRT('1500')
+      await curation.connect(curator1.signer).mint(subgraphDeploymentID1, signalled1)
 
-    async function setupIndexerDelegation(
+      // Allocate
+      const tokensToAllocate = toGRT('12500')
+      await staking.connect(indexer1.signer).stake(tokensToAllocate)
+      await staking
+        .connect(indexer1.signer)
+        .allocate(
+          subgraphDeploymentID1,
+          tokensToAllocate,
+          channelPubKey,
+          assetHolder.address,
+          metadata,
+        )
+    }
+
+    async function setupIndexerAllocationWithDelegation(
       tokensToDelegate: BigNumber,
       delegationParams: DelegationParameters,
     ) {
+      const tokensToAllocate = toGRT('12500')
+
+      // Setup
+      await epochManager.setEpochLength(10)
+
       // Transfer some funds from the curator, I don't want to mint new tokens
       await grt.connect(curator1.signer).transfer(delegator.address, tokensToDelegate)
       await grt.connect(delegator.signer).approve(staking.address, tokensToDelegate)
 
-      // Delegate
+      // Stake and set delegation parameters
+      await staking.connect(indexer1.signer).stake(tokensToAllocate)
       await staking
         .connect(indexer1.signer)
         .setDelegationParameters(
           delegationParams.indexingRewardCut,
           delegationParams.queryFeeCut,
           delegationParams.cooldownBlocks,
         )
-      await staking.connect(delegator.signer).delegate(indexer1.address, tokensToDelegate)
-    }
 
-    async function setupIndexerAllocation() {
-      // Setup
-      await epochManager.setEpochLength(10)
+      // Delegate
+      await staking.connect(delegator.signer).delegate(indexer1.address, tokensToDelegate)
 
       // Update total signalled
       const signalled1 = toGRT('1500')
       await curation.connect(curator1.signer).mint(subgraphDeploymentID1, signalled1)
 
       // Allocate
-      const tokensToAllocate = toGRT('12500')
-      await staking.connect(indexer1.signer).stake(tokensToAllocate)
       await staking
         .connect(indexer1.signer)
         .allocate(
@@ -569,14 +585,13 @@ describe('Rewards', () => {
     it('should distribute rewards on closed allocation w/delegators', async function () {
       // Setup
       const delegationParams = {
-        indexingRewardCut: toBN('50000'), // 5%
+        indexingRewardCut: toBN('823000'), // 82.30%
         queryFeeCut: toBN('80000'), // 8%
         cooldownBlocks: 5,
       }
       const tokensToDelegate = toGRT('2000')
 
-      await setupIndexerDelegation(tokensToDelegate, delegationParams)
-      await setupIndexerAllocation()
+      await setupIndexerAllocationWithDelegation(tokensToDelegate, delegationParams)
 
       // Jump
       await advanceBlocks(await epochManager.epochLength())
@@ -597,7 +612,7 @@ describe('Rewards', () => {
       // Check that rewards are put into indexer stake (only indexer cut)
       // Check that rewards are put into delegators pool accordingly
       // NOTE: calculated manually on a spreadsheet
-      const expectedIndexingRewards = toGRT('1471954234')
+      const expectedIndexingRewards = toGRT('1454109066')
       // Calculate delegators cut
       const indexerRewards = delegationParams.indexingRewardCut
         .mul(expectedIndexingRewards)

test/staking/delegation.test.ts

@@ -177,7 +177,7 @@ describe('Staking::Delegation', () => {
     }
 
     // Distribute test funds
-    for (const wallet of [me, indexer, assetHolder]) {
+    for (const wallet of [me, indexer, indexer2, assetHolder]) {
       await grt.connect(governor.signer).mint(wallet.address, toGRT('1000000'))
       await grt.connect(wallet.signer).approve(staking.address, toGRT('1000000'))
     }
@@ -302,19 +302,30 @@ describe('Staking::Delegation', () => {
   })
 
   describe('lifecycle', function () {
+    beforeEach(async function () {
+      // Stake some funds as indexer
+      await staking.connect(indexer.signer).stake(toGRT('1000'))
+    })
+
     describe('delegate', function () {
-      it('reject to delegate zero tokens', async function () {
+      it('reject delegate with zero tokens', async function () {
         const tokensToDelegate = toGRT('0')
         const tx = staking.connect(delegator.signer).delegate(indexer.address, tokensToDelegate)
         await expect(tx).revertedWith('Cannot delegate zero tokens')
       })
 
-      it('reject to delegate to empty address', async function () {
+      it('reject delegate to empty address', async function () {
         const tokensToDelegate = toGRT('100')
         const tx = staking.connect(delegator.signer).delegate(AddressZero, tokensToDelegate)
         await expect(tx).revertedWith('Cannot delegate to empty address')
       })
 
+      it('reject delegate to non-staked indexer', async function () {
+        const tokensToDelegate = toGRT('100')
+        const tx = staking.connect(delegator.signer).delegate(me.address, tokensToDelegate)
+        await expect(tx).revertedWith('Cannot delegate to non-staked indexer')
+      })
+
       it('should delegate tokens and account shares proportionally', async function () {
         // Multiple delegations should work
         await shouldDelegate(delegator, toGRT('1234'))
@@ -409,6 +420,9 @@ describe('Staking::Delegation', () => {
         await advanceToNextEpoch(epochManager) // epoch 1
         await advanceToNextEpoch(epochManager) // epoch 2
 
+        // We stake on indexer2 so the delegator is able to re-delegate to it
+        // if we didn't do it, it will revert because of indexer2 not havings stake
+        await staking.connect(indexer2.signer).stake(toGRT('1000'))
         // Withdraw
         await shouldWithdrawDelegated(delegator, indexer2.address, tokensToWithdraw)
       })