Storage Slot Collisions in Diamond-Compatible Delegatecall Architecture (Upgradeable Contracts)

[EricForgy]

🧐 Motivation
OpenZeppelin’s upgradeable contracts are widely used in standard proxy-based deployments. However, when used in a modular delegatecall architecture (such as the Diamond Standard or similar plugin-style routers), fixed storage slot constants across multiple modules can cause critical storage collisions.

Each module is executed in the same storage context (the proxy/Router), so repeated use of hardcoded slot identifiers like INITIALIZABLE_STORAGE leads to overlapping writes between otherwise isolated modules. This makes current upgradeable contracts unsafe in modular delegatecall setups — an increasingly common pattern in modern contract systems.

---

📝 Details
I propose a backward-compatible enhancement to support diamond-compatible usage by deriving storage slot positions uniquely per module based on their deployed address.

This can be done using the following pattern:

address private immutable __self = address(this);

bytes32 private immutable INITIALIZABLE_STORAGE =
    keccak256(
        abi.encode(
            uint256(keccak256(abi.encodePacked(__self, "openzeppelin.storage.Initializable"))) - 1
        )
    ) & ~bytes32(uint256(0xff));

Why this works:

• Safe: Each module gets a deterministic, unique slot namespace.
• Aligned: The bitmask aligns with EIP-1967 slot layout expectations.
• Minimal Overhead: Only requires switching some pure functions to view due to __self, with no storage reads and negligible gas impact.
• Compatible: Initialization checks are typically only called once, so runtime cost is not a concern.

Suggested Implementation

Introduce a new base contract or modifier version (e.g. DiamondCompatibleInitializable) that mirrors Initializable but uses per-module slot derivation. This could live in an experimental or diamond subdirectory to avoid impacting the existing suite.

---

Let me know if you want this as a full PR — happy to help.

[EricForgy]

Update: pure Mutability Blocks Delegatecall-Compatible Storage Slot Overrides

Upon further inspection, I realized that although Initializable provides a virtual storage slot function (_initializableStorageSlot()), it is marked as pure. This prevents it from being overridden using immutable values like __self = address(this) that depend on constructor context.

Additionally, the private helper function _getInitializableStorage() is also marked pure, which compounds the problem, since it depends on _initializableStorageSlot().

---

The Problem

In delegatecall-based modular architectures (e.g., Diamond Standard), each module needs a unique storage slot to avoid clashing. The pattern I'm looking at would be:

address internal immutable __self = address(this);

bytes32 private immutable INITIALIZABLE_STORAGE =
    keccak256(abi.encode(uint256(keccak256(abi.encodePacked(__self, "openzeppelin.storage.Initializable"))) - 1)) &
        ~bytes32(uint256(0xff));

function _initializableStorageSlot() internal view override returns (bytes32) {
    return INITIALIZABLE_STORAGE;
}

However, this is not allowed because:

• The original function is pure
• Overriding it with view causes a compiler error
• _getInitializableStorage() also expects the slot getter to be pure

---

Proposed Fix

Update the mutability of following two functions in Initializable.sol from pure to view:

function _initializableStorageSlot() internal view virtual returns (bytes32) {
    return INITIALIZABLE_STORAGE;
}

function _getInitializableStorage() private view returns (InitializableStorage storage $) {
    bytes32 slot = _initializableStorageSlot();
    assembly {
        $.slot := slot
    }
}

This would:

• Allow derived contracts to override the slot computation using immutable values (e.g., __self)
• Enable full compatibility with delegatecall-based modular contract systems
• Introduce no practical downside: these functions are only used during initialization logic, which is rarely called on-chain

---

Broader Context

This problem pattern may exist in other OpenZeppelin upgradeable contracts that expose bytes32 slot logic via pure functions. I'd be happy to help identify and propose similar updates to make the full suite compatible with modular architectures.

Let me know if this is of interest — I'm happy to open a PR.

[ernestognw]

Hi @EricForgy

All upgradeable contracts are designed to avoid storage collision via EIP-7201, and we have built a whole suite of tools to prevent storage collisions: https://docs.openzeppelin.com/upgrades-plugins/

Although the collision scenarios you describe are potentially true, it is intended for upgradeable contracts to keep using the same store locations given the library maintains Backwards Compatibility between major versions. Adding the address to the storage root calculation would (for example) clean everyone's balances after upgrading.

We're considering adding virtual to the _getStorageSlot getters but I'd insist in keeping them pure. The purpose would be to customize them according to the use case but I think it's an error (and potentially dangerous) to tie the address itself to its storage root.

[EricForgy]

Hi @ernestognw,

You're right — just making the storage slot getters virtual (while keeping them pure) would be sufficient for my use case. I appreciate you pointing that out.

If the team is open to it, I’d be happy to submit PRs to add virtual to the relevant functions across the upgradeable contracts.

Thanks again.

[EricForgy]

Hi again,

After realizing the upgradeable contracts are transpiled from the base repo, I looked into adjusting the storage slot logic more directly. The specific problem I’m addressing is in ReentrancyGuardTransientUpgradeable, which currently hardcodes its slot:

REENTRANCY_GUARD_STORAGE

In a setup where calls may be delegated to multiple contracts, this leads to slot collisions.

In my fork, I made the following change:

• Introduced a _reentrancyGuardStorageSlot() function and marked it pure virtual
• Updated all internal references to the constant to instead call this function
• Added a default implementation that returns the existing REENTRANCY_GUARD_STORAGE constant

This allows consumers like me to override just the slot position in a safe way without touching any core logic. Here's the commit for reference:

🔗 Reference commit on my fork

The actual fix would need to be implemented in the transpiler. Unfortunately, I don’t know how to do that. If there is interest, I could try to figure it ouot, but would probably need some help.

[EricForgy]

While investigating how the transpiler works, I realized that ReentrancyGuardTransient.sol in @openzeppelin/contracts is already upgrade-compatible, since it relies on a named storage slot (and requires no initialization). Because of that, I submitted a PR directly to the base contracts repo to introduce _reentrancyGuardStorageSlot() as a pure virtual function, allowing slot overrides in delegatecall-based modular systems.

Thank you for your consideration.

[gonzaotc]

Hey @EricForgy,

We’ll move forward with making the storage slot getters pure virtual since we agreed it is a good tradeoff between flexibility and security, and although is something we have already been planning, I thank you for raising it back.

However, just to clarify, this doesn’t imply we’ll be officially supporting the Diamond Pattern in our libraries — at least not at this point. Our focus remains on production-ready support for UUPS, Transparent, and Beacon patterns via @openzeppelin/upgrades for performing upgrades in a safe manner, which continue to be our recommended upgrade paths. Be aware that the openzeppelin/upgrades will not be able to validate your diamond upgrades to be safe.

[EricForgy]

Thanks so much - I really appreciate the thoughtful response and your willingness to move forward with this change 🙏

[Amxx]

Hello,

@gonzaotc mentionned this issue during our weekly call yesterday, particularly in the context of "diamond proxy" that use multiple implementation. The point was made that these multiple implementations (facets) may want to use independant reentrancy guard, and that collision of the transiant slot used for that would "couple" the modifier.

While I understand this usecase, I personally believe that making this slot virtual here is probably not the best approach.

• Lets say you have two facets, and these two facets need independant reentrancy guards, but then you want to refactor your code and merge the two facets into a single contract. How do you keep the guard independant ?

• Lets say you have two facets (A and B). A has one non-reentrant function (A.f1), and B has two non-reentrant functions (B.f2 and B.f3). Now lets say A.f1 and B.f2 should be coupled, but B.f3 should be independant. How do you do that?

IMO the better option is to keep the slot "location" of the reentrancy guard(s) constant across facets, like it is today, but allow the dev to derive multiple independant guards from it.

It would look like

abstract contract ReentrancyGuardTransient {
    using SlotDerivation for bytes32;
    using TransientSlot for *;

    // keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.ReentrancyGuard")) - 1)) & ~bytes32(uint256(0xff))
    bytes32 private constant REENTRANCY_GUARD_STORAGE =
        0x9b779b17422d0df92223018b32b4d1fa46e071723d6817e2486d003becc55f00;

    error ReentrancyGuardReentrantCall(bytes32 key);

    modifier nonReentrant() {
        _nonReentrantBefore(bytes32(0));
        _;
        _nonReentrantAfter(bytes32(0));
    }

    modifier nonReentrantWithKey(bytes32 key) {
        _nonReentrantBefore(key);
        _;
        _nonReentrantAfter(key);
    }

    function _nonReentrantBefore(bytes32 key) private {
        // On the first call to nonReentrant, REENTRANCY_GUARD_STORAGE.deriveMapping(key).asBoolean().tload() will be false
        if (_reentrancyGuardEntered(key)) {
            revert ReentrancyGuardReentrantCall(key);
        }

        // Any calls to nonReentrant after this point will fail
        REENTRANCY_GUARD_STORAGE.deriveMapping(key).asBoolean().tstore(true);
    }

    function _nonReentrantAfter(bytes32 key) private {
        REENTRANCY_GUARD_STORAGE.deriveMapping(key).asBoolean().tstore(false);
    }

    function _reentrancyGuardEntered(bytes32 key) internal view returns (bool) {
        return REENTRANCY_GUARD_STORAGE.deriveMapping(key).asBoolean().tload();
    }

    function _reentrancyGuardEntered() internal view returns (bool) {
        return _reentrancyGuardEntered(bytes32(0));
    }
}

That would allow the devs that want independant reentrancy guard in their facets to use a "per-facet" key.

@EricForgy wdyt ? In your case, __self could be the key

[EricForgy]

Hi @Amxx,

Thank you for the thoughtful response and proposed alternative.

Just to clarify upfront: I’ve moved away from the __self approach after @ernestognw’s earlier feedback. The current proposal relies solely on overriding the slot getter to provide a unique bytes32 key, as you suggested.

For example, here's how I use it in practice (for Initializable, though the same applies to ReentrancyGuard):

https://github.com/CavalRe/cavalre-contracts/blob/6a1c306a13d62e5096b3ae378d4496503e4a15a2/contracts/Multitoken/Multitoken.sol#L724-L739

By overriding the slot getter, developers can assign a unique slot (i.e., key) per module or context. This is sufficient to avoid collisions in modular systems like diamond routers while maintaining compatibility with existing tooling.

Regarding the possible merging of facets: while I understand the concern, I’d argue that in practice, the need to merge independently guarded modules is rare, and there’s no added gas cost or maintenance burden in keeping them separate. Prioritizing simplicity and efficiency in the common case seems like a good tradeoff here.

Compared to the alternative proposal - which introduces additional indirection and dependency on SlotDerivation - I believe this PR offers a cleaner, cheaper solution that still covers the necessary use cases. Both approaches work, but I’d prefer to keep the current one if possible.

Best regards,
Eric

Edit: I do like the key idea too. I can see it being useful if your want fine control even at a function level within a given facet. What if we keep this PR with pure virtual slot getter and separately add nonReentrantWithKey?