Improve security of the onlyGovernance modifier (#3147)

* add a protection mechanism to prevent relaying transaction that are not
part of an execute operation

* more accurate relay authorization

* force reset the relay authorizations after executions

* refactor of the onlyGovernor modifier

* only whitelist when executor is not governor itself

* fix lint

* add private function for call permission management

* use deque

* fix lint

* remove unecessary dependency

* remove unecessary dependency

* comment rephrasing

* Update contracts/governance/Governor.sol

Co-authored-by: Francisco Giordano <frangio.1@gmail.com>

* cache keccak256(_msgData())

* use Context

* lint

* conditionnal clear

* add test to cover queue.clear()

* lint

* write more extended docs for onlyGovernance

* add changelog entry

Co-authored-by: Francisco Giordano <frangio.1@gmail.com>

CHANGELOG.md

@@ -6,6 +6,7 @@
  * `EnumerableMap`: add new `AddressToUintMap` map type. ([#3150](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3150))
  * `ERC1155`: Add a `_afterTokenTransfer` hook for improved extensibility. ([#3166](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3166))
  * `DoubleEndedQueue`: a new data structure that supports efficient push and pop to both front and back, useful for FIFO and LIFO queues. ([#3153](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3153))
+ * `Governor`: improved security of `onlyGovernance` modifier when using an external executor contract (e.g. a timelock) that can operate without necessarily going through the governance protocol. ([#3147](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3147))
 
 ## 4.5.0 (2022-02-09)
 

contracts/governance/Governor.sol

@@ -7,6 +7,7 @@ import "../utils/cryptography/ECDSA.sol";
 import "../utils/cryptography/draft-EIP712.sol";
 import "../utils/introspection/ERC165.sol";
 import "../utils/math/SafeCast.sol";
+import "../utils/structs/DoubleEndedQueue.sol";
 import "../utils/Address.sol";
 import "../utils/Context.sol";
 import "../utils/Timers.sol";
@@ -24,6 +25,7 @@ import "./IGovernor.sol";
  * _Available since v4.3._
  */
 abstract contract Governor is Context, ERC165, EIP712, IGovernor {
+    using DoubleEndedQueue for DoubleEndedQueue.Bytes32Deque;
     using SafeCast for uint256;
     using Timers for Timers.BlockNumber;
 
@@ -40,13 +42,29 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
 
     mapping(uint256 => ProposalCore) private _proposals;
 
+    // This queue keeps track of the governor operating on itself. Calls to functions protected by the
+    // {onlyGovernance} modifier needs to be whitelisted in this queue. Whitelisting is set in {_beforeExecute},
+    // consummed by the {onlyGovernance} modifier and eventually reset in {_afterExecute}. This ensures that the
+    // execution of {onlyGovernance} protected calls can only be achieved through successful proposals.
+    DoubleEndedQueue.Bytes32Deque private _governanceCall;
+
     /**
-     * @dev Restrict access of functions to the governance executor, which may be the Governor itself or a timelock
-     * contract, as specified by {_executor}. This generally means that function with this modifier must be voted on and
-     * executed through the governance protocol.
+     * @dev Restricts a function so it can only be executed through governance proposals. For example, governance
+     * parameter setters in {GovernorSettings} are protected using this modifier.
+     *
+     * The governance executing address may be different from the Governor's own address, for example it could be a
+     * timelock. This can be customized by modules by overriding {_executor}. The executor is only able to invoke these
+     * functions during the execution of the governor's {execute} function, and not under any other circumstances. Thus,
+     * for example, additional timelock proposers are not able to change governance parameters without going through the
+     * governance protocol (since v4.6).
      */
     modifier onlyGovernance() {
         require(_msgSender() == _executor(), "Governor: onlyGovernance");
+        if (_executor() != address(this)) {
+            bytes32 msgDataHash = keccak256(_msgData());
+            // loop until poping the expected operation - throw if deque is empty (operation not authorized)
+            while (_governanceCall.popFront() != msgDataHash) {}
+        }
         _;
     }
 
@@ -197,7 +215,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
         string memory description
     ) public virtual override returns (uint256) {
         require(
-            getVotes(msg.sender, block.number - 1) >= proposalThreshold(),
+            getVotes(_msgSender(), block.number - 1) >= proposalThreshold(),
             "GovernorCompatibilityBravo: proposer votes below proposal threshold"
         );
 
@@ -251,7 +269,9 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
 
         emit ProposalExecuted(proposalId);
 
+        _beforeExecute(proposalId, targets, values, calldatas, descriptionHash);
         _execute(proposalId, targets, values, calldatas, descriptionHash);
+        _afterExecute(proposalId, targets, values, calldatas, descriptionHash);
 
         return proposalId;
     }
@@ -273,6 +293,42 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
         }
     }
 
+    /**
+     * @dev Hook before execution is trigerred.
+     */
+    function _beforeExecute(
+        uint256, /* proposalId */
+        address[] memory targets,
+        uint256[] memory, /* values */
+        bytes[] memory calldatas,
+        bytes32 /*descriptionHash*/
+    ) internal virtual {
+        if (_executor() != address(this)) {
+            for (uint256 i = 0; i < targets.length; ++i) {
+                if (targets[i] == address(this)) {
+                    _governanceCall.pushBack(keccak256(calldatas[i]));
+                }
+            }
+        }
+    }
+
+    /**
+     * @dev Hook after execution is trigerred.
+     */
+    function _afterExecute(
+        uint256, /* proposalId */
+        address[] memory, /* targets */
+        uint256[] memory, /* values */
+        bytes[] memory, /* calldatas */
+        bytes32 /*descriptionHash*/
+    ) internal virtual {
+        if (_executor() != address(this)) {
+            if (!_governanceCall.empty()) {
+                _governanceCall.clear();
+            }
+        }
+    }
+
     /**
      * @dev Internal cancel mechanism: locks up the proposal timer, preventing it from being re-submitted. Marks it as
      * canceled to allow distinguishing it from executed proposals.

contracts/mocks/GovernorTimelockControlMock.sol

@@ -105,4 +105,6 @@ contract GovernorTimelockControlMock is
     function _executor() internal view virtual override(Governor, GovernorTimelockControl) returns (address) {
         return super._executor();
     }
+
+    function nonGovernanceFunction() external {}
 }

test/governance/extensions/GovernorTimelockControl.test.js

@@ -1,4 +1,4 @@
-const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers');
+const { constants, expectEvent, expectRevert, time } = require('@openzeppelin/test-helpers');
 const { expect } = require('chai');
 const Enums = require('../../helpers/enums');
 
@@ -31,7 +31,7 @@ contract('GovernorTimelockControl', function (accounts) {
     this.timelock = await Timelock.new(3600, [], []);
     this.mock = await Governor.new(name, this.token.address, 4, 16, this.timelock.address, 0);
     this.receiver = await CallReceiver.new();
-    // normal setup: governor is proposer, everyone is executor, timelock is its own admin
+    // normal setup: governor and admin are proposers, everyone is executor, timelock is its own admin
     await this.timelock.grantRole(await this.timelock.PROPOSER_ROLE(), this.mock.address);
     await this.timelock.grantRole(await this.timelock.PROPOSER_ROLE(), admin);
     await this.timelock.grantRole(await this.timelock.EXECUTOR_ROLE(), constants.ZERO_ADDRESS);
@@ -338,6 +338,32 @@ contract('GovernorTimelockControl', function (accounts) {
       );
     });
 
+    it('protected against other proposers', async function () {
+      await this.timelock.schedule(
+        this.mock.address,
+        web3.utils.toWei('0'),
+        this.mock.contract.methods.relay(...this.call).encodeABI(),
+        constants.ZERO_BYTES32,
+        constants.ZERO_BYTES32,
+        3600,
+        { from: admin },
+      );
+
+      await time.increase(3600);
+
+      await expectRevert(
+        this.timelock.execute(
+          this.mock.address,
+          web3.utils.toWei('0'),
+          this.mock.contract.methods.relay(...this.call).encodeABI(),
+          constants.ZERO_BYTES32,
+          constants.ZERO_BYTES32,
+          { from: admin },
+        ),
+        'TimelockController: underlying transaction reverted',
+      );
+    });
+
     describe('using workflow', function () {
       beforeEach(async function () {
         this.settings = {
@@ -461,4 +487,33 @@ contract('GovernorTimelockControl', function (accounts) {
       runGovernorWorkflow();
     });
   });
+
+  describe('clear queue of pending governor calls', function () {
+    beforeEach(async function () {
+      this.settings = {
+        proposal: [
+          [ this.mock.address ],
+          [ web3.utils.toWei('0') ],
+          [ this.mock.contract.methods.nonGovernanceFunction().encodeABI() ],
+          '<proposal description>',
+        ],
+        voters: [
+          { voter: voter, support: Enums.VoteType.For },
+        ],
+        steps: {
+          queue: { delay: 3600 },
+        },
+      };
+    });
+
+    afterEach(async function () {
+      expectEvent(
+        this.receipts.execute,
+        'ProposalExecuted',
+        { proposalId: this.id },
+      );
+    });
+
+    runGovernorWorkflow();
+  });
 });