Improve security of the onlyGovernance modifier

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,22 @@ 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 Restrict access to governor executing address. Some module might override the _executor function to make
+     * sure this modifier is consistant with the execution model.
      */
     modifier onlyGovernance() {
         require(_msgSender() == _executor(), "Governor: onlyGovernance");
+        if (_executor() != address(this)) {
+            // loop until poping the expected operation - throw if deque is empty (operation not authorized)
+            while (_governanceCall.popFront() != keccak256(msg.data)) {}
+        }
         _;
     }
 
@@ -251,7 +262,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 +286,44 @@ 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)) {
+            for (uint256 i = 0; i < targets.length; ++i) {
+                if (targets[i] == address(this)) {
+                    _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.

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 = {