From 7d0e346bbd10584ac38186c30eaf31c13aab0d80 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 22 Sep 2022 19:28:47 +0200 Subject: [PATCH 01/10] change admin role allocation --- CHANGELOG.md | 3 +++ contracts/governance/TimelockController.sol | 19 ++++++++++++------- test/governance/TimelockController.test.js | 21 +++++++++++++++++---- 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9359c2edc99..eac0cee4f93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Unreleased + * `TimelockController`: Added a new `admin` constructor parameter that is assigned the admin role instead of the deployer account. ([#xxxx](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/xxxx)) * `Initializable`: add internal functions `_getInitializedVersion` and `_isInitializing` ([#3598](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3598)) * `ERC165Checker`: add `supportsERC165InterfaceUnchecked` for consulting individual interfaces without the full ERC165 protocol. ([#3339](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3339)) * `Address`: optimize `functionCall` by calling `functionCallWithValue` directly. ([#3468](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3468)) @@ -37,6 +38,8 @@ * `ERC4626`: Conversion from shares to assets (and vice-versa) in an empty vault used to consider the possible mismatch between the underlying asset's and the vault's decimals. This initial conversion rate is now set to 1-to-1 irrespective of decimals, which are meant for usability purposes only. The vault now uses the assets decimals by default, so off-chain the numbers should appear the same. Developers overriding the vault decimals to a value that does not match the underlying asset may want to override the `_initialConvertToShares` and `_initialConvertToAssets` to replicate the previous behavior. + * `TimelockController`: During deployment, the TimelockController used to grant the `TIMELOCK_ADMIN_ROLE` to the deployer and to the timelock itself. The deployer was then expected to renounce this role once configuration of the timelock is over. Failing to renounce that role allows the deployer to change the timelock permissions (but not to bypass the delay for any time-locked actions). The role is no longer given to the deployer by default. A new parameter `admin` can be set to a non-zero address to grant the admin role during construction (to the deployer or any other address). Just like previously, this admin role should be renounced after configuration. If this param is given `address(0)`, the role is not allocated and doesn't need to be revoked. In any case, the timelock itself continues to have this role + ### Deprecations * `EIP712`: Added the file `EIP712.sol` and deprecated `draft-EIP712.sol` since the EIP is no longer a Draft. Developers are encouraged to update their imports. ([#3621](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3621)) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 91fcbfcda8d..a3821c8194e 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -67,14 +67,15 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver * proposer and the canceller role (for backward compatibility). The * executors receive the executor role. * - * NOTE: At construction, both the deployer and the timelock itself are - * administrators. This helps further configuration of the timelock by the - * deployer. After configuration is done, it is recommended that the - * deployer renounces its admin position and relies on timelocked - * operations to perform future maintenance. + * NOTE: At construction, an optional admin can be set (in addition to the + * timelock itself). Setting such an admin can helps further configuration + * of the timelock. After configuration is done, it is recommended to have + * this admin renounces its admin and relies on timelocked operations to + * perform future maintenance. */ constructor( uint256 minDelay, + address admin, address[] memory proposers, address[] memory executors ) { @@ -83,10 +84,14 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver _setRoleAdmin(EXECUTOR_ROLE, TIMELOCK_ADMIN_ROLE); _setRoleAdmin(CANCELLER_ROLE, TIMELOCK_ADMIN_ROLE); - // deployer + self administration - _setupRole(TIMELOCK_ADMIN_ROLE, _msgSender()); + // self administration _setupRole(TIMELOCK_ADMIN_ROLE, address(this)); + // optional admin + if (admin != address(0)) { + _setupRole(TIMELOCK_ADMIN_ROLE, admin); + } + // register proposers and cancellers for (uint256 i = 0; i < proposers.length; ++i) { _setupRole(PROPOSER_ROLE, proposers[i]); diff --git a/test/governance/TimelockController.test.js b/test/governance/TimelockController.test.js index 2274bb0a464..2162e222311 100644 --- a/test/governance/TimelockController.test.js +++ b/test/governance/TimelockController.test.js @@ -1,5 +1,5 @@ const { BN, constants, expectEvent, expectRevert, time } = require('@openzeppelin/test-helpers'); -const { ZERO_BYTES32 } = constants; +const { ZERO_ADDRESS, ZERO_BYTES32 } = constants; const { expect } = require('chai'); @@ -52,7 +52,7 @@ function genOperationBatch (targets, values, payloads, predecessor, salt) { } contract('TimelockController', function (accounts) { - const [ admin, proposer, canceller, executor, other ] = accounts; + const [ _, admin, proposer, canceller, executor, other ] = accounts; const TIMELOCK_ADMIN_ROLE = web3.utils.soliditySha3('TIMELOCK_ADMIN_ROLE'); const PROPOSER_ROLE = web3.utils.soliditySha3('PROPOSER_ROLE'); @@ -63,9 +63,9 @@ contract('TimelockController', function (accounts) { // Deploy new timelock this.mock = await TimelockController.new( MINDELAY, + admin, [ proposer ], - [ executor ], - { from: admin }, + [ executor ] ); expect(await this.mock.hasRole(CANCELLER_ROLE, proposer)).to.be.equal(true); @@ -102,6 +102,19 @@ contract('TimelockController', function (accounts) { ))).to.be.deep.equal([ false, false, true ]); }); + it('optional admin', async function () { + const mock = await TimelockController.new( + MINDELAY, + ZERO_ADDRESS, + [ proposer ], + [ executor ], + { from: other }, + ); + + expect(await mock.hasRole(TIMELOCK_ADMIN_ROLE, admin)).to.be.equal(false); + expect(await mock.hasRole(TIMELOCK_ADMIN_ROLE, other)).to.be.equal(false); + }); + describe('methods', function () { describe('operation hashing', function () { it('hashOperation', async function () { From 1c541a35bb1c512f9af27130a3cfbbb4371af8dc Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 22 Sep 2022 19:30:35 +0200 Subject: [PATCH 02/10] PR number in changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eac0cee4f93..8bb577574b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased - * `TimelockController`: Added a new `admin` constructor parameter that is assigned the admin role instead of the deployer account. ([#xxxx](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/xxxx)) + * `TimelockController`: Added a new `admin` constructor parameter that is assigned the admin role instead of the deployer account. ([#3722](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3722)) * `Initializable`: add internal functions `_getInitializedVersion` and `_isInitializing` ([#3598](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3598)) * `ERC165Checker`: add `supportsERC165InterfaceUnchecked` for consulting individual interfaces without the full ERC165 protocol. ([#3339](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3339)) * `Address`: optimize `functionCall` by calling `functionCallWithValue` directly. ([#3468](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3468)) From e0e63e84bc36fc53bc9e86f35786f04e323d6beb Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 22 Sep 2022 19:33:03 +0200 Subject: [PATCH 03/10] typos --- contracts/governance/TimelockController.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index a3821c8194e..478cfac4740 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -68,9 +68,9 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver * executors receive the executor role. * * NOTE: At construction, an optional admin can be set (in addition to the - * timelock itself). Setting such an admin can helps further configuration + * timelock itself). Setting such an admin can help further configuration * of the timelock. After configuration is done, it is recommended to have - * this admin renounces its admin and relies on timelocked operations to + * this admin renounces its role and to rely on timelocked operations to * perform future maintenance. */ constructor( From 134371897c486fb093518182c2e534a6e636f562 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 22 Sep 2022 19:39:39 +0200 Subject: [PATCH 04/10] update documentation --- contracts/governance/README.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/governance/README.adoc b/contracts/governance/README.adoc index e34f0eae1c5..49502a06076 100644 --- a/contracts/governance/README.adoc +++ b/contracts/governance/README.adoc @@ -153,7 +153,7 @@ Operations status can be queried using the functions: [[timelock-admin]] ===== Admin -The admins are in charge of managing proposers and executors. For the timelock to be self-governed, this role should only be given to the timelock itself. Upon deployment, both the timelock and the deployer have this role. After further configuration and testing, the deployer can renounce this role such that all further maintenance operations have to go through the timelock process. +The admins are in charge of managing proposers and executors. For the timelock to be self-governed, this role should only be given to the timelock itself. Upon deployment, the admin role can be granted to any address (in addition to the timelock itself). After further configuration and testing, this optional admin should be renounce its role such that all further maintenance operations have to go through the timelock process. This role is identified by the *TIMELOCK_ADMIN_ROLE* value: `0x5f58e3a2316349923ce3780f8d587db2d72378aed66a8261c916544fa6846ca5` From 9be8b913ecba2068dd758789261dc27ef755901e Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 22 Sep 2022 19:41:02 +0200 Subject: [PATCH 05/10] fix lint --- test/governance/TimelockController.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/governance/TimelockController.test.js b/test/governance/TimelockController.test.js index 2162e222311..fe17cc12262 100644 --- a/test/governance/TimelockController.test.js +++ b/test/governance/TimelockController.test.js @@ -52,7 +52,7 @@ function genOperationBatch (targets, values, payloads, predecessor, salt) { } contract('TimelockController', function (accounts) { - const [ _, admin, proposer, canceller, executor, other ] = accounts; + const [ , admin, proposer, canceller, executor, other ] = accounts; const TIMELOCK_ADMIN_ROLE = web3.utils.soliditySha3('TIMELOCK_ADMIN_ROLE'); const PROPOSER_ROLE = web3.utils.soliditySha3('PROPOSER_ROLE'); @@ -65,7 +65,7 @@ contract('TimelockController', function (accounts) { MINDELAY, admin, [ proposer ], - [ executor ] + [ executor ], ); expect(await this.mock.hasRole(CANCELLER_ROLE, proposer)).to.be.equal(true); From c91a2457091abe63f2a028b09ff293ca88fda222 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 22 Sep 2022 21:38:37 +0200 Subject: [PATCH 06/10] fix governor test --- test/governance/extensions/GovernorTimelockControl.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 45e26c93595..2c23936c51b 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -33,7 +33,7 @@ contract('GovernorTimelockControl', function (accounts) { const [ deployer ] = await web3.eth.getAccounts(); this.token = await Token.new(tokenName, tokenSymbol); - this.timelock = await Timelock.new(3600, [], []); + this.timelock = await Timelock.new(3600, deployer, [], []); this.mock = await Governor.new( name, this.token.address, From 0b59a20fd8e1aa3c754462f9856b731ceeb2be81 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 22 Sep 2022 22:53:26 +0200 Subject: [PATCH 07/10] Apply suggestions from code review Co-authored-by: Francisco --- CHANGELOG.md | 2 +- contracts/governance/README.adoc | 2 +- contracts/governance/TimelockController.sol | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8bb577574b8..6932b4d886f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,7 +38,7 @@ * `ERC4626`: Conversion from shares to assets (and vice-versa) in an empty vault used to consider the possible mismatch between the underlying asset's and the vault's decimals. This initial conversion rate is now set to 1-to-1 irrespective of decimals, which are meant for usability purposes only. The vault now uses the assets decimals by default, so off-chain the numbers should appear the same. Developers overriding the vault decimals to a value that does not match the underlying asset may want to override the `_initialConvertToShares` and `_initialConvertToAssets` to replicate the previous behavior. - * `TimelockController`: During deployment, the TimelockController used to grant the `TIMELOCK_ADMIN_ROLE` to the deployer and to the timelock itself. The deployer was then expected to renounce this role once configuration of the timelock is over. Failing to renounce that role allows the deployer to change the timelock permissions (but not to bypass the delay for any time-locked actions). The role is no longer given to the deployer by default. A new parameter `admin` can be set to a non-zero address to grant the admin role during construction (to the deployer or any other address). Just like previously, this admin role should be renounced after configuration. If this param is given `address(0)`, the role is not allocated and doesn't need to be revoked. In any case, the timelock itself continues to have this role + * `TimelockController`: During deployment, the TimelockController used to grant the `TIMELOCK_ADMIN_ROLE` to the deployer and to the timelock itself. The deployer was then expected to renounce this role once configuration of the timelock is over. Failing to renounce that role allows the deployer to change the timelock permissions (but not to bypass the delay for any time-locked actions). The role is no longer given to the deployer by default. A new parameter `admin` can be set to a non-zero address to grant the admin role during construction (to the deployer or any other address). Just like previously, this admin role should be renounced after configuration. If this param is given `address(0)`, the role is not allocated and doesn't need to be revoked. In any case, the timelock itself continues to have this role. ### Deprecations diff --git a/contracts/governance/README.adoc b/contracts/governance/README.adoc index 49502a06076..f711c63cf1a 100644 --- a/contracts/governance/README.adoc +++ b/contracts/governance/README.adoc @@ -153,7 +153,7 @@ Operations status can be queried using the functions: [[timelock-admin]] ===== Admin -The admins are in charge of managing proposers and executors. For the timelock to be self-governed, this role should only be given to the timelock itself. Upon deployment, the admin role can be granted to any address (in addition to the timelock itself). After further configuration and testing, this optional admin should be renounce its role such that all further maintenance operations have to go through the timelock process. +The admins are in charge of managing proposers and executors. For the timelock to be self-governed, this role should only be given to the timelock itself. Upon deployment, the admin role can be granted to any address (in addition to the timelock itself). After further configuration and testing, this optional admin should renounce its role such that all further maintenance operations have to go through the timelock process. This role is identified by the *TIMELOCK_ADMIN_ROLE* value: `0x5f58e3a2316349923ce3780f8d587db2d72378aed66a8261c916544fa6846ca5` diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 478cfac4740..5f635f5ed40 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -67,10 +67,10 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver * proposer and the canceller role (for backward compatibility). The * executors receive the executor role. * - * NOTE: At construction, an optional admin can be set (in addition to the + * CAUTION: At construction, an optional admin can be set (in addition to the * timelock itself). Setting such an admin can help further configuration - * of the timelock. After configuration is done, it is recommended to have - * this admin renounces its role and to rely on timelocked operations to + * of the timelock. After configuration is done, this admin should + * renounce its role, and only timelocked operations should be used to * perform future maintenance. */ constructor( From e76e2f575a24e2e1bccfbf6f351a18824df56417 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 22 Sep 2022 22:55:19 +0200 Subject: [PATCH 08/10] move admin param to the end --- contracts/governance/TimelockController.sol | 4 ++-- test/governance/TimelockController.test.js | 4 ++-- test/governance/extensions/GovernorTimelockControl.test.js | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 5f635f5ed40..43e32b1398f 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -75,9 +75,9 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver */ constructor( uint256 minDelay, - address admin, address[] memory proposers, - address[] memory executors + address[] memory executors, + address admin ) { _setRoleAdmin(TIMELOCK_ADMIN_ROLE, TIMELOCK_ADMIN_ROLE); _setRoleAdmin(PROPOSER_ROLE, TIMELOCK_ADMIN_ROLE); diff --git a/test/governance/TimelockController.test.js b/test/governance/TimelockController.test.js index fe17cc12262..e0a32440efe 100644 --- a/test/governance/TimelockController.test.js +++ b/test/governance/TimelockController.test.js @@ -63,9 +63,9 @@ contract('TimelockController', function (accounts) { // Deploy new timelock this.mock = await TimelockController.new( MINDELAY, - admin, [ proposer ], [ executor ], + admin, ); expect(await this.mock.hasRole(CANCELLER_ROLE, proposer)).to.be.equal(true); @@ -105,9 +105,9 @@ contract('TimelockController', function (accounts) { it('optional admin', async function () { const mock = await TimelockController.new( MINDELAY, - ZERO_ADDRESS, [ proposer ], [ executor ], + ZERO_ADDRESS, { from: other }, ); diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 2c23936c51b..9bc84700fc5 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -33,7 +33,7 @@ contract('GovernorTimelockControl', function (accounts) { const [ deployer ] = await web3.eth.getAccounts(); this.token = await Token.new(tokenName, tokenSymbol); - this.timelock = await Timelock.new(3600, deployer, [], []); + this.timelock = await Timelock.new(3600, [], [], deployer); this.mock = await Governor.new( name, this.token.address, From 7e417a4924c988b315d3ea7d86e7546d1e70812f Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 22 Sep 2022 23:05:51 +0200 Subject: [PATCH 09/10] fix test --- test/governance/extensions/GovernorTimelockControl.test.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 9bc84700fc5..d06f7999252 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -322,7 +322,12 @@ contract('GovernorTimelockControl', function (accounts) { describe('updateTimelock', function () { beforeEach(async function () { - this.newTimelock = await Timelock.new(3600, [], []); + this.newTimelock = await Timelock.new( + 3600, + [ this.mock.address ], + [ this.mock.address ], + constants.ZERO_ADDRESS, + ); }); it('is protected', async function () { From 6887394e82a7b69a596f09611563fdd1fd1aa627 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 22 Sep 2022 21:01:54 -0300 Subject: [PATCH 10/10] rewrite constructor docs --- contracts/governance/TimelockController.sol | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 43e32b1398f..d5b18ace6e2 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -62,16 +62,17 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver event MinDelayChange(uint256 oldDuration, uint256 newDuration); /** - * @dev Initializes the contract with a given `minDelay`, and a list of - * initial proposers and executors. The proposers receive both the - * proposer and the canceller role (for backward compatibility). The - * executors receive the executor role. + * @dev Initializes the contract with the following parameters: * - * CAUTION: At construction, an optional admin can be set (in addition to the - * timelock itself). Setting such an admin can help further configuration - * of the timelock. After configuration is done, this admin should - * renounce its role, and only timelocked operations should be used to - * perform future maintenance. + * - `minDelay`: initial minimum delay for operations + * - `proposers`: accounts to be granted proposer and canceller roles + * - `executors`: accounts to be granted executor role + * - `admin`: optional account to be granted admin role; disable with zero address + * + * IMPORTANT: The optional admin can aid with initial configuration of roles after deployment + * without being subject to delay, but this role should be subsequently renounced in favor of + * administration through timelocked proposals. Previous versions of this contract would assign + * this admin to the deployer automatically and should be renounced as well. */ constructor( uint256 minDelay,