search

ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
23.44k stars 5.79k forks source link

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

Open ekpyron opened 2 days ago

ekpyron commented 2 days ago

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 as 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.

cameel commented 1 day ago

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?