fix: enforce minimum delegation in all cases, including undelegation

Author: pcarranzav

contracts/l2/staking/L2Staking.sol

@@ -19,7 +19,7 @@ contract L2Staking is Staking, IL2StakingBase {
     using SafeMath for uint256;
     using Stakes for Stakes.Indexer;
 
-    /// @dev Minimum delegation for the first delegator of an indexer
+    /// @dev Minimum amount of tokens that can be delegated
     uint256 private constant MINIMUM_DELEGATION = 1e18;
 
     /**
@@ -125,17 +125,12 @@ contract L2Staking is Staking, IL2StakingBase {
         Delegation storage delegation = pool.delegators[_delegationData.delegator];
 
         // Calculate shares to issue (without applying any delegation tax)
-        uint256 shares;
-        if (pool.tokens == 0) {
-            if (_amount >= MINIMUM_DELEGATION) {
-                shares = _amount;
-            }
-        } else {
-            shares = _amount.mul(pool.shares).div(pool.tokens);
-        }
+        uint256 shares = (pool.tokens == 0) ? _amount : _amount.mul(pool.shares).div(pool.tokens);
 
-        if (shares == 0) {
-            // If no shares would be issued (probably a rounding issue or attack), return the tokens to the delegator
+        if (shares == 0 || _amount < MINIMUM_DELEGATION) {
+            // If no shares would be issued (probably a rounding issue or attack),
+            // or if the amount is under the minimum delegation (which could be part of a rounding attack),
+            // return the tokens to the delegator
             graphToken().transfer(_delegationData.delegator, _amount);
             emit TransferredDelegationReturnedToDelegator(
                 _delegationData.indexer,

contracts/staking/StakingExtension.sol

@@ -26,7 +26,7 @@ contract StakingExtension is StakingV4Storage, GraphUpgradeable, IStakingExtensi
 
     /// @dev 100% in parts per million
     uint32 private constant MAX_PPM = 1000000;
-    /// @dev Minimum delegation for the first delegator of an indexer
+    /// @dev Minimum amount of tokens that can be delegated
     uint256 private constant MINIMUM_DELEGATION = 1e18;
 
     /**
@@ -530,8 +530,8 @@ contract StakingExtension is StakingV4Storage, GraphUpgradeable, IStakingExtensi
         address _indexer,
         uint256 _tokens
     ) private returns (uint256) {
-        // Only delegate a non-zero amount of tokens
-        require(_tokens > 0, "!tokens");
+        // Only allow delegations over a minimum, to prevent rounding attacks
+        require(_tokens >= MINIMUM_DELEGATION, "!minimum-delegation");
         // Only delegate to non-empty address
         require(_indexer != address(0), "!indexer");
         // Only delegate to staked indexer
@@ -546,13 +546,9 @@ contract StakingExtension is StakingV4Storage, GraphUpgradeable, IStakingExtensi
         uint256 delegatedTokens = _tokens.sub(delegationTax);
 
         // Calculate shares to issue
-        uint256 shares;
-        if (pool.tokens == 0) {
-            require(_tokens >= MINIMUM_DELEGATION, "!minimum-delegation");
-            shares = delegatedTokens;
-        } else {
-            shares = delegatedTokens.mul(pool.shares).div(pool.tokens);
-        }
+        uint256 shares = (pool.tokens == 0)
+            ? delegatedTokens
+            : delegatedTokens.mul(pool.shares).div(pool.tokens);
         require(shares > 0, "!shares");
 
         // Update the delegation pool
@@ -603,6 +599,13 @@ contract StakingExtension is StakingV4Storage, GraphUpgradeable, IStakingExtensi
 
         // Update the delegation
         delegation.shares = delegation.shares.sub(_shares);
+        // Enforce more than the minimum delegation is left,
+        // to prevent rounding attacks
+        require(
+            delegation.shares == 0 ||
+                delegation.shares.mul(pool.tokens).div(pool.shares) >= MINIMUM_DELEGATION,
+            "!minimum-delegation"
+        );
         delegation.tokensLocked = delegation.tokensLocked.add(tokens);
         delegation.tokensLockedUntil = epochManager().currentEpoch().add(
             __delegationUnbondingPeriod

test/l2/l2Staking.test.ts

@@ -337,7 +337,7 @@ describe('L2Staking', () => {
       const delegatorGRTBalanceAfter = await grt.balanceOf(other.address)
       expect(delegatorGRTBalanceAfter.sub(delegatorGRTBalanceBefore)).to.equal(toBN(1))
     })
-    it('returns delegation to the delegator if it initialize the pool with less than the minimum delegation', async function () {
+    it('returns delegation to the delegator if it initializes the pool with less than the minimum delegation', async function () {
       await fixtureContracts.rewardsManager
         .connect(governor.signer)
         .setIssuancePerBlock(toGRT('114'))
@@ -383,7 +383,7 @@ describe('L2Staking', () => {
       const delegatorGRTBalanceAfter = await grt.balanceOf(other.address)
       expect(delegatorGRTBalanceAfter.sub(delegatorGRTBalanceBefore)).to.equal(toGRT('0.1'))
     })
-    it('adds delegation under the minimum if the pool is initialized', async function () {
+    it('returns delegation under the minimum if the pool is initialized', async function () {
       await staking.connect(me.signer).stake(tokens100k)
 
       // Initialize the delegation pool to allow delegating less than 1 GRT
@@ -398,6 +398,7 @@ describe('L2Staking', () => {
         ['uint8', 'bytes'],
         [toBN(1), functionData], // code = 1 means RECEIVE_DELEGATION_CODE
       )
+      const delegatorGRTBalanceBefore = await grt.balanceOf(other.address)
       const tx = gatewayFinalizeTransfer(
         mockL1Staking.address,
         staking.address,
@@ -408,12 +409,15 @@ describe('L2Staking', () => {
       await expect(tx)
         .emit(l2GraphTokenGateway, 'DepositFinalized')
         .withArgs(mockL1GRT.address, mockL1Staking.address, staking.address, toGRT('0.1'))
-      const expectedShares = toGRT('0.1')
-      await expect(tx)
-        .emit(staking, 'StakeDelegated')
-        .withArgs(me.address, other.address, toGRT('0.1'), expectedShares)
+
       const delegation = await staking.getDelegation(me.address, other.address)
-      expect(delegation.shares).to.equal(expectedShares)
+      await expect(tx)
+        .emit(staking, 'TransferredDelegationReturnedToDelegator')
+        .withArgs(me.address, other.address, toGRT('0.1'))
+
+      expect(delegation.shares).to.equal(0)
+      const delegatorGRTBalanceAfter = await grt.balanceOf(other.address)
+      expect(delegatorGRTBalanceAfter.sub(delegatorGRTBalanceBefore)).to.equal(toGRT('0.1'))
     })
   })
   describe('onTokenTransfer with invalid messages', function () {

test/staking/delegation.test.ts

@@ -20,6 +20,7 @@ import {
 
 const { AddressZero, HashZero } = constants
 const MAX_PPM = toBN('1000000')
+const tokensToCollect = toGRT('50000000000000000000')
 
 describe('Staking::Delegation', () => {
   let me: Account
@@ -190,10 +191,12 @@ describe('Staking::Delegation', () => {
     }
 
     // Distribute test funds
-    for (const wallet of [me, indexer, indexer2, assetHolder]) {
+    for (const wallet of [me, indexer, indexer2]) {
       await grt.connect(governor.signer).mint(wallet.address, toGRT('1000000'))
       await grt.connect(wallet.signer).approve(staking.address, toGRT('1000000'))
     }
+    await grt.connect(governor.signer).mint(assetHolder.address, tokensToCollect)
+    await grt.connect(assetHolder.signer).approve(staking.address, tokensToCollect)
   })
 
   beforeEach(async function () {
@@ -369,7 +372,7 @@ describe('Staking::Delegation', () => {
       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('!tokens')
+        await expect(tx).revertedWith('!minimum-delegation')
       })
 
       it('reject delegate with less than 1 GRT when the pool is not initialized', async function () {
@@ -378,9 +381,11 @@ describe('Staking::Delegation', () => {
         await expect(tx).revertedWith('!minimum-delegation')
       })
 
-      it('allows delegating under 1 GRT when the pool is initialized', async function () {
+      it('reject delegating under 1 GRT when the pool is initialized', async function () {
         await shouldDelegate(delegator, toGRT('1'))
-        await shouldDelegate(delegator, toGRT('0.01'))
+        const tokensToDelegate = toGRT('0.5')
+        const tx = staking.connect(delegator.signer).delegate(indexer.address, tokensToDelegate)
+        await expect(tx).revertedWith('!minimum-delegation')
       })
 
       it('reject delegate to empty address', async function () {
@@ -479,6 +484,12 @@ describe('Staking::Delegation', () => {
         await advanceToNextEpoch(epochManager) // epoch 2
         await shouldUndelegate(delegator, toGRT('10'))
       })
+
+      it('reject undelegate if remaining tokens are less than the minimum', async function () {
+        await shouldDelegate(delegator, toGRT('100'))
+        const tx = staking.connect(delegator.signer).undelegate(indexer.address, toGRT('99.5'))
+        await expect(tx).revertedWith('!minimum-delegation')
+      })
     })
 
     describe('withdraw', function () {
@@ -534,7 +545,6 @@ describe('Staking::Delegation', () => {
     // Test values
     const tokensToStake = toGRT('200')
     const tokensToAllocate = toGRT('2000')
-    const tokensToCollect = toGRT('500')
     const tokensToDelegate = toGRT('1800')
     const subgraphDeploymentID = randomHexBytes()
     const channelKey = deriveChannelKey()
@@ -604,20 +614,24 @@ describe('Staking::Delegation', () => {
 
     it('revert if it cannot assign the smallest amount of shares', async function () {
       // Init the delegation pool
-      await shouldDelegate(delegator, tokensToDelegate)
-
+      await shouldDelegate(delegator, toGRT('1'))
+      const tokensToAllocate = toGRT('1')
       // Collect funds thru full allocation cycle
+      // Set rebate alpha to 0 to ensure all fees are collected
+      await staking.connect(governor.signer).setRebateParameters(0, 1, 1, 1)
       await staking.connect(governor.signer).setDelegationRatio(10)
       await staking.connect(indexer.signer).setDelegationParameters(0, 0, 0)
       await setupAllocation(tokensToAllocate)
       await advanceToNextEpoch(epochManager)
       await staking.connect(assetHolder.signer).collect(tokensToCollect, allocationID)
       await advanceToNextEpoch(epochManager)
       await staking.connect(indexer.signer).closeAllocation(allocationID, poi)
+      // We've callected 5e18 GRT (a ridiculous amount),
+      // which means the price of a share is now 5 GRT
 
-      // Delegate with such small amount of tokens (1 wei) that we do not have enough precision
-      // to even assign 1 wei of shares
-      const tx = staking.connect(delegator.signer).delegate(indexer.address, toBN(1))
+      // Delegate with such small amount of tokens (1 GRT) that we do not have enough precision
+      // to even assign 1 share
+      const tx = staking.connect(delegator.signer).delegate(indexer.address, toGRT('1'))
       await expect(tx).revertedWith('!shares')
     })
   })