Ensuring that constructors leave disabled proxy initializers

aragon · Dec 11, 2024 · f4e4632 · f4e4632
1 parent bd2e7b0
commit f4e4632
Show file tree

Hide file tree

Showing 10 changed files with 100 additions and 20 deletions.
diff --git a/src/EmergencyMultisig.sol b/src/EmergencyMultisig.sol
@@ -131,6 +131,11 @@ contract EmergencyMultisig is IEmergencyMultisig, PluginUUPSUpgradeable, Proposa
         bool onlyListed, uint16 indexed minApprovals, SignerList signerList, uint32 proposalExpirationPeriod
     );
 
+    /// @notice Disables the initializers on the implementation contract to prevent it from being left uninitialized.
+    constructor() {
+        _disableInitializers();
+    }
+
     /// @notice Initializes Release 1, Build 1.
     /// @dev This method is required to support [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822).
     /// @param _dao The IDAO interface of the associated DAO.

diff --git a/src/Multisig.sol b/src/Multisig.sol
@@ -129,6 +129,11 @@ contract Multisig is IMultisig, PluginUUPSUpgradeable, ProposalUpgradeable {
         uint32 proposalExpirationPeriod
     );
 
+    /// @notice Disables the initializers on the implementation contract to prevent it from being left uninitialized.
+    constructor() {
+        _disableInitializers();
+    }
+
     /// @notice Initializes Release 1, Build 1.
     /// @dev This method is required to support [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822).
     /// @param _dao The IDAO interface of the associated DAO.

diff --git a/src/OptimisticTokenVotingPlugin.sol b/src/OptimisticTokenVotingPlugin.sol
@@ -151,6 +151,11 @@ contract OptimisticTokenVotingPlugin is
     /// @notice Thrown if the voting power is zero
     error NoVotingPower();
 
+    /// @notice Disables the initializers on the implementation contract to prevent it from being left uninitialized.
+    constructor() {
+        _disableInitializers();
+    }
+
     /// @notice Initializes the component to be used by inheriting contracts.
     /// @dev This method is required to support [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822).
     /// @param _dao The IDAO interface of the associated DAO.

diff --git a/test/EmergencyMultisig.t.sol b/test/EmergencyMultisig.t.sol
@@ -69,15 +69,31 @@ contract EmergencyMultisigTest is AragonTest {
         ).build();
     }
 
-    modifier givenANewlyDeployedContract() {
+    function test_WhenDeployingTheContract() external {
+        // It should disable the initializers
+        EmergencyMultisig.MultisigSettings memory settings = EmergencyMultisig.MultisigSettings({
+            onlyListed: true,
+            minApprovals: 3,
+            signerList: signerList,
+            proposalExpirationPeriod: EMERGENCY_MULTISIG_PROPOSAL_EXPIRATION_PERIOD
+        });
+
+        eMultisig = new EmergencyMultisig();
+
+        // It should refuse to initialize
+        vm.expectRevert("Initializable: contract is already initialized");
+        eMultisig.initialize(dao, settings);
+    }
+
+    modifier givenANewProxyContract() {
         _;
     }
 
     modifier givenCallingInitialize() {
         _;
     }
 
-    function test_GivenCallingInitialize() external givenANewlyDeployedContract givenCallingInitialize {
+    function test_GivenCallingInitialize() external givenANewProxyContract givenCallingInitialize {
         EmergencyMultisig.MultisigSettings memory settings = EmergencyMultisig.MultisigSettings({
             onlyListed: true,
             minApprovals: 3,
@@ -175,7 +191,7 @@ contract EmergencyMultisigTest is AragonTest {
 
     function test_RevertWhen_MinApprovalsIsGreaterThanSignerListLengthOnInitialize()
         external
-        givenANewlyDeployedContract
+        givenANewProxyContract
         givenCallingInitialize
     {
         // It should revert
@@ -235,11 +251,7 @@ contract EmergencyMultisigTest is AragonTest {
         );
     }
 
-    function test_RevertWhen_MinApprovalsIsZeroOnInitialize()
-        external
-        givenANewlyDeployedContract
-        givenCallingInitialize
-    {
+    function test_RevertWhen_MinApprovalsIsZeroOnInitialize() external givenANewProxyContract givenCallingInitialize {
         // It should revert
         EmergencyMultisig.MultisigSettings memory settings = EmergencyMultisig.MultisigSettings({
             onlyListed: true,
@@ -297,11 +309,7 @@ contract EmergencyMultisigTest is AragonTest {
         );
     }
 
-    function test_RevertWhen_SignerListIsInvalidOnInitialize()
-        external
-        givenANewlyDeployedContract
-        givenCallingInitialize
-    {
+    function test_RevertWhen_SignerListIsInvalidOnInitialize() external givenANewProxyContract givenCallingInitialize {
         // It should revert
         EmergencyMultisig.MultisigSettings memory settings = EmergencyMultisig.MultisigSettings({
             onlyListed: false,

diff --git a/test/EmergencyMultisig.t.yaml b/test/EmergencyMultisig.t.yaml
@@ -1,6 +1,9 @@
 EmergencyMultisigTest:
   # Plugin lifecycle
-  - given: a newly deployed contract
+  - when: deploying the contract
+    then:
+      - it: should disable the initializers
+  - given: a new proxy contract
     then:
       - given: calling initialize
         then:

diff --git a/test/Multisig.t.sol b/test/Multisig.t.sol
@@ -69,15 +69,33 @@ contract MultisigTest is AragonTest {
         ).build();
     }
 
-    modifier givenANewlyDeployedContract() {
+    function test_WhenDeployingTheContract() external {
+        // It should disable the initializers
+        Multisig.MultisigSettings memory settings = Multisig.MultisigSettings({
+            onlyListed: true,
+            minApprovals: 3,
+            destinationProposalDuration: 4 days,
+            signerList: signerList,
+            proposalExpirationPeriod: MULTISIG_PROPOSAL_EXPIRATION_PERIOD
+        });
+
+        // It should initialize the first time
+        multisig = new Multisig();
+
+        // It should refuse to initialize again
+        vm.expectRevert("Initializable: contract is already initialized");
+        multisig.initialize(dao, settings);
+    }
+
+    modifier givenANewProxyContract() {
         _;
     }
 
     modifier givenCallingInitialize() {
         _;
     }
 
-    function test_GivenCallingInitialize() external givenANewlyDeployedContract givenCallingInitialize {
+    function test_GivenCallingInitialize() external givenANewProxyContract givenCallingInitialize {
         Multisig.MultisigSettings memory settings = Multisig.MultisigSettings({
             onlyListed: true,
             minApprovals: 3,
@@ -169,7 +187,7 @@ contract MultisigTest is AragonTest {
 
     function test_RevertWhen_MinApprovalsIsGreaterThanSignerListLengthOnInitialize()
         external
-        givenANewlyDeployedContract
+        givenANewProxyContract
         givenCallingInitialize
     {
         // It should revert
@@ -223,7 +241,7 @@ contract MultisigTest is AragonTest {
 
     function test_RevertWhen_MinApprovalsIsZeroOnInitialize()
         external
-        givenANewlyDeployedContract
+        givenANewProxyContract
         givenCallingInitialize
     {
         // It should revert
@@ -277,7 +295,7 @@ contract MultisigTest is AragonTest {
 
     function test_RevertWhen_SignerListIsInvalidOnInitialize()
         external
-        givenANewlyDeployedContract
+        givenANewProxyContract
         givenCallingInitialize
     {
         // It should revert

diff --git a/test/Multisig.t.yaml b/test/Multisig.t.yaml
@@ -1,6 +1,9 @@
 MultisigTest:
   # Plugin lifecycle
-  - given: a newly deployed contract
+  - when: deploying the contract
+    then:
+      - it: should disable the initializers
+  - given: a new proxy contract
     then:
       - given: calling initialize
         then:

diff --git a/test/OptimisticTokenVotingPlugin.t.sol b/test/OptimisticTokenVotingPlugin.t.sol
@@ -56,6 +56,24 @@ contract OptimisticTokenVotingPluginTest is AragonTest {
         (dao, optimisticPlugin,,, votingToken,,, taikoL1) = builder.build();
     }
 
+    // Constructor
+    function test_ConstructorDisablesInitializers() public {
+        optimisticPlugin = new OptimisticTokenVotingPlugin();
+
+        OptimisticTokenVotingPlugin.OptimisticGovernanceSettings memory settings = OptimisticTokenVotingPlugin
+            .OptimisticGovernanceSettings({
+            minVetoRatio: uint32(RATIO_BASE / 10),
+            minDuration: 10 days,
+            timelockPeriod: 7 days,
+            l2InactivityPeriod: 10 minutes,
+            l2AggregationGracePeriod: 2 days,
+            skipL2: false
+        });
+
+        vm.expectRevert(bytes("Initializable: contract is already initialized"));
+        optimisticPlugin.initialize(dao, settings, votingToken, address(taikoL1), taikoBridge);
+    }
+
     // Initialize
     function test_InitializeRevertsIfInitialized() public {
         OptimisticTokenVotingPlugin.OptimisticGovernanceSettings memory settings = OptimisticTokenVotingPlugin

diff --git a/test/SignerList.t.sol b/test/SignerList.t.sol
@@ -54,6 +54,14 @@ contract SignerListTest is AragonTest {
     }
 
     function test_WhenDeployingTheContract() external {
+        // It should disable the initializers
+        signerList = new SignerList();
+
+        vm.expectRevert(bytes("Initializable: contract is already initialized"));
+        signerList.initialize(dao, signers);
+    }
+
+    function test_WhenCloningTheContract() external {
         // It should initialize normally
         signerList = SignerList(
             createProxyAndCall(address(SIGNER_LIST_BASE), abi.encodeCall(SignerList.initialize, (dao, signers)))
@@ -1042,6 +1050,10 @@ contract SignerListTest is AragonTest {
         assertEq(votingWallet, address(0), "Should be 0");
     }
 
+    modifier whenCallingGetEncryptionAgents() {
+        _;
+    }
+
     modifier whenCallingGetEncryptionRecipients() {
         _;
     }

diff --git a/test/SignerList.t.yaml b/test/SignerList.t.yaml
@@ -1,6 +1,9 @@
 SignerListTest:
   # contract lifecycle
   - when: deploying the contract
+    then:
+      - it: should disable the initializers
+  - when: cloning the contract
     then:
       - it: should initialize normally
   - given: a deployed contract