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

Restore previous behavior of initializer during construction #3344

Merged
merged 9 commits into from
May 25, 2022

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Apr 16, 2022

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

@Amxx Amxx force-pushed the fix/initializable/constructors branch from a93cc94 to fb6a39d Compare April 16, 2022 07:31
@frangio frangio changed the title Fix an issue where multiple contract in the inheritance tree have initializer constructors Restore previous behavior of initializer during construction Apr 20, 2022
@frangio
Copy link
Contributor

frangio commented Apr 20, 2022

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
Copy link
Collaborator Author

Amxx commented Apr 21, 2022

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
Copy link
Contributor

frangio commented Apr 21, 2022

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
Copy link
Collaborator Author

Amxx commented Apr 22, 2022

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

@frangio
Copy link
Contributor

frangio commented Apr 23, 2022

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

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

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

@frangio frangio added this to the 4.7 milestone Apr 23, 2022
@Amxx
Copy link
Collaborator Author

Amxx commented Apr 24, 2022

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.

it('initializer modifier reverts', async function () {
await expectRevert(this.contract.initializerNested(), 'Initializable: contract is already initialized');
it('initializer modifier can be nested', async function () {
await this.contract.initializerNested();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just fixed !

@frangio
Copy link
Contributor

frangio commented May 4, 2022

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
Copy link
Contributor

frangio commented May 23, 2022

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
Copy link
Collaborator Author

Amxx commented May 24, 2022

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
Copy link
Collaborator Author

Amxx commented May 24, 2022

Would

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

Possibly be enough?

@frangio
Copy link
Contributor

frangio commented May 25, 2022

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.

@frangio frangio merged commit 61294a6 into OpenZeppelin:master May 25, 2022
@frangio frangio mentioned this pull request Jun 2, 2022
@Amxx Amxx deleted the fix/initializable/constructors branch June 3, 2022 15:08
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 this pull request may close these issues.

2 participants