Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add existing contract read for factory upgrading #159

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/dry-dragons-breathe.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@zoralabs/nft-drop-contracts': patch
---

Adding a contract name check in \_authorizeUpgrade to prevent incorrect contract name upgrades.
6 changes: 5 additions & 1 deletion src/ZoraNFTCreatorV1.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
3 changes: 3 additions & 0 deletions src/interfaces/IContractMetadata.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
9 changes: 8 additions & 1 deletion test/ZoraNFTCreatorV1.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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));
}
}
54 changes: 54 additions & 0 deletions test/ZoraNFTCreatorV1_Fork.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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({
Expand Down Expand Up @@ -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);
oveddan marked this conversation as resolved.
Show resolved Hide resolved
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));
}
}
25 changes: 25 additions & 0 deletions test/utils/MockContractMetadata.sol
Original file line number Diff line number Diff line change
@@ -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 {}
}