fix: audit fixes for post horizon transition period clean up

packages/contracts/contracts/l2/curation/L2Curation.sol

@@ -171,11 +171,8 @@ contract L2Curation is CurationV3Storage, GraphUpgradeable, IL2Curation {
      * @param _tokens Amount of Graph Tokens to add to reserves
      */
     function collect(bytes32 _subgraphDeploymentID, uint256 _tokens) external override {
-        // Only SubgraphService and Staking contract are authorized as callers
-        require(
-            msg.sender == subgraphService || msg.sender == address(staking()),
-            "Caller must be the subgraph service or staking contract"
-        );
+        // Only SubgraphService is authorized as caller
+        require(msg.sender == subgraphService, "Caller must be the subgraph service");
 
         // Must be curated to accept tokens
         require(isCurated(_subgraphDeploymentID), "Subgraph deployment must be curated to collect fees");

packages/contracts/test/tests/unit/l2/l2Curation.test.ts

@@ -154,7 +154,7 @@ describe('L2Curation', () => {
   let me: SignerWithAddress
   let governor: SignerWithAddress
   let curator: SignerWithAddress
-  let stakingMock: SignerWithAddress
+  let subgraphServiceMock: SignerWithAddress
   let gnsImpersonator: Signer
 
   let fixture: NetworkFixture
@@ -310,8 +310,8 @@ describe('L2Curation', () => {
     const beforeTotalBalance = await grt.balanceOf(curation.address)
 
     // Source of tokens must be the staking for this to work
-    await grt.connect(stakingMock).transfer(curation.address, tokensToCollect)
-    const tx = curation.connect(stakingMock).collect(subgraphDeploymentID, tokensToCollect)
+    await grt.connect(subgraphServiceMock).transfer(curation.address, tokensToCollect)
+    const tx = curation.connect(subgraphServiceMock).collect(subgraphDeploymentID, tokensToCollect)
     await expect(tx).emit(curation, 'Collected').withArgs(subgraphDeploymentID, tokensToCollect)
 
     // After state
@@ -325,7 +325,7 @@ describe('L2Curation', () => {
 
   before(async function () {
     // Use stakingMock so we can call collect
-    ;[me, curator, stakingMock] = await graph.getTestAccounts()
+    ;[me, curator, subgraphServiceMock] = await graph.getTestAccounts()
     ;({ governor } = await graph.getNamedAccounts())
     fixture = new NetworkFixture(graph.provider)
     contracts = await fixture.load(governor, true)
@@ -343,8 +343,11 @@ describe('L2Curation', () => {
     await grt.connect(gnsImpersonator).approve(curation.address, curatorTokens)
 
     // Give some funds to the staking contract and approve the curation contract
-    await grt.connect(governor).mint(stakingMock.address, tokensToCollect)
-    await grt.connect(stakingMock).approve(curation.address, tokensToCollect)
+    await grt.connect(governor).mint(subgraphServiceMock.address, tokensToCollect)
+    await grt.connect(subgraphServiceMock).approve(curation.address, tokensToCollect)
+
+    // Set the subgraph service
+    await curation.connect(governor).setSubgraphService(subgraphServiceMock.address)
   })
 
   beforeEach(async function () {
@@ -514,10 +517,10 @@ describe('L2Curation', () => {
     context('> not curated', function () {
       it('reject collect tokens distributed to the curation pool', async function () {
         // Source of tokens must be the staking for this to work
-        await controller.connect(governor).setContractProxy(utils.id('Staking'), stakingMock.address)
+        await controller.connect(governor).setContractProxy(utils.id('Staking'), subgraphServiceMock.address)
         await curation.connect(governor).syncAllContracts() // call sync because we change the proxy for staking
 
-        const tx = curation.connect(stakingMock).collect(subgraphDeploymentID, tokensToCollect)
+        const tx = curation.connect(subgraphServiceMock).collect(subgraphDeploymentID, tokensToCollect)
         await expect(tx).revertedWith('Subgraph deployment must be curated to collect fees')
       })
     })
@@ -529,11 +532,11 @@ describe('L2Curation', () => {
 
       it('reject collect tokens distributed from invalid address', async function () {
         const tx = curation.connect(me).collect(subgraphDeploymentID, tokensToCollect)
-        await expect(tx).revertedWith('Caller must be the subgraph service or staking contract')
+        await expect(tx).revertedWith('Caller must be the subgraph service')
       })
 
       it('should collect tokens distributed to the curation pool', async function () {
-        await controller.connect(governor).setContractProxy(utils.id('Staking'), stakingMock.address)
+        await controller.connect(governor).setContractProxy(utils.id('Staking'), subgraphServiceMock.address)
         await curation.connect(governor).syncAllContracts() // call sync because we change the proxy for staking
 
         await shouldCollect(toGRT('1'))
@@ -544,7 +547,7 @@ describe('L2Curation', () => {
       })
 
       it('should collect tokens and then unsignal all', async function () {
-        await controller.connect(governor).setContractProxy(utils.id('Staking'), stakingMock.address)
+        await controller.connect(governor).setContractProxy(utils.id('Staking'), subgraphServiceMock.address)
         await curation.connect(governor).syncAllContracts() // call sync because we change the proxy for staking
 
         // Collect increase the pool reserves
@@ -556,7 +559,7 @@ describe('L2Curation', () => {
       })
 
       it('should collect tokens and then unsignal multiple times', async function () {
-        await controller.connect(governor).setContractProxy(utils.id('Staking'), stakingMock.address)
+        await controller.connect(governor).setContractProxy(utils.id('Staking'), subgraphServiceMock.address)
         await curation.connect(governor).syncAllContracts() // call sync because we change the proxy for staking
 
         // Collect increase the pool reserves

packages/contracts/test/tests/unit/l2/l2GNS.test.ts

@@ -2,12 +2,10 @@ import { L2GNS } from '@graphprotocol/contracts'
 import { L2GraphTokenGateway } from '@graphprotocol/contracts'
 import { L2Curation } from '@graphprotocol/contracts'
 import { GraphToken } from '@graphprotocol/contracts'
-import { IL2Staking } from '@graphprotocol/contracts'
 import { L1GNS, L1GraphTokenGateway } from '@graphprotocol/contracts'
 import {
   buildSubgraph,
   buildSubgraphId,
-  deriveChannelKey,
   GraphNetworkContracts,
   helpers,
   PublishSubgraph,
@@ -44,7 +42,6 @@ interface L1SubgraphParams {
 describe('L2GNS', () => {
   const graph = hre.graph()
   let me: SignerWithAddress
-  let attacker: SignerWithAddress
   let other: SignerWithAddress
   let governor: SignerWithAddress
   let fixture: NetworkFixture
@@ -58,7 +55,6 @@ describe('L2GNS', () => {
   let gns: L2GNS
   let curation: L2Curation
   let grt: GraphToken
-  let staking: IL2Staking
 
   let newSubgraph0: PublishSubgraph
   let newSubgraph1: PublishSubgraph
@@ -109,7 +105,7 @@ describe('L2GNS', () => {
 
   before(async function () {
     newSubgraph0 = buildSubgraph()
-    ;[me, attacker, other] = await graph.getTestAccounts()
+    ;[me, other] = await graph.getTestAccounts()
     ;({ governor } = await graph.getNamedAccounts())
 
     fixture = new NetworkFixture(graph.provider)
@@ -118,7 +114,6 @@ describe('L2GNS', () => {
     fixtureContracts = await fixture.load(governor, true)
     l2GraphTokenGateway = fixtureContracts.L2GraphTokenGateway as L2GraphTokenGateway
     gns = fixtureContracts.L2GNS as L2GNS
-    staking = fixtureContracts.L2Staking as unknown as IL2Staking
     curation = fixtureContracts.L2Curation as L2Curation
     grt = fixtureContracts.GraphToken as GraphToken
 
@@ -354,61 +349,6 @@ describe('L2GNS', () => {
         .emit(gns, 'SignalMinted')
         .withArgs(l2SubgraphId, me.address, expectedNSignal, expectedSignal, curatedTokens)
     })
-    it('protects the owner against a rounding attack', async function () {
-      const { l1SubgraphId, curatedTokens, subgraphMetadata, versionMetadata } = await defaultL1SubgraphParams()
-      const collectTokens = curatedTokens.mul(20)
-
-      await staking.connect(governor).setCurationPercentage(100000)
-
-      // Set up an indexer account with some stake
-      await grt.connect(governor).mint(attacker.address, toGRT('1000000'))
-      // Curate 1 wei GRT by minting 1 GRT and burning most of it
-      await grt.connect(attacker).approve(curation.address, toBN(1))
-      await curation.connect(attacker).mint(newSubgraph0.subgraphDeploymentID, toBN(1), 0)
-
-      // Check this actually gave us 1 wei signal
-      expect(await curation.getCurationPoolTokens(newSubgraph0.subgraphDeploymentID)).eq(1)
-      await grt.connect(attacker).approve(staking.address, toGRT('1000000'))
-      await staking.connect(attacker).stake(toGRT('100000'))
-      const channelKey = deriveChannelKey()
-      // Allocate to the same deployment ID
-      await staking
-        .connect(attacker)
-        .allocateFrom(
-          attacker.address,
-          newSubgraph0.subgraphDeploymentID,
-          toGRT('100000'),
-          channelKey.address,
-          randomHexBytes(32),
-          await channelKey.generateProof(attacker.address),
-        )
-      // Spoof some query fees, 10% of which will go to the Curation pool
-      await staking.connect(attacker).collect(collectTokens, channelKey.address)
-      // The curation pool now has 1 wei shares and a lot of tokens, so the rounding attack is prepared
-      // But L2GNS will protect the owner by sending the tokens
-      const callhookData = defaultAbiCoder.encode(['uint8', 'uint256', 'address'], [toBN(0), l1SubgraphId, me.address])
-      await gatewayFinalizeTransfer(l1GNSMock.address, gns.address, curatedTokens, callhookData)
-
-      const l2SubgraphId = await gns.getAliasedL2SubgraphID(l1SubgraphId)
-      const tx = gns
-        .connect(me)
-        .finishSubgraphTransferFromL1(
-          l2SubgraphId,
-          newSubgraph0.subgraphDeploymentID,
-          subgraphMetadata,
-          versionMetadata,
-        )
-      await expect(tx)
-        .emit(gns, 'SubgraphPublished')
-        .withArgs(l2SubgraphId, newSubgraph0.subgraphDeploymentID, DEFAULT_RESERVE_RATIO)
-      await expect(tx).emit(gns, 'SubgraphMetadataUpdated').withArgs(l2SubgraphId, subgraphMetadata)
-      await expect(tx).emit(gns, 'CuratorBalanceReturnedToBeneficiary')
-      await expect(tx).emit(gns, 'SubgraphUpgraded').withArgs(l2SubgraphId, 0, 0, newSubgraph0.subgraphDeploymentID)
-      await expect(tx)
-        .emit(gns, 'SubgraphVersionUpdated')
-        .withArgs(l2SubgraphId, newSubgraph0.subgraphDeploymentID, versionMetadata)
-      await expect(tx).emit(gns, 'SubgraphL2TransferFinalized').withArgs(l2SubgraphId)
-    })
     it('cannot be called by someone other than the subgraph owner', async function () {
       const { l1SubgraphId, curatedTokens, subgraphMetadata, versionMetadata } = await defaultL1SubgraphParams()
       const callhookData = defaultAbiCoder.encode(['uint8', 'uint256', 'address'], [toBN(0), l1SubgraphId, me.address])
@@ -654,50 +594,6 @@ describe('L2GNS', () => {
       expect(gnsBalanceAfter).eq(gnsBalanceBefore)
     })
 
-    it('protects the curator against a rounding attack', async function () {
-      // Transfer a subgraph from L1 with only 1 wei GRT of curated signal
-      const { l1SubgraphId, subgraphMetadata, versionMetadata } = await defaultL1SubgraphParams()
-      const curatedTokens = toBN('1')
-      await transferMockSubgraphFromL1(l1SubgraphId, curatedTokens, subgraphMetadata, versionMetadata)
-      // Prepare the rounding attack by setting up an indexer and collecting a lot of query fees
-      const curatorTokens = toGRT('10000')
-      const collectTokens = curatorTokens.mul(20)
-      await staking.connect(governor).setCurationPercentage(100000)
-      // Set up an indexer account with some stake
-      await grt.connect(governor).mint(attacker.address, toGRT('1000000'))
-
-      await grt.connect(attacker).approve(staking.address, toGRT('1000000'))
-      await staking.connect(attacker).stake(toGRT('100000'))
-      const channelKey = deriveChannelKey()
-      // Allocate to the same deployment ID
-      await staking
-        .connect(attacker)
-        .allocateFrom(
-          attacker.address,
-          newSubgraph0.subgraphDeploymentID,
-          toGRT('100000'),
-          channelKey.address,
-          randomHexBytes(32),
-          await channelKey.generateProof(attacker.address),
-        )
-      // Spoof some query fees, 10% of which will go to the Curation pool
-      await staking.connect(attacker).collect(collectTokens, channelKey.address)
-
-      const callhookData = defaultAbiCoder.encode(['uint8', 'uint256', 'address'], [toBN(1), l1SubgraphId, me.address])
-      const curatorTokensBefore = await grt.balanceOf(me.address)
-      const gnsBalanceBefore = await grt.balanceOf(gns.address)
-      const tx = gatewayFinalizeTransfer(l1GNSMock.address, gns.address, curatorTokens, callhookData)
-      await expect(tx)
-        .emit(gns, 'CuratorBalanceReturnedToBeneficiary')
-        .withArgs(l1SubgraphId, me.address, curatorTokens)
-      const curatorTokensAfter = await grt.balanceOf(me.address)
-      expect(curatorTokensAfter).eq(curatorTokensBefore.add(curatorTokens))
-      const gnsBalanceAfter = await grt.balanceOf(gns.address)
-      // gatewayFinalizeTransfer will mint the tokens that are sent to the curator,
-      // so the GNS balance should be the same
-      expect(gnsBalanceAfter).eq(gnsBalanceBefore)
-    })
-
     it('if a subgraph was deprecated after transfer, it returns the tokens to the beneficiary', async function () {
       const l1GNSMockL2Alias = await helpers.getL2SignerFromL1(l1GNSMock.address)
       // Eth for gas:

packages/horizon/contracts/staking/HorizonStaking.sol

@@ -216,6 +216,11 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
         require(prov.createdAt != 0, HorizonStakingInvalidProvision(serviceProvider, verifier));
 
         if ((prov.maxVerifierCutPending != prov.maxVerifierCut) || (prov.thawingPeriodPending != prov.thawingPeriod)) {
+            // Re-validate thawing period in case governor reduced _maxThawingPeriod after staging
+            require(
+                prov.thawingPeriodPending <= _maxThawingPeriod,
+                HorizonStakingInvalidThawingPeriod(prov.thawingPeriodPending, _maxThawingPeriod)
+            );
             prov.maxVerifierCut = prov.maxVerifierCutPending;
             prov.thawingPeriod = prov.thawingPeriodPending;
             emit ProvisionParametersSet(serviceProvider, verifier, prov.maxVerifierCut, prov.thawingPeriod);
@@ -837,8 +842,8 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
      * @dev The parameter `nThawRequests` can be set to a non zero value to fulfill a specific number of thaw
      * requests in the event that fulfilling all of them results in a gas limit error. Otherwise, the function
      * will attempt to fulfill all thaw requests until the first one that is not yet expired is found.
-     * @dev If the delegation pool was completely slashed before withdrawing, calling this function will fulfill
-     * the thaw requests with an amount equal to zero.
+     * @dev If the delegation pool was completely slashed before withdrawing, calling this function will revert
+     * until the pool state is repaired with {IHorizonStakingMain-addToDelegationPool}.
      * @param _serviceProvider The service provider address
      * @param _verifier The verifier address
      * @param _newServiceProvider The new service provider address
@@ -1123,10 +1128,7 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
 
         // Validation
         uint256 tokensToWithdraw = 0;
-        uint256 currentEpoch = _graphEpochManager().currentEpoch();
-        if (
-            delegation.__DEPRECATED_tokensLockedUntil > 0 && currentEpoch >= delegation.__DEPRECATED_tokensLockedUntil
-        ) {
+        if (delegation.__DEPRECATED_tokensLockedUntil > 0) {
             tokensToWithdraw = delegation.__DEPRECATED_tokensLocked;
         }
         require(tokensToWithdraw > 0, HorizonStakingNothingToWithdraw());

packages/horizon/contracts/staking/HorizonStakingBase.sol

@@ -182,14 +182,26 @@ abstract contract HorizonStakingBase is
         }
 
         uint256 thawedTokens = 0;
-        Provision storage prov = _provisions[serviceProvider][verifier];
-        uint256 tokensThawing = prov.tokensThawing;
-        uint256 sharesThawing = prov.sharesThawing;
+        uint256 tokensThawing;
+        uint256 sharesThawing;
+        uint256 thawingNonce;
+
+        if (requestType == ThawRequestType.Provision) {
+            Provision storage prov = _provisions[serviceProvider][verifier];
+            tokensThawing = prov.tokensThawing;
+            sharesThawing = prov.sharesThawing;
+            thawingNonce = prov.thawingNonce;
+        } else {
+            DelegationPoolInternal storage pool = _getDelegationPool(serviceProvider, verifier);
+            tokensThawing = pool.tokensThawing;
+            sharesThawing = pool.sharesThawing;
+            thawingNonce = pool.thawingNonce;
+        }
 
         bytes32 thawRequestId = thawRequestList.head;
         while (thawRequestId != bytes32(0)) {
             ThawRequest storage thawRequest = _getThawRequest(requestType, thawRequestId);
-            if (thawRequest.thawingNonce == prov.thawingNonce) {
+            if (thawRequest.thawingNonce == thawingNonce) {
                 if (thawRequest.thawingUntil <= block.timestamp) {
                     // sharesThawing cannot be zero if there is a valid thaw request so the next division is safe
                     uint256 tokens = (thawRequest.shares * tokensThawing) / sharesThawing;

packages/horizon/test/unit/staking/delegation/forceWithdrawDelegated.t.sol

@@ -113,22 +113,4 @@ contract HorizonStakingForceWithdrawDelegatedTest is HorizonStakingTest {
         vm.expectRevert(expectedError);
         staking.forceWithdrawDelegated(users.indexer, users.delegator);
     }
-
-    function testForceWithdrawDelegated_RevertWhen_StillLocked(uint256 tokensLocked) public useDelegator {
-        vm.assume(tokensLocked > 0);
-
-        // Set a future epoch for tokensLockedUntil
-        uint256 futureEpoch = 1000;
-        _setStorage_DelegationPool(users.indexer, tokensLocked, 0, 0);
-        _setLegacyDelegation(users.indexer, users.delegator, 0, tokensLocked, futureEpoch);
-        token.transfer(address(staking), tokensLocked);
-
-        // switch to a third party
-        resetPrank(users.operator);
-
-        // Should revert because tokens are still locked (current epoch < futureEpoch)
-        bytes memory expectedError = abi.encodeWithSignature("HorizonStakingNothingToWithdraw()");
-        vm.expectRevert(expectedError);
-        staking.forceWithdrawDelegated(users.indexer, users.delegator);
-    }
 }