From 440b9f49c52b42e4521a661a38d993268e74b7ce Mon Sep 17 00:00:00 2001 From: QiuhaoLi Date: Thu, 15 Aug 2024 17:33:42 +0800 Subject: [PATCH] tool: add detector for multiple new reinitializers --- scripts/ci_test_upgradability.sh | 14 +++ .../tools/upgradeability/checks/all_checks.py | 1 + .../upgradeability/checks/initialization.py | 93 +++++++++++++++++++ .../contract_initialization.sol | 42 +++++++++ tests/tools/check_upgradeability/test_10.txt | 2 +- tests/tools/check_upgradeability/test_12.txt | 2 +- tests/tools/check_upgradeability/test_13.txt | 2 +- tests/tools/check_upgradeability/test_15.txt | 15 +++ tests/tools/check_upgradeability/test_2.txt | 2 +- tests/tools/check_upgradeability/test_3.txt | 2 +- tests/tools/check_upgradeability/test_4.txt | 2 +- 11 files changed, 171 insertions(+), 6 deletions(-) create mode 100644 tests/tools/check_upgradeability/test_15.txt diff --git a/scripts/ci_test_upgradability.sh b/scripts/ci_test_upgradability.sh index 9d320e2957..a4da93873c 100755 --- a/scripts/ci_test_upgradability.sh +++ b/scripts/ci_test_upgradability.sh @@ -195,6 +195,19 @@ then exit 255 fi +slither-check-upgradeability "$DIR_TESTS/contract_initialization.sol" Contract_reinitializer_V2 --new-contract-name Counter_reinitializer_V3_V4 > test_15.txt 2>&1 +DIFF=$(diff test_15.txt "$DIR_TESTS/test_15.txt") +if [ "$DIFF" != "" ] +then + echo "slither-check-upgradeability 14 failed" + cat test_15.txt + echo "" + cat "$DIR_TESTS/test_15.txt" + echo "" + echo "$DIFF" + exit 255 +fi + rm test_1.txt rm test_2.txt rm test_3.txt @@ -209,3 +222,4 @@ rm test_11.txt rm test_12.txt rm test_13.txt rm test_14.txt +rm test_15.txt diff --git a/slither/tools/upgradeability/checks/all_checks.py b/slither/tools/upgradeability/checks/all_checks.py index 2289c38085..1581ab5308 100644 --- a/slither/tools/upgradeability/checks/all_checks.py +++ b/slither/tools/upgradeability/checks/all_checks.py @@ -7,6 +7,7 @@ MissingCalls, MultipleCalls, InitializeTarget, + MultipleReinitializers, ) from slither.tools.upgradeability.checks.functions_ids import IDCollision, FunctionShadowing diff --git a/slither/tools/upgradeability/checks/initialization.py b/slither/tools/upgradeability/checks/initialization.py index cd420ee45e..53db874cbb 100644 --- a/slither/tools/upgradeability/checks/initialization.py +++ b/slither/tools/upgradeability/checks/initialization.py @@ -400,3 +400,96 @@ def _check(self): ] json = self.generate_result(info) return [json] + +class MultipleReinitializers(AbstractCheck): + ARGUMENT = "multiple-new-reinitializers" + IMPACT = CheckClassification.LOW + + HELP = "Multiple new reinitializers in the updated contract" + WIKI = "https://github.com/crytic/slither/wiki/Upgradeability-Checks#multiple-new-reinitializers" + WIKI_TITLE = "Multiple new reinitializers in the updated contract" + + # region wiki_description + WIKI_DESCRIPTION = """ +Detect multiple new reinitializers in the updated contract`. +""" + # endregion wiki_description + + # region wiki_exploit_scenario + WIKI_EXPLOIT_SCENARIO = """ +```solidity +contract Counter is Initializable { + uint256 public x; + + function initialize(uint256 _x) public initializer { + x = _x; + } + + function changeX() public { + x++; + } +} + +contract CounterV2 is Initializable { + uint256 public x; + uint256 public y; + uint256 public z; + + function initializeV2(uint256 _y) public reinitializer(2) { + y = _y; + } + + function initializeV3(uint256 _z) public reinitializer(3) { + z = _z; + } + + function changeX() public { + x = x + y + z; + } +} +``` +`CounterV2` has two reinitializers with new versions `2` and `3`. If `initializeV3()` is called first, the `initializeV2()` can't be called to initialize `y`, which may brings security risks. +""" + # endregion wiki_exploit_scenario + + # region wiki_recommendation + WIKI_RECOMMENDATION = """ +Do not use multiple reinitializers with higher versions in the updated contract. +""" + # endregion wiki_recommendation + + REQUIRE_CONTRACT = True + REQUIRE_CONTRACT_V2 = True + + def _check(self): + contract_v1 = self.contract + contract_v2 = self.contract_v2 + + if contract_v2 is None: + raise Exception("multiple-new-reinitializers requires a V2 contract") + + initializerV1 = contract_v1.get_modifier_from_canonical_name("Initializable.initializer()") + reinitializerV1 = contract_v1.get_modifier_from_canonical_name("Initializable.reinitializer(uint64)") + reinitializerV2 = contract_v2.get_modifier_from_canonical_name("Initializable.reinitializer(uint64)") + + # contractV1 has initializer or reinitializer + if initializerV1 is None and reinitializerV1 is None: + return [] + # contractV2 has reinitializer + if reinitializerV2 is None: + return [] + + initializer_funcs_v1 = _get_initialize_functions(contract_v1) + initializer_funcs_v2 = _get_initialize_functions(contract_v2) + new_reinitializer_funcs = [] + for fv2 in initializer_funcs_v2: + if not any((fv2.full_name == fv1.full_name) for fv1 in initializer_funcs_v1): + new_reinitializer_funcs.append(fv2) + + results = [] + if len(new_reinitializer_funcs) > 1: + for f in new_reinitializer_funcs: + info = [f, " multiple new reinitializers.\n"] + json = self.generate_result(info) + results.append(json) + return results diff --git a/tests/tools/check_upgradeability/contract_initialization.sol b/tests/tools/check_upgradeability/contract_initialization.sol index 5494dff42c..ab6ab8e515 100644 --- a/tests/tools/check_upgradeability/contract_initialization.sol +++ b/tests/tools/check_upgradeability/contract_initialization.sol @@ -59,3 +59,45 @@ contract Contract_double_call is Contract_no_bug, Contract_no_bug_inherits{ } } + +contract Contract_reinitializer_V2 is Initializable { + uint256 public x; + + function initialize(uint256 _x) public initializer { + x = _x; + } + + function initializeV2(uint256 _x) public reinitializer(2) { + x = _x; + } + + function changeX() public { + x++; + } +} + +contract Counter_reinitializer_V3_V4 is Initializable { + uint256 public x; + uint256 public y; + uint256 public z; + + function initialize(uint256 _x) public initializer { + x = _x; + } + + function initializeV2(uint256 _x) public reinitializer(2) { + x = _x; + } + + function initializeV3(uint256 _y) public reinitializer(3) { + y = _y; + } + + function initializeV4(uint256 _z) public reinitializer(4) { + z = _z; + } + + function changeX() public { + x = x + y + z; + } +} \ No newline at end of file diff --git a/tests/tools/check_upgradeability/test_10.txt b/tests/tools/check_upgradeability/test_10.txt index bbd8f17979..9a7e022c1e 100644 --- a/tests/tools/check_upgradeability/test_10.txt +++ b/tests/tools/check_upgradeability/test_10.txt @@ -10,4 +10,4 @@ Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#missing- INFO:Slither: Initializable contract not found, the contract does not follow a standard initalization schema. Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#initializable-is-missing -INFO:Slither:4 findings, 21 detectors run +INFO:Slither:4 findings, 22 detectors run diff --git a/tests/tools/check_upgradeability/test_12.txt b/tests/tools/check_upgradeability/test_12.txt index 353d8ebdb7..7641e8335b 100644 --- a/tests/tools/check_upgradeability/test_12.txt +++ b/tests/tools/check_upgradeability/test_12.txt @@ -4,4 +4,4 @@ Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#initiali INFO:Slither: Initializable contract not found, the contract does not follow a standard initalization schema. Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#initializable-is-missing -INFO:Slither:2 findings, 21 detectors run +INFO:Slither:2 findings, 22 detectors run diff --git a/tests/tools/check_upgradeability/test_13.txt b/tests/tools/check_upgradeability/test_13.txt index c9d02a522d..e39b9b1ac3 100644 --- a/tests/tools/check_upgradeability/test_13.txt +++ b/tests/tools/check_upgradeability/test_13.txt @@ -9,4 +9,4 @@ Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#incorrec INFO:Slither: Initializable contract not found, the contract does not follow a standard initalization schema. Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#initializable-is-missing -INFO:Slither:3 findings, 21 detectors run +INFO:Slither:3 findings, 22 detectors run diff --git a/tests/tools/check_upgradeability/test_15.txt b/tests/tools/check_upgradeability/test_15.txt new file mode 100644 index 0000000000..960f7b3565 --- /dev/null +++ b/tests/tools/check_upgradeability/test_15.txt @@ -0,0 +1,15 @@ +INFO:Slither: +Contract_reinitializer_V2 (tests/tools/check_upgradeability/contract_initialization.sol#63-77) needs to be initialized by Contract_reinitializer_V2.initialize(uint256) (tests/tools/check_upgradeability/contract_initialization.sol#66-68). +Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#initialize-function +INFO:Slither: +Extra variables in Counter_reinitializer_V3_V4 (tests/tools/check_upgradeability/contract_initialization.sol#79-104): Counter_reinitializer_V3_V4.y (tests/tools/check_upgradeability/contract_initialization.sol#81) +Extra variables in Counter_reinitializer_V3_V4 (tests/tools/check_upgradeability/contract_initialization.sol#79-104): Counter_reinitializer_V3_V4.z (tests/tools/check_upgradeability/contract_initialization.sol#82) +Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#extra-variables-in-the-v2 +INFO:Slither: +Counter_reinitializer_V3_V4.initializeV3(uint256) (tests/tools/check_upgradeability/contract_initialization.sol#92-94) multiple new reinitializers. +Counter_reinitializer_V3_V4.initializeV4(uint256) (tests/tools/check_upgradeability/contract_initialization.sol#96-98) multiple new reinitializers. +Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#multiple-new-reinitializers +INFO:Slither: +Counter_reinitializer_V3_V4 (tests/tools/check_upgradeability/contract_initialization.sol#79-104) needs to be initialized by Counter_reinitializer_V3_V4.initialize(uint256) (tests/tools/check_upgradeability/contract_initialization.sol#84-86). +Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#initialize-function +INFO:Slither:6 findings, 22 detectors run diff --git a/tests/tools/check_upgradeability/test_2.txt b/tests/tools/check_upgradeability/test_2.txt index dcf910c00d..a3970ffc12 100644 --- a/tests/tools/check_upgradeability/test_2.txt +++ b/tests/tools/check_upgradeability/test_2.txt @@ -4,4 +4,4 @@ Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#initiali INFO:Slither: Initializable contract not found, the contract does not follow a standard initalization schema. Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#initializable-is-missing -INFO:Slither:2 findings, 25 detectors run +INFO:Slither:2 findings, 26 detectors run diff --git a/tests/tools/check_upgradeability/test_3.txt b/tests/tools/check_upgradeability/test_3.txt index 265f0d15e7..5cdc9fc7e2 100644 --- a/tests/tools/check_upgradeability/test_3.txt +++ b/tests/tools/check_upgradeability/test_3.txt @@ -20,4 +20,4 @@ Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#extra-va INFO:Slither: Initializable contract not found, the contract does not follow a standard initalization schema. Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#initializable-is-missing -INFO:Slither:6 findings, 25 detectors run +INFO:Slither:6 findings, 26 detectors run diff --git a/tests/tools/check_upgradeability/test_4.txt b/tests/tools/check_upgradeability/test_4.txt index 1a4ba08fbb..eb088324d2 100644 --- a/tests/tools/check_upgradeability/test_4.txt +++ b/tests/tools/check_upgradeability/test_4.txt @@ -17,4 +17,4 @@ Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#extra-va INFO:Slither: Initializable contract not found, the contract does not follow a standard initalization schema. Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#initializable-is-missing -INFO:Slither:5 findings, 25 detectors run +INFO:Slither:5 findings, 26 detectors run