Change admin role allocation in TimelockControler constructor (#3722)

Co-authored-by: Francisco <frangio.1@gmail.com>
(cherry picked from commit 408055dfab)

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## 4.8.0
 
+ * `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))
@@ -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))

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 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`
 

contracts/governance/TimelockController.sol

@@ -62,31 +62,37 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver
     event MinDelayChange(uint256 oldDuration, uint256 newDuration);
 
     /**
-     * @dev Initializes the contract with a given `minDelay`, and a list of
+     * @dev Initializes the contract with the following parameters:
-     * initial proposers and executors. The proposers receive both the
-     * 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
+     * - `minDelay`: initial minimum delay for operations
-     * administrators. This helps further configuration of the timelock by the
+     * - `proposers`: accounts to be granted proposer and canceller roles
-     * deployer. After configuration is done, it is recommended that the
+     * - `executors`: accounts to be granted executor role
-     * deployer renounces its admin position and relies on timelocked
+     * - `admin`: optional account to be granted admin role; disable with zero address
-     * operations to perform future maintenance.
+     *
+     * 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,
         address[] memory proposers,
-        address[] memory executors
+        address[] memory executors,
+        address admin
     ) {
         _setRoleAdmin(TIMELOCK_ADMIN_ROLE, TIMELOCK_ADMIN_ROLE);
         _setRoleAdmin(PROPOSER_ROLE, TIMELOCK_ADMIN_ROLE);
         _setRoleAdmin(EXECUTOR_ROLE, TIMELOCK_ADMIN_ROLE);
         _setRoleAdmin(CANCELLER_ROLE, TIMELOCK_ADMIN_ROLE);
 
-        // deployer + self administration
+        // self administration
-        _setupRole(TIMELOCK_ADMIN_ROLE, _msgSender());
         _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]);

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');
@@ -65,7 +65,7 @@ contract('TimelockController', function (accounts) {
       MINDELAY,
       [ proposer ],
       [ executor ],
-      { from: admin },
+      admin,
     );
 
     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,
+      [ proposer ],
+      [ executor ],
+      ZERO_ADDRESS,
+      { 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 () {

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,
@@ -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 () {