Restore previous behavior of initializer during construction

[Amxx]

The following code is broken with 4.x:

contract A is Initializable {
    constructor() initializer { /* ... */ }
    /* ... */
}

contract B is A {
    constructor() initializer { /* ... */ }
    /* ... */
}

When B's constructor enters the initializer modifier, A's construction is already over. We are:

• not initializing
• version already set to 1.

This falls into the else part of _setInitializedVersion, which requires the next version to be greater than the previous one.

---

This would work, but requires the dev to affect version numbers that match the linearisation ...

contract B is A {
    constructor() reinitializer(2) { /* ... */ }
    /* ... */
}

---

PR Checklist

• Tests
• Documentation
• Changelog entry

[frangio]

I thought this PR was restoring previous behavior, but based on testing I don't think the example that this intends to fix was working before. In fact none of the two examples below were working before #3232.

contract A is Initializable {
    constructor() initializer {}
}

contract B is Initializable {
    constructor() initializer {}
}

contract DoesItInitialize1 is A, B {
    constructor() {}
}

contract DoesItInitialize2 is A, B {
    constructor() initializer {}
}

I think this PR would be restoring behavior that we intentionally (!) broke when we introduced onlyInitializing.

[Amxx]

I think this PR would be restoring behavior that we intentionally (!) broke when we introduced onlyInitializing.

I'm not sure it was intentional, and if it was, then I clearly didn't understand the full extend of it.

I would argue that the example you show should all work, and if they didn't then it's indeed not re-enabling an old behaviour, but I'd argue it's a desirable update nonetheless.

Do you want to delay that for 4.7 ?

[frangio]

After further review this indeed seems unrelated to #3232, and not related either to #3006 (introduction of onlyInitializing). This use case never worked, so I would put this in queue for 4.7.

[Amxx]

Indeed, this behavior has been broken since 4.0. It was possible to do so in 3.4.

[frangio]

I see. For reference, this was the 3.x code:

openzeppelin-contracts/contracts/proxy/Initializable.sol

Line 36 in 8e02960

require(_initializing || _isConstructor() || !_initialized, "Initializable: contract is already initialized");

The isConstructor check was removed in #2531, for no particularly compelling reason.

[Amxx]

The isConstructor check was removed in #2531, for no particularly compelling reason.

I think the reason was that we didn't see this modifier being used in constructor of the implementation. We discussed it covering a proxy under construction that would delegating call into the initialiser. That doesn't require special treatment, because you can expect all the initialiser code to be nested under the external initialiser.

Since then, we realised implementations should be locked at construction, and we encourage a usage that is different, that doesn't allow nesting, and this is problematic.

[frangio]

This feels like it enables reentrancy, like in GHSA-9c22-pwxw-p6hx. No?

[Amxx]

This was just fixed !

[frangio]

Since then, we realised implementations should be locked at construction, and we encourage a usage that is different

This is a great point to restore the behavior for constructors.

[frangio]

So apparently this isn't working with _disableInitializers in multiple parents. This is now the replacement for constructor() initializer so we should probably support it.

Could we do:

require((isTopLevelCall && version > currentVersion) || (!Address.isContract(address(this)) && version >= currentVersion));

The difference being the branch on the left uses strict >, and the branch on the right uses non-strict >=.

[Amxx]

This will only partially work.

If we have

contract A {
    constructor() initializer() {}
}
contract B {
    constructor() { _disableInitializers(); }
}
contract C is A, B {}
contract D is B, A {}

C is constructible, but D is not:

• B's constructor will set the currentVersion to 255
• A's constructor will try to run
  • we are in a isTopLevelCall but version < currentVersion (1 < 255)
  • there is no code at address(this) but version < currentVersion (1 < 255)

With
(isTopLevelCall && version > currentVersion) || (!Address.isContract(address(this)) && version == 1),
Both C & D are constructible.

[Amxx]

Would

(isTopLevelCall && version > currentVersion) || !Address.isContract(address(this)),

Possibly be enough?

[frangio]

That seems a bit reckless to me.

Alternatively we can do this:
(isTopLevelCall && version > currentVersion) || (!Address.isContract(address(this)) && (version == 1 || version == 255))
So as to support this only for the old and new patterns to disable initialization.