Ownable2StepUpgradeable requires initializing OwnableUpgradeable

[ericglau]

🧐 Motivation Ownable2StepUpgradeable inherits OwnableUpgradeable.

Ownable2StepUpgradeable has initializer __Ownable2Step_init() OwnableUpgradeable has initializer __Ownable_init(address initialOwner)

Currently contracts that inherit Ownable2StepUpgradeable need to call __Ownable_init(address initialOwner) to set the initial owner. This is not obvious because users who are just inheriting Ownable2StepUpgradeable directly may not know about this transitive requirement.

This differs from the non-upgradeable scenario which was discussed in https://github.com/OpenZeppelin/openzeppelin-contracts/issues/4368#issuecomment-1595924091, because contracts that extend the non-upgradeable Ownable2Step would fail compilation if they did not call the Ownable constructor.

Next steps Consider whether the initializer of Ownable2StepUpgradeable should also initialize the owner. This would occur if Ownable2Step calls its parent constructor, however there are some drawbacks to multiple inheritance mentioned in comment https://github.com/OpenZeppelin/openzeppelin-contracts/issues/4368#issuecomment-1597675190

[ernestognw]

Note we have other examples of contract initializers only calling their parent's initializable.

• ERC2771Forwarder
• ERC20Permit

[veljkoTNFT]

All good if that is how it should, it just needs to be mentioned somewhere, that that is how it generaly works, and for people to pay attention to it.

[ernestognw]

In my opinion, it should call Ownable's constructor because just writing it in the docs creates friction and is error-prone because it's not enforced by any checks.

Although mentioned, I'd prefer having a dev experience that's secure by default. It's also true that is not likely that someone deploys an uninitialized Ownable seriously to production, but it was the default behavior in 4.9 so it isn't crazy to think someone will ignore it.

[veljkoTNFT]

It happened to me, but I had tests that cover this case, not sure if everyone does. Maybe something like breaking changes will help, because this is really easy to miss, as it was for me.

[ericglau]

A relevant detail is that in 4.9.3, Ownable2StepUpgradeable's initializer does call OwnableUpgradeable's initializer: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/v4.9.3/contracts/access/Ownable2StepUpgradeable.sol#L21

But in 5.0.0, it doesn't: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/v5.0.0/contracts/access/Ownable2StepUpgradeable.sol#L37

[veljkoTNFT]

Yup, that was exactly where I was coming from initially. I used 4.7.3 and then switched to 5.

[aviggiano]

Hello

I was about to submit this.

While developing a new project with v5, I was expecting Ownable2StepUpgradeable to automatically initialize OwnableUpgradeable, but then my onlyOwner-guarded functions would not work, which led me to find this issue.

I agree with @ernestognw's suggestion, this is not only a matter of having better docs.

[ernestognw]

Thanks for the feedback to both.

Something worth noting is that we've previously identified that OZ upgrade plugins should check lack of initialization as well (among other things).

For now, adding Ownable to Ownable2Step's constructor shouldn't be a breaking change for 5.1. I'll tag this as a potential 5.x milestone but may not be implemented if we find any backwards compatibility issue.

[ernestognw]

We chatted about this internally and @Amxx reached the following conclusion:

Consider the following example:

import "@openzeppelin/[email protected]/access/Ownable.sol";

contract OwnableExtension1 is Ownable {
    bool public a;
    constructor(address initialOwner) Ownable(initialOwner) {
        a = true;
    }
}

contract OwnableExtension2 is Ownable {
    bool public b;
    constructor(address initialOwner) Ownable(initialOwner) {
        b = true;
    }
}

contract MyContract is OwnableExtension1, OwnableExtension2 {
    constructor(address initialOwner
        OwnableExtension1(initialOwner)
        OwnableExtension2(initialOwner)
    {}
}

If either OwnableExtension1's or OwnableExtension2's constructor is not called in MyContract ... then we get errors saying: TypeError: No arguments passed to the base constructor. Specify the arguments or mark "MyContract" as abstract. But if we declare both constructors, we get: DeclarationError: Base constructor arguments given twice.

This is a problem that not only applies to Ownable but to any contract following the pattern of calling the parent's constructor. It just happened to Ownable because it's the only case in the library where we also extend the base contract (Ownable).

Generally, this will be a problem for any inheritance tree where the linearization graph is either incomplete or initializes something twice. So I personally don't see a point in fixing just the case of Ownable if it's a broader problem.

We've decided that this is something we should definitely provide along with upgrade-plugins. In a way, is like the replacement for the Solidity inheritance checks (eg. double constructor or missing constructors) but for upgrades. Quoting our current documentation:

The function __{ContractName}_init_unchained found in every contract is the initializer function minus the calls to parent initializers, and can be used to avoid the double initialization problem, but doing this manually is not recommended. We hope to be able to implement safety checks for this in future versions of the Upgrades Plugins.

We'll keep track of this update through this issue in upgrades plugins.