Update initializer modifier to prevent reentrancy during initialization

[Amxx]

This issue was identified and reported by @chaitinblockchain through our bug bounty on Immunefi.

---

It is currently possible for initializer()-protected functions to be executed twice, if this happens in the same transaction. For this to happen, either one call has to be a subcall the other, or both call have to be subcalls of a common initializer()-protected function.

This can particularly be dangerous is the initialization is not part of the proxy construction, and reentrancy is possible by executing an external call to an untrusted address.

This PR is a move toward fixing this potential issue, by forbidding initializer()-protected function to be nested when the contract is already constructed and re-entrance is possible.

⚠ This change is breaking in some cases. ⚠

This change require changes to the upgradeable contracts:

• During the transpilation process, the __XXX_init function must use the new onlyInitializing() modifier instead of initializer()

PR Checklist

• Tests
• Documentation
• Changelog entry

[Amxx]

looking at the old mocks, and what they have become, I really think it was better before ... despite re-entrancy potential :/

[slendermaan]

Hello everyone,i'm Lucas_LMC. I'm sorry to bother you. I'm a little care about whether this issue will be included in the Security advisories section of the openzepplin library.

[frangio]

Hi @slendermaan yes once we release. We had to postpone the release until tomorrow.

[PaulRBerg]

Hey guys, I stumbled upon this PR after reading the comments added in the latest implementation of Initializable. I recently upgraded to the most recent version of the @openzeppelin/contracts-upgradeable package and that has made some of our tests to fail - it looks like we were indeed executing two initializers, one of which was a subcall of the other.

I'm trying to wrap my head around this, though:

This can particularly be dangerous is the initialization is not part of the proxy construction, and reentrancy is possible by executing an external call to an untrusted address.

@Amxx, could you share an example for when the previous behavior would have been problematic?

[Amxx]

Let's say you have:

• A proxy (P)
• A logic contract (L) that includes an initialize function. This function is protected by the initializer modifier
• An external contract (E)

Now the flow:

• Step 1: P is constructed, and set to point to L, but not initialized.
• Step 2: We call the initializer in P
  • 2.A: this is expected to do the initialization, and set the and register that it has been done, so it cannot be initialized
  • 2.B: (the issue is here) the initialization might call (E). We are still in the initialization phase.
  • 2.C: E can call initialize on P, with different settings. We are still in the initialization phase.
  • 2.D: initializer accepts that call, because we are in the initialization phase.
  • 2.E: P possibly gets reinitialized, with E controlled parameters.

By spliting the initializer modifier (for external functions) and the onlyInitializating (for internal functions) we make sure that the external initializer call can still trigger the internal ones, but 2.D will throw.

If the initializer is done during construction, then we don't have this threat because E would not be able to call P (because P is not yet constructed)

[PaulRBerg]

Very interesting. But I think this was low-risk, wasn't it?

Firstly, most upgradeable proxy deployers take care of initialization atomically - I think this is what hardhat-upgrades does.

Secondly, I haven't seen many contracts making calls to external contracts during construction. The vast majority of contracts just call some internal function inherited from other contracts.

[frangio]

Yes, this was low severity.

Have you been able to fix your issue? You should probably use onlyInitializing instead of initializer.

[PaulRBerg]

Have you been able to fix your issue? You should probably use onlyInitializing instead of initializer.

Yeah that's exactly what I did. Thanks for confirming!

[Rassska]

But how does initialization invokes E? Why it should make a call into some external contract which is not under our control?

[frangio]

Why it should make a call into some external contract which is not under our control?

This may be part of the logic of the initializer body for some reason. To be clear: the initializer modifier would never initiate this external call on its own.