From feb2dc084c7d817b0d93cbd533396881ba24bb30 Mon Sep 17 00:00:00 2001 From: sudo rm -rf --no-preserve-root / Date: Wed, 26 Jun 2024 14:53:39 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=20Revoke=20Admin=20Role=20From=20D?= =?UTF-8?q?eployer=20(#259)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### 🕓 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: https://github.com/OpenZeppelin/openzeppelin-contracts/issues/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 --- .gas-snapshot | 34 ++++---- .../mocks/timelock_controller_mock.vy | 2 + .../governance/timelock_controller.vy | 7 ++ src/snekmate/tokens/erc1155.vy | 1 - src/snekmate/tokens/erc20.vy | 1 - src/snekmate/tokens/erc721.vy | 1 - test/governance/TimelockController.t.sol | 77 +++++++++++++++++++ 7 files changed, 103 insertions(+), 20 deletions(-) diff --git a/.gas-snapshot b/.gas-snapshot index 56706631..ba6b13d8 100644 --- 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 index 032a662f..755af3e3 100644 --- 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 index 3ead8ac9..ffb62476 100644 --- 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 index 9a3be177..8e5d5a32 100644 --- 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 index 04f6dd76..55ddfc67 100644 --- 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 index 353c174d..e1e26902 100644 --- 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 index 56d6b150..71826ca2 100644 --- 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(),