Change admin role allocation in TimelockControler constructor

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. ([#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 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`
 

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 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
+     * 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]);

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

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,