Skip to content

Commit

Permalink
🔒 Revoke Admin Role From Deployer (#259)
Browse files Browse the repository at this point in the history
### 🕓 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]>
  • Loading branch information
pcaversaccio authored Jun 26, 2024
1 parent 4605c6d commit feb2dc0
Show file tree
Hide file tree
Showing 7 changed files with 103 additions and 20 deletions.
34 changes: 17 additions & 17 deletions .gas-snapshot
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions src/snekmate/governance/mocks/timelock_controller_mock.vy
Original file line number Diff line number Diff line change
Expand Up @@ -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_)
7 changes: 7 additions & 0 deletions src/snekmate/governance/timelock_controller.vy
Original file line number Diff line number Diff line change
Expand Up @@ -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_)
Expand Down
1 change: 0 additions & 1 deletion src/snekmate/tokens/erc1155.vy
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
1 change: 0 additions & 1 deletion src/snekmate/tokens/erc20.vy
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
1 change: 0 additions & 1 deletion src/snekmate/tokens/erc721.vy
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
77 changes: 77 additions & 0 deletions test/governance/TimelockController.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,12 @@ contract TimelockControllerTest is Test {
self
)
);
assertTrue(
!timelockController.hasRole(
timelockController.DEFAULT_ADMIN_ROLE(),
deployer
)
);
assertTrue(
timelockController.hasRole(
timelockController.PROPOSER_ROLE(),
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -323,6 +349,12 @@ contract TimelockControllerTest is Test {
self
)
);
assertTrue(
!timelockControllerInitialEventEmptyAdmin.hasRole(
timelockControllerInitialEventEmptyAdmin.DEFAULT_ADMIN_ROLE(),
deployer
)
);
assertTrue(
timelockControllerInitialEventEmptyAdmin.hasRole(
timelockControllerInitialEventEmptyAdmin.PROPOSER_ROLE(),
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -508,6 +560,13 @@ contract TimelockControllerTest is Test {
self
)
);
assertTrue(
!timelockControllerInitialEventNonEmptyAdmin.hasRole(
timelockControllerInitialEventNonEmptyAdmin
.DEFAULT_ADMIN_ROLE(),
deployer
)
);
assertTrue(
timelockControllerInitialEventNonEmptyAdmin.hasRole(
timelockControllerInitialEventNonEmptyAdmin.PROPOSER_ROLE(),
Expand Down Expand Up @@ -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(),
Expand Down

0 comments on commit feb2dc0

Please sign in to comment.