Add upgradeability utils #1757

webthethird · 2023-03-14T17:21:03Z

Extends #1699, adding more utilities to slither.utils.upgradeability, specifically related to finding a proxy's implementation address storage slot.

…grades # Conflicts: # README.md

in `slither.utils.upgradeability`

cut down on branches and nested blocks by adding an `is_function_modified` helper function

…grades

…ame state variable(s) as a new/modified function

when finding functions tainted by `new_modified_function_vars`

when finding functions tainted by `new_modified_functions`

when finding modified functions

like `get_proxy_implementation_slot(proxy: Contract)`

out of `get_proxy_implementation_slot` since either one could be more useful.

…lity-utils # Conflicts: # README.md

webthethird · 2023-03-14T18:10:00Z

I'm not sure why Test slither-read-storage is failing here, since this PR doesn't touch it. I checked the test details, here's what it says:

Run pytest tests/test_read_storage.py
bash: cannot set terminal process group (669): Inappropriate ioctl for device
bash: no job control in this shell
============================= test session starts ==============================
platform linux -- Python 3.8.16, pytest-[7](https://github.com/crytic/slither/actions/runs/4418537918/jobs/7745793871#step:7:8).2.2, pluggy-1.0.0
rootdir: /home/runner/work/slither/slither
plugins: cov-4.0.0, web3-6.0.0, xdist-3.2.1
collected 0 items / 1 error

==================================== ERRORS ====================================
_________________ ERROR collecting tests/test_read_storage.py __________________
slither/tools/read_storage/read_storage.py:9: in <module>
    from eth_abi import decode_single, encode_abi
E   ImportError: cannot import name 'decode_single' from 'eth_abi' (/opt/hostedtoolcache/Python/3.[8](https://github.com/crytic/slither/actions/runs/4418537918/jobs/7745793871#step:7:9).16/x64/lib/python3.8/site-packages/eth_abi/__init__.py)

During handling of the above exception, another exception occurred:
tests/test_read_storage.py:13: in <module>
    from slither.tools.read_storage import SlitherReadStorage
slither/tools/read_storage/__init__.py:1: in <module>
    from .read_storage import SlitherReadStorage
slither/tools/read_storage/read_storage.py:1[9](https://github.com/crytic/slither/actions/runs/4418537918/jobs/7745793871#step:7:10): in <module>
    sys.exit(-1)
E   SystemExit: -1
------------------------------- Captured stdout --------------------------------
ERROR: in order to use slither-read-storage, you need to install web3
$ pip3 install web3 --user

=========================== short test summary info ============================
ERROR tests/test_read_storage.py - SystemExit: -1
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
=============================== 1 error in 0.61s ===============================
Error: Process completed with exit code 2.

Seems unrelated to me, though I was hoping the problem would go away when I merged dev into this PR.

elopez · 2023-03-14T18:17:28Z

I think that is being tracked in #1710

Due to the non-deterministic order of `Function.all_nodes()`

i.e., `delegatecall(gas(), sload(0x3608...), 0, calldatasize(), 0, 0)` where the slot is not defined as a bytes32 constant but rather is hardcoded in the fallback.

…lity-utils

(see issue crytic#1775 for why it fails)

montyly · 2023-03-23T20:36:16Z

Outside of the conflict with dev, is this ready for another review?

…lity-utils # Conflicts: # slither/tools/upgradeability/checks/variables_order.py

webthethird · 2023-03-23T21:12:28Z

Outside of the conflict with dev, is this ready for another review?

I believe so

montyly

This looks good. Only minor comment to address, and we can merge it

slither/utils/upgradeability.py

webthethird · 2023-03-24T20:00:13Z

@montyly I believe I've addressed the minor changes

montyly · 2023-03-25T08:47:33Z

slither/utils/upgradeability.py

+
+
+# pylint: disable=too-many-branches
+def encode_ir_for_compare(ir: Union[Operation, Variable]) -> str:


It's great that we have added the types, because this does not sound right :)

Should we split this function in two?

encore_ir_for_compare

encore_variable_for_compare

Note that we probably have the same issue in the slither-similar code, but that should be fixed in another PR

Should we split this function in two?

Done!

montyly · 2023-03-28T11:40:16Z

@0xalpharush : should I merge this, or do you want to take a stab at the tests architecture first?

0xalpharush · 2023-03-28T13:17:15Z

slither/core/declarations/contract.py

@@ -663,6 +666,24 @@ def functions_and_modifiers_declared(self) -> List["Function"]:
        """
        return self.functions_declared + self.modifiers_declared  # type: ignore

+    @property


Can we add a test case under slither/tests/unit/core to ensure that it works for examples like #1331 (comment)

0xalpharush · 2023-03-28T13:19:39Z

tests/test_upgradeability_util.py

Can you move this test to slither/tests/unit/utils/ and put the tests folder and its files under slither/tests/unit/testdata/?

0xalpharush · 2023-03-28T13:28:38Z

@montyly I think we should merge dev into this branch so that the tests are run in CI

…lity-utils

and `Contract.receive_function`

webthethird · 2023-03-28T15:04:55Z

I think we should merge dev into this branch so that the tests are run in CI

Done, done and done!

webthethird added 16 commits February 27, 2023 12:09

Start slither.utils.upgradeability

d04fc9e

Merge remote-tracking branch 'crytic/slither/dev' into dev-compare-up…

ec537e5

…grades # Conflicts: # README.md

Implement compare(v1: Contract, v2: Contract)

f08d2af

in `slither.utils.upgradeability`

Pylint

ebd2201

cut down on branches and nested blocks by adding an `is_function_modified` helper function

Add return statement (whoops!)

04c71c2

Merge remote-tracking branch 'crytic/slither/dev' into dev-compare-up…

6b0124c

…grades

Also consider an unmodified function tainted if it reads/writes the s…

5b361e8

…ame state variable(s) as a new/modified function

Make pylint happy (reduce branches)

6b9d21a

Avoid duplicates, constants and immutables

3757601

when finding functions tainted by `new_modified_function_vars`

Avoid constructor

770ca81

when finding functions tainted by `new_modified_functions`

Avoid slitherConstructorConstantVariables()

596b4d0

when finding modified functions

Avoid slitherConstructorConstantVariables()

480ee60

when finding modified functions

Bump to re-run CI tests

37554ec

Add additional upgradeability utils

f1947bb

like `get_proxy_implementation_slot(proxy: Contract)`

Add docstrings

8181fae

Separate get_proxy_implementation_var

4c57168

out of `get_proxy_implementation_slot` since either one could be more useful.

webthethird requested review from elopez, montyly, 0xalpharush and smonicas as code owners March 14, 2023 17:21

webthethird added 2 commits March 14, 2023 12:27

pylint

09128ab

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

a6d49d5

…lity-utils # Conflicts: # README.md

webthethird added 2 commits March 14, 2023 14:32

Redesign utils.upgradeability.is_function_modified

3655708

Due to the non-deterministic order of `Function.all_nodes()`

Handle sload from slot in delegatecall args

c195a15

i.e., `delegatecall(gas(), sload(0x3608...), 0, calldatasize(), 0, 0)` where the slot is not defined as a bytes32 constant but rather is hardcoded in the fallback.

webthethird mentioned this pull request Mar 15, 2023

Compare upgrades in slither.utils.upgradeability #1699

Closed

2 tasks

webthethird added 3 commits March 16, 2023 11:34

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

ccb98d8

…lity-utils

Minor bug fixes

317af45

Include variables touched by tainted functions

9f4be7d

webthethird requested review from montyly and removed request for elopez, smonicas and 0xalpharush March 17, 2023 18:41

webthethird added 4 commits March 17, 2023 13:49

Fix compare return signature

1588334

Handle sload from integer slot, i.e., sload(0)

0e708e6

Comment out ZosProxy test for now

9192fef

(see issue crytic#1775 for why it fails)

Add SynthProxy.sol to test_upgradeability_util.py test

72c6d78

webthethird mentioned this pull request Mar 20, 2023

slither-read-storage: Handle unstructured storage #1752

Closed

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

a292d8d

…lity-utils # Conflicts: # slither/tools/upgradeability/checks/variables_order.py

montyly reviewed Mar 24, 2023

View reviewed changes

slither/utils/upgradeability.py Outdated Show resolved Hide resolved

slither/utils/upgradeability.py Outdated Show resolved Hide resolved

slither/utils/upgradeability.py Show resolved Hide resolved

webthethird added 3 commits March 24, 2023 08:27

Add types to function signatures

78e2ea3

Pylint

c094818

Black

0ac4c01

montyly reviewed Mar 25, 2023

View reviewed changes

webthethird added 2 commits March 25, 2023 11:42

Create separate encore_var_for_compare

2695243

Fix ir encoding in comparison

47c92f8

0xalpharush reviewed Mar 28, 2023

View reviewed changes

webthethird added 4 commits March 28, 2023 09:07

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

ac88e8c

…lity-utils

Move upgradeability util test files

be0e405

Add Path import

8731a92

Add test for Contract.fallback_function

5703b9d

and `Contract.receive_function`

montyly merged commit 8b2fd9a into crytic:dev Mar 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add upgradeability utils #1757

Add upgradeability utils #1757

webthethird commented Mar 14, 2023

webthethird commented Mar 14, 2023

elopez commented Mar 14, 2023

montyly commented Mar 23, 2023

webthethird commented Mar 23, 2023

montyly left a comment •

edited

Loading

webthethird commented Mar 24, 2023

montyly Mar 25, 2023

webthethird Mar 25, 2023

montyly commented Mar 28, 2023

0xalpharush Mar 28, 2023

webthethird Mar 28, 2023

0xalpharush Mar 28, 2023

webthethird Mar 28, 2023

0xalpharush commented Mar 28, 2023

webthethird commented Mar 28, 2023



		# pylint: disable=too-many-branches
		def encode_ir_for_compare(ir: Union[Operation, Variable]) -> str:

Add upgradeability utils #1757

Add upgradeability utils #1757

Conversation

webthethird commented Mar 14, 2023

webthethird commented Mar 14, 2023

elopez commented Mar 14, 2023

montyly commented Mar 23, 2023

webthethird commented Mar 23, 2023

montyly left a comment • edited Loading

Choose a reason for hiding this comment

webthethird commented Mar 24, 2023

montyly Mar 25, 2023

Choose a reason for hiding this comment

webthethird Mar 25, 2023

Choose a reason for hiding this comment

montyly commented Mar 28, 2023

0xalpharush Mar 28, 2023

Choose a reason for hiding this comment

webthethird Mar 28, 2023

Choose a reason for hiding this comment

0xalpharush Mar 28, 2023

Choose a reason for hiding this comment

webthethird Mar 28, 2023

Choose a reason for hiding this comment

0xalpharush commented Mar 28, 2023

webthethird commented Mar 28, 2023

montyly left a comment •

edited

Loading