feat: remove asset holder allowlist, with additional fixes

config/graph.arbitrum-goerli.yml

@@ -123,9 +123,6 @@ contracts:
       - fn: "setSlasher"
         slasher: "${{DisputeManager.address}}"
         allowed: true
-      - fn: "setAssetHolder"
-        assetHolder: "${{AllocationExchange.address}}"
-        allowed: true
       - fn: "syncAllContracts"
   RewardsManager:
     proxy: true

config/graph.arbitrum-localhost.yml

@@ -123,9 +123,6 @@ contracts:
       - fn: "setSlasher"
         slasher: "${{DisputeManager.address}}"
         allowed: true
-      - fn: "setAssetHolder"
-        assetHolder: "${{AllocationExchange.address}}"
-        allowed: true
       - fn: "syncAllContracts"
   RewardsManager:
     proxy: true

config/graph.arbitrum-one.yml

@@ -123,9 +123,6 @@ contracts:
       - fn: "setSlasher"
         slasher: "${{DisputeManager.address}}"
         allowed: true
-      - fn: "setAssetHolder"
-        assetHolder: "${{AllocationExchange.address}}"
-        allowed: true
       - fn: "syncAllContracts"
   RewardsManager:
     proxy: true

config/graph.goerli.yml

@@ -126,9 +126,6 @@ contracts:
       - fn: "setSlasher"
         slasher: "${{DisputeManager.address}}"
         allowed: true
-      - fn: "setAssetHolder"
-        assetHolder: "${{AllocationExchange.address}}"
-        allowed: true
       - fn: "syncAllContracts"
   RewardsManager:
     proxy: true

config/graph.localhost.yml

@@ -131,9 +131,6 @@ contracts:
       - fn: "setSlasher"
         slasher: "${{DisputeManager.address}}"
         allowed: true
-      - fn: "setAssetHolder"
-        assetHolder: "${{AllocationExchange.address}}"
-        allowed: true
       - fn: "syncAllContracts"
   RewardsManager:
     proxy: true

config/graph.mainnet.yml

@@ -126,9 +126,6 @@ contracts:
       - fn: "setSlasher"
         slasher: "${{DisputeManager.address}}"
         allowed: true
-      - fn: "setAssetHolder"
-        assetHolder: "${{AllocationExchange.address}}"
-        allowed: true
       - fn: "syncAllContracts"
   RewardsManager:
     proxy: true

contracts/l2/staking/L2Staking.sol

@@ -19,6 +19,9 @@ contract L2Staking is Staking, IL2StakingBase {
     using SafeMath for uint256;
     using Stakes for Stakes.Indexer;
 
+    /// @dev Minimum amount of tokens that can be delegated
+    uint256 private constant MINIMUM_DELEGATION = 1e18;
+
     /**
      * @dev Emitted when `delegator` delegated `tokens` to the `indexer`, the delegator
      * gets `shares` for the delegation pool proportionally to the tokens staked.
@@ -124,8 +127,10 @@ contract L2Staking is Staking, IL2StakingBase {
         // Calculate shares to issue (without applying any delegation tax)
         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/IStakingBase.sol

@@ -93,12 +93,6 @@ interface IStakingBase is IStakingData {
         uint32 cooldownBlocks
     );
 
-    /**
-     * @dev Emitted when `caller` set `assetHolder` address as `allowed` to send funds
-     * to staking contract.
-     */
-    event AssetHolderUpdate(address indexed caller, address indexed assetHolder, bool allowed);
-
     /**
      * @dev Emitted when `indexer` set `operator` access.
      */
@@ -219,14 +213,6 @@ interface IStakingBase is IStakingData {
         uint32 _lambdaDenominator
     ) external;
 
-    /**
-     * @notice Set an address as allowed asset holder.
-     * @dev This function can only be called by the governor.
-     * @param _assetHolder Address of allowed source for state channel funds
-     * @param _allowed True if asset holder is allowed
-     */
-    function setAssetHolder(address _assetHolder, bool _allowed) external;
-
     /**
      * @notice Authorize or unauthorize an address to be an operator for the caller.
      * @param _operator Address to authorize or unauthorize

contracts/staking/IStakingExtension.sol

@@ -237,15 +237,6 @@ interface IStakingExtension is IStakingData {
      */
     function rewardsDestination(address _indexer) external view returns (address);
 
-    /**
-     * @notice Getter for assetHolders[_maybeAssetHolder]:
-     * returns true if the address is an asset holder, i.e. an entity that can collect
-     * query fees into the Staking contract.
-     * @param _maybeAssetHolder The address that may or may not be an asset holder
-     * @return True if the address is an asset holder
-     */
-    function assetHolders(address _maybeAssetHolder) external view returns (bool);
-
     /**
      * @notice Getter for subgraphAllocations[_subgraphDeploymentId]:
      * returns the amount of tokens allocated to a subgraph deployment.

contracts/staking/Staking.sol

@@ -220,17 +220,6 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M
         );
     }
 
-    /**
-     * @notice Set an address as allowed asset holder.
-     * @param _assetHolder Address of allowed source for state channel funds
-     * @param _allowed True if asset holder is allowed
-     */
-    function setAssetHolder(address _assetHolder, bool _allowed) external override onlyGovernor {
-        require(_assetHolder != address(0), "!assetHolder");
-        __assetHolders[_assetHolder] = _allowed;
-        emit AssetHolderUpdate(msg.sender, _assetHolder, _allowed);
-    }
-
     /**
      * @notice Authorize or unauthorize an address to be an operator for the caller.
      * @param _operator Address to authorize or unauthorize
@@ -354,7 +343,6 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M
 
     /**
      * @dev Collect and rebate query fees from state channels to the indexer
-     * Funds received are only accepted from a valid sender.
      * To avoid reverting on the withdrawal from channel flow this function will accept calls with zero tokens.
      * We use an exponential rebate formula to calculate the amount of tokens to rebate to the indexer.
      * This implementation allows collecting multiple times on the same allocation, keeping track of the
@@ -366,14 +354,22 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M
         // Allocation identifier validation
         require(_allocationID != address(0), "!alloc");
 
-        // The contract caller must be an authorized asset holder
-        require(__assetHolders[msg.sender] == true, "!assetHolder");
-
         // Allocation must exist
         AllocationState allocState = _getAllocationState(_allocationID);
         require(allocState != AllocationState.Null, "!collect");
 
+        // Get allocation
         Allocation storage alloc = __allocations[_allocationID];
+        // The allocation must've been opened at least 1 epoch ago,
+        // to prevent manipulation of the curation or delegation pools
+        require(alloc.createdAtEpoch < epochManager().currentEpoch(), "!epoch");
+
+        // If the query fees are zero, we don't want to revert
+        // but we also don't need to do anything, so just return
+        if (_tokens == 0) {
+            return;
+        }
+
         bytes32 subgraphDeploymentID = alloc.subgraphDeploymentID;
 
         uint256 queryFees = _tokens; // Tokens collected from the channel
@@ -382,9 +378,8 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M
         uint256 queryRebates = 0; // Tokens to distribute to indexer
         uint256 delegationRewards = 0; // Tokens to distribute to delegators
 
-        // Process query fees only if non-zero amount
-        if (queryFees > 0) {
-            // -- Pull tokens from the authorized sender --
+        {
+            // -- Pull tokens from the sender --
             IGraphToken graphToken = graphToken();
             TokenUtils.pullTokens(graphToken, msg.sender, queryFees);
 
@@ -789,7 +784,7 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M
 
         // Creates an allocation
         // Allocation identifiers are not reused
-        // The assetHolder address can send collected funds to the allocation
+        // Anyone can send collected funds to the allocation using collect()
         Allocation memory alloc = Allocation(
             _indexer,
             _subgraphDeploymentID,

contracts/staking/StakingExtension.sol

@@ -26,6 +26,8 @@ contract StakingExtension is StakingV4Storage, GraphUpgradeable, IStakingExtensi
 
     /// @dev 100% in parts per million
     uint32 private constant MAX_PPM = 1000000;
+    /// @dev Minimum amount of tokens that can be delegated
+    uint256 private constant MINIMUM_DELEGATION = 1e18;
 
     /**
      * @dev Check if the caller is the slasher.
@@ -301,17 +303,6 @@ contract StakingExtension is StakingV4Storage, GraphUpgradeable, IStakingExtensi
         return __rewardsDestination[_indexer];
     }
 
-    /**
-     * @notice Getter for assetHolders[_maybeAssetHolder]:
-     * returns true if the address is an asset holder, i.e. an entity that can collect
-     * query fees into the Staking contract.
-     * @param _maybeAssetHolder The address that may or may not be an asset holder
-     * @return True if the address is an asset holder
-     */
-    function assetHolders(address _maybeAssetHolder) external view override returns (bool) {
-        return __assetHolders[_maybeAssetHolder];
-    }
-
     /**
      * @notice Getter for operatorAuth[_indexer][_maybeOperator]:
      * returns true if the operator is authorized to operate on behalf of the indexer.
@@ -539,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
@@ -608,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

contracts/staking/StakingStorage.sol

@@ -89,8 +89,8 @@ contract StakingV1Storage is Managed {
 
     // -- Asset Holders --
 
-    /// @dev Allowed AssetHolders that can collect query fees: assetHolder => is allowed
-    mapping(address => bool) internal __assetHolders;
+    /// @dev DEPRECATED: Allowed AssetHolders: assetHolder => is allowed
+    mapping(address => bool) private __DEPRECATED_assetHolders; // solhint-disable-line var-name-mixedcase
 }
 
 /**

e2e/deployment/config/staking.test.ts

@@ -26,11 +26,6 @@ describe('Staking configuration', () => {
     expect(isSlasher).eq(true)
   })
 
-  it('should allow AllocationExchange to collect query fees', async function () {
-    const allowed = await Staking.assetHolders(AllocationExchange.address)
-    expect(allowed).eq(true)
-  })
-
   it('minimumIndexerStake should match "minimumIndexerStake" in the config file', async function () {
     const value = await Staking.minimumIndexerStake()
     const expected = getItemValue(graphConfig, `contracts/${contractName}/init/minimumIndexerStake`)

test/disputes/poi.test.ts

@@ -105,9 +105,6 @@ describe('DisputeManager:POI', async () => {
     // Give some funds to the fisherman
     await grt.connect(governor.signer).mint(fisherman.address, fishermanTokens)
     await grt.connect(fisherman.signer).approve(disputeManager.address, fishermanTokens)
-
-    // Allow the asset holder
-    await staking.connect(governor.signer).setAssetHolder(assetHolder.address, true)
   })
 
   beforeEach(async function () {

test/disputes/query.test.ts

@@ -153,9 +153,6 @@ describe('DisputeManager:Query', async () => {
       await grt.connect(dst.signer).approve(disputeManager.address, fishermanTokens)
     }
 
-    // Allow the asset holder
-    await staking.connect(governor.signer).setAssetHolder(assetHolder.address, true)
-
     // Create an attestation
     const attestation = await buildAttestation(receipt, indexer1ChannelKey.privKey)
 

test/l2/l2Staking.test.ts

@@ -295,7 +295,8 @@ describe('L2Staking', () => {
         .setIssuancePerBlock(toGRT('114'))
 
       await staking.connect(me.signer).stake(tokens100k)
-      await staking.connect(me.signer).delegate(me.address, toBN(1)) // 1 weiGRT == 1 share
+      // Initialize the delegation pool to allow delegating less than 1 GRT
+      await staking.connect(me.signer).delegate(me.address, tokens10k)
 
       await staking.connect(me.signer).setDelegationParameters(1000, 1000, 1000)
       await grt.connect(me.signer).approve(fixtureContracts.curation.address, tokens10k)
@@ -336,6 +337,88 @@ 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 initializes the pool with less than the minimum delegation', async function () {
+      await fixtureContracts.rewardsManager
+        .connect(governor.signer)
+        .setIssuancePerBlock(toGRT('114'))
+
+      await staking.connect(me.signer).stake(tokens100k)
+
+      await staking.connect(me.signer).setDelegationParameters(1000, 1000, 1000)
+      await grt.connect(me.signer).approve(fixtureContracts.curation.address, tokens10k)
+      await fixtureContracts.curation.connect(me.signer).mint(subgraphDeploymentID, tokens10k, 0)
+
+      await allocate(tokens100k)
+      await advanceToNextEpoch(fixtureContracts.epochManager)
+      await advanceToNextEpoch(fixtureContracts.epochManager)
+      await staking.connect(me.signer).closeAllocation(allocationID, randomHexBytes(32))
+      // Now there are some rewards sent to delegation pool, so 1 weiGRT is less than 1 share
+
+      const functionData = defaultAbiCoder.encode(
+        ['tuple(address,address)'],
+        [[me.address, other.address]],
+      )
+
+      const callhookData = defaultAbiCoder.encode(
+        ['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,
+        toGRT('0.1'), // Less than 1 GRT!
+        callhookData,
+      )
+
+      await expect(tx)
+        .emit(l2GraphTokenGateway, 'DepositFinalized')
+        .withArgs(mockL1GRT.address, mockL1Staking.address, staking.address, toGRT('0.1'))
+      const delegation = await staking.getDelegation(me.address, other.address)
+      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'))
+    })
+    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
+      await staking.connect(me.signer).delegate(me.address, tokens10k)
+
+      const functionData = defaultAbiCoder.encode(
+        ['tuple(address,address)'],
+        [[me.address, other.address]],
+      )
+
+      const callhookData = defaultAbiCoder.encode(
+        ['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,
+        toGRT('0.1'),
+        callhookData,
+      )
+
+      await expect(tx)
+        .emit(l2GraphTokenGateway, 'DepositFinalized')
+        .withArgs(mockL1GRT.address, mockL1Staking.address, staking.address, toGRT('0.1'))
+
+      const delegation = await staking.getDelegation(me.address, other.address)
+      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 () {
     it('reverts if the code is invalid', async function () {