Merge pull request #760 from nounsDAO/verbs-dao-v3-lock-constructors

lock initliazers on upgradable implementation contracts
nounsDAO · Jul 18, 2023 · ea70f10 · ea70f10
2 parents 64a1974 + 6fdcd94
commit ea70f10
Show file tree

Hide file tree

Showing 7 changed files with 57 additions and 3 deletions.
diff --git a/packages/nouns-contracts/contracts/governance/NounsDAOExecutorV2.sol b/packages/nouns-contracts/contracts/governance/NounsDAOExecutorV2.sol
@@ -92,6 +92,8 @@ contract NounsDAOExecutorV2 is UUPSUpgradeable, Initializable {
 
     mapping(bytes32 => bool) public queuedTransactions;
 
+    constructor() initializer {}
+
     function initialize(address admin_, uint256 delay_) public virtual initializer {
         require(delay_ >= MINIMUM_DELAY, 'NounsDAOExecutor::constructor: Delay must exceed minimum delay.');
         require(delay_ <= MAXIMUM_DELAY, 'NounsDAOExecutor::setDelay: Delay must not exceed maximum delay.');

diff --git a/packages/nouns-contracts/contracts/governance/data/NounsDAOData.sol b/packages/nouns-contracts/contracts/governance/data/NounsDAOData.sol
@@ -67,7 +67,7 @@ contract NounsDAOData is OwnableUpgradeable, UUPSUpgradeable, NounsDAODataEvents
     /// @notice The account to send ETH fees to.
     address payable public feeRecipient;
 
-    constructor(address nounsToken_, address nounsDao_) {
+    constructor(address nounsToken_, address nounsDao_) initializer {
         nounsToken = NounsTokenLike(nounsToken_);
         nounsDao = nounsDao_;
     }

diff --git a/packages/nouns-contracts/contracts/governance/fork/newdao/NounsAuctionHouseFork.sol b/packages/nouns-contracts/contracts/governance/fork/newdao/NounsAuctionHouseFork.sol
@@ -68,6 +68,8 @@ contract NounsAuctionHouseFork is
     // The active auction
     INounsAuctionHouse.Auction public auction;
 
+    constructor() initializer {}
+
     /**
      * @notice Initialize the auction house and base contracts,
      * populate configuration values, and pause the contract.

diff --git a/packages/nouns-contracts/contracts/governance/fork/newdao/governance/NounsDAOLogicV1Fork.sol b/packages/nouns-contracts/contracts/governance/fork/newdao/governance/NounsDAOLogicV1Fork.sol
@@ -149,6 +149,8 @@ contract NounsDAOLogicV1Fork is UUPSUpgradeable, ReentrancyGuardUpgradeable, Nou
     /// @notice The EIP-712 typehash for the ballot struct used by the contract
     bytes32 public constant BALLOT_TYPEHASH = keccak256('Ballot(uint256 proposalId,uint8 support)');
 
+    constructor() initializer {}
+
     /**
      * @notice Used to initialize the contract during delegator contructor
      * @dev Not asserting that param values are within the hard-coded bounds in order to make it easier to run
@@ -778,7 +780,7 @@ contract NounsDAOLogicV1Fork is UUPSUpgradeable, ReentrancyGuardUpgradeable, Nou
         return nouns.totalSupply() - nouns.balanceOf(address(timelock)) + nouns.remainingTokensToClaim();
     }
 
-    function erc20TokensToIncludeInQuitArray() public view returns(address[] memory) {
+    function erc20TokensToIncludeInQuitArray() public view returns (address[] memory) {
         return erc20TokensToIncludeInQuit;
     }
 

diff --git a/packages/nouns-contracts/contracts/governance/fork/newdao/token/NounsTokenFork.sol b/packages/nouns-contracts/contracts/governance/fork/newdao/token/NounsTokenFork.sol
@@ -116,6 +116,8 @@ contract NounsTokenFork is INounsTokenFork, OwnableUpgradeable, ERC721Checkpoint
         _;
     }
 
+    constructor() initializer {}
+
     function initialize(
         address _owner,
         address _minter,

diff --git a/packages/nouns-contracts/test/foundry/InitializersLocked.t.sol b/packages/nouns-contracts/test/foundry/InitializersLocked.t.sol
@@ -0,0 +1,43 @@
+// SPDX-License-Identifier: GPL-3.0
+pragma solidity ^0.8.19;
+
+import 'forge-std/Test.sol';
+import { NounsDAOExecutorV2 } from '../../contracts/governance/NounsDAOExecutorV2.sol';
+import { NounsDAOData } from '../../contracts/governance/data/NounsDAOData.sol';
+import { NounsAuctionHouseFork } from '../../contracts/governance/fork/newdao/NounsAuctionHouseFork.sol';
+import { INounsToken } from '../../contracts/interfaces/INounsToken.sol';
+import { NounsDAOLogicV1Fork } from '../../contracts/governance/fork/newdao/governance/NounsDAOLogicV1Fork.sol';
+import { NounsTokenFork } from '../../contracts/governance/fork/newdao/token/NounsTokenFork.sol';
+import { INounsDAOForkEscrow } from '../../contracts/governance/NounsDAOInterfaces.sol';
+
+contract InitializersLocked is Test {
+    function test_NounsDAOExecutorV2_locks_initializer() public {
+        NounsDAOExecutorV2 c = new NounsDAOExecutorV2();
+        vm.expectRevert('Initializable: contract is already initialized');
+        c.initialize(address(0), 3 days);
+    }
+
+    function test_NounsDAOData_locks_initializer() public {
+        NounsDAOData c = new NounsDAOData(address(1), address(2));
+        vm.expectRevert('Initializable: contract is already initialized');
+        c.initialize(address(1), 0, 0, payable(address(2)));
+    }
+
+    function test_NounsAuctionHouseFork_locks_initializer() public {
+        NounsAuctionHouseFork c = new NounsAuctionHouseFork();
+        vm.expectRevert('Initializable: contract is already initialized');
+        c.initialize(address(0), INounsToken(address(0)), address(0), 0, 0, 0, 0);
+    }
+
+    function test_NounsDAOLogicV1Fork_locks_initializer() public {
+        NounsDAOLogicV1Fork c = new NounsDAOLogicV1Fork();
+        vm.expectRevert('Initializable: contract is already initialized');
+        c.initialize(address(0), address(1), 0, 0, 0, 0, new address[](0), 0);
+    }
+
+    function test_NounsTokenFork_locks_initializer() public {
+        NounsTokenFork c = new NounsTokenFork();
+        vm.expectRevert('Initializable: contract is already initialized');
+        c.initialize(address(0), address(1), INounsDAOForkEscrow(address(3)), 0, 0, 0, 0);
+    }
+}
diff --git a/packages/nouns-contracts/test/foundry/governance/fork/NounsTokenFork.t.sol b/packages/nouns-contracts/test/foundry/governance/fork/NounsTokenFork.t.sol
@@ -12,9 +12,11 @@ import { NounsToken } from '../../../../contracts/NounsToken.sol';
 import { IProxyRegistry } from '../../../../contracts/external/opensea/IProxyRegistry.sol';
 import { NounsTokenLike } from '../../../../contracts/governance/NounsDAOInterfaces.sol';
 import { ECDSA } from '@openzeppelin/contracts/utils/cryptography/ECDSA.sol';
+import { ERC1967Proxy } from '@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol';
 
 abstract contract NounsTokenForkBase is DeployUtilsFork {
     NounsTokenFork token;
+    NounsTokenFork tokenImpl;
     NounsToken originalToken;
     uint32 forkId;
     NounsSeeder seeder;
@@ -49,7 +51,8 @@ abstract contract NounsTokenForkBase is DeployUtilsFork {
         tokenIds.push(4);
         tokenIds.push(2);
 
-        token = new NounsTokenFork();
+        tokenImpl = new NounsTokenFork();
+        token = NounsTokenFork(address(new ERC1967Proxy(address(tokenImpl), '')));
         token.initialize(treasury, minter, escrow, forkId, 0, 3, block.timestamp + FORK_PERIOD);
         vm.startPrank(token.owner());
         token.setSeeder(seeder);