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

Update initializer modifier to prevent reentrancy during initialization #3006

Merged
merged 10 commits into from
Dec 10, 2021

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Dec 7, 2021

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

Amxx commented Dec 7, 2021

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

@frangio frangio changed the title update initializer contract to prevent reentry during initialization Update initializer modifier to prevent reentrancy during initialization Dec 7, 2021
CHANGELOG.md Show resolved Hide resolved
@frangio frangio merged commit 08840b9 into OpenZeppelin:master Dec 10, 2021
frangio added a commit that referenced this pull request Dec 10, 2021
…on (#3006)

Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
(cherry picked from commit 08840b9)
@Amxx Amxx deleted the refactor/initialize branch December 10, 2021 23:30
@slendermaan
Copy link

slendermaan commented Dec 13, 2021

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

frangio commented Dec 13, 2021

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

davidbrai added a commit to withtally/dao.new that referenced this pull request Jan 26, 2022
@PaulRBerg
Copy link
Contributor

PaulRBerg commented Mar 2, 2022

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

Amxx commented Mar 3, 2022

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

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

frangio commented Mar 3, 2022

Yes, this was low severity.

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

@PaulRBerg
Copy link
Contributor

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!

PaulRBerg added a commit to hifi-finance/hifi that referenced this pull request Mar 3, 2022
refactor(protocol): mark init function as "internal" in OwnableUpgradeable
refactor(protocol): rename "__OwnableUpgradeable_init" to "__Ownable_init"
test(protocol): new GodModeOwnableUpgradeable to expose init function

See releated discussion in OpenZeppelin/openzeppelin-contracts#3006
@Rassska
Copy link

Rassska commented Oct 30, 2022

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

@frangio
Copy link
Contributor

frangio commented Nov 4, 2022

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.

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.

5 participants