audit: disable timelock in constructor (#224)

* refactor: constructor disables timelock * chore: remove non proxy timelock tests * chore: remove constructor params --------- Co-authored-by: Orlando <[email protected]>
snapshot-labs · Jun 27, 2023 · 3bc77f3 · 3bc77f3
1 parent 9f5f1fe
commit 3bc77f3
Show file tree

Hide file tree

Showing 2 changed files with 6 additions and 38 deletions.
diff --git a/src/execution-strategies/TimelockExecutionStrategy.sol b/src/execution-strategies/TimelockExecutionStrategy.sol
@@ -81,20 +81,11 @@ contract TimelockExecutionStrategy is SimpleQuorumExecutionStrategy, IERC1155Rec
     ///         renounce ownership of the contract while still maintaining a veto guardian.
     address public vetoGuardian;
 
-    /// @notice Constructor
-    /// @param _owner Address of the owner of this contract.
-    /// @param _vetoGuardian Address of the veto guardian.
-    /// @param _spaces Array of whitelisted space contracts.
-    /// @param _timelockDelay The timelock delay in seconds.
-    /// @param _quorum The quorum required to execute a proposal.
-    constructor(
-        address _owner,
-        address _vetoGuardian,
-        address[] memory _spaces,
-        uint256 _timelockDelay,
-        uint256 _quorum
-    ) {
-        setUp(abi.encode(_owner, _vetoGuardian, _spaces, _timelockDelay, _quorum));
+    /// @notice Constructor.
+    /// @dev We enforce implementations of this contract to be disabled as a security measure to prevent delegate
+    ///      calls to the SELFDESTRUCT opcode, irrecoverably disabling all the proxies using that implementation.
+    constructor() {
+        setUp(abi.encode(address(1), address(1), new address[](0), 0, 0));
     }
 
     /// @notice Initialization function, should be called immediately after deploying a new proxy to this contract.

diff --git a/test/TimelockExecutionStrategy.t.sol b/test/TimelockExecutionStrategy.t.sol
@@ -508,10 +508,6 @@ abstract contract TimelockExecutionStrategyTest is SpaceTest {
     }
 
     function testSetUp() public {
-        address[] memory spaces = new address[](1);
-        spaces[0] = address(space);
-        timelockExecutionStrategy = new TimelockExecutionStrategy(owner, vetoGuardian, spaces, 1000, quorum);
-
         assertEq(timelockExecutionStrategy.owner(), owner);
         assertEq(timelockExecutionStrategy.vetoGuardian(), vetoGuardian);
         assertEq(timelockExecutionStrategy.quorum(), quorum);
@@ -520,33 +516,14 @@ abstract contract TimelockExecutionStrategyTest is SpaceTest {
     }
 }
 
-contract TimelockExecutionStrategyTestDirect is TimelockExecutionStrategyTest {
-    function setUp() public override {
-        super.setUp();
-
-        address[] memory spaces = new address[](1);
-        spaces[0] = address(space);
-
-        timelockExecutionStrategy = new TimelockExecutionStrategy(owner, vetoGuardian, spaces, 1000, quorum);
-        vm.deal(address(owner), 1000);
-        payable(timelockExecutionStrategy).transfer(1000);
-    }
-}
-
 contract TimelockExecutionStrategyTestProxy is TimelockExecutionStrategyTest {
     function setUp() public override {
         super.setUp();
 
         address[] memory spaces = new address[](1);
         spaces[0] = address(space);
         address[] memory emptyArray = new address[](1);
-        TimelockExecutionStrategy masterExecutionStrategy = new TimelockExecutionStrategy(
-            address(1),
-            address(0),
-            emptyArray,
-            0,
-            0
-        );
+        TimelockExecutionStrategy masterExecutionStrategy = new TimelockExecutionStrategy();
 
         timelockExecutionStrategy = TimelockExecutionStrategy(
             payable(