Merge pull request #527 from graphprotocol/ariel/gns-nft-descriptor

Refactor Subgraph NFT using composition

Author: abarmat

audits/ConsenSysDiligence/2022-01-graph-pr527-audit.pdf

@@ -4,9 +4,9 @@ pragma solidity ^0.7.6;
 pragma abicoder v2;
 
 import "@openzeppelin/contracts/math/SafeMath.sol";
+import "@openzeppelin/contracts/utils/Address.sol";
 
 import "../base/Multicall.sol";
-import "../base/SubgraphNFT.sol";
 import "../bancor/BancorFormula.sol";
 import "../upgrades/GraphUpgradeable.sol";
 import "../utils/TokenUtils.sol";
@@ -38,6 +38,8 @@ contract GNS is GNSV2Storage, GraphUpgradeable, IGNS, Multicall {
 
     // -- Events --
 
+    event SubgraphNFTUpdated(address subgraphNFT);
+
     /**
      * @dev Emitted when graph account sets its default name
      */
@@ -143,16 +145,16 @@ contract GNS is GNSV2Storage, GraphUpgradeable, IGNS, Multicall {
     function initialize(
         address _controller,
         address _bondingCurve,
-        address _tokenDescriptor
+        address _subgraphNFT
     ) external onlyImpl {
         Managed._initialize(_controller);
 
         // Dependencies
         bondingCurve = _bondingCurve;
-        __SubgraphNFT_init(_tokenDescriptor);
 
         // Settings
         _setOwnerTaxPercentage(500000);
+        _setSubgraphNFT(_subgraphNFT);
     }
 
     /**
@@ -162,6 +164,8 @@ contract GNS is GNSV2Storage, GraphUpgradeable, IGNS, Multicall {
         graphToken().approve(address(curation()), MAX_UINT256);
     }
 
+    // -- Config --
+
     /**
      * @dev Set the owner fee percentage. This is used to prevent a subgraph owner to drain all
      * the name curators tokens while upgrading or deprecating and is configurable in parts per million.
@@ -171,14 +175,6 @@ contract GNS is GNSV2Storage, GraphUpgradeable, IGNS, Multicall {
         _setOwnerTaxPercentage(_ownerTaxPercentage);
     }
 
-    /**
-     * @dev Set the token descriptor contract.
-     * @param _tokenDescriptor Address of the contract that creates the NFT token URI
-     */
-    function setTokenDescriptor(address _tokenDescriptor) external override onlyGovernor {
-        _setTokenDescriptor(_tokenDescriptor);
-    }
-
     /**
      * @dev Internal: Set the owner tax percentage. This is used to prevent a subgraph owner to drain all
      * the name curators tokens while upgrading or deprecating and is configurable in parts per million.
@@ -190,6 +186,32 @@ contract GNS is GNSV2Storage, GraphUpgradeable, IGNS, Multicall {
         emit ParameterUpdated("ownerTaxPercentage");
     }
 
+    /**
+     * @dev Set the NFT registry contract
+     * NOTE: Calling this function will break the ownership model unless
+     * it is replaced with a fully migrated version of the NFT contract state
+     * Use with care.
+     * @param _subgraphNFT Address of the ERC721 contract
+     */
+    function setSubgraphNFT(address _subgraphNFT) public onlyGovernor {
+        _setSubgraphNFT(_subgraphNFT);
+    }
+
+    /**
+     * @dev Internal: Set the NFT registry contract
+     * @param _subgraphNFT Address of the ERC721 contract
+     */
+    function _setSubgraphNFT(address _subgraphNFT) private {
+        require(
+            _subgraphNFT != address(0) && Address.isContract(_subgraphNFT),
+            "NFT must be valid"
+        );
+        subgraphNFT = ISubgraphNFT(_subgraphNFT);
+        emit SubgraphNFTUpdated(_subgraphNFT);
+    }
+
+    // -- Actions --
+
     /**
      * @dev Allows a graph account to set a default name
      * @param _graphAccount Account that is setting its name
@@ -217,7 +239,7 @@ contract GNS is GNSV2Storage, GraphUpgradeable, IGNS, Multicall {
         override
         onlySubgraphAuth(_subgraphID)
     {
-        emit SubgraphMetadataUpdated(_subgraphID, _subgraphMetadata);
+        _setSubgraphMetadata(_subgraphID, _subgraphMetadata);
     }
 
     /**
@@ -241,12 +263,14 @@ contract GNS is GNSV2Storage, GraphUpgradeable, IGNS, Multicall {
         subgraphData.subgraphDeploymentID = _subgraphDeploymentID;
         subgraphData.reserveRatio = defaultReserveRatio;
 
-        // Mint the NFT. Use the subgraphID as tokenId.
-        // This function will check the if tokenId already exists.
-        _mint(subgraphOwner, subgraphID);
-
+        // Mint the NFT. Use the subgraphID as tokenID.
+        // This function will check the if tokenID already exists.
+        _mintNFT(subgraphOwner, subgraphID);
         emit SubgraphPublished(subgraphID, _subgraphDeploymentID, defaultReserveRatio);
-        emit SubgraphMetadataUpdated(subgraphID, _subgraphMetadata);
+
+        // Set the token metadata
+        _setSubgraphMetadata(subgraphID, _subgraphMetadata);
+
         emit SubgraphVersionUpdated(subgraphID, _subgraphDeploymentID, _versionMetadata);
     }
 
@@ -356,7 +380,7 @@ contract GNS is GNSV2Storage, GraphUpgradeable, IGNS, Multicall {
         // subgraphData.subgraphDeploymentID = 0;
 
         // Burn the NFT
-        _burn(_subgraphID);
+        _burnNFT(_subgraphID);
 
         emit SubgraphDeprecated(_subgraphID, subgraphData.withdrawableGRT);
     }
@@ -637,21 +661,17 @@ contract GNS is GNSV2Storage, GraphUpgradeable, IGNS, Multicall {
         return 0;
     }
 
-    /**
-     * @dev Return the URI describing a particular token ID for a Subgraph.
-     * @param _subgraphID Subgraph ID
-     * @return The URI of the ERC721-compliant metadata
-     */
-    function tokenURI(uint256 _subgraphID) public view override returns (string memory) {
-        return tokenDescriptor.tokenURI(this, _subgraphID);
-    }
-
     /**
      * @dev Create subgraphID for legacy subgraph and mint ownership NFT.
      * @param _graphAccount Account that created the subgraph
      * @param _subgraphNumber The sequence number of the created subgraph
+     * @param _subgraphMetadata IPFS hash for the subgraph metadata
      */
-    function migrateLegacySubgraph(address _graphAccount, uint256 _subgraphNumber) external {
+    function migrateLegacySubgraph(
+        address _graphAccount,
+        uint256 _subgraphNumber,
+        bytes32 _subgraphMetadata
+    ) external {
         // Must be an existing legacy subgraph
         bool legacySubgraphExists = legacySubgraphData[_graphAccount][_subgraphNumber]
             .subgraphDeploymentID != 0;
@@ -675,9 +695,11 @@ contract GNS is GNSV2Storage, GraphUpgradeable, IGNS, Multicall {
 
         // Mint the NFT and send to owner
         // The subgraph owner is the graph account that created it
-        _mint(_graphAccount, subgraphID);
-
+        _mintNFT(_graphAccount, subgraphID);
         emit LegacySubgraphClaimed(_graphAccount, _subgraphNumber);
+
+        // Set the token metadata
+        _setSubgraphMetadata(subgraphID, _subgraphMetadata);
     }
 
     /**
@@ -757,4 +779,45 @@ contract GNS is GNSV2Storage, GraphUpgradeable, IGNS, Multicall {
         require(_isPublished(subgraphData) == true, "GNS: Must be active");
         return subgraphData;
     }
+
+    // -- NFT --
+
+    /**
+     * @dev Return the owner of a subgraph.
+     * @param _tokenID Subgraph ID
+     * @return Owner address
+     */
+    function ownerOf(uint256 _tokenID) public view override returns (address) {
+        return subgraphNFT.ownerOf(_tokenID);
+    }
+
+    /**
+     * @dev Mint the NFT for the subgraph.
+     * @param _owner Owner address
+     * @param _tokenID Subgraph ID
+     */
+    function _mintNFT(address _owner, uint256 _tokenID) internal {
+        subgraphNFT.mint(_owner, _tokenID);
+    }
+
+    /**
+     * @dev Burn the NFT for the subgraph.
+     * @param _tokenID Subgraph ID
+     */
+    function _burnNFT(uint256 _tokenID) internal {
+        subgraphNFT.burn(_tokenID);
+    }
+
+    /**
+     * @dev Set the subgraph metadata.
+     * @param _tokenID Subgraph ID
+     * @param _subgraphMetadata IPFS hash of the subgraph metadata
+     */
+    function _setSubgraphMetadata(uint256 _tokenID, bytes32 _subgraphMetadata) internal {
+        subgraphNFT.setSubgraphMetadata(_tokenID, _subgraphMetadata);
+
+        // Even if the following event is emitted in the NFT we emit it here to facilitate
+        // subgraph indexing
+        emit SubgraphMetadataUpdated(_tokenID, _subgraphMetadata);
+    }
 }

contracts/base/ISubgraphNFTDescriptor.sol

@@ -3,11 +3,11 @@
 pragma solidity ^0.7.6;
 pragma abicoder v2;
 
-import "../base/SubgraphNFT.sol";
 import "../governance/Managed.sol";
 
 import "./erc1056/IEthereumDIDRegistry.sol";
 import "./IGNS.sol";
+import "./ISubgraphNFT.sol";
 
 abstract contract GNSV1Storage is Managed {
     // -- State --
@@ -37,12 +37,15 @@ abstract contract GNSV1Storage is Managed {
     IEthereumDIDRegistry private __DEPRECATED_erc1056Registry;
 }
 
-abstract contract GNSV2Storage is GNSV1Storage, SubgraphNFT {
+abstract contract GNSV2Storage is GNSV1Storage {
     // Use it whenever a legacy (v1) subgraph NFT was claimed to maintain compatibility
     // Keep a reference from subgraphID => (graphAccount, subgraphNumber)
     mapping(uint256 => IGNS.LegacySubgraphKey) public legacySubgraphKeys;
 
     // Store data for all NFT-based (v2) subgraphs
     // subgraphID => SubgraphData
     mapping(uint256 => IGNS.SubgraphData) public subgraphs;
+
+    // Contract that represents subgraph ownership through an NFT
+    ISubgraphNFT public subgraphNFT;
 }

contracts/base/SubgraphNFT.sol

@@ -26,8 +26,6 @@ interface IGNS {
 
     function setOwnerTaxPercentage(uint32 _ownerTaxPercentage) external;
 
-    function setTokenDescriptor(address _tokenDescriptor) external;
-
     // -- Publishing --
 
     function setDefaultName(
@@ -71,6 +69,8 @@ interface IGNS {
 
     // -- Getters --
 
+    function ownerOf(uint256 _tokenID) external view returns (address);
+
     function subgraphSignal(uint256 _subgraphID) external view returns (uint256);
 
     function subgraphTokens(uint256 _subgraphID) external view returns (uint256);

contracts/base/SubgraphNFTDescriptor.sol

@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+pragma solidity ^0.7.6;
+
+import "@openzeppelin/contracts/token/ERC721/IERC721.sol";
+
+interface ISubgraphNFT is IERC721 {
+    // -- Config --
+
+    function setMinter(address _minter) external;
+
+    function setTokenDescriptor(address _tokenDescriptor) external;
+
+    function setBaseURI(string memory _baseURI) external;
+
+    // -- Actions --
+
+    function mint(address _to, uint256 _tokenId) external;
+
+    function burn(uint256 _tokenId) external;
+
+    function setSubgraphMetadata(uint256 _tokenId, bytes32 _subgraphMetadata) external;
+
+    function tokenURI(uint256 _tokenId) external view returns (string memory);
+}

contracts/discovery/GNS.sol

@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+pragma solidity ^0.7.6;
+
+/// @title Describes subgraph NFT tokens via URI
+interface ISubgraphNFTDescriptor {
+    /// @notice Produces the URI describing a particular token ID for a Subgraph
+    /// @dev Note this URI may be data: URI with the JSON contents directly inlined
+    /// @param _minter Address of the allowed minter
+    /// @param _tokenId The ID of the subgraph NFT for which to produce a description, which may not be valid
+    /// @param _baseURI The base URI that could be prefixed to the final URI
+    /// @param _subgraphMetadata Subgraph metadata set for the subgraph
+    /// @return The URI of the ERC721-compliant metadata
+    function tokenURI(
+        address _minter,
+        uint256 _tokenId,
+        string calldata _baseURI,
+        bytes32 _subgraphMetadata
+    ) external view returns (string memory);
+}