Merge pull request #2203 from vovikhangcdv/dev

update: improve unhandled initializers in unprotected-upgrade detector

Author: 0xalpharush

slither/core/declarations/contract.py

@@ -1372,8 +1372,6 @@ def update_read_write_using_ssa(self) -> None:
     def is_upgradeable(self) -> bool:
         if self._is_upgradeable is None:
             self._is_upgradeable = False
-            if self.is_upgradeable_proxy:
-                return False
             initializable = self.file_scope.get_contract_from_name("Initializable")
             if initializable:
                 if initializable in self.inheritance:

slither/detectors/statements/unprotected_upgradeable.py

@@ -52,7 +52,14 @@ def _whitelisted_modifiers(f: Function) -> bool:
 
 def _initialize_functions(contract: Contract) -> List[Function]:
     return list(
-        filter(_whitelisted_modifiers, [f for f in contract.functions if f.name == "initialize"])
+        filter(
+            _whitelisted_modifiers,
+            [
+                f
+                for f in contract.functions
+                if any((m.name in ["initializer", "reinitializer"]) for m in f.modifiers)
+            ],
+        )
     )
 
 

tests/e2e/detectors/snapshots/detectors__detector_UnprotectedUpgradeable_0_4_25_AnyInitializer_sol__0.txt

@@ -0,0 +1 @@
+AnyInitializer (tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/AnyInitializer.sol#3-15) is an upgradeable contract that does not protect its initialize functions: AnyInitializer.anyName() (tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/AnyInitializer.sol#6-9). Anyone can delete the contract with: AnyInitializer.kill() (tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/AnyInitializer.sol#11-14)

tests/e2e/detectors/snapshots/detectors__detector_UnprotectedUpgradeable_0_4_25_Reinitializer_sol__0.txt

@@ -0,0 +1 @@
+Reinitializer (tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/Reinitializer.sol#3-15) is an upgradeable contract that does not protect its initialize functions: Reinitializer.initialize() (tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/Reinitializer.sol#6-9). Anyone can delete the contract with: Reinitializer.kill() (tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/Reinitializer.sol#11-14)

tests/e2e/detectors/snapshots/detectors__detector_UnprotectedUpgradeable_0_5_16_AnyInitializer_sol__0.txt

@@ -0,0 +1 @@
+AnyInitializer (tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/AnyInitializer.sol#3-15) is an upgradeable contract that does not protect its initialize functions: AnyInitializer.anyName() (tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/AnyInitializer.sol#6-9). Anyone can delete the contract with: AnyInitializer.kill() (tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/AnyInitializer.sol#11-14)

tests/e2e/detectors/snapshots/detectors__detector_UnprotectedUpgradeable_0_5_16_Reinitializer_sol__0.txt

@@ -0,0 +1 @@
+Reinitializer (tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/Reinitializer.sol#3-15) is an upgradeable contract that does not protect its initialize functions: Reinitializer.initialize() (tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/Reinitializer.sol#6-9). Anyone can delete the contract with: Reinitializer.kill() (tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/Reinitializer.sol#11-14)

tests/e2e/detectors/snapshots/detectors__detector_UnprotectedUpgradeable_0_6_11_AnyInitializer_sol__0.txt

@@ -0,0 +1 @@
+AnyInitializer (tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/AnyInitializer.sol#3-15) is an upgradeable contract that does not protect its initialize functions: AnyInitializer.anyName() (tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/AnyInitializer.sol#6-9). Anyone can delete the contract with: AnyInitializer.kill() (tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/AnyInitializer.sol#11-14)

tests/e2e/detectors/snapshots/detectors__detector_UnprotectedUpgradeable_0_6_11_Reinitializer_sol__0.txt

@@ -0,0 +1 @@
+Reinitializer (tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/Reinitializer.sol#3-15) is an upgradeable contract that does not protect its initialize functions: Reinitializer.initialize() (tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/Reinitializer.sol#6-9). Anyone can delete the contract with: Reinitializer.kill() (tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/Reinitializer.sol#11-14)

tests/e2e/detectors/snapshots/detectors__detector_UnprotectedUpgradeable_0_7_6_AnyInitializer_sol__0.txt

@@ -0,0 +1 @@
+AnyInitializer (tests/e2e/detectors/test_data/unprotected-upgrade/0.7.6/AnyInitializer.sol#3-15) is an upgradeable contract that does not protect its initialize functions: AnyInitializer.anyName() (tests/e2e/detectors/test_data/unprotected-upgrade/0.7.6/AnyInitializer.sol#6-9). Anyone can delete the contract with: AnyInitializer.kill() (tests/e2e/detectors/test_data/unprotected-upgrade/0.7.6/AnyInitializer.sol#11-14)

tests/e2e/detectors/snapshots/detectors__detector_UnprotectedUpgradeable_0_7_6_Reinitializer_sol__0.txt

@@ -0,0 +1 @@
+Reinitializer (tests/e2e/detectors/test_data/unprotected-upgrade/0.7.6/Reinitializer.sol#3-15) is an upgradeable contract that does not protect its initialize functions: Reinitializer.initialize() (tests/e2e/detectors/test_data/unprotected-upgrade/0.7.6/Reinitializer.sol#6-9). Anyone can delete the contract with: Reinitializer.kill() (tests/e2e/detectors/test_data/unprotected-upgrade/0.7.6/Reinitializer.sol#11-14)

tests/e2e/detectors/snapshots/detectors__detector_UnprotectedUpgradeable_0_8_15_AnyInitializer_sol__0.txt

@@ -0,0 +1 @@
+AnyInitializer (tests/e2e/detectors/test_data/unprotected-upgrade/0.8.15/AnyInitializer.sol#3-15) is an upgradeable contract that does not protect its initialize functions: AnyInitializer.anyName() (tests/e2e/detectors/test_data/unprotected-upgrade/0.8.15/AnyInitializer.sol#6-9). Anyone can delete the contract with: AnyInitializer.kill() (tests/e2e/detectors/test_data/unprotected-upgrade/0.8.15/AnyInitializer.sol#11-14)

tests/e2e/detectors/snapshots/detectors__detector_UnprotectedUpgradeable_0_8_15_Reinitializer_sol__0.txt

@@ -0,0 +1 @@
+Reinitializer (tests/e2e/detectors/test_data/unprotected-upgrade/0.8.15/Reinitializer.sol#3-15) is an upgradeable contract that does not protect its initialize functions: Reinitializer.initialize() (tests/e2e/detectors/test_data/unprotected-upgrade/0.8.15/Reinitializer.sol#6-9). Anyone can delete the contract with: Reinitializer.kill() (tests/e2e/detectors/test_data/unprotected-upgrade/0.8.15/Reinitializer.sol#11-14)

tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/AnyInitializer.sol

@@ -0,0 +1,15 @@
+import "./Initializable.sol";
+
+contract AnyInitializer is Initializable {
+    address owner;
+
+    function anyName() external initializer {
+        require(owner == address(0));
+        owner = msg.sender;
+    }
+
+    function kill() external {
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
+}

tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/AnyInitializer.sol-0.4.25.zip

@@ -1,5 +1,9 @@
-contract Initializable{
-      modifier initializer() {
-          _;
-      }
-}
+contract Initializable {
+    modifier initializer() {
+        _;
+    }
+
+    modifier reinitializer(uint64 version) {
+        _;
+    }
+}

tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/Initializable.sol

@@ -0,0 +1,15 @@
+import "./Initializable.sol";
+
+contract Reinitializer is Initializable {
+    address owner;
+
+    function initialize() external reinitializer(2) {
+        require(owner == address(0));
+        owner = msg.sender;
+    }
+
+    function kill() external {
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
+}

tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/Reinitializer.sol

@@ -0,0 +1,15 @@
+import "./Initializable.sol";
+
+contract AnyInitializer is Initializable {
+    address payable owner;
+
+    function anyName() external initializer {
+        require(owner == address(0));
+        owner = msg.sender;
+    }
+
+    function kill() external {
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
+}

tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/Reinitializer.sol-0.4.25.zip

@@ -1,5 +1,9 @@
-contract Initializable{
-      modifier initializer() {
-          _;
-      }
-}
+contract Initializable {
+    modifier initializer() {
+        _;
+    }
+
+    modifier reinitializer(uint64 version) {
+        _;
+    }
+}

tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/AnyInitializer.sol

@@ -0,0 +1,15 @@
+import "./Initializable.sol";
+
+contract Reinitializer is Initializable {
+    address payable owner;
+
+    function initialize() external reinitializer(2) {
+        require(owner == address(0));
+        owner = msg.sender;
+    }
+
+    function kill() external {
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
+}

tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/AnyInitializer.sol-0.5.16.zip

@@ -0,0 +1,15 @@
+import "./Initializable.sol";
+
+contract AnyInitializer is Initializable {
+    address payable owner;
+
+    function anyName() external initializer {
+        require(owner == address(0));
+        owner = payable(msg.sender);
+    }
+
+    function kill() external {
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
+}

tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/Initializable.sol

@@ -1,5 +1,9 @@
-contract Initializable{
-      modifier initializer() {
-          _;
-      }
-}
+contract Initializable {
+    modifier initializer() {
+        _;
+    }
+
+    modifier reinitializer(uint64 version) {
+        _;
+    }
+}

tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/Reinitializer.sol

@@ -0,0 +1,15 @@
+import "./Initializable.sol";
+
+contract Reinitializer is Initializable {
+    address payable owner;
+
+    function initialize() external reinitializer(2) {
+        require(owner == address(0));
+        owner = payable(msg.sender);
+    }
+
+    function kill() external {
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
+}

tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/Reinitializer.sol-0.5.16.zip

@@ -0,0 +1,15 @@
+import "./Initializable.sol";
+
+contract AnyInitializer is Initializable {
+    address payable owner;
+
+    function anyName() external initializer {
+        require(owner == address(0));
+        owner = payable(msg.sender);
+    }
+
+    function kill() external {
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
+}

tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/AnyInitializer.sol

@@ -1,15 +1,19 @@
-contract Initializable{
+contract Initializable {
     uint8 private _initialized;
     bool private _initializing;
 
     modifier initializer() {
         _;
     }
 
+    modifier reinitializer(uint64 version) {
+        _;
+    }
+
     function _disableInitializers() internal virtual {
         require(!_initializing, "Initializable: contract is initializing");
         if (_initialized < type(uint8).max) {
             _initialized = type(uint8).max;
         }
     }
-}
+}

tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/AnyInitializer.sol-0.6.11.zip

@@ -0,0 +1,15 @@
+import "./Initializable.sol";
+
+contract Reinitializer is Initializable {
+    address payable owner;
+
+    function initialize() external reinitializer(2) {
+        require(owner == address(0));
+        owner = payable(msg.sender);
+    }
+
+    function kill() external {
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
+}

tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/Initializable.sol

@@ -0,0 +1,15 @@
+import "./Initializable.sol";
+
+contract AnyInitializer is Initializable {
+    address payable owner;
+
+    function anyName() external initializer {
+        require(owner == address(0));
+        owner = payable(msg.sender);
+    }
+
+    function kill() external {
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
+}

tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/Reinitializer.sol

@@ -6,10 +6,14 @@ contract Initializable {
         _;
     }
 
+    modifier reinitializer(uint64 version) {
+        _;
+    }
+
     function _disableInitializers() internal virtual {
         require(!_initializing, "Initializable: contract is initializing");
         if (_initialized < type(uint8).max) {
             _initialized = type(uint8).max;
         }
     }
-}
+}

tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/Reinitializer.sol-0.6.11.zip

@@ -0,0 +1,15 @@
+import "./Initializable.sol";
+
+contract Reinitializer is Initializable {
+    address payable owner;
+
+    function initialize() external reinitializer(2) {
+        require(owner == address(0));
+        owner = payable(msg.sender);
+    }
+
+    function kill() external {
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
+}

tests/e2e/detectors/test_data/unprotected-upgrade/0.7.6/AnyInitializer.sol

@@ -938,6 +938,16 @@ def id_test(test_item: Test):
         "whitelisted.sol",
         "0.4.25",
     ),
+    Test(
+        all_detectors.UnprotectedUpgradeable,
+        "Reinitializer.sol",
+        "0.4.25",
+    ),
+    Test(
+        all_detectors.UnprotectedUpgradeable,
+        "AnyInitializer.sol",
+        "0.4.25",
+    ),
     Test(
         all_detectors.UnprotectedUpgradeable,
         "Buggy.sol",
@@ -953,6 +963,16 @@ def id_test(test_item: Test):
         "whitelisted.sol",
         "0.5.16",
     ),
+    Test(
+        all_detectors.UnprotectedUpgradeable,
+        "Reinitializer.sol",
+        "0.5.16",
+    ),
+    Test(
+        all_detectors.UnprotectedUpgradeable,
+        "AnyInitializer.sol",
+        "0.5.16",
+    ),
     Test(
         all_detectors.UnprotectedUpgradeable,
         "Buggy.sol",
@@ -968,6 +988,16 @@ def id_test(test_item: Test):
         "whitelisted.sol",
         "0.6.11",
     ),
+    Test(
+        all_detectors.UnprotectedUpgradeable,
+        "Reinitializer.sol",
+        "0.6.11",
+    ),
+    Test(
+        all_detectors.UnprotectedUpgradeable,
+        "AnyInitializer.sol",
+        "0.6.11",
+    ),
     Test(
         all_detectors.UnprotectedUpgradeable,
         "Buggy.sol",
@@ -978,6 +1008,16 @@ def id_test(test_item: Test):
         "Fixed.sol",
         "0.7.6",
     ),
+    Test(
+        all_detectors.UnprotectedUpgradeable,
+        "Reinitializer.sol",
+        "0.7.6",
+    ),
+    Test(
+        all_detectors.UnprotectedUpgradeable,
+        "AnyInitializer.sol",
+        "0.7.6",
+    ),
     Test(
         all_detectors.UnprotectedUpgradeable,
         "whitelisted.sol",
@@ -998,6 +1038,16 @@ def id_test(test_item: Test):
         "whitelisted.sol",
         "0.8.15",
     ),
+    Test(
+        all_detectors.UnprotectedUpgradeable,
+        "Reinitializer.sol",
+        "0.8.15",
+    ),
+    Test(
+        all_detectors.UnprotectedUpgradeable,
+        "AnyInitializer.sol",
+        "0.8.15",
+    ),
     Test(
         all_detectors.ABIEncoderV2Array,
         "storage_ABIEncoderV2_array.sol",