Skip to content

Inconsistent Treatment of Storage Arrays on the Slot Overflow Boundary #15587

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

Open
ekpyron opened this issue Nov 25, 2024 · 2 comments · May be fixed by #15984
Open

Inconsistent Treatment of Storage Arrays on the Slot Overflow Boundary #15587

ekpyron opened this issue Nov 25, 2024 · 2 comments · May be fixed by #15984
Labels
bug 🐛 low impact Changes are not very noticeable or potential benefits are limited. medium effort Default level of effort must have Something we consider an essential part of Solidity 1.0.
Milestone

Comments

@ekpyron
Copy link
Member

ekpyron commented Nov 25, 2024

Solidity's storage layout relies on the assumption of de-facto collision-free-ness of keccak hashes.
The keccak hashes involved in mappings and the location of data areas for arrays are assumed to result in disjoint storage areas.
In case that very large static arrays are allocated in storage, the compiler emits a warning about the increased risk of collisions.

However, compiler behavior is inconsistent at the storage slot overflow boundary at 2^256: e.g. assigning to and copying data to static arrays that cross the overflow boundaries wraps to storage slot zero, while arrays crossing the boundaries are not correctly cleared on delete.

Generally, storage areas that cross the boundary at 2^256 are ill-defined: since regular contract state variables are per default allocated at slot zero, an overflow in storage slots is generally expected to result in undefined/incorrect behaviour.

The likelihood of storage areas as assigned by the compiler based on hashes to fall into the overflow boundary, is very low (similar to the likelihood of storage collisions in general), except for cases with large static arrays in storage for which we already emit a warning about potential storage collisions.

Nevertheless, we should survey the inconsistencies and aim to adjust for consistent behavior.

Known examples of inconsistencies are at YulUtilFunctions::clearStorageRangeFunction (https://github.com/ethereum/solidity/blob/develop/libsolidity/codegen/YulUtilFunctions.cpp#L1830-L1833) and at ArrayUtils::clearStorageLoop (https://github.com/ethereum/solidity/blob/develop/libsolidity/codegen/ArrayUtils.cpp#L939-L959), which are not robust against overflows (failing to clear storage on delete), while e.g. YulUtilFunctions::copyArrayToStorageFunction performs a wrapping copy.

We should also refine the documentation and potentially compiler warnings to raise awareness of this issue.

@ekpyron ekpyron added bug 🐛 must have Something we consider an essential part of Solidity 1.0. labels Nov 25, 2024
@cameel cameel added medium effort Default level of effort low impact Changes are not very noticeable or potential benefits are limited. labels Nov 26, 2024
@cameel
Copy link
Member

cameel commented Nov 26, 2024

One case I did not think about during the earlier discussion: with #597 it will be possible to manually position your storage variables close to the boundary. Perhaps we should detect it and warn about it just like we warn about large arrays/structs?

@ekpyron ekpyron added this to the 0.8.30 milestone Mar 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 low impact Changes are not very noticeable or potential benefits are limited. medium effort Default level of effort must have Something we consider an essential part of Solidity 1.0.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants
@cameel @ekpyron and others