EIP712Upgradeable introduces a storage layout conflict

[mrmemes-eth]

Introducing the EIP712Upgradeable base contract creates a storage layout conflict at slot 352.

💻 Environment

Openzeppelin is pegged to v4.8.1 and we use foundry for our tooling.

📝 Details

We're using forge inspect --pretty for analyzing storage layout and comparing them with an existing known storage layout using diff -a --suppress-common-lines.

For an ERC721 NFT of ours, we see this conflict:

24,27c24,30
< | _tokenIdCounter         | struct CountersUpgradeable.Counter                           | 351  | 0      | 32    | src/OrigamiMembershipToken.sol:OrigamiMembershipToken |
< | tokenIdToBlockTimestamp | mapping(uint256 => uint256)                                  | 352  | 0      | 32    | src/OrigamiMembershipToken.sol:OrigamiMembershipToken |
< | _metadataBaseURI        | string                                                       | 353  | 0      | 32    | src/OrigamiMembershipToken.sol:OrigamiMembershipToken |
< | _transferEnabled        | bool                                                         | 354  | 0      | 1     | src/OrigamiMembershipToken.sol:OrigamiMembershipToken |
---
> | _HASHED_NAME            | bytes32                                                      | 351  | 0      | 32    | src/OrigamiMembershipToken.sol:OrigamiMembershipToken |
> | _HASHED_VERSION         | bytes32                                                      | 352  | 0      | 32    | src/OrigamiMembershipToken.sol:OrigamiMembershipToken |
> | __gap                   | uint256[50]                                                  | 353  | 0      | 1600  | src/OrigamiMembershipToken.sol:OrigamiMembershipToken |
> | _tokenIdCounter         | struct CountersUpgradeable.Counter                           | 403  | 0      | 32    | src/OrigamiMembershipToken.sol:OrigamiMembershipToken |
> | tokenIdToBlockTimestamp | mapping(uint256 => uint256)                                  | 404  | 0      | 32    | src/OrigamiMembershipToken.sol:OrigamiMembershipToken |
> | _metadataBaseURI        | string                                                       | 405  | 0      | 32    | src/OrigamiMembershipToken.sol:OrigamiMembershipToken |
> | _transferEnabled        | bool                                                         | 406  | 0      | 1     | src/OrigamiMembershipToken.sol:OrigamiMembershipToken |

And for an ERC20 token of ours we see this conflict:

20,22c20,25
< | _burnEnabled     | bool                                                           | 352  | 0      | 1     | src/OrigamiGovernanceToken.sol:OrigamiGovernanceToken |
< | _transferEnabled | bool                                                           | 352  | 1      | 1     | src/OrigamiGovernanceToken.sol:OrigamiGovernanceToken |
< | lockup           | mapping(address => struct OrigamiGovernanceToken.TransferLock) | 353  | 0      | 32    | src/OrigamiGovernanceToken.sol:OrigamiGovernanceToken |
---
> | _HASHED_NAME     | bytes32                                                        | 352  | 0      | 32    | src/OrigamiGovernanceToken.sol:OrigamiGovernanceToken |
> | _HASHED_VERSION  | bytes32                                                        | 353  | 0      | 32    | src/OrigamiGovernanceToken.sol:OrigamiGovernanceToken |
> | __gap            | uint256[50]                                                    | 354  | 0      | 1600  | src/OrigamiGovernanceToken.sol:OrigamiGovernanceToken |
> | _burnEnabled     | bool                                                           | 404  | 0      | 1     | src/OrigamiGovernanceToken.sol:OrigamiGovernanceToken |
> | _transferEnabled | bool                                                           | 404  | 1      | 1     | src/OrigamiGovernanceToken.sol:OrigamiGovernanceToken |
> | lockup           | mapping(address => struct OrigamiGovernanceToken.TransferLock) | 405  | 0      | 32    | src/OrigamiGovernanceToken.sol:OrigamiGovernanceToken |

With both of these diffs, you can see conflicts in storage slot 352 (and 353 as well for the ERC721).

🔢 Code to reproduce bug

I have a Pull Request open on my own repository that reproduces the issue. The only change (as shown by the diff) is the addition of EIP712 as a contract base and the calls to its init.

[Amxx]

Hello @mrmemes-eth

I'm sorry but I am not familiar with foundry's tooling and I don't fully understand what the conflit is here.

Can you point out which version version of OZ contracts you used to work with, and which one you updated to?

[mrmemes-eth]

Hi @Amxx, thanks for the response. The top (above the ---) is the contract's storage layout prior to adding EIP712Upgradeable. The bottom (below the ---) is the contract's storage layout after adding it. The third column is the storage slot number and you can see that the variable (column 1) and the type (column 2) are changed by the addition of EIP712Upgradeable for the variables in storage slots 352 and 353.

Does that do a better job of explaining the output?

The contracts used v4.6.0 when they were originally created, but this storage slot conflict doesn't seem to be dependent on that version. I can deploy a new contract on 4.8.1, add EIP712Upgradeable and will experience the same problem with conflicts at these slots.

Focusing solely on the ERC20, as the simpler case, adding EIP712Upgradeable tries to store _HASHED_NAME and _HASHED_VERSION and 352 and 353, respectively. The layout of this contract has the following variables in those slots:

https://github.com/JoinOrigami/crane/blob/5eabb19c3fc2fea4a3f9104498abe60afb2af410/src/OrigamiGovernanceToken.sol#L45-L49

Which would be overwritten as a result.

[mrmemes-eth]

Here's the commit in question that demonstrates the only changes necessary to cause this storage layout problem: https://github.com/JoinOrigami/crane/pull/5/commits/d45d0648de70da58c492a6688317f257168494bd

[Amxx]

The top (above the ---) is the contract's storage layout prior to adding EIP712Upgradeable. The bottom (below the ---) is the contract's storage layout after adding it.

If I understand correctly, you are saying that you have

• a contract A that is not EIP712Upgradeable
• a contract B that is EIP712Upgradeable

And A and B are not storage compatible?

That is probably because you updated you contract in a way that inserts EIP712Upgradeable in the middle of the storage of A.

The simplest option to achieve that is to keep A (the old version) in your codebase, and write B (the new version) as

contract B is A, EIP712Upgradeable {
…
}

[mrmemes-eth]

Hi @Amxx, no, this is the same contract, just before and after adding EIP712Upgradeable. Merely adding EIP712Upgradeable as a base contract, at the end of the existing base contracts demonstrates this issue. Please see https://github.com/JoinOrigami/crane/pull/5/commits/d45d0648de70da58c492a6688317f257168494bd for the exact code change that introduces this issue.

[Amxx]

Your contract includes some storage variable of its own, which are after all the storage variables from all the parents. When you add a new parent, its storage it placed BEFORE these variable which causes the issue.

To simplify lets imagine your have

contract A { uint256 a; }
contract B { uint256 b; }
contract C { uint256 c; }

contract T1 is A, B { uint256 t; }
contract T2 is A, B, C { uint256 t; }

the storage layout of T1 will be:

• 0x00 uint256 a (from A)
• 0x20 uint256 b (from B)
• 0x40 uint256 t (from T1)

The storage layout of T2 will be

• 0x00 uint256 a (from A)
• 0x20 uint256 b (from B)
• 0x40 uint256 c (from C)
• 0x60 uint256 t (from T2)

You see that adding the inheritance on C "pushed" the storage, and created the inconsistency

That is what I'm saying your need to keep the "old" version (T1), and you need to write the new version (T2) as

contract T2 is T1, C {}

[frangio]

Closing since this is expected Solidity behavior. Please apply the workaround shared by @Amxx.

[mrmemes-eth]

Closing since this is expected Solidity behavior. Please apply the workaround shared by @Amxx.

I understand very well that the storage layout can get pushed. However, isn't the goal of the storage gaps in the OZ upgradeable contracts to prevent the addition of OZ dependencies from clobbering existing storage allocations? This seems like a problem that prevents the upgrade-safe contracts for actually being upgrade-safe.

[Amxx]

The gaps are here so we can add variables in a contract while simultaneously shirking the gap. They are not here so we can add new contracts.

Is you case we would need to reduce the gap in Votes to accommodate the storage needs of EIP712Upgradeable. I'm sure you understand that we can't do such a change as it would affect everyone and would be breaking for users don't plan on doing the same changes as you do.