Merge pull request #523 from graphprotocol/audit/gns-ownership-x01

Audit: [X01] Subgraph deployment not updated when no signal

Author: abarmat

contracts/discovery/GNS.sol

@@ -290,6 +290,7 @@ contract GNS is GNSV2Storage, GraphUpgradeable, IGNS, Multicall {
         // NOTE: We will only do this as long as there is signal on the subgraph
         if (subgraphData.nSignal > 0) {
             // Burn all version signal in the name pool for tokens (w/no slippage protection)
+            // Sell all signal from the old deployment
             uint256 tokens = curation.burn(
                 subgraphData.subgraphDeploymentID,
                 subgraphData.vSignal,
@@ -306,12 +307,9 @@ contract GNS is GNSV2Storage, GraphUpgradeable, IGNS, Multicall {
             );
 
             // Update pool: constant nSignal, vSignal can change (w/no slippage protection)
-            subgraphData.subgraphDeploymentID = _subgraphDeploymentID;
-            (subgraphData.vSignal, ) = curation.mint(
-                subgraphData.subgraphDeploymentID,
-                tokensWithTax,
-                0
-            );
+            // Buy all signal from the new deployment
+            (subgraphData.vSignal, ) = curation.mint(_subgraphDeploymentID, tokensWithTax, 0);
+
             emit SubgraphUpgraded(
                 _subgraphID,
                 subgraphData.vSignal,
@@ -320,6 +318,9 @@ contract GNS is GNSV2Storage, GraphUpgradeable, IGNS, Multicall {
             );
         }
 
+        // Update target deployment
+        subgraphData.subgraphDeploymentID = _subgraphDeploymentID;
+
         emit SubgraphVersionUpdated(_subgraphID, _subgraphDeploymentID, _versionMetadata);
     }
 

test/gns.test.ts

@@ -190,10 +190,9 @@ describe('GNS', () => {
 
     // Check what selling all nSignal, which == selling all vSignal, should return for tokens
     // NOTE - no tax on burning on nSignal
-    const { 1: tokensReceivedEstimate } = await gns.nSignalToTokens(
-      subgraphID,
-      beforeSubgraph.nSignal,
-    )
+    const tokensReceivedEstimate = beforeSubgraph.nSignal.gt(0)
+      ? (await gns.nSignalToTokens(subgraphID, beforeSubgraph.nSignal))[1]
+      : toBN(0)
     // Example:
     // Deposit 100, 5 is taxed, 95 GRT in curve
     // Upgrade - calculate 5% tax on 95 --> 4.75 GRT
@@ -211,10 +210,10 @@ describe('GNS', () => {
     const totalAdjustedUp = totalWithOwnerTax.mul(MAX_PPM).div(MAX_PPM - curationTaxPercentage)
 
     // Re-estimate amount of signal to get considering the owner tax paid by the owner
-    const { 0: newVSignalEstimate, 1: newCurationTaxEstimate } = await curation.tokensToSignal(
-      newSubgraph.subgraphDeploymentID,
-      totalAdjustedUp,
-    )
+
+    const { 0: newVSignalEstimate, 1: newCurationTaxEstimate } = beforeSubgraph.nSignal.gt(0)
+      ? await curation.tokensToSignal(newSubgraph.subgraphDeploymentID, totalAdjustedUp)
+      : [toBN(0), toBN(0)]
 
     // Send tx
     const tx = gns
@@ -620,6 +619,11 @@ describe('GNS', () => {
         await publishNewVersion(me, subgraph.id, newSubgraph1)
       })
 
+      it('should publish a new version on an existing subgraph with no current signal', async function () {
+        const emptySignalSubgraph = await publishNewSubgraph(me, buildSubgraph())
+        await publishNewVersion(me, emptySignalSubgraph.id, newSubgraph1)
+      })
+
       it('should reject a new version with the same subgraph deployment ID', async function () {
         const tx = gns
           .connect(me.signer)