audit: Inconsistency in Quorum Modifiability (#205)

* fix: remove immutable keyword for quorum and emergency quorum * refactor: use initializer for emergencyQuorum * refactor: add setQuorum / setEmergencyQuorum and tests for EmergencyQuorum * refactor: add setQuorum for OptimisticQuorum * fix: fix quorum declaration in Script * fix: remove useless quorum encoding in Space test * add SetQuorum and tests to SimpleQuorumExecutionStrategy * chore: renamed emergency execution strategy file * chore: rename emergency quorum contract * chore: updated emergency quorum test --------- Co-authored-by: Orland0x <[email protected]>
snapshot-labs · Jun 15, 2023 · 5b87623 · 5b87623
1 parent c3c173f
commit 5b87623
Show file tree

Hide file tree

Showing 18 changed files with 238 additions and 30 deletions.
diff --git a/.forge-snapshots/ProposeSigComp.snap b/.forge-snapshots/ProposeSigComp.snap
@@ -1 +1 @@
-149973
+149997
diff --git a/.forge-snapshots/VoteSigComp.snap b/.forge-snapshots/VoteSigComp.snap
@@ -1 +1 @@
-50656
+50709
diff --git a/.forge-snapshots/VoteSigCompMetadata.snap b/.forge-snapshots/VoteSigCompMetadata.snap
@@ -1 +1 @@
-52207
+52252
diff --git a/.forge-snapshots/VoteTxComp.snap b/.forge-snapshots/VoteTxComp.snap
@@ -1 +1 @@
-44204
+44279
diff --git a/.forge-snapshots/VoteTxCompMetadata.snap b/.forge-snapshots/VoteTxCompMetadata.snap
@@ -1 +1 @@
-45795
+45863
diff --git a/script/ModulesDeployment.s.sol b/script/ModulesDeployment.s.sol
@@ -36,7 +36,8 @@ contract ModulesDeployment is Script {
         ethSigAuthenticator = new EthSigAuthenticator("snapshot-x", "0.1.0");
         ethTxAuthenticator = new EthTxAuthenticator();
         // TODO: set quorum prior to this deploy (or remove)
-        vanillaExecutionStrategy = new VanillaExecutionStrategy(1);
+        address deployerAddr = vm.addr(deployerPrivateKey);
+        vanillaExecutionStrategy = new VanillaExecutionStrategy(deployerAddr, 1);
         propositionPowerProposalValidationStrategy = new PropositionPowerProposalValidationStrategy();
         spaceFactory = new ProxyFactory();
         vm.stopBroadcast();

diff --git a/script/SpaceSetup.s.sol b/script/SpaceSetup.s.sol
@@ -45,12 +45,12 @@ contract SpaceSetup is Script {
         authenticators[1] = ethSigAuthenticator;
         authenticators[2] = ethTxAuthenticator;
         Strategy[] memory executionStrategies = new Strategy[](1);
+        quorum = 1;
         executionStrategies[0] = Strategy(vanillaExecutionStrategy, new bytes(quorum));
         votingDelay = 0;
         minVotingDuration = 0;
         maxVotingDuration = 1000;
         proposalThreshold = 1;
-        quorum = 1;
         votingPowerProposalValidationStrategy = Strategy(
             votingPowerProposalValidationContract,
             abi.encode(proposalThreshold, votingStrategies)

diff --git a/...on-strategies/EmergencyQuorumStrategy.sol → ...gies/EmergencyQuorumExecutionStrategy.sol b/...on-strategies/EmergencyQuorumStrategy.sol → ...gies/EmergencyQuorumExecutionStrategy.sol
@@ -4,14 +4,33 @@ pragma solidity ^0.8.18;
 
 import { IExecutionStrategy } from "../interfaces/IExecutionStrategy.sol";
 import { FinalizationStatus, Proposal, ProposalStatus } from "../types.sol";
+import { SpaceManager } from "../utils/SpaceManager.sol";
 
-abstract contract EmergencyQuorumStrategy is IExecutionStrategy {
-    uint256 public immutable quorum;
-    uint256 public immutable emergencyQuorum;
+abstract contract EmergencyQuorumExecutionStrategy is IExecutionStrategy, SpaceManager {
+    uint256 public quorum;
+    uint256 public emergencyQuorum;
 
-    constructor(uint256 _quorum, uint256 _emergencyQuorum) {
+    event QuorumUpdated(uint256 _quorum);
+    event EmergencyQuorumUpdated(uint256 _emergencyQuorum);
+
+    /// @dev Initializer
+    // solhint-disable-next-line func-name-mixedcase
+    function __EmergencyQuorumExecutionStrategy_init(
+        uint256 _quorum,
+        uint256 _emergencyQuorum
+    ) internal onlyInitializing {
+        quorum = _quorum;
+        emergencyQuorum = _emergencyQuorum;
+    }
+
+    function setQuorum(uint256 _quorum) external onlyOwner {
         quorum = _quorum;
+        emit QuorumUpdated(_quorum);
+    }
+
+    function setEmergencyQuorum(uint256 _emergencyQuorum) external onlyOwner {
         emergencyQuorum = _emergencyQuorum;
+        emit EmergencyQuorumUpdated(_emergencyQuorum);
     }
 
     function execute(

diff --git a/src/execution-strategies/OptimisticQuorumExecutionStrategy.sol b/src/execution-strategies/OptimisticQuorumExecutionStrategy.sol
@@ -8,6 +8,8 @@ import { SpaceManager } from "../utils/SpaceManager.sol";
 
 /// @title Optimistic Quorum Base Execution Strategy
 abstract contract OptimisticQuorumExecutionStrategy is IExecutionStrategy, SpaceManager {
+    event QuorumUpdated(uint256 newQuorum);
+
     /// @notice The quorum required to execute a proposal using this strategy.
     uint256 public quorum;
 
@@ -17,6 +19,11 @@ abstract contract OptimisticQuorumExecutionStrategy is IExecutionStrategy, Space
         quorum = _quorum;
     }
 
+    function setQuorum(uint256 _quorum) external onlyOwner {
+        quorum = _quorum;
+        emit QuorumUpdated(_quorum);
+    }
+
     function execute(
         Proposal memory proposal,
         uint256 votesFor,

diff --git a/src/execution-strategies/SimpleQuorumExecutionStrategy.sol b/src/execution-strategies/SimpleQuorumExecutionStrategy.sol
@@ -8,6 +8,8 @@ import { SpaceManager } from "../utils/SpaceManager.sol";
 
 /// @title Simple Quorum Base Execution Strategy
 abstract contract SimpleQuorumExecutionStrategy is IExecutionStrategy, SpaceManager {
+    event QuorumUpdated(uint256 newQuorum);
+
     /// @notice The quorum required to execute a proposal using this strategy.
     uint256 public quorum;
 
@@ -17,6 +19,11 @@ abstract contract SimpleQuorumExecutionStrategy is IExecutionStrategy, SpaceMana
         quorum = _quorum;
     }
 
+    function setQuorum(uint256 _quorum) external onlyOwner {
+        quorum = _quorum;
+        emit QuorumUpdated(_quorum);
+    }
+
     function execute(
         Proposal memory proposal,
         uint256 votesFor,

diff --git a/src/execution-strategies/VanillaExecutionStrategy.sol b/src/execution-strategies/VanillaExecutionStrategy.sol
@@ -9,8 +9,15 @@ import { Proposal, ProposalStatus } from "../types.sol";
 contract VanillaExecutionStrategy is SimpleQuorumExecutionStrategy {
     uint256 internal numExecuted;
 
-    constructor(uint256 _quorum) {
-        quorum = _quorum;
+    constructor(address _owner, uint256 _quorum) {
+        setUp(abi.encode(_owner, _quorum));
+    }
+
+    function setUp(bytes memory initParams) public initializer {
+        (address _owner, uint256 _quorum) = abi.decode(initParams, (address, uint256));
+        __Ownable_init();
+        transferOwnership(_owner);
+        __SimpleQuorumExecutionStrategy_init(_quorum);
     }
 
     function execute(

diff --git a/test/EmergencyQuorum.t.sol → test/EmergencyQuorumExecutionStrategy.t.sol b/test/EmergencyQuorum.t.sol → test/EmergencyQuorumExecutionStrategy.t.sol
@@ -4,13 +4,24 @@ pragma solidity ^0.8.18;
 
 import { SpaceTest } from "./utils/Space.t.sol";
 import { Choice, IndexedStrategy, Proposal, ProposalStatus, Strategy, UpdateSettingsCalldata } from "../src/types.sol";
-import { EmergencyQuorumStrategy } from "../src/execution-strategies/EmergencyQuorumStrategy.sol";
+import { EmergencyQuorumExecutionStrategy } from "../src/execution-strategies/EmergencyQuorumExecutionStrategy.sol";
 
-contract EmergencyQuorumExec is EmergencyQuorumStrategy {
+contract EmergencyQuorumExec is EmergencyQuorumExecutionStrategy {
     uint256 internal numExecuted;
 
-    // solhint-disable-next-line no-empty-blocks
-    constructor(uint256 _quorum, uint256 _emergencyQuorum) EmergencyQuorumStrategy(_quorum, _emergencyQuorum) {}
+    constructor(address _owner, uint256 _quorum, uint256 _emergencyQuorum) {
+        setUp(abi.encode(_owner, _quorum, _emergencyQuorum));
+    }
+
+    function setUp(bytes memory initParams) public initializer {
+        (address _owner, uint256 _quorum, uint256 _emergencyQuorum) = abi.decode(
+            initParams,
+            (address, uint256, uint256)
+        );
+        __Ownable_init();
+        transferOwnership(_owner);
+        __EmergencyQuorumExecutionStrategy_init(_quorum, _emergencyQuorum);
+    }
 
     function execute(
         Proposal memory proposal,
@@ -34,14 +45,17 @@ contract EmergencyQuorumExec is EmergencyQuorumStrategy {
 }
 
 contract EmergencyQuorumTest is SpaceTest {
+    event EmergencyQuorumUpdated(uint256 newEmergencyQuorum);
+    event QuorumUpdated(uint256 newQuorum);
+
     Strategy internal emergencyStrategy;
     uint256 internal emergencyQuorum = 2;
     EmergencyQuorumExec internal emergency;
 
     function setUp() public override {
         super.setUp();
 
-        emergency = new EmergencyQuorumExec(quorum, emergencyQuorum);
+        emergency = new EmergencyQuorumExec(owner, quorum, emergencyQuorum);
         emergencyStrategy = Strategy(address(emergency), new bytes(0));
 
         minVotingDuration = 100;
@@ -150,7 +164,7 @@ contract EmergencyQuorumTest is SpaceTest {
     }
 
     function testEmergencyQuorumLowerThanQuorum() public {
-        EmergencyQuorumExec emergencyQuorumExec = new EmergencyQuorumExec(quorum, quorum - 1);
+        EmergencyQuorumExec emergencyQuorumExec = new EmergencyQuorumExec(owner, quorum, quorum - 1);
 
         emergencyStrategy = Strategy(address(emergencyQuorumExec), new bytes(0));
 
@@ -222,4 +236,81 @@ contract EmergencyQuorumTest is SpaceTest {
     function testGetStrategyType() public {
         assertEq(emergency.getStrategyType(), "EmergencyQuorumExecution");
     }
+
+    function testEmergencyQuorumSetEmergencyQuorum() public {
+        uint256 newEmergencyQuorum = 4; // emergencyQuorum * 2
+
+        vm.expectEmit(true, true, true, true);
+        emit EmergencyQuorumUpdated(newEmergencyQuorum);
+        emergency.setEmergencyQuorum(newEmergencyQuorum);
+
+        uint256 proposalId = _createProposal(
+            author,
+            proposalMetadataURI,
+            emergencyStrategy,
+            abi.encode(userVotingStrategies)
+        );
+        _vote(author, proposalId, Choice.For, userVotingStrategies, voteMetadataURI); // 1
+        _vote(address(42), proposalId, Choice.For, userVotingStrategies, voteMetadataURI); // 2
+
+        // The proposal should not be executed because the new emergency quorum hasn't been reached yet.
+        vm.expectRevert(abi.encodeWithSelector(InvalidProposalStatus.selector, ProposalStatus.VotingPeriod));
+        space.execute(proposalId, emergencyStrategy.params);
+
+        _vote(address(43), proposalId, Choice.For, userVotingStrategies, voteMetadataURI); // 3
+        _vote(address(44), proposalId, Choice.For, userVotingStrategies, voteMetadataURI); // 4
+
+        // EmergencyQuorum has been reached; the proposal should get executed!
+        vm.expectEmit(true, true, true, true);
+        emit ProposalExecuted(proposalId);
+        space.execute(proposalId, emergencyStrategy.params);
+
+        assertEq(uint8(space.getProposalStatus(proposalId)), uint8(ProposalStatus.Executed));
+    }
+
+    function testEmergencyQuorumSetEmergencyQuorumUnauthorized() public {
+        uint256 newEmergencyQuorum = 4; // emergencyQuorum * 2
+        vm.prank(address(0xdeadbeef));
+        vm.expectRevert("Ownable: caller is not the owner");
+        emergency.setEmergencyQuorum(newEmergencyQuorum);
+    }
+
+    function testEmergencyQuorumSetQuorum() public {
+        uint256 newQuorum = quorum * 2; // 2
+
+        vm.expectEmit(true, true, true, true);
+        emit QuorumUpdated(newQuorum);
+        emergency.setQuorum(newQuorum);
+
+        uint256 proposalId = _createProposal(
+            author,
+            proposalMetadataURI,
+            emergencyStrategy,
+            abi.encode(userVotingStrategies)
+        );
+        _vote(author, proposalId, Choice.For, userVotingStrategies, voteMetadataURI); // 1
+
+        // Warp to the minimum voting duration
+        vm.warp(block.timestamp + minVotingDuration);
+
+        // The proposal should not be executed because the new emergency quorum hasn't been reached yet.
+        vm.expectRevert(abi.encodeWithSelector(InvalidProposalStatus.selector, ProposalStatus.VotingPeriod));
+        space.execute(proposalId, emergencyStrategy.params);
+
+        _vote(address(42), proposalId, Choice.For, userVotingStrategies, voteMetadataURI); // 2
+
+        // Quorum has been reached; the proposal should get executed!
+        vm.expectEmit(true, true, true, true);
+        emit ProposalExecuted(proposalId);
+        space.execute(proposalId, emergencyStrategy.params);
+
+        assertEq(uint8(space.getProposalStatus(proposalId)), uint8(ProposalStatus.Executed));
+    }
+
+    function testEmergencyQuorumSetQuorumUnauthorized() public {
+        uint256 newQuorum = quorum * 2; // 2
+        vm.prank(address(0xdeadbeef));
+        vm.expectRevert("Ownable: caller is not the owner");
+        emergency.setQuorum(newQuorum);
+    }
 }
diff --git a/test/EthTxAuthenticator.t.sol b/test/EthTxAuthenticator.t.sol
@@ -41,7 +41,7 @@ contract EthTxAuthenticatorTest is SpaceTest {
             )
         );
 
-        newStrategy = Strategy(address(new VanillaExecutionStrategy(quorum)), new bytes(0));
+        newStrategy = Strategy(address(new VanillaExecutionStrategy(owner, quorum)), new bytes(0));
     }
 
     function testAuthenticateTxPropose() public {

diff --git a/test/OptimisticQuorum.t.sol b/test/OptimisticQuorum.t.sol
@@ -8,12 +8,14 @@ import { Choice, IndexedStrategy, Proposal, ProposalStatus, Strategy } from "../
 
 // Dummy implementation of the optimistic quorum
 contract OptimisticExec is OptimisticQuorumExecutionStrategy {
-    constructor(uint256 _quorum) {
-        setUp(abi.encode(_quorum));
+    constructor(address _owner, uint256 _quorum) {
+        setUp(abi.encode(_owner, _quorum));
     }
 
     function setUp(bytes memory initParams) public initializer {
-        uint256 _quorum = abi.decode(initParams, (uint256));
+        (address _owner, uint256 _quorum) = abi.decode(initParams, (address, uint256));
+        __Ownable_init();
+        transferOwnership(_owner);
         __OptimisticQuorumExecutionStrategy_init(_quorum);
     }
 
@@ -41,14 +43,16 @@ contract OptimisticExec is OptimisticQuorumExecutionStrategy {
 }
 
 contract OptimisticTest is SpaceTest {
+    event QuorumUpdated(uint256 newQuorum);
+
     OptimisticExec internal optimisticQuorumStrategy;
 
     function setUp() public virtual override {
         super.setUp();
 
         // Update Quorum. Will need 2 `NO` votes in order to be rejected.
         quorum = 2;
-        optimisticQuorumStrategy = new OptimisticExec(quorum);
+        optimisticQuorumStrategy = new OptimisticExec(owner, quorum);
 
         executionStrategy = Strategy(address(optimisticQuorumStrategy), new bytes(0));
     }
@@ -132,7 +136,7 @@ contract OptimisticTest is SpaceTest {
         // SET A QUORUM OF 100
         {
             quorum = 100;
-            address optimisticQuorumStrategy2 = address(new OptimisticExec(quorum));
+            address optimisticQuorumStrategy2 = address(new OptimisticExec(owner, quorum));
             executionStrategy = Strategy(optimisticQuorumStrategy2, new bytes(0));
         }
 
@@ -157,4 +161,32 @@ contract OptimisticTest is SpaceTest {
 
         assertEq(uint8(space.getProposalStatus(proposalId)), uint8(ProposalStatus.Rejected));
     }
+
+    function testOptimisticQuorumSetQuorum() public {
+        uint256 newQuorum = quorum * 2; // 4
+
+        vm.expectEmit(true, true, true, true);
+        emit QuorumUpdated(newQuorum);
+        optimisticQuorumStrategy.setQuorum(newQuorum);
+
+        uint256 proposalId = _createProposal(author, proposalMetadataURI, executionStrategy, new bytes(0));
+
+        // Cast two votes against. This should be enough to trigger the old quorum but not the new one.
+        _vote(author, proposalId, Choice.Against, userVotingStrategies, voteMetadataURI);
+        _vote(address(42), proposalId, Choice.Against, userVotingStrategies, voteMetadataURI);
+        vm.warp(block.timestamp + space.maxVotingDuration());
+
+        // vm.expectEmit(true, true, true, true);
+        // emit ProposalExecuted(proposalId);
+        space.execute(proposalId, executionStrategy.params);
+
+        assertEq(uint8(space.getProposalStatus(proposalId)), uint8(ProposalStatus.Executed));
+    }
+
+    function testOptimisticQuorumSetQuorumUnauthorized() public {
+        uint256 newQuorum = quorum * 2; // 4
+        vm.prank(address(0xdeadbeef));
+        vm.expectRevert("Ownable: caller is not the owner");
+        optimisticQuorumStrategy.setQuorum(newQuorum);
+    }
 }
diff --git a/test/ProxyFactory.t.sol b/test/ProxyFactory.t.sol
@@ -40,14 +40,14 @@ contract SpaceFactoryTest is Test, IProxyFactoryEvents, IProxyFactoryErrors, ISp
     string public proposalValidationStrategyMetadataURI;
 
     function setUp() public {
+        owner = address(1);
         masterSpace = new Space();
         factory = new ProxyFactory();
         vanillaVotingStrategy = new VanillaVotingStrategy();
         vanillaAuthenticator = new VanillaAuthenticator();
-        vanillaExecutionStrategy = new VanillaExecutionStrategy(quorum);
+        vanillaExecutionStrategy = new VanillaExecutionStrategy(owner, quorum);
         vanillaProposalValidationStrategy = new VanillaProposalValidationStrategy();
 
-        owner = address(1);
         votingDelay = 0;
         minVotingDuration = 0;
         maxVotingDuration = 1000;
-Original file line number
+Diff line change
@@ Expand Up / @@ -41,7 +41,7 @@ contract EthTxAuthenticatorTest is SpaceTest { @@
                 )
             );
-            newStrategy = Strategy(address(new VanillaExecutionStrategy(quorum)), new bytes(0));
+            newStrategy = Strategy(address(new VanillaExecutionStrategy(owner, quorum)), new bytes(0));
         }
         function testAuthenticateTxPropose() public {
@@ Expand Down @@