Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add explicit admin address to TimelockControl constructor #3720

Closed
DanielVF opened this issue Sep 22, 2022 · 4 comments · Fixed by #3722
Closed

Add explicit admin address to TimelockControl constructor #3720

DanielVF opened this issue Sep 22, 2022 · 4 comments · Fixed by #3722

Comments

@DanielVF
Copy link
Contributor

I've found a dozen or active projects on mainnet using multi-sigs or voting that missed the note about revoking the admin permissions from the deployer of TimelockController.

The timelock is often deployed unmodified, and often a developer's only interaction with the timelock is a line or two in a deploy file. This means that the only thing they see is the constructor and its method names. Seeing these names, it's logical to conclude that the timelock only has two permissions - and to not know that super admin permissions are granted in the constructor to the deployer key.

I think a way of clearing up this confusion is to have the super admin powers, outside the timelock controlling itself, be explicitly granted as a list in the constructor, just like the other permissions.

(Tweets: https://twitter.com/danielvf/status/1572963306725318657)

@DanielVF DanielVF changed the title Add admin roles to TimelockControl constructor? Add explicit admin addresses to TimelockControl constructor? Sep 22, 2022
@DanielVF
Copy link
Contributor Author

image

@frangio
Copy link
Contributor

frangio commented Sep 22, 2022

We agree and will be changing this in the upcoming 4.8 release.

@frangio frangio changed the title Add explicit admin addresses to TimelockControl constructor? Add explicit admin address to TimelockControl constructor Sep 22, 2022
@0xJacobs
Copy link

Please what the difference between setting Oz ownable (msg.sender) to the constructor of a contract and just importing it and the only owner modifier from the Oz to a child contract.

@0xJacobs
Copy link

So that means there should be no need to set msg.sender in the constructor of the child contract? It automatically does that?

pcaversaccio added a commit to pcaversaccio/snekmate that referenced this issue Jun 26, 2024
### 🕓 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 <pascal.caversaccio@hotmail.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants