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