staking:

- Add check to DisputeManager to avoid accepting disputes of indexers with zero tokens to slash
- Add check to Staking to avoid slashing zero tokens
- Add check to Staking to avoid staking zero tokens
- Add check in Rebates calculation to avoid division by zero when no fees in rebate pool
- Improve check in Staking contract `settle()` when comparing allocation channelID
- Allow to claim a zero tokens settlement from the rebate pool
- Fix comment in test helpers config
- Add tests for all of the above

Author: abarmat

contracts/DisputeManager.sol

@@ -203,7 +203,7 @@ contract DisputeManager is Governed {
      * @return Amount of tokens to slash
      */
     function getTokensToSlash(address _indexer) public view returns (uint256) {
-        uint256 tokens = staking.getIndexerStakeTokens(_indexer); // slashable tokens
+        uint256 tokens = staking.getIndexerStakedTokens(_indexer); // slashable tokens
         return slashingPercentage.mul(tokens).div(MAX_PPM);
     }
 
@@ -316,8 +316,10 @@ contract DisputeManager is Governed {
 
         // Have staking contract slash the indexer and reward the fisherman
         // Give the fisherman a reward equal to the fishermanRewardPercentage of slashed amount
-        uint256 tokensToReward = getTokensToReward(dispute.indexer);
         uint256 tokensToSlash = getTokensToSlash(dispute.indexer);
+        uint256 tokensToReward = getTokensToReward(dispute.indexer);
+
+        require(tokensToSlash > 0, "Dispute has zero tokens to slash");
         staking.slash(dispute.indexer, tokensToSlash, tokensToReward, dispute.fisherman);
 
         // Give the fisherman their deposit back

contracts/Staking.sol

@@ -244,7 +244,7 @@ contract Staking is Governed {
      * @param _indexer Address of the indexer
      * @return Amount of tokens staked by the indexer
      */
-    function getIndexerStakeTokens(address _indexer) public view returns (uint256) {
+    function getIndexerStakedTokens(address _indexer) public view returns (uint256) {
         return stakes[_indexer].tokensIndexer;
     }
 
@@ -292,9 +292,10 @@ contract Staking is Governed {
     ) external onlySlasher {
         Stakes.Indexer storage indexerStake = stakes[_indexer];
 
+        require(_tokens > 0, "Slashing: cannot slash zero tokens");
+        require(_tokens >= _reward, "Slashing: reward cannot be higher than slashed amount");
         require(indexerStake.hasTokens(), "Slashing: indexer has no stakes");
         require(_beneficiary != address(0), "Slashing: beneficiary must not be an empty address");
-        require(_tokens >= _reward, "Slashing: reward cannot be higher than slashed amount");
         require(
             _tokens <= indexerStake.tokensSlashable(),
             "Slashing: cannot slash more than staked amount"
@@ -337,11 +338,14 @@ contract Staking is Governed {
     function stake(uint256 _tokens) external {
         address indexer = msg.sender;
 
+        require(_tokens > 0, "Staking: cannot stake zero tokens");
+
         // Transfer tokens to stake from indexer to this contract
         require(
             token.transferFrom(indexer, address(this), _tokens),
             "Staking: Cannot transfer tokens to stake"
         );
+
         // Stake the transferred tokens
         _stake(indexer, _tokens);
     }
@@ -474,20 +478,22 @@ contract Staking is Governed {
 
         // Process rebate
         uint256 tokensToClaim = pool.redeem(indexer, _subgraphID);
-        require(tokensToClaim > 0, "Rebate: no tokens available to claim");
 
-        // All settlements processed then prune rebate pool
+        // When all settlements processed then prune rebate pool
         if (pool.settlementsCount == 0) {
             delete rebates[_epoch];
         }
 
-        // Assign claimed tokens
-        if (_restake) {
-            // Restake to place fees into the indexer stake
-            _stake(indexer, tokensToClaim);
-        } else {
-            // Transfer funds back to the indexer
-            require(token.transfer(indexer, tokensToClaim), "Rebate: cannot transfer tokens");
+        // When there are tokens to claim from the rebate pool, transfer or restake
+        if (tokensToClaim > 0) {
+            // Assign claimed tokens
+            if (_restake) {
+                // Restake to place fees into the indexer stake
+                _stake(indexer, tokensToClaim);
+            } else {
+                // Transfer funds back to the indexer
+                require(token.transfer(indexer, tokensToClaim), "Rebate: cannot transfer tokens");
+            }
         }
 
         emit RebateClaimed(
@@ -527,8 +533,11 @@ contract Staking is Governed {
         bytes32 subgraphID = channels[_channelID].subgraphID;
         Stakes.Allocation storage alloc = stakes[indexer].allocations[subgraphID];
 
-        require(alloc.hasChannel(), "Channel: Must be active for settlement");
-        require(alloc.channelID == _channelID, "Channel: Cannot settle an already closed channel");
+        require(_channelID != address(0), "Channel: ChannelID cannot be empty address");
+        require(
+            alloc.channelID == _channelID,
+            "Channel: The allocation has no channel, or the channel was already settled"
+        );
 
         // Time conditions
         (uint256 epochs, uint256 currentEpoch) = epochManager.epochsSince(alloc.createdAtEpoch);

contracts/libs/Rebates.sol

@@ -97,6 +97,9 @@ library Rebates {
         uint256 _poolFees
     ) public pure returns (uint256) {
         // NOTE: We sqrt() because alpha is fractional so we expect the inverse of alpha
+        if (_poolAlloc == 0 || _poolFees == 0) {
+            return 0;
+        }
 
         // Here we use ABDKMath64x64 to do the square root of terms
         // We have to covert it to a 64.64 fixed point number, do sqrt(), then convert it

test/disputes.test.js

@@ -397,8 +397,18 @@ contract('Disputes', ([me, other, governor, arbitrator, indexer, fisherman, othe
             )
           })
 
+          it('reject to accept a dispute if zero tokens to slash', async function() {
+            await this.disputeManager.setSlashingPercentage(new BN(0), { from: governor })
+            await expectRevert(
+              this.disputeManager.acceptDispute(this.dispute.id, {
+                from: arbitrator,
+              }),
+              'Dispute has zero tokens to slash',
+            )
+          })
+
           it('should resolve dispute, slash indexer and reward the fisherman', async function() {
-            const indexerStakeBefore = await this.staking.getIndexerStakeTokens(indexer)
+            const indexerStakeBefore = await this.staking.getIndexerStakedTokens(indexer)
             const tokensToSlash = await this.disputeManager.getTokensToSlash(indexer)
             const fishermanBalanceBefore = await this.grt.balanceOf(fisherman)
             const totalSupplyBefore = await this.grt.totalSupply()
@@ -416,7 +426,7 @@ contract('Disputes', ([me, other, governor, arbitrator, indexer, fisherman, othe
             )
 
             // Indexer slashed
-            const indexerStakeAfter = await this.staking.getIndexerStakeTokens(indexer)
+            const indexerStakeAfter = await this.staking.getIndexerStakedTokens(indexer)
             expect(indexerStakeAfter).to.be.bignumber.eq(indexerStakeBefore.sub(tokensToSlash))
 
             // Slashed funds burned

test/lib/testHelpers.js

@@ -78,7 +78,7 @@ module.exports = {
       slashingPercentage: new BN(1000), // in basis points
     },
     epochs: {
-      lengthInBlocks: new BN((5 * 60) / 15), // One day in blocks
+      lengthInBlocks: new BN((5 * 60) / 15), // 5 minutes in blocks
     },
     staking: {
       channelDisputeEpochs: 1,