Improve security of the onlyGovernance modifier

contracts/governance/Governor.sol

@@ -9,6 +9,7 @@ import "../utils/introspection/ERC165.sol";
 import "../utils/math/SafeCast.sol";
 import "../utils/Address.sol";
 import "../utils/Context.sol";
+import "../utils/Counters.sol";
 import "../utils/Timers.sol";
 import "./IGovernor.sol";
 
@@ -26,6 +27,7 @@ import "./IGovernor.sol";
 abstract contract Governor is Context, ERC165, EIP712, IGovernor {
     using SafeCast for uint256;
     using Timers for Timers.BlockNumber;
+    using Counters for Counters.Counter;
 
     bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,uint8 support)");
 
@@ -40,12 +42,21 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
 
     mapping(uint256 => ProposalCore) private _proposals;
 
+    // This mapping keeps track of the governor operating on itself. Calls to functions protected by the
+    // {onlyGovernance} modifier needs to be whitelisted in this mapping. Whitelisting is set in {_beforeExecute},
+    // and reset in {_afterExecute}. This ensures that the execution of {onlyGovernance} protected calls can only
+    // be achieved through successful proposals.
+    mapping(bytes => Counters.Counter) private _governanceCall;
+
     /**
      * @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)) {
+            _governanceCall[msg.data].decrement();
+        }
         _;
     }
 
@@ -250,7 +261,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;
     }
@@ -272,6 +285,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[calldatas[i]].increment();
+                }
+            }
+        }
+    }
+
+    /**
+     * @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[calldatas[i]].reset();
+                }
+            }
+        }
+    }
+
     /**
      * @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 = {