staking,disputes: slash can be bypassed

The DisputeManager contract was enforcing a minimum indexer stake for slashing leading to indexers below that stake to be able to bypass slashing. This is a non-intended consequence of adding a minimum indexer stake to avoid the abuse of the conflicting attestation feature.

This commit add a configurable minimum indexer stake in the Staking contract that is enforced when staking and unstaking. Now, the DisputeManager will always slash indexers not matter their current stake as long as it is non-zero.

Author: abarmat

contracts/disputes/DisputeManager.sol

@@ -92,9 +92,6 @@ contract DisputeManager is Managed {
     // Minimum deposit required to create a Dispute
     uint256 public minimumDeposit;
 
-    // Minimum stake an indexer needs to have to allow be disputed
-    uint256 public minimumIndexerStake;
-
     // Percentage of indexer slashed funds to assign as a reward to fisherman in successful dispute
     // Parts per million. (Allows for 4 decimal points, 999,999 = 99.9999%)
     uint32 public fishermanRewardPercentage;
@@ -260,15 +257,6 @@ contract DisputeManager is Managed {
         emit ParameterUpdated("slashingPercentage");
     }
 
-    /**
-     * @dev Set the minimum indexer stake required to allow be disputed.
-     * @param _minimumIndexerStake Minimum indexer stake
-     */
-    function setMinimumIndexerStake(uint256 _minimumIndexerStake) external onlyGovernor {
-        minimumIndexerStake = _minimumIndexerStake;
-        emit ParameterUpdated("minimumIndexerStake");
-    }
-
     /**
      * @dev Return whether a dispute exists or not.
      * @notice Return if dispute with ID `_disputeID` exists
@@ -460,10 +448,7 @@ contract DisputeManager is Managed {
         address indexer = getAttestationIndexer(_attestation);
 
         // The indexer is disputable
-        require(
-            staking().getIndexerStakedTokens(indexer) >= minimumIndexerStake,
-            "Dispute under minimum indexer stake amount"
-        );
+        require(staking().hasStake(indexer), "Dispute indexer has no stake");
 
         // Create a disputeID
         bytes32 disputeID = keccak256(
@@ -540,10 +525,7 @@ contract DisputeManager is Managed {
         require(alloc.indexer != address(0), "Dispute allocation must exist");
 
         // The indexer must be disputable
-        require(
-            staking().getIndexerStakedTokens(alloc.indexer) >= minimumIndexerStake,
-            "Dispute under minimum indexer stake amount"
-        );
+        require(staking().hasStake(alloc.indexer), "Dispute indexer has no stake");
 
         // Store dispute
         disputes[disputeID] = Dispute(alloc.indexer, _fisherman, _deposit, 0);

contracts/staking/IStaking.sol

@@ -56,6 +56,8 @@ interface IStaking {
 
     // -- Configuration --
 
+    function setMinimumIndexerStake(uint256 _minimumIndexerStake) external;
+
     function setThawingPeriod(uint32 _thawingPeriod) external;
 
     function setCurationPercentage(uint32 _percentage) external;

contracts/staking/Staking.sol

@@ -216,6 +216,15 @@ contract Staking is StakingV1Storage, GraphUpgradeable, IStaking {
         Staking(address(_proxy)).initialize(_controller);
     }
 
+    /**
+     * @dev Set the minimum indexer stake required to.
+     * @param _minimumIndexerStake Minimum indexer stake
+     */
+    function setMinimumIndexerStake(uint256 _minimumIndexerStake) external override onlyGovernor {
+        minimumIndexerStake = _minimumIndexerStake;
+        emit ParameterUpdated("minimumIndexerStake");
+    }
+
     /**
      * @dev Set the thawing period for unstaking.
      * @param _thawingPeriod Period in blocks to wait for token withdrawals after unstaking
@@ -478,6 +487,7 @@ contract Staking is StakingV1Storage, GraphUpgradeable, IStaking {
 
     /**
      * @dev Get the total amount of tokens available to use in allocations.
+     * This considers the indexer stake and delegated tokens according to delegation ratio
      * @param _indexer Address of the indexer
      * @return Amount of tokens staked by the indexer
      */
@@ -490,20 +500,7 @@ contract Staking is StakingV1Storage, GraphUpgradeable, IStaking {
             ? pool.tokens
             : tokensDelegatedMax;
 
-        uint256 tokensUsed = indexerStake.tokensUsed();
-        uint256 tokensCapacity = indexerStake.tokensStaked.add(tokensDelegated);
-
-        // If more tokens are used than the current capacity, the indexer is overallocated.
-        // This means the indexer doesn't have available capacity to create new allocations.
-        // We can reach this state when the indexer has funds allocated and then any
-        // of these conditions happen:
-        // - The delegationRatio ratio is reduced.
-        // - The indexer stake is slashed.
-        // - A delegator removes enough stake.
-        if (tokensUsed > tokensCapacity) {
-            return 0;
-        }
-        return tokensCapacity.sub(tokensUsed);
+        return indexerStake.tokensAvailableWithDelegation(tokensDelegated);
     }
 
     /**
@@ -533,6 +530,12 @@ contract Staking is StakingV1Storage, GraphUpgradeable, IStaking {
     function stakeTo(address _indexer, uint256 _tokens) public override notPartialPaused {
         require(_tokens > 0, "Cannot stake zero tokens");
 
+        // Ensure minimum stake
+        require(
+            stakes[_indexer].tokensSecureStake().add(_tokens) >= minimumIndexerStake,
+            "Stake must be above minimum required"
+        );
+
         // Transfer tokens to stake from caller to this contract
         require(graphToken().transferFrom(msg.sender, address(this), _tokens), "!transfer");
 
@@ -554,6 +557,13 @@ contract Staking is StakingV1Storage, GraphUpgradeable, IStaking {
             "Not enough tokens available to unstake"
         );
 
+        // Ensure minimum stake
+        uint256 newStake = indexerStake.tokensSecureStake().sub(_tokens);
+        require(
+            newStake == 0 || newStake >= minimumIndexerStake,
+            "Stake must be above minimum required"
+        );
+
         indexerStake.lockTokens(_tokens, thawingPeriod);
 
         emit StakeLocked(indexer, indexerStake.tokensLocked, indexerStake.tokensLockedUntil);
@@ -564,10 +574,9 @@ contract Staking is StakingV1Storage, GraphUpgradeable, IStaking {
      */
     function withdraw() external override notPaused {
         address indexer = msg.sender;
-        Stakes.Indexer storage indexerStake = stakes[indexer];
 
         // Get tokens available for withdraw and update balance
-        uint256 tokensToWithdraw = indexerStake.withdrawTokens();
+        uint256 tokensToWithdraw = stakes[indexer].withdrawTokens();
         require(tokensToWithdraw > 0, "No tokens available to withdraw");
 
         // Return tokens to the indexer
@@ -849,7 +858,10 @@ contract Staking is StakingV1Storage, GraphUpgradeable, IStaking {
     }
 
     /**
-     * @dev Stake tokens on the indexer
+     * @dev Stake tokens on the indexer.
+     * This function does not check minimum indexer stake requirement to allow
+     * to be called by functions that increase the stake when collecting rewards
+     * without reverting
      * @param _indexer Address of staking party
      * @param _tokens Amount of tokens to stake
      */

contracts/staking/StakingStorage.sol

@@ -9,6 +9,9 @@ import "./libs/Stakes.sol";
 contract StakingV1Storage is Managed {
     // -- Staking --
 
+    // Minimum amount of tokens an indexer needs to stake
+    uint256 public minimumIndexerStake;
+
     // Time in blocks to unstake
     uint32 public thawingPeriod; // in blocks
 

contracts/staking/libs/Stakes.sol

@@ -151,19 +151,57 @@ library Stakes {
     }
 
     /**
-     * @dev Tokens free balance on the indexer stake.
-     * tokensStaked - tokensAllocated - tokensLocked
+     * @dev Return the amount of tokens staked not considering the ones that are already going
+     * through the thawing period or are ready for withdrawal. We call it secure stake because
+     * it is not subject to change by a withdraw call from the indexer.
+     * @param stake Stake data
+     * @return Token amount
+     */
+    function tokensSecureStake(Stakes.Indexer memory stake) internal pure returns (uint256) {
+        return stake.tokensStaked.sub(stake.tokensLocked);
+    }
+
+    /**
+     * @dev Tokens free balance on the indexer stake that can be used for any purpose.
+     * Any token that is allocated cannot be used as well as tokens that are going through the
+     * thawing period or are withdrawable
+     * Calc: tokensStaked - tokensAllocated - tokensLocked
      * @param stake Stake data
      * @return Token amount
      */
     function tokensAvailable(Stakes.Indexer memory stake) internal pure returns (uint256) {
+        return stake.tokensAvailableWithDelegation(0);
+    }
+
+    /**
+     * @dev Tokens free balance on the indexer stake that can be used for allocations.
+     * This function accepts a parameter for extra delegated capacity that takes into
+     * account delegated tokens
+     * @param stake Stake data
+     * @param _delegatedCapacity Amount of tokens used from delegators to calculate availability
+     * @return Token amount
+     */
+    function tokensAvailableWithDelegation(Stakes.Indexer memory stake, uint256 _delegatedCapacity)
+        internal
+        pure
+        returns (uint256)
+    {
+        uint256 tokensCapacity = stake.tokensStaked.add(_delegatedCapacity);
         uint256 _tokensUsed = stake.tokensUsed();
-        // Indexer stake is over allocated: return 0 to avoid stake to be used until
-        // the overallocation is restored by staking more tokens or unallocating tokens
-        if (_tokensUsed > stake.tokensStaked) {
+        // If more tokens are used than the current capacity, the indexer is overallocated.
+        // This means the indexer doesn't have available capacity to create new allocations.
+        // We can reach this state when the indexer has funds allocated and then any
+        // of these conditions happen:
+        // - The delegationCapacity ratio is reduced.
+        // - The indexer stake is slashed.
+        // - A delegator removes enough stake.
+        if (_tokensUsed > tokensCapacity) {
+            // Indexer stake is over allocated: return 0 to avoid stake to be used until
+            // the overallocation is restored by staking more tokens, unallocating tokens
+            // or using more delegated funds
             return 0;
         }
-        return stake.tokensStaked.sub(_tokensUsed);
+        return tokensCapacity.sub(_tokensUsed);
     }
 
     /**

graph.config.yml

@@ -53,9 +53,6 @@ contracts:
       minimumDeposit: "100000000000000000000" # 100 GRT (in wei)
       fishermanRewardPercentage: 100000 # 10% (in basis points)
       slashingPercentage: 100000 # 10% (in basis points)
-    calls:
-      - fn: "setMinimumIndexerStake"
-        minimumIndexerStake: "1000000000000000000" # 1 GRT (in wei)
   GNS:
     init:
       controller: "${{Controller.address}}"
@@ -86,6 +83,8 @@ contracts:
       - fn: "setSlasher"
         slasher: "${{DisputeManager.address}}"
         allowed: true
+      - fn: "setMinimumIndexerStake"
+        minimumIndexerStake: "10000000000000000000" # 10 GRT (in wei)
   RewardsManager:
     init:
       controller: "${{Controller.address}}"

test/disputes/configuration.test.ts

@@ -4,7 +4,7 @@ import { DisputeManager } from '../../build/typechain/contracts/DisputeManager'
 
 import { defaults } from '../lib/deployment'
 import { NetworkFixture } from '../lib/fixtures'
-import { getAccounts, toBN, toGRT, Account } from '../lib/testHelpers'
+import { getAccounts, toBN, Account } from '../lib/testHelpers'
 
 const MAX_PPM = 1000000
 
@@ -70,26 +70,6 @@ describe('DisputeManager:Config', () => {
       })
     })
 
-    describe('minimumIndexerStake', function () {
-      it('should set `minimumIndexerStake`', async function () {
-        const oldValue = defaults.dispute.minimumIndexerStake
-        const newValue = toGRT('100')
-
-        // Set right in the constructor
-        expect(await disputeManager.minimumIndexerStake()).eq(oldValue)
-
-        // Set new value
-        await disputeManager.connect(governor.signer).setMinimumIndexerStake(newValue)
-        expect(await disputeManager.minimumIndexerStake()).eq(newValue)
-      })
-
-      it('reject set `minimumIndexerStake` if not allowed', async function () {
-        const newValue = toGRT('100')
-        const tx = disputeManager.connect(me.signer).setMinimumIndexerStake(newValue)
-        await expect(tx).revertedWith('Caller must be Controller governor')
-      })
-    })
-
     describe('fishermanRewardPercentage', function () {
       it('should set `fishermanRewardPercentage`', async function () {
         const newValue = defaults.dispute.fishermanRewardPercentage

test/disputes/poi.test.ts

@@ -140,7 +140,7 @@ describe('DisputeManager:POI', async () => {
       const tx = disputeManager
         .connect(fisherman.signer)
         .createIndexingDispute(event1.allocationID, fishermanDeposit)
-      await expect(tx).revertedWith('Dispute under minimum indexer stake amount')
+      await expect(tx).revertedWith('Dispute indexer has no stake')
     })
 
     context('> when indexer is staked', function () {

test/disputes/query.test.ts

@@ -233,7 +233,7 @@ describe('DisputeManager:Query', async () => {
       const tx = disputeManager
         .connect(fisherman.signer)
         .createQueryDispute(dispute.encodedAttestation, fishermanDeposit)
-      await expect(tx).revertedWith('Dispute under minimum indexer stake amount')
+      await expect(tx).revertedWith('Dispute indexer has no stake')
     })
 
     context('> when indexer is staked', function () {

test/lib/deployment.ts

@@ -29,14 +29,14 @@ export const defaults = {
   },
   dispute: {
     minimumDeposit: toGRT('100'),
-    minimumIndexerStake: toGRT('1'),
     fishermanRewardPercentage: toBN('1000'), // in basis points
     slashingPercentage: toBN('1000'), // in basis points
   },
   epochs: {
     lengthInBlocks: toBN((15 * 60) / 15), // 15 minutes in blocks
   },
   staking: {
+    minimumIndexerStake: toGRT('10'),
     channelDisputeEpochs: 1,
     maxAllocationEpochs: 5,
     thawingPeriod: 20, // in blocks
@@ -128,20 +128,15 @@ export async function deployDisputeManager(
   arbitrator: string,
 ): Promise<DisputeManager> {
   // Deploy
-  const contract = ((await deployContract(
+  return deployContract(
     'DisputeManager',
     deployer,
     controller,
     arbitrator,
     defaults.dispute.minimumDeposit.toString(),
     defaults.dispute.fishermanRewardPercentage.toString(),
     defaults.dispute.slashingPercentage.toString(),
-  )) as unknown) as DisputeManager
-
-  // Config
-  await contract.connect(deployer).setMinimumIndexerStake(defaults.dispute.minimumIndexerStake)
-
-  return contract
+  ) as Promise<DisputeManager>
 }
 
 export async function deployEpochManager(
@@ -212,6 +207,7 @@ export async function deployStaking(deployer: Signer, controller: string): Promi
 
   // Configure
   const staking = contract.attach(proxy.address)
+  await staking.connect(deployer).setMinimumIndexerStake(defaults.staking.minimumIndexerStake)
   await staking.connect(deployer).setChannelDisputeEpochs(defaults.staking.channelDisputeEpochs)
   await staking.connect(deployer).setMaxAllocationEpochs(defaults.staking.maxAllocationEpochs)
   await staking.connect(deployer).setThawingPeriod(defaults.staking.thawingPeriod)