Merge pull request #159 from ourzora/isabella/add-contract-read-for-f…

…actory-upgrade Add existing contract read for factory upgrading
ourzora · Oct 11, 2023 · dadc1aa · dadc1aa
2 parents 6361928 + 67f8ed0
commit dadc1aa
Show file tree

Hide file tree

Showing 6 changed files with 100 additions and 2 deletions.
diff --git a/.changeset/dry-dragons-breathe.md b/.changeset/dry-dragons-breathe.md
@@ -0,0 +1,5 @@
+---
+'@zoralabs/nft-drop-contracts': patch
+---
+
+Adding a contract name check in \_authorizeUpgrade to prevent incorrect contract name upgrades.
diff --git a/src/ZoraNFTCreatorV1.sol b/src/ZoraNFTCreatorV1.sol
@@ -74,7 +74,11 @@ contract ZoraNFTCreatorV1 is OwnableUpgradeable, UUPSUpgradeable, IContractMetad
         internal
         override
         onlyOwner
-    {}
+    {
+        if (!(keccak256(bytes(IContractMetadata(_newImplementation).contractName())) == keccak256(bytes(this.contractName())))) {
+            revert UpgradeToMismatchedContractName(this.contractName(), IContractMetadata(_newImplementation).contractName());
+        }
+    }
 
     function createAndConfigureDrop(
         string memory name,

diff --git a/src/interfaces/IContractMetadata.sol b/src/interfaces/IContractMetadata.sol
@@ -2,6 +2,9 @@
 pragma solidity 0.8.17;
 
 interface IContractMetadata {
+    /// Contract names do not match
+    error UpgradeToMismatchedContractName(string expected, string actual);
+
     /// @notice Contract name returns the pretty contract name
     function contractName() external returns (string memory);
 

diff --git a/test/ZoraNFTCreatorV1.t.sol b/test/ZoraNFTCreatorV1.t.sol
@@ -8,6 +8,7 @@ import {ProtocolRewards} from "@zoralabs/protocol-rewards/src/ProtocolRewards.so
 import {IMetadataRenderer} from "../src/interfaces/IMetadataRenderer.sol";
 import "../src/ZoraNFTCreatorV1.sol";
 import "../src/ZoraNFTCreatorProxy.sol";
+import {MockContractMetadata} from "./utils/MockContractMetadata.sol";
 import {MockMetadataRenderer} from "./metadata/MockMetadataRenderer.sol";
 import {FactoryUpgradeGate} from "../src/FactoryUpgradeGate.sol";
 import {IERC721AUpgradeable} from "erc721a-upgradeable/IERC721AUpgradeable.sol";
@@ -156,5 +157,11 @@ contract ZoraNFTCreatorV1Test is Test {
         assertEq(drop.tokenURI(1), "DEMO");
     }
 
-
+    function test_UpgradeFailsWithDifferentContractName() public {
+        MockContractMetadata mockMetadata = new MockContractMetadata("uri", "test name");
+        address owner = creator.owner();
+        vm.prank(owner);
+        vm.expectRevert(abi.encodeWithSignature("UpgradeToMismatchedContractName(string,string)", "ZORA NFT Creator", "test name"));
+        creator.upgradeTo(address(mockMetadata));
+    }
 }
diff --git a/test/ZoraNFTCreatorV1_Fork.t.sol b/test/ZoraNFTCreatorV1_Fork.t.sol
@@ -6,12 +6,14 @@ import {Test} from "forge-std/Test.sol";
 import {IMetadataRenderer} from "../src/interfaces/IMetadataRenderer.sol";
 import "../src/ZoraNFTCreatorV1.sol";
 import "../src/ZoraNFTCreatorProxy.sol";
+import {MockContractMetadata} from "./utils/MockContractMetadata.sol";
 import {MockMetadataRenderer} from "./metadata/MockMetadataRenderer.sol";
 import {FactoryUpgradeGate} from "../src/FactoryUpgradeGate.sol";
 import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
 import {IERC721AUpgradeable} from "erc721a-upgradeable/IERC721AUpgradeable.sol";
 import {ForkHelper} from "./utils/ForkHelper.sol";
 import {DropDeployment, ChainConfig} from "../src/DeploymentConfig.sol";
+import {ProtocolRewards} from "@zoralabs/protocol-rewards/src/ProtocolRewards.sol";
 
 contract ZoraNFTCreatorV1Test is Test, ForkHelper {
     address public constant DEFAULT_OWNER_ADDRESS = address(0x23499);
@@ -24,6 +26,15 @@ contract ZoraNFTCreatorV1Test is Test, ForkHelper {
     EditionMetadataRenderer public editionMetadataRenderer;
     DropMetadataRenderer public dropMetadataRenderer;
 
+    function deployCore() internal {
+        ProtocolRewards protocolRewards = new ProtocolRewards();
+
+        dropImpl = new ERC721Drop(address(1234), FactoryUpgradeGate(address(0)), mintFee, mintFeeRecipient, address(protocolRewards));
+
+        editionMetadataRenderer = new EditionMetadataRenderer();
+        dropMetadataRenderer = new DropMetadataRenderer();
+    }
+
     function makeDefaultSalesConfiguration(uint104 price) internal returns (IERC721Drop.SalesConfiguration memory) {
         return
             IERC721Drop.SalesConfiguration({
@@ -178,4 +189,47 @@ contract ZoraNFTCreatorV1Test is Test, ForkHelper {
         drop.purchase{value: fee}(1);
         assertEq(drop.tokenURI(1), "DEMO");
     }
+
+    // The current contracts on chain do not have the contract name check in _authorizeUpgrade
+    // so it will not check for miss matched contract names. In order to get around that we upgrade first and then check
+    function test_ForkUpgradeWithDifferentContractName() external {
+        string[] memory forkTestChains = getForkTestChains();
+
+        for (uint256 i = 0; i < forkTestChains.length; i++) {
+            string memory chainName = forkTestChains[i];
+            vm.createSelectFork(vm.rpcUrl(chainName));
+            creator = ZoraNFTCreatorV1(getDeployment().factory);
+            verifyAddressesFork(chainName);
+            deployCore();
+            revertsWhenContractNameMismatches();
+            forkUpgradeSucceedsWithMatchingContractName();
+        }
+    }
+
+    function revertsWhenContractNameMismatches() internal {
+        ZoraNFTCreatorV1 newCreatorImpl = new ZoraNFTCreatorV1(address(dropImpl), editionMetadataRenderer, dropMetadataRenderer);
+        address owner = creator.owner();
+
+        vm.prank(owner);
+        creator.upgradeTo(address(newCreatorImpl));
+
+        MockContractMetadata mockMetadata = new MockContractMetadata("uri", "test name");
+
+        vm.prank(owner);
+        vm.expectRevert(abi.encodeWithSignature("UpgradeToMismatchedContractName(string,string)", "ZORA NFT Creator", "test name"));
+        creator.upgradeTo(address(mockMetadata));
+    }
+
+    function forkUpgradeSucceedsWithMatchingContractName() internal {
+        ZoraNFTCreatorV1 newCreatorImpl = new ZoraNFTCreatorV1(address(dropImpl), editionMetadataRenderer, dropMetadataRenderer);
+        address owner = creator.owner();
+
+        vm.prank(owner);
+        creator.upgradeTo(address(newCreatorImpl));
+
+        MockContractMetadata mockMetadata = new MockContractMetadata("uri", "ZORA NFT Creator");
+
+        vm.prank(owner);
+        creator.upgradeTo(address(mockMetadata));
+    }
 }
diff --git a/test/utils/MockContractMetadata.sol b/test/utils/MockContractMetadata.sol
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.10;
+
+import {IContractMetadata} from "../../src/interfaces/IContractMetadata.sol";
+import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
+
+contract MockContractMetadata is IContractMetadata, UUPSUpgradeable {
+    string public override contractURI;
+    string public override contractName;
+
+    constructor(string memory _contractURI, string memory _contractName) {
+        contractURI = _contractURI;
+        contractName = _contractName;
+    }
+
+    function setContractURI(string memory _contractURI) external {
+        contractURI = _contractURI;
+    }
+
+    function setContractName(string memory _contractName) external {
+        contractName = _contractName;
+    }
+
+    function _authorizeUpgrade(address _newImplementation) internal override {}
+}