Add Ownable2Step extension with 2-step transfer

[heldersepu]

Fixes #2369
Resolves #1488

This contract adds an additional layer to the trusted Ownable.
The transfer is a two step process transferOwnership initiates the process and acceptOwnership makes it official.

Transfer of ownership is a delicate and irreversible process, it could leave a contract useless, with a two step process we add a guard against typos or bad copy/paste.

PR Checklist

• Tests
• Documentation
• Changelog entry

[frangio]

Thank you @heldersepu. However, it is not clear that this is the solution we want to implement.

I've been thinking we should add the two-step functionality as an opt-in feature into Ownable itself (so not as an extension). That way, the owner can choose if they prefer to use the two-step process for added safety.

@Amxx What do you think of this?

[Amxx]

I personally don't like the name of this extensions. Calling it SafeOwnable somehow implies that the current Ownable is not safe.

This I like the idea that it can be implemented as an extension. IMO extensions are the best way to "opt-in" without adding any bytecode to contracts that are not opting in.

[Amxx]

Still, I'm not really a fan of the proposed code. What I would do:

• I would rename _newOwner to _pendingOwner (and include a public accessor)
• I would keep the transferOwnership behavior to be a simple transfer (not override it).
• I would override the _transferOwnership so that it deletes the _pendingOwner
• I would add a dedicated function to initialize the transfer, and this function could be call anytime (by the active owner) to override the _pendingOwner.

[frangio]

I'd feel comfortable making the choice to include 2-step transfer by default with Ownable. Do you think users would be against that?

[nchamo]

I would love to Ownable have a 2 step transfer by default. We've built our own Ownable that does that, and I would love to get rid of it haha

[heldersepu]

I personally don't like the name of this extensions. Calling it SafeOwnable somehow implies that the current Ownable is not safe.

Ye, to me the current Ownable screams YOLO! ...
I feel many would agree that it leans to the not safe side, like not wearing a helmet while riding a motorbike, nothing wrong with that, I did it all the time, it is just not as safe as the alternative, but many do not want to be bother with those safeguards and accept the risks of the fast & less safe option.

... but I'm fine with any other name (maybe Ownable2Step or OwnableSafeTransfer)

[heldersepu]

Still, I'm not really a fan of the proposed code. What I would do:

• I would rename _newOwner to _pendingOwner (and include a public accessor)
• I would keep the transferOwnership behavior to be a simple transfer (not override it).
• I would override the _transferOwnership so that it deletes the _pendingOwner
• I would add a dedicated function to initialize the transfer, and this function could be call anytime (by the active owner) to override the _pendingOwner.

• Yes the name change I 100% agree _pendingOwner is a better description than _newOwner to that variable (done)
• keep the transferOwnership behavior ... big NO for me, that is the safety issue we are trying to plug.
• override the _transferOwnership ... I was going to do that, but in my initial research it require me to change the _owner variable from private, and that changed my mind, on second look it is easy with a call to parent via super (done)
• add a dedicated function to initialize the transfer ... ties back to point 2, that is what we want to prevent

[heldersepu]

I'd feel comfortable making the choice to include 2-step transfer by default with Ownable. Do you think users would be against that?

I wish we had statistic to back up our decisions on this topic ...

My feeling is that most developers are fine with Ownable as is, and are not really planning to use the transfer, they just like to have that option...
But for those more security conscious that will use the transfer often, an enforced 2-step transfer is the way

.. so no I would not change the existing Ownable but offer an option

[heldersepu]

I would love to Ownable have a 2 step transfer by default. We've built our own Ownable that does that, and I would love to get rid of it haha

Small world, we did the same, that is why I decided to contribute back what we did...
@nchamo how does your code compare to this PR?

[koolamusic]

I personally don't like the name of this extensions. Calling it SafeOwnable somehow implies that the current Ownable is not safe.

This I like the idea that it can be implemented as an extension. IMO extensions are the best way to "opt-in" without adding any bytecode to contracts that are not opting in.

I agree with @Amxx on making it an extension to reduce the amount of bytecode for users who do not want to opt-in. So using something like OwnableRequestExtension or OwnableRequest could suffice

[Amxx]

I'd feel comfortable making the choice to include 2-step transfer by default with Ownable. Do you think users would be against that?

That would break ERC173 compliance. Big no to me

[Amxx]

One thing I'd like to point out is that, even though is seems "unsafe" to transferOwnership directly, that is just like sending ERC20 tokens directly with no receive function on if the contract is a receiver. That is in retrospect a bad idea, but that is how the ecosystem has been working for years.

IMO changing that such a fundamental things would create more confusion then anything.

[Amxx]

I also think its important for a contract to be able to renounce ownership ... which is difficult to do in a clean way with the two step process.

So if we override transferOwnership so that it sets the pending owner, we should also have a forceTransferOwnership (or someting like that) is the owner really wants to send the send owership to 0xdead or something like that. Note that we consider removing the renounceOwnership from Ownable for better ERC173 compliance.

[frangio]

LGTM! Great work @heldersepu championing this, and thanks for the patience!

[gitpoap-bot]

Woohoo, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2022 OpenZeppelin Contracts Contributor:

Head on over to GitPOAP.io and connect your GitHub account to mint!

[CJ42]

@frangio @Amxx
We did something very similar at LUKSO called ClaimOwnership with the function claimOwnership() for the second step.

https://github.com/lukso-network/lsp-smart-contracts/blob/develop/contracts/Custom/ClaimOwnership.sol
https://github.com/lukso-network/lsp-smart-contracts/blob/develop/contracts/Custom/IClaimOwnership.sol

Just to curious to have your feedbacks on it, regarding the naming claimOwnership() and the differences in general (You can ignore the 2 steps part for renounceOwnership() in our implementation for the context of the question).

[frangio]

claimOwnership is good and it was actually the function name we had in Claimable in OpenZeppelin v1. I don't think there are other significant diferences.

I wonder if we should use claimOwnership in this contract as well given that it was the name we used previously.

[heldersepu]

Just to curious to have your feedbacks on it, regarding the naming claimOwnership() and the differences in general

@CJ42 I don't think that claim truly conveys what that contract (or function) is for ...

---

formally request or demand; say that one owns or has earned something

that feel like someone can put a claim of ownership, and that later if uncontested it will become reality...

... now, english is my 3rd language, so take my feedback with a grain of salt ...
When I first read your comment I thought your contract flipped the two step process, instead of current owner setting the pending, the process is started with future owner making the claim, that could be an interesting case, but that is not the case.

So no I don't think that claim is best used there

[SvenMeyer]

Great that we finally, after years, get a 2 step transfer of ownership process !
(The only OZ contract which I always regarded as unsafe.)

Just an additional idea (suggestion). I used the synthetix Owned.sol instead for years (which has likely been seen and used by many other projects as well).
https://github.com/Synthetixio/synthetix/blob/develop/contracts/Owned.sol
They named the functions nominateNewOwner and acceptOwnership.

... and why not just add these functions to Ownable ? It would result in ...

• full backwards compatibility
• current project would "auto-upgrade" to the new, safer version of Ownable
• transferOwnership in Ownable2Step actually does not do a transfer (which is kind of confusing).

[frangio]

I also argued above for including the 2-step functionality directly in Ownable and I still believe it's the best option.

We couldn't get consensus to go for that option mostly because it adds to bytecode size.