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

Consider use of Ownable2Step over Ownable #7

Closed
0x-r4bbit opened this issue Sep 5, 2023 · 2 comments · Fixed by #10
Closed

Consider use of Ownable2Step over Ownable #7

0x-r4bbit opened this issue Sep 5, 2023 · 2 comments · Fixed by #10

Comments

@0x-r4bbit
Copy link
Member

There are a few contracts in this repository that inherit from OZ's Ownable and will be deployed by Status.
At the time of writing this issue, these contracts are:

  • CommunityTokenDeployer
  • OwnerTokenFactory
  • MasterTokenFactory

And, if #2 gets extended to use minimal contract proxies within the token factories, then both, OwnerToken and MasterToken contracts will be Ownable as they need to be deployed by Status as templates as well.

The entire system relies on a flawless transfer of ownership, which can be considered a security risk.
If, for whatever reason, Ownable.transferOwnership() is called with the wrong address, all future tokens deployed by community owners could be exploited.

I think we should consider the use of Ownable2Step over Ownable in these cases.
Ownable2Step makes transferring ownership a 2-step process, requiring the new owner to claim the transferred ownership, allowing the original owner to pull back.

Given the discussion happening in OpenZeppelin/openzeppelin-contracts#3620 there seems to be plenty of consensus that Ownable should've behaved like Ownable2Step in the first place. Another reason we should probably favour Ownable2Step over Ownable.

@3esmit what's your take on this?

@3esmit
Copy link
Member

3esmit commented Sep 8, 2023

The suggestion is an improvement to security, than it should be done. The proposal is to add a fool proof mechanic to the ownership transfer, and it does not hurt any other property of the system, it would only be slightly more expansive in gas, on an uncommon opreation.

@0x-r4bbit
Copy link
Member Author

Thank you for you opinion!
Will prepare a PR for this then!

0x-r4bbit added a commit that referenced this issue Sep 13, 2023
This has been discussed in #7 and be further explored in OpenZeppelin/openzeppelin-contracts#3620

Closes #7
0x-r4bbit added a commit that referenced this issue Sep 22, 2023
This has been discussed in #7 and be further explored in OpenZeppelin/openzeppelin-contracts#3620

Closes #7
0x-r4bbit added a commit that referenced this issue Sep 22, 2023
This has been discussed in #7 and be further explored in OpenZeppelin/openzeppelin-contracts#3620

Closes #7
0x-r4bbit added a commit that referenced this issue Sep 22, 2023
This has been discussed in #7 and be further explored in OpenZeppelin/openzeppelin-contracts#3620

Closes #7
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.

2 participants