From b413688327a8f170a41032ef1751174ccd9dac1b Mon Sep 17 00:00:00 2001
From: Hadrien Croubois <hadrien.croubois@gmail.com>
Date: Thu, 27 Jan 2022 11:36:59 +0100
Subject: [PATCH 01/21] add a protection mechanism to prevent relaying
 transaction that are not part of an execute operation

---
 contracts/governance/Governor.sol             | 40 +++++++++++++++++++
 .../GovernorTimelockControl.test.js           | 30 +++++++++++++-
 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
index 19a930ddb5d..efd4fd7ea3d 100644
--- a/contracts/governance/Governor.sol
+++ b/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)");
 
@@ -39,6 +41,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
     string private _name;
 
     mapping(uint256 => ProposalCore) private _proposals;
+    mapping(bytes => Counters.Counter) private _relayAuthorized;
 
     /**
      * @dev Restrict access to governor executing address. Some module might override the _executor function to make
@@ -250,7 +253,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 +277,40 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
         }
     }
 
+    /**
+     * @dev Hook before execution is trigerred. Authorizes relaying.
+     */
+    function _beforeExecute(
+        uint256, /* proposalId */
+        address[] memory targets,
+        uint256[] memory /* values */,
+        bytes[] memory calldatas,
+        bytes32 /*descriptionHash*/
+    ) internal virtual {
+        for (uint256 i = 0; i < targets.length; ++i) {
+            if (targets[i] == address(this)) {
+                _relayAuthorized[calldatas[i]].increment();
+            }
+        }
+    }
+
+    /**
+     * @dev Hook before execution is trigerred. Authorizes relaying.
+     */
+    function _afterExecute(
+        uint256, /* proposalId */
+        address[] memory targets,
+        uint256[] memory /* values */,
+        bytes[] memory calldatas,
+        bytes32 /*descriptionHash*/
+    ) internal virtual {
+        for (uint256 i = 0; i < targets.length; ++i) {
+            if (targets[i] == address(this)) {
+                _relayAuthorized[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.
@@ -371,6 +410,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
         uint256 value,
         bytes calldata data
     ) external virtual onlyGovernance {
+        _relayAuthorized[msg.data].decrement();
         Address.functionCallWithValue(target, data, value);
     }
 
diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js
index 35242740b48..e15c4e2bfba 100644
--- a/test/governance/extensions/GovernorTimelockControl.test.js
+++ b/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 = {

From e5d80c75afce6762bc26b1769b487296c6af7885 Mon Sep 17 00:00:00 2001
From: Hadrien Croubois <hadrien.croubois@gmail.com>
Date: Thu, 27 Jan 2022 12:16:38 +0100
Subject: [PATCH 02/21] more accurate relay authorization

---
 contracts/governance/Governor.sol | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
index efd4fd7ea3d..831d4b112b9 100644
--- a/contracts/governance/Governor.sol
+++ b/contracts/governance/Governor.sol
@@ -13,6 +13,12 @@ import "../utils/Counters.sol";
 import "../utils/Timers.sol";
 import "./IGovernor.sol";
 
+function extractSelector(bytes memory data) returns (bytes4 selector) {
+    assembly {
+        selector := mload(add(data, 0x20))
+    }
+}
+
 /**
  * @dev Core of the governance system, designed to be extended though various modules.
  *
@@ -278,7 +284,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
     }
 
     /**
-     * @dev Hook before execution is trigerred. Authorizes relaying.
+     * @dev Hook before execution is trigerred.
      */
     function _beforeExecute(
         uint256, /* proposalId */
@@ -288,14 +294,14 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
         bytes32 /*descriptionHash*/
     ) internal virtual {
         for (uint256 i = 0; i < targets.length; ++i) {
-            if (targets[i] == address(this)) {
+            if (targets[i] == address(this) && extractSelector(calldatas[i]) == this.relay.selector) {
                 _relayAuthorized[calldatas[i]].increment();
             }
         }
     }
 
     /**
-     * @dev Hook before execution is trigerred. Authorizes relaying.
+     * @dev Hook after execution is trigerred.
      */
     function _afterExecute(
         uint256, /* proposalId */
@@ -304,11 +310,6 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
         bytes[] memory calldatas,
         bytes32 /*descriptionHash*/
     ) internal virtual {
-        for (uint256 i = 0; i < targets.length; ++i) {
-            if (targets[i] == address(this)) {
-                _relayAuthorized[calldatas[i]].reset();
-            }
-        }
     }
 
     /**

From 52b2ea2ca4c3fd0eaf105140ba8b476cc0213a40 Mon Sep 17 00:00:00 2001
From: Hadrien Croubois <hadrien.croubois@gmail.com>
Date: Thu, 27 Jan 2022 17:27:22 +0100
Subject: [PATCH 03/21] force reset the relay authorizations after executions

---
 contracts/governance/Governor.sol | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
index 831d4b112b9..7a966205248 100644
--- a/contracts/governance/Governor.sol
+++ b/contracts/governance/Governor.sol
@@ -310,6 +310,11 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
         bytes[] memory calldatas,
         bytes32 /*descriptionHash*/
     ) internal virtual {
+        for (uint256 i = 0; i < targets.length; ++i) {
+            if (targets[i] == address(this) && extractSelector(calldatas[i]) == this.relay.selector) {
+                _relayAuthorized[calldatas[i]].reset();
+            }
+        }
     }
 
     /**

From 052fa2043a9178c405b61b334a3f4cccdc4c1601 Mon Sep 17 00:00:00 2001
From: Hadrien Croubois <hadrien.croubois@gmail.com>
Date: Fri, 28 Jan 2022 15:36:26 +0100
Subject: [PATCH 04/21] refactor of the onlyGovernor modifier

---
 contracts/governance/Governor.sol | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
index 7a966205248..7975b905c2a 100644
--- a/contracts/governance/Governor.sol
+++ b/contracts/governance/Governor.sol
@@ -13,12 +13,6 @@ import "../utils/Counters.sol";
 import "../utils/Timers.sol";
 import "./IGovernor.sol";
 
-function extractSelector(bytes memory data) returns (bytes4 selector) {
-    assembly {
-        selector := mload(add(data, 0x20))
-    }
-}
-
 /**
  * @dev Core of the governance system, designed to be extended though various modules.
  *
@@ -47,7 +41,12 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
     string private _name;
 
     mapping(uint256 => ProposalCore) private _proposals;
-    mapping(bytes => Counters.Counter) private _relayAuthorized;
+
+    // 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
@@ -55,6 +54,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
      */
     modifier onlyGovernance() {
         require(_msgSender() == _executor(), "Governor: onlyGovernance");
+        _governanceCall[msg.data].decrement();
         _;
     }
 
@@ -294,8 +294,8 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
         bytes32 /*descriptionHash*/
     ) internal virtual {
         for (uint256 i = 0; i < targets.length; ++i) {
-            if (targets[i] == address(this) && extractSelector(calldatas[i]) == this.relay.selector) {
-                _relayAuthorized[calldatas[i]].increment();
+            if (targets[i] == address(this)) {
+                _governanceCall[calldatas[i]].increment();
             }
         }
     }
@@ -311,8 +311,8 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
         bytes32 /*descriptionHash*/
     ) internal virtual {
         for (uint256 i = 0; i < targets.length; ++i) {
-            if (targets[i] == address(this) && extractSelector(calldatas[i]) == this.relay.selector) {
-                _relayAuthorized[calldatas[i]].reset();
+            if (targets[i] == address(this)) {
+                _governanceCall[calldatas[i]].reset();
             }
         }
     }
@@ -416,7 +416,6 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
         uint256 value,
         bytes calldata data
     ) external virtual onlyGovernance {
-        _relayAuthorized[msg.data].decrement();
         Address.functionCallWithValue(target, data, value);
     }
 

From b25723944484a74abcad75dbe1ff349451e40b2c Mon Sep 17 00:00:00 2001
From: Francisco Giordano <frangio.1@gmail.com>
Date: Fri, 28 Jan 2022 13:01:33 -0300
Subject: [PATCH 05/21] only whitelist when executor is not governor itself

---
 contracts/governance/Governor.sol | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
index 7975b905c2a..20c5c1d71f5 100644
--- a/contracts/governance/Governor.sol
+++ b/contracts/governance/Governor.sol
@@ -54,7 +54,9 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
      */
     modifier onlyGovernance() {
         require(_msgSender() == _executor(), "Governor: onlyGovernance");
-        _governanceCall[msg.data].decrement();
+        if (_executor() != address(this)) {
+            _governanceCall[msg.data].decrement();
+        }
         _;
     }
 
@@ -293,9 +295,11 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
         bytes[] memory calldatas,
         bytes32 /*descriptionHash*/
     ) internal virtual {
-        for (uint256 i = 0; i < targets.length; ++i) {
-            if (targets[i] == address(this)) {
-                _governanceCall[calldatas[i]].increment();
+        if (_executor() != address(this)) {
+            for (uint256 i = 0; i < targets.length; ++i) {
+                if (targets[i] == address(this)) {
+                    _governanceCall[calldatas[i]].increment();
+                }
             }
         }
     }
@@ -310,9 +314,11 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
         bytes[] memory calldatas,
         bytes32 /*descriptionHash*/
     ) internal virtual {
-        for (uint256 i = 0; i < targets.length; ++i) {
-            if (targets[i] == address(this)) {
-                _governanceCall[calldatas[i]].reset();
+        if (_executor() != address(this)) {
+            for (uint256 i = 0; i < targets.length; ++i) {
+                if (targets[i] == address(this)) {
+                    _governanceCall[calldatas[i]].reset();
+                }
             }
         }
     }

From 6a61fcc0e53826fa8d5ba4185d87fa109145fa0d Mon Sep 17 00:00:00 2001
From: Hadrien Croubois <hadrien.croubois@gmail.com>
Date: Fri, 28 Jan 2022 18:10:54 +0100
Subject: [PATCH 06/21] fix lint

---
 test/governance/extensions/GovernorTimelockControl.test.js | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js
index e15c4e2bfba..2f840096757 100644
--- a/test/governance/extensions/GovernorTimelockControl.test.js
+++ b/test/governance/extensions/GovernorTimelockControl.test.js
@@ -346,7 +346,7 @@ contract('GovernorTimelockControl', function (accounts) {
         constants.ZERO_BYTES32,
         constants.ZERO_BYTES32,
         3600,
-        { from: admin},
+        { from: admin },
       );
 
       await time.increase(3600);
@@ -358,7 +358,7 @@ contract('GovernorTimelockControl', function (accounts) {
           this.mock.contract.methods.relay(...this.call).encodeABI(),
           constants.ZERO_BYTES32,
           constants.ZERO_BYTES32,
-          { from: admin},
+          { from: admin },
         ),
         'TimelockController: underlying transaction reverted',
       );

From 84a1dd19fea74e4970742f1007b0ab4bd2d05fe1 Mon Sep 17 00:00:00 2001
From: Hadrien Croubois <hadrien.croubois@gmail.com>
Date: Fri, 28 Jan 2022 21:14:04 +0100
Subject: [PATCH 07/21] add private function for call permission management

---
 contracts/governance/Governor.sol | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
index 20c5c1d71f5..4112e4f15b6 100644
--- a/contracts/governance/Governor.sol
+++ b/contracts/governance/Governor.sol
@@ -55,7 +55,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
     modifier onlyGovernance() {
         require(_msgSender() == _executor(), "Governor: onlyGovernance");
         if (_executor() != address(this)) {
-            _governanceCall[msg.data].decrement();
+            _useCallPermission(msg.data);
         }
         _;
     }
@@ -298,7 +298,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
         if (_executor() != address(this)) {
             for (uint256 i = 0; i < targets.length; ++i) {
                 if (targets[i] == address(this)) {
-                    _governanceCall[calldatas[i]].increment();
+                    _addCallPermission(calldatas[i]);
                 }
             }
         }
@@ -317,7 +317,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
         if (_executor() != address(this)) {
             for (uint256 i = 0; i < targets.length; ++i) {
                 if (targets[i] == address(this)) {
-                    _governanceCall[calldatas[i]].reset();
+                    _clearCallPermission(calldatas[i]);
                 }
             }
         }
@@ -432,4 +432,16 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
     function _executor() internal view virtual returns (address) {
         return address(this);
     }
+
+    function _addCallPermission(bytes memory call) private {
+        _governanceCall[call].increment();
+    }
+
+    function _useCallPermission(bytes memory call) private {
+        _governanceCall[call].decrement();
+    }
+
+    function _clearCallPermission(bytes memory call) private {
+        _governanceCall[call].reset();
+    }
 }

From ac524df2f4d5d83b9de2c70fc4b6a5d1e6b744ab Mon Sep 17 00:00:00 2001
From: Hadrien Croubois <hadrien.croubois@gmail.com>
Date: Wed, 16 Feb 2022 12:20:32 +0100
Subject: [PATCH 08/21] use deque

---
 contracts/governance/Governor.sol | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
index 091c397eaa7..6e67db42c10 100644
--- a/contracts/governance/Governor.sol
+++ b/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/Counters.sol";
@@ -25,9 +26,10 @@ import "./IGovernor.sol";
  * _Available since v4.3._
  */
 abstract contract Governor is Context, ERC165, EIP712, IGovernor {
+    using Counters for Counters.Counter;
+    using DoubleEndedQueue for DoubleEndedQueue.Bytes32Deque;
     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)");
 
@@ -46,17 +48,17 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
     // {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;
+    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)) {
-            _useCallPermission(msg.data);
+            // loop until poping the expected operation - throw if deque is empty (operation not authorized)
+            while (_governanceCall.popFront() != keccak256(msg.data)) {}
         }
         _;
     }
@@ -299,7 +301,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
         if (_executor() != address(this)) {
             for (uint256 i = 0; i < targets.length; ++i) {
                 if (targets[i] == address(this)) {
-                    _addCallPermission(calldatas[i]);
+                    _governanceCall.pushBack(keccak256(calldatas[i]));
                 }
             }
         }
@@ -318,7 +320,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
         if (_executor() != address(this)) {
             for (uint256 i = 0; i < targets.length; ++i) {
                 if (targets[i] == address(this)) {
-                    _clearCallPermission(calldatas[i]);
+                    _governanceCall.clear();
                 }
             }
         }
@@ -433,16 +435,4 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
     function _executor() internal view virtual returns (address) {
         return address(this);
     }
-
-    function _addCallPermission(bytes memory call) private {
-        _governanceCall[call].increment();
-    }
-
-    function _useCallPermission(bytes memory call) private {
-        _governanceCall[call].decrement();
-    }
-
-    function _clearCallPermission(bytes memory call) private {
-        _governanceCall[call].reset();
-    }
 }

From 1f4896fe748279dc734ba5a0b4a1c3c9313039a8 Mon Sep 17 00:00:00 2001
From: Hadrien Croubois <hadrien.croubois@gmail.com>
Date: Wed, 16 Feb 2022 14:09:28 +0100
Subject: [PATCH 09/21] fix lint

---
 contracts/governance/Governor.sol | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
index 6e67db42c10..ab442658ea1 100644
--- a/contracts/governance/Governor.sol
+++ b/contracts/governance/Governor.sol
@@ -294,7 +294,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
     function _beforeExecute(
         uint256, /* proposalId */
         address[] memory targets,
-        uint256[] memory /* values */,
+        uint256[] memory, /* values */
         bytes[] memory calldatas,
         bytes32 /*descriptionHash*/
     ) internal virtual {
@@ -313,8 +313,8 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
     function _afterExecute(
         uint256, /* proposalId */
         address[] memory targets,
-        uint256[] memory /* values */,
-        bytes[] memory calldatas,
+        uint256[] memory, /* values */
+        bytes[] memory, /* calldatas */
         bytes32 /*descriptionHash*/
     ) internal virtual {
         if (_executor() != address(this)) {

From 332bc3e8383176b7783e6c575f9dbea1a3ac87f3 Mon Sep 17 00:00:00 2001
From: Hadrien Croubois <hadrien.croubois@gmail.com>
Date: Wed, 16 Feb 2022 14:56:44 +0100
Subject: [PATCH 10/21] remove unecessary dependency

---
 contracts/governance/Governor.sol | 1 -
 1 file changed, 1 deletion(-)

diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
index ab442658ea1..5c793be06c8 100644
--- a/contracts/governance/Governor.sol
+++ b/contracts/governance/Governor.sol
@@ -10,7 +10,6 @@ import "../utils/math/SafeCast.sol";
 import "../utils/structs/DoubleEndedQueue.sol";
 import "../utils/Address.sol";
 import "../utils/Context.sol";
-import "../utils/Counters.sol";
 import "../utils/Timers.sol";
 import "./IGovernor.sol";
 

From 43e2e3daa3599850a741a7e557f051491de45f08 Mon Sep 17 00:00:00 2001
From: Hadrien Croubois <hadrien.croubois@gmail.com>
Date: Wed, 16 Feb 2022 14:56:58 +0100
Subject: [PATCH 11/21] remove unecessary dependency

---
 contracts/governance/Governor.sol | 1 -
 1 file changed, 1 deletion(-)

diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
index 5c793be06c8..ac76a981785 100644
--- a/contracts/governance/Governor.sol
+++ b/contracts/governance/Governor.sol
@@ -25,7 +25,6 @@ import "./IGovernor.sol";
  * _Available since v4.3._
  */
 abstract contract Governor is Context, ERC165, EIP712, IGovernor {
-    using Counters for Counters.Counter;
     using DoubleEndedQueue for DoubleEndedQueue.Bytes32Deque;
     using SafeCast for uint256;
     using Timers for Timers.BlockNumber;

From 7e75fabd5260a91bfa7933102f6da8a9cfcd71b6 Mon Sep 17 00:00:00 2001
From: Hadrien Croubois <hadrien.croubois@gmail.com>
Date: Wed, 16 Feb 2022 15:00:34 +0100
Subject: [PATCH 12/21] comment rephrasing

---
 contracts/governance/Governor.sol | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
index ac76a981785..06cf3bd59eb 100644
--- a/contracts/governance/Governor.sol
+++ b/contracts/governance/Governor.sol
@@ -42,10 +42,10 @@ 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.
+    // 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;
 
     /**

From b7fe0d33780ce33e5ac1e3758149ac8b1501d273 Mon Sep 17 00:00:00 2001
From: Hadrien Croubois <hadrien.croubois@gmail.com>
Date: Fri, 18 Feb 2022 10:11:04 +0100
Subject: [PATCH 13/21] Update contracts/governance/Governor.sol

Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
---
 contracts/governance/Governor.sol | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
index 06cf3bd59eb..f764969672d 100644
--- a/contracts/governance/Governor.sol
+++ b/contracts/governance/Governor.sol
@@ -316,11 +316,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
         bytes32 /*descriptionHash*/
     ) internal virtual {
         if (_executor() != address(this)) {
-            for (uint256 i = 0; i < targets.length; ++i) {
-                if (targets[i] == address(this)) {
-                    _governanceCall.clear();
-                }
-            }
+            _governanceCall.clear();
         }
     }
 

From d3808127a2599dc66f20136477877f38b06f974a Mon Sep 17 00:00:00 2001
From: Hadrien Croubois <hadrien.croubois@gmail.com>
Date: Fri, 18 Feb 2022 10:14:22 +0100
Subject: [PATCH 14/21] cache keccak256(_msgData())

---
 contracts/governance/Governor.sol | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
index f764969672d..432cceab1d3 100644
--- a/contracts/governance/Governor.sol
+++ b/contracts/governance/Governor.sol
@@ -55,8 +55,9 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
     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() != keccak256(msg.data)) {}
+            while (_governanceCall.popFront() != msgDataHash) {}
         }
         _;
     }

From 319c0d8e820b3358d8a7df8371e063ff78d7cc04 Mon Sep 17 00:00:00 2001
From: Hadrien Croubois <hadrien.croubois@gmail.com>
Date: Fri, 18 Feb 2022 10:15:16 +0100
Subject: [PATCH 15/21] use Context

---
 contracts/governance/Governor.sol | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
index 432cceab1d3..14eb69b8f79 100644
--- a/contracts/governance/Governor.sol
+++ b/contracts/governance/Governor.sol
@@ -209,7 +209,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"
         );
 

From d2913ace11b5efdb1f5e8856fcfddb8f85e7877f Mon Sep 17 00:00:00 2001
From: Francisco Giordano <frangio.1@gmail.com>
Date: Fri, 18 Feb 2022 11:50:48 -0300
Subject: [PATCH 16/21] lint

---
 contracts/governance/Governor.sol | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
index 14eb69b8f79..6f056adc294 100644
--- a/contracts/governance/Governor.sol
+++ b/contracts/governance/Governor.sol
@@ -311,7 +311,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
      */
     function _afterExecute(
         uint256, /* proposalId */
-        address[] memory targets,
+        address[] memory, /* targets */
         uint256[] memory, /* values */
         bytes[] memory, /* calldatas */
         bytes32 /*descriptionHash*/

From 65d5f4d91aa753518d7dd8fbd2cb44ba2b78decf Mon Sep 17 00:00:00 2001
From: Hadrien Croubois <hadrien.croubois@gmail.com>
Date: Fri, 18 Feb 2022 16:49:49 +0100
Subject: [PATCH 17/21] conditionnal clear

---
 contracts/governance/Governor.sol | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
index 6f056adc294..0b2fbe09057 100644
--- a/contracts/governance/Governor.sol
+++ b/contracts/governance/Governor.sol
@@ -317,7 +317,9 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
         bytes32 /*descriptionHash*/
     ) internal virtual {
         if (_executor() != address(this)) {
-            _governanceCall.clear();
+            if (!_governanceCall.empty()) {
+                _governanceCall.clear();
+            }
         }
     }
 

From b0bbcc077a18d318273b08ce09f7ed7246f17ad0 Mon Sep 17 00:00:00 2001
From: Francisco Giordano <frangio.1@gmail.com>
Date: Fri, 18 Feb 2022 14:57:32 -0300
Subject: [PATCH 18/21] add test to cover queue.clear()

---
 .../mocks/GovernorTimelockControlMock.sol     |  3 ++
 .../GovernorTimelockControl.test.js           | 29 +++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/contracts/mocks/GovernorTimelockControlMock.sol b/contracts/mocks/GovernorTimelockControlMock.sol
index 97376c8253a..1cd5ea6c6f9 100644
--- a/contracts/mocks/GovernorTimelockControlMock.sol
+++ b/contracts/mocks/GovernorTimelockControlMock.sol
@@ -105,4 +105,7 @@ contract GovernorTimelockControlMock is
     function _executor() internal view virtual override(Governor, GovernorTimelockControl) returns (address) {
         return super._executor();
     }
+
+    function nonGovernanceFunction() external {
+    }
 }
diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js
index 2f840096757..9efeb463518 100644
--- a/test/governance/extensions/GovernorTimelockControl.test.js
+++ b/test/governance/extensions/GovernorTimelockControl.test.js
@@ -487,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();
+  });
 });

From 5e4115380a3b0801e80a8beb2d5eefdc2356053e Mon Sep 17 00:00:00 2001
From: Francisco Giordano <frangio.1@gmail.com>
Date: Fri, 18 Feb 2022 15:05:55 -0300
Subject: [PATCH 19/21] lint

---
 contracts/mocks/GovernorTimelockControlMock.sol | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/contracts/mocks/GovernorTimelockControlMock.sol b/contracts/mocks/GovernorTimelockControlMock.sol
index 1cd5ea6c6f9..6150b7b3c6d 100644
--- a/contracts/mocks/GovernorTimelockControlMock.sol
+++ b/contracts/mocks/GovernorTimelockControlMock.sol
@@ -106,6 +106,5 @@ contract GovernorTimelockControlMock is
         return super._executor();
     }
 
-    function nonGovernanceFunction() external {
-    }
+    function nonGovernanceFunction() external {}
 }

From ee1a34baa38d64c026ed9cdd63cd2d79e41c6de9 Mon Sep 17 00:00:00 2001
From: Francisco Giordano <frangio.1@gmail.com>
Date: Fri, 18 Feb 2022 15:21:44 -0300
Subject: [PATCH 20/21] write more extended docs for onlyGovernance

---
 contracts/governance/Governor.sol | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
index 0b2fbe09057..0da3f11706a 100644
--- a/contracts/governance/Governor.sol
+++ b/contracts/governance/Governor.sol
@@ -49,8 +49,14 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
     DoubleEndedQueue.Bytes32Deque 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.
+     * @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");

From eca234f40e0104eccd70723315eb9418210ff59f Mon Sep 17 00:00:00 2001
From: Francisco Giordano <frangio.1@gmail.com>
Date: Fri, 18 Feb 2022 15:25:02 -0300
Subject: [PATCH 21/21] add changelog entry

---
 CHANGELOG.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index be8df954a9f..950e6b24926 100644
--- a/CHANGELOG.md
+++ b/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)