Skip to content

Fix incorrect storage layout calculation for fixed-size arrays - #3068

Open
Nakamura73914 wants to merge 5 commits into
crytic:masterfrom
Nakamura73914:fix/storage-layout-calculation
Open

Fix incorrect storage layout calculation for fixed-size arrays#3068
Nakamura73914 wants to merge 5 commits into
crytic:masterfrom
Nakamura73914:fix/storage-layout-calculation

Conversation

@Nakamura73914

Copy link
Copy Markdown

This PR fixes a bug in _compute_storage_layout where Slither incorrectly calculated the storage slots for fixed-size arrays (e.g., address[10], bytes20[10]).

Root Cause

Slither incorrectly treated fixed-size arrays as a continuous byte stream (math.ceil((elem_size * length) / 32)). However, Solidity mandates that array elements cannot cross 32-byte slot boundaries.

For example, address[10] (20 bytes each):

  • Old Logic: ceil(200 / 32) = 7 slots.
  • Solidity Reality: A slot holds only one 20-byte address, requiring 10 slots.

This offset error caused all subsequent state variables to be assigned incorrect slot numbers.

Fix

The fix intercepts fixed-size arrays (ArrayType where is_dynamic is False) and applies the correct Solidity packing logic:

  1. If the element size is > 32 bytes (e.g., structs), each element occupies math.ceil(elem_size / 32) slots.
  2. If the element size is <= 32 bytes, we calculate how many elements fit into a single slot (32 // elem_size), and then calculate the total slots needed for all elements.

A try-except block is retained as a fallback to prevent Slither from crashing if AST parsing for array length fails unexpectedly.

Testing & Verification

Array Type Element Size Old Logic (Slots) New Logic (Slots) Actual Solidity (Slots)
uint256[10] 32 bytes 10 10 10
uint128[10] 16 bytes 5 5 5
bytes1[10] 1 byte 1 1 1
address[10] 20 bytes 7 (Bug) 10 10
bytes20[10] 20 bytes 7 (Bug) 10 10

@Nakamura73914
Nakamura73914 requested a review from smonicas as a code owner August 3, 2026 03:48
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@smonicas

smonicas commented Aug 4, 2026

Copy link
Copy Markdown
Member

Hi, thanks for the PR. The issue is real however your proposed fix is not the best way to do it. For example the storage layout of the following example would be wrong.

struct S { address[10] inner; }
contract C {
 S s;
 uint b;
}

A better fix is to correctly compute the storage_size of the ArrayType

@property
def storage_size(self) -> tuple[int, bool]:
if self._length_value:
elem_size, _ = self._type.storage_size
return elem_size * int(str(self._length_value)), True
return 32, True

Something like this should work without any other changes

@property
def storage_size(self) -> tuple[int, bool]:
    if self._length_value:
        elem_size, _ = self._type.storage_size
        length = int(str(self._length_value))
        if elem_size > 32:
            return length * math.ceil(elem_size / 32) * 32, True
        elem_per_slot = 32 // max(elem_size, 1)
        return math.ceil(length / elem_per_slot) * 32, True
    return 32, True

Please also add a few test cases.

@Nakamura73914

Copy link
Copy Markdown
Author

Hi, thanks for the PR. The issue is real however your proposed fix is not the best way to do it. For example the storage layout of the following example would be wrong.

struct S { address[10] inner; }
contract C {
 S s;
 uint b;
}

A better fix is to correctly compute the storage_size of the ArrayType

@property
def storage_size(self) -> tuple[int, bool]:
if self._length_value:
elem_size, _ = self._type.storage_size
return elem_size * int(str(self._length_value)), True
return 32, True

Something like this should work without any other changes

@property
def storage_size(self) -> tuple[int, bool]:
    if self._length_value:
        elem_size, _ = self._type.storage_size
        length = int(str(self._length_value))
        if elem_size > 32:
            return length * math.ceil(elem_size / 32) * 32, True
        elem_per_slot = 32 // max(elem_size, 1)
        return math.ceil(length / elem_per_slot) * 32, True
    return 32, True

Please also add a few test cases.

Thanks for sharing this approach—it's definitely much better than my original change.

I've pulled your code and added some tests to cover it. All tests passed smoothly.
test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants