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) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 837a9b6108c..0da3f11706a 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/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. diff --git a/contracts/mocks/GovernorTimelockControlMock.sol b/contracts/mocks/GovernorTimelockControlMock.sol index 97376c8253a..6150b7b3c6d 100644 --- a/contracts/mocks/GovernorTimelockControlMock.sol +++ b/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 {} } diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 35242740b48..9efeb463518 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 = { @@ -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() ], + '', + ], + voters: [ + { voter: voter, support: Enums.VoteType.For }, + ], + steps: { + queue: { delay: 3600 }, + }, + }; + }); + + afterEach(async function () { + expectEvent( + this.receipts.execute, + 'ProposalExecuted', + { proposalId: this.id }, + ); + }); + + runGovernorWorkflow(); + }); });