6

In OpenZeppelin's documentation, it's mentioned that we should be invoking the _disableInitializers function, otherwise our contract might be vulnerable to certain attacks. Can someone explain how would the attack be performed and why adding this function can prevent the attack from success?

I have been seeing contracts that don't following this advice, does that make them vulnerable to the attack or have OpenZeppelin fixed this vulnerability in newer versions of their upgradeable contracts.

3 Answers 3

3

A proxy works by borrowing mirroring all the execution logic of an implementation contract. The context of the proxy contract is kept, however (meaning the storage is read and written to in the proxy).

Take the following example. If you deploy an implementation I and have stuff in its constructor to modify/set variables, such as name, symbol for an ERC721 contract, then these variables will be set in the storage of I, once deployed. So, when deploying proxy P, that borrows the logic of I, those variables that were set in I don't ever affect P. This is because P will look for them in their pre-determined slots in its own local storage, instead of in I where they have actually been set.

That's why, in order to initialize the variables in P itself, the implementation contracts need to have an init() function that can be called when the proxy itself is deployed to set the variables in the context of P. This public init() function now needs some additional protection, because it should to be only called once and then never again (like a constructor). That's where the initializer modifier comes into play.

Let me add that the way OpenZeppelin handles the initializer modifier is, imo, very unnecessarily confusing, and prone to errors. But simply think of it as a boolean value isInitialized that is set to true and which blocks any additional calls.

So, we have now a public init() function in I, which is meant to be only called once in the context of the proxy P. However, currently, there is no guard from simply calling the init() function directly in I. This would initialize variables in the context of I, even though most of the time this is not intended to be done.

Often the init() function sets the owner to allow for future upgrades, for example. Since when using UUPS, all the upgrade logic is contained in I, it also contains the upgradeToAndCall function. Now, if anyone is able to run this function, then they could "upgrade" the implementation contract I itself to some malicious contract which contains some new logic, for example "selfdestruct". This way, contract I can be selfdestructed and P, because its a simple dumb proxy pointing to I will stop working. P also can't be upgraded anymore, because all the upgrade logic was contained in I.

Wormhole is a good example of a contract that forgot to secure the init() function.

OpenZeppelin recommends to run additional logic in the constructor of I (disableInitializer) to block the ability to call a public function secured by initializer.

My thoughts on how initializer should actually be implemented: Initializable.sol.

  • NEVER allow this to be called in the implementation contract directly
  • only allow this to be called during a deployment or upgrade migration
3

Previous 2 answers explain a lot.

  • In Transparent and Minimal Proxies it's not a problem.
  • in UUPS Contracts it's recommended for safety to prevent someone else from initializing your Implementation contract

From my experience, we have 2 nice options:

  1. add initializer modifier to the constructor, so the deployment of the Implementation will be a call of "alternative" initializer (you can have constructor and init both with modifier), hence preventing the call of others. But simply saying it will just set isInitialized to true

  2. Call special function inside the constructor

constructor() {
    _disableInitializers();
} 

which is also similar to setting isInitialized to true

I would recommend going with the second approach because it leaves a contract with only one initializer function, which is intended to be called only by proxy and only once.

2

TransparentProxy or minimal proxies don't suffer this kind of attack. The vulnerability is when using the UUPS style Proxy

That's why you have not see them implementing something like

constructor() {
    _disableInitializers();
}

On other upgradable contracts

The vulnerability for Open Zeppelin UUPS is nicely explained at https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.