slither-read-storage: Handle unstructured storage #1752

webthethird · 2023-03-13T17:48:59Z

This approach scans for constant bytes32 variables and uses a couple heuristics to determine (a) that it is a storage slot, and (b) the type of value stored in the slot.

For example, running the following command yields the table below:

slither-read-storage 0x8315177aB297bA92A06054cE80a67Ed4DBd7ed3a --contract TransparentUpgradeableProxy --value --table --rpc-url <MY_RPC_URL>

+----------------------+-------------+-------------------------------------------------------------------------------+------+--------+--------------------------------------------+
|         name         | type_string |                                      slot                                     | size | offset |                   value                    |
+----------------------+-------------+-------------------------------------------------------------------------------+------+--------+--------------------------------------------+
|     _BEACON_SLOT     |   address   | 74152234768234802001998023604048924213078445070507226371336425913862612794704 | 160  |   0    | 0x0000000000000000000000000000000000000000 |
|     _ADMIN_SLOT      |   address   | 81955473079516046949633743016697847541294818689821282749996681496272635257091 | 160  |   0    | 0x554723262467F125Ac9e1cDFa9Ce15cc53822dbD |
| _IMPLEMENTATION_SLOT |   address   | 24440054405305269366569402256811496959409073762505157381672968839269610695612 | 160  |   0    | 0x1066CEcC8880948FE55e427E94F1FF221d626591 |
|    _ROLLBACK_SLOT    |     bool    | 33048860383849004559742813297059419343339852917517107368639918720169455489347 |  1   |   0    |                   False                    |
+----------------------+-------------+-------------------------------------------------------------------------------+------+--------+--------------------------------------------+

in `coerce_type(solidity_type, value)`

i.e., all constant bytes32 variables

before calling `get_storage_layout`

montyly · 2023-03-14T17:43:30Z

Can we add a test with an address that uses this format?

webthethird · 2023-03-14T18:17:37Z

Can we add a test with an address that uses this format?

The address I used is 0x8315177aB297bA92A06054cE80a67Ed4DBd7ed3a, which is the Arbitrum Bridge's Transparent Upgradeable Proxy.

Where would you recommend adding the test? /scripts/ci_test_upgradeability.sh currently just tests slither-check-upgradeability, and this isn't really part of that tool. But at a glance I don't see a test script for slither-read-storage, though I do see the /tests/storage-layout directory.

0xalpharush · 2023-03-14T20:21:50Z

@webthethird You would deploy a contract to ganache and run slither-read-storage against the local network as is done here https://github.com/crytic/slither/blob/master/tests/test_read_storage.py

webthethird · 2023-03-14T21:20:48Z

@0xalpharush Would it make sense to add it to the existing test? Since this PR applies to slither-read-storage itself, just adding to what would print if the contract used any constant storage slots.

using slither-read-storage

…ge-unstructured

webthethird · 2023-03-15T15:49:04Z

@montyly @0xalpharush I added a new test, tests/test_unstructured_storage.py.

I believe the failing CI test is related to #1710

0xalpharush · 2023-03-15T16:52:23Z

slither/tools/read_storage/read_storage.py

+                ret = func.return_type[0]
+                size, _ = ret.storage_size
+                return str(ret), size * 8
+            for node in func.all_nodes():


You may be able to use https://crytic.github.io/slither/slither/core/declarations/contract.html#Contract.get_functions_reading_from_variable to make this more general and not have to rely on matching these specific function names. If it only returns one function and that function has one return value that's less than or equal to 32 bytes, assume the return type is that.

If it only returns one function and that function has one return value that's less than or equal to 32 bytes, assume the return type is that.

For the most part I am doing this on lines 275-278. Though I could streamline it by using Contract.get_functions_reading_from_variable.

to make this more general and not have to rely on matching these specific function names.

The reason I resorted to checking for the function names from the StorageSlot library is because there may be some slot (i.e., _ROLLBACK_SLOT in the standard ERC1967Upgrade.sol) which do not have any setter or getter functions. With the _ROLLBACK_SLOT example, the only function which reads from that variable is _upgradeToAndCallUUPS which contains the line if (StorageSlot.getBooleanSlot(_ROLLBACK_SLOT).value).

slither/tools/read_storage/read_storage.py

tests/test_unstructured_storage.py

slither/tools/read_storage/read_storage.py

check function return type sizes, and add comments in `find_constant_slot_storage_type`

instead of string casting

0xalpharush · 2023-04-17T15:06:12Z

slither/tools/read_storage/read_storage.py

+                state_var_slots = [
+                    self.get_variable_info(contract, var)[0]
+                    for contract, var in self.target_variables
+                ]
+                if int_value in state_var_slots:
+                    return None


One thing to be aware of is that the storage layout is only calculated for contracts_derived and not contracts so there's a potential that a KeyError happens if a variable is found in an abstract contract. It's not clear to me this can happen in practice, but the error is handled by get_storage_slot for this reason.

Is it possible for a variable in an abstract contract to end up in self.target_variables?

…ge-unstructured # Conflicts: # tests/conftest.py

webthethird · 2023-04-26T19:47:25Z

@0xalpharush After merging from dev I'm getting some odd test errors. Any idea why?

Also please let me know if there's anything else to do on this PR to get it ready to merge. This one has probably been around too long!

tests/tools/read-storage/test_read_storage.py

webthethird · 2023-04-26T20:10:58Z

Now I'm getting an error when trying to run Black

Error: Unable to resolve action `github/super-linter@v4.9.2`, unable to find version `v4.9.2`

webthethird · 2023-04-27T16:40:52Z

With the Black issue resolved, all checks are passing

0xalpharush · 2023-05-25T17:05:57Z

I think we can squash and merge

0xalpharush · 2023-06-08T12:43:00Z

@webthethird Can you resolve merge conflicts?

webthethird · 2023-06-08T18:08:04Z

@0xalpharush merge conflicts resolved, and tests passing

montyly · 2023-06-09T13:22:54Z

slither/visitors/expression/constants_folding.py

@@ -63,25 +64,35 @@ def result(self) -> "Literal":
            # emulate 256-bit wrapping
            if str(self._type).startswith("uint"):
                value = value & (2**256 - 1)
+        if str(self._type).startswith("byte"):


mhh would this cause problem for bytes1/ bytes2/ ...

@0xalpharush what do you think? This is part of the constant folding change you made I think.

webthethird · 2023-06-09T15:26:57Z

@0xalpharush I just merged dev into this branch again, and suddenly we're getting a bunch of test failures. Any idea what the cause might be? I had already merged from dev yesterday, so it must be something recent.

webthethird added 6 commits March 13, 2023 12:29

Handle conversion from hex string to int

636d055

in `coerce_type(solidity_type, value)`

Get constant storage slots,

7311ca0

i.e., all constant bytes32 variables

Determine value type stored in constant slot

f53d725

Include constant slots in get_storage_layout()

200f2cc

Black format

ea0146c

Call get_constant_storage_slots in main

dabe318

before calling `get_storage_layout`

webthethird requested a review from 0xalpharush as a code owner March 13, 2023 17:49

webthethird added 2 commits March 15, 2023 09:20

Handle keccak256 in bytes32 constant declaration

4db313b

Add test for unstructured storage support

1fa25cc

using slither-read-storage

webthethird requested review from montyly and smonicas as code owners March 15, 2023 15:43

Merge remote-tracking branch 'crytic/slither/dev' into dev-read-stora…

340a022

…ge-unstructured

webthethird added 2 commits March 15, 2023 11:37

Merge get_constant_storage_slots into get_all_storage_variables

0dd6321

Handle storage slot hardcoded in fallback

8dfe072

0xalpharush reviewed Mar 15, 2023

View reviewed changes

slither/tools/read_storage/read_storage.py Show resolved Hide resolved

0xalpharush reviewed Mar 15, 2023

View reviewed changes

tests/test_unstructured_storage.py Outdated Show resolved Hide resolved

0xalpharush reviewed Mar 15, 2023

View reviewed changes

slither/tools/read_storage/read_storage.py Outdated Show resolved Hide resolved

webthethird added 3 commits March 15, 2023 12:24

Handle storage slot hardcoded in fallback

7c8f14e

Compact get_all_storage_variables

90df442

Black

ea72130

0xalpharush reviewed Mar 15, 2023

View reviewed changes

slither/tools/read_storage/read_storage.py Outdated Show resolved Hide resolved

webthethird added 3 commits March 15, 2023 12:58

Use Literal.value instead of string casting

c8d7420

Use Contract.get_functions_reading_from_variable,

6b0f49a

check function return type sizes, and add comments in `find_constant_slot_storage_type`

Get Literal from expression

285a6ba

instead of string casting

0xalpharush reviewed Apr 17, 2023

View reviewed changes

webthethird added 6 commits April 17, 2023 11:21

Put self._constant_storage_slots behind flag

d9ce126

Handle NotConstant exception

677849d

Better bytes to string conversion

0adecf6

Black

906fa8c

Merge remote-tracking branch 'crytic/slither/dev' into dev-read-stora…

04d7b67

…ge-unstructured # Conflicts: # tests/conftest.py

Pylint after merge

02e7466

Disable too-many-locals in test_constant_folding.py

bb58ea2

0xalpharush reviewed Apr 26, 2023

View reviewed changes

tests/tools/read-storage/test_read_storage.py Outdated Show resolved Hide resolved

webthethird added 2 commits April 26, 2023 14:57

Add missing solc_binary_path

34e3902

Add missing solc_binary_path

846e2af

Format comments, fix typo

d2dad0f

webthethird requested a review from 0xalpharush May 4, 2023 16:14

Merge branch 'dev' into dev-read-storage-unstructured

132bf89

0xalpharush approved these changes May 25, 2023

View reviewed changes

Merge branch 'dev' into dev-read-storage-unstructured

a7fdd4b

webthethird added 2 commits June 8, 2023 12:54

Merge branch 'dev' into dev-read-storage-unstructured

01722f6

Update test_unstructured_storage

abb7622

0xalpharush added this to the 0.9.4 milestone Jun 9, 2023

montyly reviewed Jun 9, 2023

View reviewed changes

Merge branch 'dev' into dev-read-storage-unstructured

1fe9f25

0xalpharush mentioned this pull request Jun 12, 2023

feat: add --unstructured to slither-read-storage; retrieve custom storage layouts #1963

Merged

0xalpharush closed this Jun 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

slither-read-storage: Handle unstructured storage #1752

slither-read-storage: Handle unstructured storage #1752

webthethird commented Mar 13, 2023

montyly commented Mar 14, 2023

webthethird commented Mar 14, 2023

0xalpharush commented Mar 14, 2023

webthethird commented Mar 14, 2023

webthethird commented Mar 15, 2023

0xalpharush Mar 15, 2023

webthethird Mar 15, 2023

0xalpharush Apr 17, 2023

webthethird Apr 17, 2023

webthethird commented Apr 26, 2023

webthethird commented Apr 26, 2023

webthethird commented Apr 27, 2023

0xalpharush commented May 25, 2023

0xalpharush commented Jun 8, 2023

webthethird commented Jun 8, 2023

montyly Jun 9, 2023

webthethird Jun 9, 2023

webthethird commented Jun 9, 2023

slither-read-storage: Handle unstructured storage #1752

slither-read-storage: Handle unstructured storage #1752

Conversation

webthethird commented Mar 13, 2023

montyly commented Mar 14, 2023

webthethird commented Mar 14, 2023

0xalpharush commented Mar 14, 2023

webthethird commented Mar 14, 2023

webthethird commented Mar 15, 2023

0xalpharush Mar 15, 2023

Choose a reason for hiding this comment

webthethird Mar 15, 2023

Choose a reason for hiding this comment

0xalpharush Apr 17, 2023

Choose a reason for hiding this comment

webthethird Apr 17, 2023

Choose a reason for hiding this comment

webthethird commented Apr 26, 2023

webthethird commented Apr 26, 2023

webthethird commented Apr 27, 2023

0xalpharush commented May 25, 2023

0xalpharush commented Jun 8, 2023

webthethird commented Jun 8, 2023

montyly Jun 9, 2023

Choose a reason for hiding this comment

webthethird Jun 9, 2023

Choose a reason for hiding this comment

webthethird commented Jun 9, 2023