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

Rigidity in Initializable's INITIALIZABLE_STORAGE slot #4782

Open
howydev opened this issue Dec 6, 2023 · 4 comments · May be fixed by #5337
Open

Rigidity in Initializable's INITIALIZABLE_STORAGE slot #4782

howydev opened this issue Dec 6, 2023 · 4 comments · May be fixed by #5337
Milestone

Comments

@howydev
Copy link

howydev commented Dec 6, 2023

🧐 Motivation
Most ERC4337/ERC6900 smart contract accounts use the UUPSUpgradeable proxy pattern today. In OZ5.0's Initializable, the same INITIALIZABLE_STORAGE slot is used. As such, if more than 1 SCA implementation uses OZ5.0's Initializable, when a proxy is initialized with one of those implementations, it would not upgrade to other SCA implementations that require an initializer function to set up - as such, for security reasons, no SCA can use the Initializable library

📝 Details

  1. Consider switching _getInitializableStorage() or INITIALIZABLE_STORAGE to internal virtual and allow parent contracts to choose which storage slot to use. This also allows child contracts to group storage slots together when verkle trees are adopted without having to worrying about storage collisions across versions.
  2. Consider switching to a deterministic, variable INITIALIZABLE_STORAGE instead. One possible implementation is:
bytes32 private immutable INITIALIZABLE_STORAGE;
constructor() {
   bytes32 seed = keccak256(abi.encode("openzeppelin.storage.Initializable", address(this) ));
   INITIALIZABLE_STORAGE = keccak256(abi.encode(uint256(seed ) - 1)) & ~bytes32(uint256(0xff));
}

Credit: @gpersoon

@ernestognw
Copy link
Member

Hey @howydev, sorry for the late response.

I'm not sure if I'm following. If I understand correctly, the issue is that most SCA use the UUPSUpgradeable pattern (I assume including an Initializable in the inheritance tree), and therefore, if one of these SCA implementations uses Initializable as well, a contract can't be initialized twice. Is this correct?

If this is the case, I think you can use the reinitializer function to reuse the _initialized slot, but that depends on the setup.

Can you provide a concrete example where the use of Initializable becomes a problem?

Consider switching _getInitializableStorage() or INITIALIZABLE_STORAGE to internal virtual and allow parent contracts to choose which storage slot to use. This also allows child contracts to group storage slots together when verkle trees are adopted without having to worrying about storage collisions across versions.

The INITIALIZABLE_STORAGE is following the EIP-7201 formula to avoid storage collisions and also considers grouping variables for verkle tree adoption. I think we'd need a huge reason to allow overriding the storage pointer since implementing it correctly is not trivial.

Consider switching to a deterministic, variable INITIALIZABLE_STORAGE instead. One possible implementation is:

This sounds better, but I think an example of how Initializable can't be reused would help us have an opinion on this.

@howydev
Copy link
Author

howydev commented Jan 9, 2024

Thanks for the response @ernestognw. Big fan of yours, hope you're having a great new year so far!

Yep, thats a good summary!

The INITIALIZABLE_STORAGE is following the EIP-7201 formula to avoid storage collisions and also considers grouping variables for verkle tree adoption

Yup the chosen storage slot is in line with 7201! However, to fully reap the benefits of 7201, contracts have to intentionally design storage to be clumped. For a private initializer, all implementations are forced to use the 99 slots after keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.Initializable")) - 1)) & ~bytes32(uint256(0xff)). For the average SC using Initializable, it would be slightly bad having to declare keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.Initializable")) - 1)) & ~bytes32(uint256(0xff)) as a const in their main contracts to reap this benefit, when they would much prefer to use an identifier like companyABC.storage.ProtocolXYZ instead

Can you provide a concrete example where the use of Initializable becomes a problem?

From a high level, these are how we think about smart accounts:

  1. Users need to be able to upgrade/downgrade to different versions of the accounts. E.g. Alchemy v1 smart account -> Alchemy v2 smart account and vice versa. There are 2 possibilities here - v2 accounts are backward compatible and we want to reuse the same storage layout through an inherited storage pattern, or v2 accounts are not backward compatible so we want empty storage
  2. Users need to have optionality, to be able to upgrade to a smart account from a different company. E.g. Alchemy v1 smart account -> Zerodev v1 smart account
  3. Each implementation should only be able to call initialize once - this is a security requirement since initialize usually sets up ownership of the account, so it's a path a user can lose their account to. Having it not be single-use-per-implementation incurs additional education/security overhead which is very undesirable

For regular protocols, because they design every version of their protocol, they can use the same storage structs or use append-only inherited storage as a solution to minimize storage collision risk. We can adhere to this internally to allow safe upgrades from an Alchemy v1 account to an Alchemy v2 account. But what about the Alchemy v1 smart account -> Zerodev v1 smart account path? If Alchemy and Zerodev both use OZ.Initializable, over a long timespan of versions, storage collisions are nearly guaranteed. Therefore, to derisk, both Alchemy and Zerodev (and all SCA builders) must each choose our own unique storage locations

I can't think of a way to achieve the above with the current reinit design without requiring massive collaboration across the ecosystem around assigning version id numbers since we need them to be unique. I think the strictly increasing id numbers also prevent downgrades. Let me know if I overlooked something here

I think we'd need a huge reason to allow overriding the storage pointer since implementing it correctly is not trivial.

Thinking about this a big more - a large number of Initializable contracts must use some kind of access control to protect the upgradeTo/upgradeToAndCall function. Looking at the most used one, OwnableUpgradeable, it looks like any contract using both would not be able to fully reap the benefits of verkle trees. If that's the case, then i think the slot should either be variable, or the verkle tree optimization is not used and maybe should be removed

@Amxx Amxx added this to the 5.3 milestone Nov 20, 2024
@ernestognw
Copy link
Member

Heads up we're reconsidering this @howydev, I agree with the use cases you pointed out and I also believe that an account should easily change between implementations safely. I had a couple of repeated convos about storage collisions.

I guess the solution for fixing this for the Initializablecontract would be to make it internal and virtual so that it can be overridden with a big warning.

Looking at the most used one, OwnableUpgradeable, it looks like any contract using both would not be able to fully reap the benefits of verkle trees

We're still to see when Verkles are going to become a thing so this is still open for discussion. The general idea is that storage would cost pretty much the same for that namespace even if it's not optimized for Verkle, whereas the optimization will help with data structures that are natively implemented in sequential storage locations

@howydev
Copy link
Author

howydev commented Nov 20, 2024

Thanks for reconsidering! internal virtual works great and would allow smart account builders to use it

For the verkle case, you would need to access multiple storage slots beyond just the initializable slot. Since we should only be doing that on first init, probably not much savings for packing them for this use case

@ernestognw ernestognw linked a pull request Dec 3, 2024 that will close this issue
3 tasks
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 a pull request may close this issue.

3 participants