🔒 Revoke Admin Role From Deployer (#259)

### 🕓 Changelog This PR revokes the `DEFAULT_ADMIN_ROLE` role from the deployer account in the `timelock_controller` contract. The underlying reason for this design is that deployer accounts may forget to revoke the admin rights from the timelock controller contract after deployment. For further insights also, see the following issue: OpenZeppelin/openzeppelin-contracts#3720. In addition, we remove the redundant ownership transfers in the `erc20`, `erc721`, and `erc1155` constructors. At initialisation time, the `owner` role will be already assigned to the `msg.sender` since we `uses` the `ownable` module. -------- Signed-off-by: Pascal Marco Caversaccio <[email protected]>
pcaversaccio · Jun 26, 2024 · feb2dc0 · feb2dc0
1 parent 4605c6d
commit feb2dc0
Show file tree

Hide file tree

Showing 7 changed files with 103 additions and 20 deletions.
diff --git a/.gas-snapshot b/.gas-snapshot
@@ -186,7 +186,7 @@ ERC1155Test:testFuzzTotalSupplyAfterSingleMint(uint256,uint256,bytes) (runs: 256
 ERC1155Test:testFuzzTransferOwnershipNonOwner(address,address) (runs: 256, μ: 14049, ~: 14049)
 ERC1155Test:testFuzzTransferOwnershipSuccess(address,address) (runs: 256, μ: 75704, ~: 75672)
 ERC1155Test:testHasOwner() (gas: 12680)
-ERC1155Test:testInitialSetup() (gas: 2894237)
+ERC1155Test:testInitialSetup() (gas: 2892461)
 ERC1155Test:testRenounceOwnershipNonOwner() (gas: 10881)
 ERC1155Test:testRenounceOwnershipSuccess() (gas: 22906)
 ERC1155Test:testSafeBatchTransferFromByApprovedOperator() (gas: 309682)
@@ -253,9 +253,9 @@ ERC1155Test:testTotalSupplyBeforeMint() (gas: 10460)
 ERC1155Test:testTransferOwnershipNonOwner() (gas: 12439)
 ERC1155Test:testTransferOwnershipSuccess() (gas: 53849)
 ERC1155Test:testTransferOwnershipToZeroAddress() (gas: 15610)
-ERC1155Test:testUriBaseAndTokenUriNotSet() (gas: 2853197)
+ERC1155Test:testUriBaseAndTokenUriNotSet() (gas: 2851421)
 ERC1155Test:testUriBaseAndTokenUriSet() (gas: 64196)
-ERC1155Test:testUriNoBaseURI() (gas: 2902603)
+ERC1155Test:testUriNoBaseURI() (gas: 2900827)
 ERC1155Test:testUriNoTokenUri() (gas: 18817)
 ERC20Invariants:statefulFuzzOwner() (runs: 256, calls: 3840, reverts: 3442)
 ERC20Invariants:statefulFuzzTotalSupply() (runs: 256, calls: 3840, reverts: 3442)
@@ -302,7 +302,7 @@ ERC20Test:testFuzzTransferOwnershipNonOwner(address,address) (runs: 256, μ: 140
 ERC20Test:testFuzzTransferOwnershipSuccess(address,address) (runs: 256, μ: 75667, ~: 75636)
 ERC20Test:testFuzzTransferSuccess(address,uint256) (runs: 256, μ: 205511, ~: 205572)
 ERC20Test:testHasOwner() (gas: 12658)
-ERC20Test:testInitialSetup() (gas: 1568383)
+ERC20Test:testInitialSetup() (gas: 1566615)
 ERC20Test:testMintNonMinter() (gas: 12470)
 ERC20Test:testMintOverflow() (gas: 16796)
 ERC20Test:testMintSuccess() (gas: 51814)
@@ -397,7 +397,7 @@ ERC4626VaultTest:testFuzzDomainSeparator(uint8) (runs: 256, μ: 11931, ~: 11959)
 ERC4626VaultTest:testFuzzEIP712Domain(bytes1,uint8,bytes32,uint256[]) (runs: 256, μ: 21760, ~: 21810)
 ERC4626VaultTest:testFuzzPermitInvalid(string,string,uint16) (runs: 256, μ: 44559, ~: 44558)
 ERC4626VaultTest:testFuzzPermitSuccess(string,string,uint16) (runs: 256, μ: 70491, ~: 70494)
-ERC4626VaultTest:testInitialSetup() (gas: 5973607)
+ERC4626VaultTest:testInitialSetup() (gas: 5968285)
 ERC4626VaultTest:testMintWithNoApproval() (gas: 24358)
 ERC4626VaultTest:testMintZero() (gas: 41205)
 ERC4626VaultTest:testMultipleMintDepositRedeemWithdraw() (gas: 377043)
@@ -489,7 +489,7 @@ ERC721Test:testGetApprovedApprovedTokenId() (gas: 193918)
 ERC721Test:testGetApprovedInvalidTokenId() (gas: 11082)
 ERC721Test:testGetApprovedNotApprovedTokenId() (gas: 170344)
 ERC721Test:testHasOwner() (gas: 12629)
-ERC721Test:testInitialSetup() (gas: 2514550)
+ERC721Test:testInitialSetup() (gas: 2512774)
 ERC721Test:testOwnerOf() (gas: 166052)
 ERC721Test:testOwnerOfInvalidTokenId() (gas: 11015)
 ERC721Test:testPermitBadChainId() (gas: 199389)
@@ -537,7 +537,7 @@ ERC721Test:testTokenOfOwnerByIndexReverts() (gas: 545709)
 ERC721Test:testTokenURIAfterBurning() (gas: 138559)
 ERC721Test:testTokenURIDefault() (gas: 168197)
 ERC721Test:testTokenURIInvalidTokenId() (gas: 13120)
-ERC721Test:testTokenURINoBaseURI() (gas: 2633992)
+ERC721Test:testTokenURINoBaseURI() (gas: 2632217)
 ERC721Test:testTokenURINoTokenUri() (gas: 125697)
 ERC721Test:testTotalSupply() (gas: 328194)
 ERC721Test:testTransferFrom() (gas: 574236)
@@ -665,13 +665,13 @@ SignatureCheckerTest:testFuzzEOAWithInvalidSignature(bytes,string) (runs: 256,
 SignatureCheckerTest:testFuzzEOAWithInvalidSigner(string,string) (runs: 256, μ: 20434, ~: 20438)
 SignatureCheckerTest:testFuzzEOAWithValidSignature(string,string) (runs: 256, μ: 20366, ~: 20370)
 SignatureCheckerTest:testInitialSetup() (gas: 8359)
-TimelockControllerInvariants:statefulFuzzExecutedLessThanOrEqualToScheduled() (runs: 256, calls: 3840, reverts: 1260)
-TimelockControllerInvariants:statefulFuzzExecutedProposalCancellation() (runs: 256, calls: 3840, reverts: 1282)
-TimelockControllerInvariants:statefulFuzzExecutingCancelledProposal() (runs: 256, calls: 3840, reverts: 1210)
-TimelockControllerInvariants:statefulFuzzExecutingNotReadyProposal() (runs: 256, calls: 3840, reverts: 1234)
-TimelockControllerInvariants:statefulFuzzOnceProposalExecution() (runs: 256, calls: 3840, reverts: 1268)
-TimelockControllerInvariants:statefulFuzzProposalsExecutedMatchCount() (runs: 256, calls: 3840, reverts: 1260)
-TimelockControllerInvariants:statefulFuzzSumOfProposals() (runs: 256, calls: 3840, reverts: 1260)
+TimelockControllerInvariants:statefulFuzzExecutedLessThanOrEqualToScheduled() (runs: 256, calls: 3840, reverts: 1283)
+TimelockControllerInvariants:statefulFuzzExecutedProposalCancellation() (runs: 256, calls: 3840, reverts: 1259)
+TimelockControllerInvariants:statefulFuzzExecutingCancelledProposal() (runs: 256, calls: 3840, reverts: 1282)
+TimelockControllerInvariants:statefulFuzzExecutingNotReadyProposal() (runs: 256, calls: 3840, reverts: 1280)
+TimelockControllerInvariants:statefulFuzzOnceProposalExecution() (runs: 256, calls: 3840, reverts: 1284)
+TimelockControllerInvariants:statefulFuzzProposalsExecutedMatchCount() (runs: 256, calls: 3840, reverts: 1283)
+TimelockControllerInvariants:statefulFuzzSumOfProposals() (runs: 256, calls: 3840, reverts: 1283)
 TimelockControllerTest:testAdminCannotBatchExecute() (gas: 750638)
 TimelockControllerTest:testAdminCannotBatchSchedule() (gas: 748425)
 TimelockControllerTest:testAdminCannotCancel() (gas: 13375)
@@ -713,11 +713,11 @@ TimelockControllerTest:testFuzzBatchValue(uint256) (runs: 256, μ: 4650559, ~: 4
 TimelockControllerTest:testFuzzHashOperation(address,uint256,bytes,bytes32,bytes32) (runs: 256, μ: 10606, ~: 10586)
 TimelockControllerTest:testFuzzHashOperationBatch(address[],uint256[],bytes[],bytes32,bytes32) (runs: 256, μ: 1826841, ~: 1817250)
 TimelockControllerTest:testFuzzOperationValue(uint256) (runs: 256, μ: 111619, ~: 111431)
-TimelockControllerTest:testHandleERC1155() (gas: 41561916)
-TimelockControllerTest:testHandleERC721() (gas: 7162262)
+TimelockControllerTest:testHandleERC1155() (gas: 41560140)
+TimelockControllerTest:testHandleERC721() (gas: 7160486)
 TimelockControllerTest:testHashOperation() (gas: 12368)
 TimelockControllerTest:testHashOperationBatch() (gas: 1525342)
-TimelockControllerTest:testInitialSetup() (gas: 4311627)
+TimelockControllerTest:testInitialSetup() (gas: 4325533)
 TimelockControllerTest:testInvalidOperation() (gas: 10719)
 TimelockControllerTest:testOperationAlreadyScheduled() (gas: 51498)
 TimelockControllerTest:testOperationCancelFinished() (gas: 99525)

diff --git a/src/snekmate/governance/mocks/timelock_controller_mock.vy b/src/snekmate/governance/mocks/timelock_controller_mock.vy
@@ -96,4 +96,6 @@ def __init__(minimum_delay_: uint256, proposers_: DynArray[address, tc._DYNARRAY
     # The following line assigns the `DEFAULT_ADMIN_ROLE`
     # to the `msg.sender`.
     ac.__init__()
+    # The following line revokes the `DEFAULT_ADMIN_ROLE`
+    # from the `msg.sender`.
     tc.__init__(minimum_delay_, proposers_, executors_, admin_)
diff --git a/src/snekmate/governance/timelock_controller.vy b/src/snekmate/governance/timelock_controller.vy
@@ -253,6 +253,13 @@ def __init__(minimum_delay_: uint256, proposers_: DynArray[address, _DYNARRAY_BO
     # Configure the contract to be self-administered.
     access_control._grant_role(access_control.DEFAULT_ADMIN_ROLE, self)
 
+    # Revoke the `DEFAULT_ADMIN_ROLE` role from the deployer account.
+    # The underlying reason for this design is that deployer accounts may
+    # forget to revoke the admin rights from the timelock controller contract
+    # after deployment. For further insights also, see the following issue:
+    # https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3720.
+    access_control._revoke_role(access_control.DEFAULT_ADMIN_ROLE, msg.sender)
+
     # Set the optional admin.
     if (admin_ != empty(address)):
         access_control._grant_role(access_control.DEFAULT_ADMIN_ROLE, admin_)

diff --git a/src/snekmate/tokens/erc1155.vy b/src/snekmate/tokens/erc1155.vy
@@ -155,7 +155,6 @@ def __init__(base_uri_: String[80]):
     """
     _BASE_URI = base_uri_
 
-    ownable._transfer_ownership(msg.sender)
     self.is_minter[msg.sender] = True
     log RoleMinterChanged(msg.sender, True)
 

diff --git a/src/snekmate/tokens/erc20.vy b/src/snekmate/tokens/erc20.vy
@@ -201,7 +201,6 @@ def __init__(name_: String[25], symbol_: String[5], decimals_: uint8, name_eip71
     symbol = symbol_
     decimals = decimals_
 
-    ownable._transfer_ownership(msg.sender)
     self.is_minter[msg.sender] = True
     log RoleMinterChanged(msg.sender, True)
 

diff --git a/src/snekmate/tokens/erc721.vy b/src/snekmate/tokens/erc721.vy
@@ -273,7 +273,6 @@ def __init__(name_: String[25], symbol_: String[5], base_uri_: String[80], name_
     symbol = symbol_
     _BASE_URI = base_uri_
 
-    ownable._transfer_ownership(msg.sender)
     self.is_minter[msg.sender] = True
     log RoleMinterChanged(msg.sender, True)
 

diff --git a/test/governance/TimelockController.t.sol b/test/governance/TimelockController.t.sol
@@ -139,6 +139,12 @@ contract TimelockControllerTest is Test {
                 self
             )
         );
+        assertTrue(
+            !timelockController.hasRole(
+                timelockController.DEFAULT_ADMIN_ROLE(),
+                deployer
+            )
+        );
         assertTrue(
             timelockController.hasRole(
                 timelockController.PROPOSER_ROLE(),
@@ -193,6 +199,24 @@ contract TimelockControllerTest is Test {
                 self
             )
         );
+        assertTrue(
+            !timelockController.hasRole(
+                timelockController.PROPOSER_ROLE(),
+                deployer
+            )
+        );
+        assertTrue(
+            !timelockController.hasRole(
+                timelockController.CANCELLER_ROLE(),
+                deployer
+            )
+        );
+        assertTrue(
+            !timelockController.hasRole(
+                timelockController.EXECUTOR_ROLE(),
+                deployer
+            )
+        );
         assertTrue(
             !timelockController.hasRole(
                 timelockController.PROPOSER_ROLE(),
@@ -257,6 +281,8 @@ contract TimelockControllerTest is Test {
             deployer
         );
         vm.expectEmit(true, true, true, false);
+        emit IAccessControl.RoleRevoked(DEFAULT_ADMIN_ROLE, deployer, deployer);
+        vm.expectEmit(true, true, true, false);
         emit IAccessControl.RoleGranted(PROPOSER_ROLE, proposers[0], deployer);
         vm.expectEmit(true, true, true, false);
         emit IAccessControl.RoleGranted(CANCELLER_ROLE, proposers[0], deployer);
@@ -323,6 +349,12 @@ contract TimelockControllerTest is Test {
                 self
             )
         );
+        assertTrue(
+            !timelockControllerInitialEventEmptyAdmin.hasRole(
+                timelockControllerInitialEventEmptyAdmin.DEFAULT_ADMIN_ROLE(),
+                deployer
+            )
+        );
         assertTrue(
             timelockControllerInitialEventEmptyAdmin.hasRole(
                 timelockControllerInitialEventEmptyAdmin.PROPOSER_ROLE(),
@@ -377,6 +409,24 @@ contract TimelockControllerTest is Test {
                 self
             )
         );
+        assertTrue(
+            !timelockControllerInitialEventEmptyAdmin.hasRole(
+                timelockControllerInitialEventEmptyAdmin.PROPOSER_ROLE(),
+                deployer
+            )
+        );
+        assertTrue(
+            !timelockControllerInitialEventEmptyAdmin.hasRole(
+                timelockControllerInitialEventEmptyAdmin.CANCELLER_ROLE(),
+                deployer
+            )
+        );
+        assertTrue(
+            !timelockControllerInitialEventEmptyAdmin.hasRole(
+                timelockControllerInitialEventEmptyAdmin.EXECUTOR_ROLE(),
+                deployer
+            )
+        );
         assertTrue(
             !timelockControllerInitialEventEmptyAdmin.hasRole(
                 timelockControllerInitialEventEmptyAdmin.PROPOSER_ROLE(),
@@ -438,6 +488,8 @@ contract TimelockControllerTest is Test {
             deployer
         );
         vm.expectEmit(true, true, true, false);
+        emit IAccessControl.RoleRevoked(DEFAULT_ADMIN_ROLE, deployer, deployer);
+        vm.expectEmit(true, true, true, false);
         emit IAccessControl.RoleGranted(DEFAULT_ADMIN_ROLE, self, deployer);
         vm.expectEmit(true, true, true, false);
         emit IAccessControl.RoleGranted(PROPOSER_ROLE, proposers[0], deployer);
@@ -508,6 +560,13 @@ contract TimelockControllerTest is Test {
                 self
             )
         );
+        assertTrue(
+            !timelockControllerInitialEventNonEmptyAdmin.hasRole(
+                timelockControllerInitialEventNonEmptyAdmin
+                    .DEFAULT_ADMIN_ROLE(),
+                deployer
+            )
+        );
         assertTrue(
             timelockControllerInitialEventNonEmptyAdmin.hasRole(
                 timelockControllerInitialEventNonEmptyAdmin.PROPOSER_ROLE(),
@@ -562,6 +621,24 @@ contract TimelockControllerTest is Test {
                 self
             )
         );
+        assertTrue(
+            !timelockControllerInitialEventNonEmptyAdmin.hasRole(
+                timelockControllerInitialEventNonEmptyAdmin.PROPOSER_ROLE(),
+                deployer
+            )
+        );
+        assertTrue(
+            !timelockControllerInitialEventNonEmptyAdmin.hasRole(
+                timelockControllerInitialEventNonEmptyAdmin.CANCELLER_ROLE(),
+                deployer
+            )
+        );
+        assertTrue(
+            !timelockControllerInitialEventNonEmptyAdmin.hasRole(
+                timelockControllerInitialEventNonEmptyAdmin.EXECUTOR_ROLE(),
+                deployer
+            )
+        );
         assertTrue(
             !timelockControllerInitialEventNonEmptyAdmin.hasRole(
                 timelockControllerInitialEventNonEmptyAdmin.PROPOSER_ROLE(),