From f4e463297a677a25f14cf2525808219efbdfd817 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8r=E2=88=82=C2=A1?= Date: Wed, 11 Dec 2024 17:35:45 +0100 Subject: [PATCH] Ensuring that constructors leave disabled proxy initializers --- src/EmergencyMultisig.sol | 5 ++++ src/Multisig.sol | 5 ++++ src/OptimisticTokenVotingPlugin.sol | 5 ++++ test/EmergencyMultisig.t.sol | 34 ++++++++++++++++---------- test/EmergencyMultisig.t.yaml | 5 +++- test/Multisig.t.sol | 28 +++++++++++++++++---- test/Multisig.t.yaml | 5 +++- test/OptimisticTokenVotingPlugin.t.sol | 18 ++++++++++++++ test/SignerList.t.sol | 12 +++++++++ test/SignerList.t.yaml | 3 +++ 10 files changed, 100 insertions(+), 20 deletions(-) diff --git a/src/EmergencyMultisig.sol b/src/EmergencyMultisig.sol index 5d25d41..802684d 100644 --- 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 index a326237..8d234c1 100644 --- 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 index 2ecf6e8..ed2d091 100644 --- 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 index 0571fe9..268c4b7 100644 --- a/test/EmergencyMultisig.t.sol +++ b/test/EmergencyMultisig.t.sol @@ -69,7 +69,23 @@ 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() { _; } @@ -77,7 +93,7 @@ contract EmergencyMultisigTest is AragonTest { _; } - 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 index 0209798..6781093 100644 --- 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 index 3f56c83..ca6eb8f 100644 --- a/test/Multisig.t.sol +++ b/test/Multisig.t.sol @@ -69,7 +69,25 @@ 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() { _; } @@ -77,7 +95,7 @@ contract MultisigTest is AragonTest { _; } - 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 index cad10df..ef7cb01 100644 --- 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 index 721c535..efab3bd 100644 --- 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 index 0a636d3..9fc5d82 100644 --- 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 index 732f732..f05e87d 100644 --- 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