-
Notifications
You must be signed in to change notification settings - Fork 455
✨ Add opt-in storage layout compatibility guard for UUPS #1500
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
base: main
Are you sure you want to change the base?
Conversation
|
Etherscan doesn’t support automatic verification of different storage slots as defined in EIP-1967. Therefore, adding a Also, if a developer wants to include this kind of logic, they can simply add it inside the |
|
Comment: Hi @atarpara, thanks for the review. I understand the hesitation to add surface area, but I'd like to make a case for why this specific safeguard belongs in the core library:
The goal isn't to force overhead, but to make the safest path the default path for developers who want it. If the concern is the number of hooks, would you be open to a single |
|
|
||
| /// @dev Optional hook to guard against storage layout incompatibility. | ||
| /// Override in implementation if desired (e.g. compare a constant layout hash). | ||
| function _checkStorageLayout(address newImplementation) internal virtual {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codermaybe The child contract won’t compile if it doesn’t implement _checkStorageLayout, because solady.UUPSUpgradeable is marked as abstract and the function is declared but not implemented. Abstract contracts with unimplemented functions require the child to provide an implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Therefore, the dev is required to implement both functions.
|
I believe there might be a misunderstanding. Since the function is declared with an empty body ({}), it is not an abstract function. Therefore, child contracts are not required to implement it. It serves as an optional hook (opt-in) with zero overhead for developers who don't need it. |
Description
Adds an optional storage layout compatibility check mechanism for UUPS upgrades to prevent storage collision vulnerabilities.
Changes
StorageLayoutMismatch()error toUUPSUpgradeable.sol_checkStorageLayout(address newImplementation)virtual hook (empty by default for backwards compatibility)MockUUPSImplementation.solusing version hash comparisonUsage Pattern
Implementations can override
_checkStorageLayoutto enforce storage compatibility:Design Decisions
Addresses the storage layout safety concern raised in the original UUPS test demo.
Closes #1489
Checklist
Ensure you completed all of the steps below before submitting your pull request:
forge fmt?forge test?