From d20a932980b5b4b2ebaeb8d7c295033ebd3510f4 Mon Sep 17 00:00:00 2001 From: Isabella Smallcombe Date: Wed, 4 Oct 2023 11:49:51 -0400 Subject: [PATCH 1/7] feat: add existing contract read for factory upgrading --- src/ZoraNFTCreatorV1.sol | 9 ++++++++- test/ZoraNFTCreatorV1.t.sol | 9 ++++++++- test/utils/MockContractMetadata.sol | 26 ++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 2 deletions(-) create mode 100644 test/utils/MockContractMetadata.sol diff --git a/src/ZoraNFTCreatorV1.sol b/src/ZoraNFTCreatorV1.sol index d093ad6..381ddc6 100644 --- a/src/ZoraNFTCreatorV1.sol +++ b/src/ZoraNFTCreatorV1.sol @@ -17,6 +17,9 @@ import {IContractMetadata} from "./interfaces/IContractMetadata.sol"; contract ZoraNFTCreatorV1 is OwnableUpgradeable, UUPSUpgradeable, IContractMetadata, Version(8) { string private constant CANNOT_BE_ZERO = "Cannot be 0 address"; + /// Contract names do not match + error UpgradeToMismatchedContractName(string expected, string actual); + /// @notice Emitted when a edition is created reserving the corresponding token IDs. event CreatedDrop( address indexed creator, @@ -74,7 +77,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/test/ZoraNFTCreatorV1.t.sol b/test/ZoraNFTCreatorV1.t.sol index d963ce5..bb0b810 100644 --- 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/utils/MockContractMetadata.sol b/test/utils/MockContractMetadata.sol new file mode 100644 index 0000000..2604ffa --- /dev/null +++ b/test/utils/MockContractMetadata.sol @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.10; + +import {IContractMetadata} from "../../src/interfaces/IContractMetadata.sol"; + +contract MockContractMetadata is IContractMetadata { + 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; + + } + +} \ No newline at end of file From 790660765835ad5f771ae306ab0cddfaca1fe630 Mon Sep 17 00:00:00 2001 From: Isabella Smallcombe Date: Wed, 11 Oct 2023 13:10:22 -0400 Subject: [PATCH 2/7] feat: add fork tests --- test/ZoraNFTCreatorV1_Fork.t.sol | 52 +++++++++++++++++++++++++++++ test/utils/MockContractMetadata.sol | 9 +++-- 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/test/ZoraNFTCreatorV1_Fork.t.sol b/test/ZoraNFTCreatorV1_Fork.t.sol index 8124c0e..2bcf163 100644 --- 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,45 @@ contract ZoraNFTCreatorV1Test is Test, ForkHelper { drop.purchase{value: fee}(1); assertEq(drop.tokenURI(1), "DEMO"); } + + 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(); + forkContractNameWithRevert(); + forkContractNameSucceed(); + } + } + + function forkContractNameWithRevert() 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 forkContractNameSucceed() 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 index 2604ffa..8cd1c10 100644 --- a/test/utils/MockContractMetadata.sol +++ b/test/utils/MockContractMetadata.sol @@ -2,25 +2,24 @@ 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 { +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; - } -} \ No newline at end of file + function _authorizeUpgrade(address _newImplementation) internal override {} +} From e30297cffa20578b656b3bcfc7f8142440b4876b Mon Sep 17 00:00:00 2001 From: Isabella Smallcombe Date: Wed, 11 Oct 2023 14:55:09 -0400 Subject: [PATCH 3/7] fix: update test names --- test/ZoraNFTCreatorV1_Fork.t.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/ZoraNFTCreatorV1_Fork.t.sol b/test/ZoraNFTCreatorV1_Fork.t.sol index 2bcf163..64b5cb0 100644 --- a/test/ZoraNFTCreatorV1_Fork.t.sol +++ b/test/ZoraNFTCreatorV1_Fork.t.sol @@ -199,12 +199,12 @@ contract ZoraNFTCreatorV1Test is Test, ForkHelper { creator = ZoraNFTCreatorV1(getDeployment().factory); verifyAddressesFork(chainName); deployCore(); - forkContractNameWithRevert(); - forkContractNameSucceed(); + revertsWhenContractNameMismatches(); + forkUpgradeSucceedsWithMatchingContractName(); } } - function forkContractNameWithRevert() internal { + function revertsWhenContractNameMismatches() internal { ZoraNFTCreatorV1 newCreatorImpl = new ZoraNFTCreatorV1(address(dropImpl), editionMetadataRenderer, dropMetadataRenderer); address owner = creator.owner(); @@ -218,7 +218,7 @@ contract ZoraNFTCreatorV1Test is Test, ForkHelper { creator.upgradeTo(address(mockMetadata)); } - function forkContractNameSucceed() internal { + function forkUpgradeSucceedsWithMatchingContractName() internal { ZoraNFTCreatorV1 newCreatorImpl = new ZoraNFTCreatorV1(address(dropImpl), editionMetadataRenderer, dropMetadataRenderer); address owner = creator.owner(); From 2724fe1054382e994ff602de72f1079046b663dd Mon Sep 17 00:00:00 2001 From: Isabella Smallcombe Date: Wed, 11 Oct 2023 15:09:47 -0400 Subject: [PATCH 4/7] feat: move UpgradeToMismatchedContractName to IContractMetadata interface --- src/ZoraNFTCreatorV1.sol | 3 --- src/interfaces/IContractMetadata.sol | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/ZoraNFTCreatorV1.sol b/src/ZoraNFTCreatorV1.sol index 381ddc6..e859228 100644 --- a/src/ZoraNFTCreatorV1.sol +++ b/src/ZoraNFTCreatorV1.sol @@ -17,9 +17,6 @@ import {IContractMetadata} from "./interfaces/IContractMetadata.sol"; contract ZoraNFTCreatorV1 is OwnableUpgradeable, UUPSUpgradeable, IContractMetadata, Version(8) { string private constant CANNOT_BE_ZERO = "Cannot be 0 address"; - /// Contract names do not match - error UpgradeToMismatchedContractName(string expected, string actual); - /// @notice Emitted when a edition is created reserving the corresponding token IDs. event CreatedDrop( address indexed creator, diff --git a/src/interfaces/IContractMetadata.sol b/src/interfaces/IContractMetadata.sol index 10dccd9..a2e938f 100644 --- 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); From da1f933e6e392595c11e3ac87edee1fdadb9134f Mon Sep 17 00:00:00 2001 From: Isabella Smallcombe Date: Wed, 11 Oct 2023 15:35:42 -0400 Subject: [PATCH 5/7] feat: add comment to test_ForkUpgradeWithDifferentContractName --- test/ZoraNFTCreatorV1_Fork.t.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/ZoraNFTCreatorV1_Fork.t.sol b/test/ZoraNFTCreatorV1_Fork.t.sol index 64b5cb0..1566f7e 100644 --- a/test/ZoraNFTCreatorV1_Fork.t.sol +++ b/test/ZoraNFTCreatorV1_Fork.t.sol @@ -190,6 +190,8 @@ contract ZoraNFTCreatorV1Test is Test, ForkHelper { 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(); From 478c5cee5ac65a07bd75a34e4e9c9589e232485c Mon Sep 17 00:00:00 2001 From: Isabella Smallcombe Date: Wed, 11 Oct 2023 15:36:56 -0400 Subject: [PATCH 6/7] fix: comment --- test/ZoraNFTCreatorV1_Fork.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/ZoraNFTCreatorV1_Fork.t.sol b/test/ZoraNFTCreatorV1_Fork.t.sol index 1566f7e..a993243 100644 --- a/test/ZoraNFTCreatorV1_Fork.t.sol +++ b/test/ZoraNFTCreatorV1_Fork.t.sol @@ -190,7 +190,7 @@ contract ZoraNFTCreatorV1Test is Test, ForkHelper { assertEq(drop.tokenURI(1), "DEMO"); } - // The current contracts on chain do not have the contract name check in _authorizeUpgrade + // 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(); From 67f8ed04455aa8e54f4c5dbffd07651b0295e129 Mon Sep 17 00:00:00 2001 From: Isabella Smallcombe Date: Wed, 11 Oct 2023 17:08:44 -0400 Subject: [PATCH 7/7] feat: add changeset --- .changeset/dry-dragons-breathe.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/dry-dragons-breathe.md diff --git a/.changeset/dry-dragons-breathe.md b/.changeset/dry-dragons-breathe.md new file mode 100644 index 0000000..18f7639 --- /dev/null +++ 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.