fix: add a rounding error protection when receiving subgraphs or signal (OZ H-01)

pcarranzav · pcarranzav · commit 842445d01485 · 2023-11-22T16:32:03.000-03:00
diff --git a/contracts/l2/curation/IL2Curation.sol b/contracts/l2/curation/IL2Curation.sol
@@ -29,4 +29,17 @@ interface IL2Curation {
         external
         view
         returns (uint256);
+
+    /**
+     * @notice Calculate the amount of tokens that would be recovered if minting signal with
+     * the input tokens and then burning it. This can be used to compute rounding error.
+     * This function does not account for curation tax.
+     * @param _subgraphDeploymentID Subgraph deployment for which to mint signal
+     * @param _tokensIn Amount of tokens used to mint signal
+     * @return Amount of tokens that would be recovered after minting and burning signal
+     */
+    function tokensToSignalToTokensNoTax(bytes32 _subgraphDeploymentID, uint256 _tokensIn)
+        external
+        view
+        returns (uint256);
 }
diff --git a/contracts/l2/curation/L2Curation.sol b/contracts/l2/curation/L2Curation.sol
@@ -421,6 +421,28 @@ contract L2Curation is CurationV2Storage, GraphUpgradeable, IL2Curation {
         return _tokensToSignal(_subgraphDeploymentID, _tokensIn);
     }
 
+    /**
+     * @notice Calculate the amount of tokens that would be recovered if minting signal with
+     * the input tokens and then burning it. This can be used to compute rounding error.
+     * This function does not account for curation tax.
+     * @param _subgraphDeploymentID Subgraph deployment for which to mint signal
+     * @param _tokensIn Amount of tokens used to mint signal
+     * @return Amount of tokens that would be recovered after minting and burning signal
+     */
+    function tokensToSignalToTokensNoTax(bytes32 _subgraphDeploymentID, uint256 _tokensIn)
+        external
+        view
+        override
+        returns (uint256)
+    {
+        require(_tokensIn != 0, "Can't calculate with 0 tokens");
+        uint256 signal = _tokensToSignal(_subgraphDeploymentID, _tokensIn);
+        CurationPool memory curationPool = pools[_subgraphDeploymentID];
+        uint256 poolSignalAfter = getCurationPoolSignal(_subgraphDeploymentID).add(signal);
+        uint256 poolTokensAfter = curationPool.tokens.add(_tokensIn);
+        return poolTokensAfter.mul(signal).div(poolSignalAfter);
+    }
+
     /**
      * @notice Calculate number of tokens to get when burning signal from a curation pool.
      * @param _subgraphDeploymentID Subgraph deployment for which to burn signal
diff --git a/contracts/l2/discovery/IL2GNS.sol b/contracts/l2/discovery/IL2GNS.sol
@@ -46,4 +46,11 @@ interface IL2GNS is ICallhookReceiver {
      * @return L2 subgraph ID
      */
     function getAliasedL2SubgraphID(uint256 _l1SubgraphID) external pure returns (uint256);
+
+    /**
+     * @notice Return the unaliased L1 subgraph ID from a transferred L2 subgraph ID
+     * @param _l2SubgraphID L2 subgraph ID
+     * @return L1subgraph ID
+     */
+    function getUnaliasedL1SubgraphID(uint256 _l2SubgraphID) external pure returns (uint256);
 }
diff --git a/contracts/l2/discovery/L2GNS.sol b/contracts/l2/discovery/L2GNS.sol
@@ -26,9 +26,17 @@ import { IL2Curation } from "../curation/IL2Curation.sol";
 contract L2GNS is GNS, L2GNSV1Storage, IL2GNS {
     using SafeMathUpgradeable for uint256;
 
+    /// Offset added to an L1 subgraph ID to compute the L2 subgraph ID alias
     uint256 public constant SUBGRAPH_ID_ALIAS_OFFSET =
         uint256(0x1111000000000000000000000000000000000000000000000000000000001111);
 
+    /// Maximum rounding error when receiving signal tokens from L1, in parts-per-million.
+    /// If the error from minting signal is above this, tokens will be sent back to the curator.
+    uint256 public constant MAX_ROUNDING_ERROR = 1000;
+
+    /// @dev 100% expressed in parts-per-million
+    uint256 private constant MAX_PPM = 1000000;
+
     /// @dev Emitted when a subgraph is received from L1 through the bridge
     event SubgraphReceivedFromL1(
         uint256 indexed _l1SubgraphID,
@@ -122,10 +130,32 @@ contract L2GNS is GNS, L2GNSV1Storage, IL2GNS {
         require(_subgraphDeploymentID != 0, "GNS: deploymentID != 0");
 
         IL2Curation curation = IL2Curation(address(curation()));
-        // Update pool: constant nSignal, vSignal can change (w/no slippage protection)
-        // Buy all signal from the new deployment
-        uint256 vSignal = curation.mintTaxFree(_subgraphDeploymentID, transferData.tokens);
-        uint256 nSignal = vSignalToNSignal(_l2SubgraphID, vSignal);
+
+        uint256 vSignal;
+        uint256 nSignal;
+        uint256 roundingError;
+        uint256 tokens = transferData.tokens;
+        {
+            uint256 tokensAfter = curation.tokensToSignalToTokensNoTax(
+                _subgraphDeploymentID,
+                tokens
+            );
+            roundingError = tokens.sub(tokensAfter).mul(MAX_PPM).div(tokens);
+        }
+        if (roundingError <= MAX_ROUNDING_ERROR) {
+            vSignal = curation.mintTaxFree(_subgraphDeploymentID, tokens);
+            nSignal = vSignalToNSignal(_l2SubgraphID, vSignal);
+            emit SignalMinted(_l2SubgraphID, msg.sender, nSignal, vSignal, tokens);
+            emit SubgraphUpgraded(_l2SubgraphID, vSignal, tokens, _subgraphDeploymentID);
+        } else {
+            graphToken().transfer(msg.sender, tokens);
+            emit CuratorBalanceReturnedToBeneficiary(
+                getUnaliasedL1SubgraphID(_l2SubgraphID),
+                msg.sender,
+                tokens
+            );
+            emit SubgraphUpgraded(_l2SubgraphID, vSignal, 0, _subgraphDeploymentID);
+        }
 
         subgraphData.disabled = false;
         subgraphData.vSignal = vSignal;
@@ -134,16 +164,8 @@ contract L2GNS is GNS, L2GNSV1Storage, IL2GNS {
         subgraphData.subgraphDeploymentID = _subgraphDeploymentID;
         // Set the token metadata
         _setSubgraphMetadata(_l2SubgraphID, _subgraphMetadata);
-
         emit SubgraphPublished(_l2SubgraphID, _subgraphDeploymentID, fixedReserveRatio);
-        emit SubgraphUpgraded(
-            _l2SubgraphID,
-            subgraphData.vSignal,
-            transferData.tokens,
-            _subgraphDeploymentID
-        );
         emit SubgraphVersionUpdated(_l2SubgraphID, _subgraphDeploymentID, _versionMetadata);
-        emit SignalMinted(_l2SubgraphID, msg.sender, nSignal, vSignal, transferData.tokens);
         emit SubgraphL2TransferFinalized(_l2SubgraphID);
     }
 
@@ -227,6 +249,20 @@ contract L2GNS is GNS, L2GNSV1Storage, IL2GNS {
         return _l1SubgraphID + SUBGRAPH_ID_ALIAS_OFFSET;
     }
 
+    /**
+     * @notice Return the unaliased L1 subgraph ID from a transferred L2 subgraph ID
+     * @param _l2SubgraphID L2 subgraph ID
+     * @return L1subgraph ID
+     */
+    function getUnaliasedL1SubgraphID(uint256 _l2SubgraphID)
+        public
+        pure
+        override
+        returns (uint256)
+    {
+        return _l2SubgraphID - SUBGRAPH_ID_ALIAS_OFFSET;
+    }
+
     /**
      * @dev Receive a subgraph from L1.
      * This function will initialize a subgraph received through the bridge,
@@ -278,13 +314,21 @@ contract L2GNS is GNS, L2GNSV1Storage, IL2GNS {
         IL2GNS.SubgraphL2TransferData storage transferData = subgraphL2TransferData[l2SubgraphID];
         SubgraphData storage subgraphData = _getSubgraphData(l2SubgraphID);
 
+        IL2Curation curation = IL2Curation(address(curation()));
+        uint256 roundingError;
+        if (transferData.l2Done && !subgraphData.disabled) {
+            uint256 tokensAfter = curation.tokensToSignalToTokensNoTax(
+                subgraphData.subgraphDeploymentID,
+                _tokensIn
+            );
+            roundingError = _tokensIn.sub(tokensAfter).mul(MAX_PPM).div(_tokensIn);
+        }
         // If subgraph transfer wasn't finished, we should send the tokens to the curator
-        if (!transferData.l2Done || subgraphData.disabled) {
+        if (!transferData.l2Done || subgraphData.disabled || roundingError > MAX_ROUNDING_ERROR) {
             graphToken().transfer(_curator, _tokensIn);
             emit CuratorBalanceReturnedToBeneficiary(_l1SubgraphID, _curator, _tokensIn);
         } else {
             // Get name signal to mint for tokens deposited
-            IL2Curation curation = IL2Curation(address(curation()));
             uint256 vSignal = curation.mintTaxFree(subgraphData.subgraphDeploymentID, _tokensIn);
             uint256 nSignal = vSignalToNSignal(l2SubgraphID, vSignal);
 
diff --git a/contracts/staking/Staking.sol b/contracts/staking/Staking.sol
@@ -358,18 +358,13 @@ abstract contract Staking is StakingV4Storage, GraphUpgradeable, IStakingBase, M
         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;
         }
 
+        Allocation storage alloc = __allocations[_allocationID];
         bytes32 subgraphDeploymentID = alloc.subgraphDeploymentID;
 
         uint256 queryFees = _tokens; // Tokens collected from the channel
diff --git a/test/l2/l2GNS.test.ts b/test/l2/l2GNS.test.ts
@@ -10,6 +10,7 @@ import {
   getL2SignerFromL1,
   setAccountBalance,
   latestBlock,
+  deriveChannelKey,
 } from '../lib/testHelpers'
 import { L2FixtureContracts, NetworkFixture } from '../lib/fixtures'
 import { toBN } from '../lib/testHelpers'
@@ -30,6 +31,7 @@ import {
 } from '../lib/gnsUtils'
 import { L2Curation } from '../../build/types/L2Curation'
 import { GraphToken } from '../../build/types/GraphToken'
+import { IL2Staking } from '../../build/types/IL2Staking'
 
 const { HashZero } = ethers.constants
 
@@ -43,6 +45,7 @@ interface L1SubgraphParams {
 describe('L2GNS', () => {
   let me: Account
   let other: Account
+  let attacker: Account
   let governor: Account
   let mockRouter: Account
   let mockL1GRT: Account
@@ -56,6 +59,7 @@ describe('L2GNS', () => {
   let gns: L2GNS
   let curation: L2Curation
   let grt: GraphToken
+  let staking: IL2Staking
 
   let newSubgraph0: PublishSubgraph
   let newSubgraph1: PublishSubgraph
@@ -114,12 +118,21 @@ describe('L2GNS', () => {
 
   before(async function () {
     newSubgraph0 = buildSubgraph()
-    ;[me, other, governor, mockRouter, mockL1GRT, mockL1Gateway, mockL1GNS, mockL1Staking] =
-      await getAccounts()
+    ;[
+      me,
+      other,
+      governor,
+      mockRouter,
+      mockL1GRT,
+      mockL1Gateway,
+      mockL1GNS,
+      mockL1Staking,
+      attacker,
+    ] = await getAccounts()
 
     fixture = new NetworkFixture()
     fixtureContracts = await fixture.loadL2(governor.signer)
-    ;({ l2GraphTokenGateway, gns, curation, grt } = fixtureContracts)
+    ;({ l2GraphTokenGateway, gns, curation, grt, staking } = fixtureContracts)
 
     await grt.connect(governor.signer).mint(me.address, toGRT('10000'))
     await fixture.configureL2Bridge(
@@ -398,6 +411,67 @@ 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.signer).setCurationPercentage(100000)
+
+      // Set up an indexer account with some stake
+      await grt.connect(governor.signer).mint(attacker.address, toGRT('1000000'))
+      // Curate 1 wei GRT by minting 1 GRT and burning most of it
+      await grt.connect(attacker.signer).approve(curation.address, toBN(1))
+      await curation.connect(attacker.signer).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.signer).approve(staking.address, toGRT('1000000'))
+      await staking.connect(attacker.signer).stake(toGRT('100000'))
+      const channelKey = deriveChannelKey()
+      // Allocate to the same deployment ID
+      await staking
+        .connect(attacker.signer)
+        .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.signer).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(mockL1GNS.address, gns.address, curatedTokens, callhookData)
+
+      const l2SubgraphId = await gns.getAliasedL2SubgraphID(l1SubgraphId)
+      const tx = gns
+        .connect(me.signer)
+        .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()
@@ -741,6 +815,63 @@ 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.signer).setCurationPercentage(100000)
+      // Set up an indexer account with some stake
+      await grt.connect(governor.signer).mint(attacker.address, toGRT('1000000'))
+
+      await grt.connect(attacker.signer).approve(staking.address, toGRT('1000000'))
+      await staking.connect(attacker.signer).stake(toGRT('100000'))
+      const channelKey = deriveChannelKey()
+      // Allocate to the same deployment ID
+      await staking
+        .connect(attacker.signer)
+        .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.signer).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(
+        mockL1GNS.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 mockL1GNSL2Alias = await getL2SignerFromL1(mockL1GNS.address)
       // Eth for gas:
diff --git a/test/staking/allocation.test.ts b/test/staking/allocation.test.ts
@@ -633,11 +633,6 @@ describe('Staking:Allocation', () => {
       await expect(tx).revertedWith('!alloc')
     })
 
-    it('reject collect if allocation has been open for less than 1 epoch', async function () {
-      const tx = staking.connect(indexer.signer).collect(tokensToCollect, allocationID)
-      await expect(tx).revertedWith('!epoch')
-    })
-
     it('reject collect if allocation does not exist', async function () {
       const invalidAllocationID = randomHexBytes(20)
       const tx = staking.connect(assetHolder.signer).collect(tokensToCollect, invalidAllocationID)
