fix issue-1029, FP on unprotected-upgrade detector

slither/detectors/statements/unprotected_upgradeable.py

@@ -20,6 +20,20 @@ def _can_be_destroyed(contract) -> List[Function]:
                 break
     return targets
 
+def _has_initializer_modifier(functions: List[Function]) -> bool:
+    for f in functions:
+        for m in f.modifiers:
+            if m.name == "initializer":
+                return True
+    return False
+
+def _has_protected_initialize(functions: List[Function]) -> bool:
+    for f in functions:
+        if f.name == "initialize":
+            for m in f.modifiers:
+                if m.name == "initializer":
+                    return True
+    return False
 
 class UnprotectedUpgradeable(AbstractDetector):
 
@@ -61,34 +75,36 @@ class UnprotectedUpgradeable(AbstractDetector):
 
         for contract in self.compilation_unit.contracts_derived:
             if contract.is_upgradeable:
-                functions_that_can_destroy = _can_be_destroyed(contract)
-                if functions_that_can_destroy:
-                    initiliaze_functions = [f for f in contract.functions if f.name == "initialize"]
-                    vars_init_ = [
-                        init.all_state_variables_written() for init in initiliaze_functions
-                    ]
-                    vars_init = [item for sublist in vars_init_ for item in sublist]
-
-                    vars_init_in_constructors_ = [
-                        f.all_state_variables_written() for f in contract.constructors
-                    ]
-                    vars_init_in_constructors = [
-                        item for sublist in vars_init_in_constructors_ for item in sublist
-                    ]
-                    if vars_init and (set(vars_init) - set(vars_init_in_constructors)):
-                        info = (
-                            [
-                                contract,
-                                " is an upgradeable contract that does not protect its initiliaze functions: ",
+                    if not _has_initializer_modifier(contract.constructors) or not _has_protected_initialize(contract.functions):
+                        functions_that_can_destroy = _can_be_destroyed(contract)
+                        if functions_that_can_destroy:
+                            initiliaze_functions = [f for f in contract.functions if f.name == "initialize"]
+
+                            vars_init_ = [
+                                init.all_state_variables_written() for init in initiliaze_functions
+                            ]
+                            vars_init = [item for sublist in vars_init_ for item in sublist]
+
+                            vars_init_in_constructors_ = [
+                                f.all_state_variables_written() for f in contract.constructors
                             ]
-                            + initiliaze_functions
-                            + [
-                                ". Anyone can delete the contract with: ",
+                            vars_init_in_constructors = [
+                                item for sublist in vars_init_in_constructors_ for item in sublist
                             ]
-                            + functions_that_can_destroy
-                        )
+                            if vars_init and (set(vars_init) - set(vars_init_in_constructors)):
+                                info = (
+                                    [
+                                        contract,
+                                        " is an upgradeable contract that does not protect its initiliaze functions: ",
+                                    ]
+                                    + initiliaze_functions
+                                    + [
+                                        ". Anyone can delete the contract with: ",
+                                    ]
+                                    + functions_that_can_destroy
+                                )
 
-                        res = self.generate_result(info)
-                        results.append(res)
+                                res = self.generate_result(info)
+                                results.append(res)
 
-        return results
+        return results

tests/detectors/unprotected-upgrade/0.4.25/Fixed.sol

@@ -21,7 +21,6 @@ contract Fixed is Initializable{
     }
 }
 
-
 contract Not_Upgradeable{
 }
 
@@ -36,4 +35,20 @@ contract UpgradeableNoDestruct is Initializable{
         require(owner == address(0));
         owner = msg.sender;
     }
+}
+
+contract Fixed2 is Initializable {
+    address owner;
+
+    constructor() public initializer {}
+
+    function initialize() external initializer {
+        require(owner == address(0));
+        owner = msg.sender;
+    }
+
+    function kill() external {
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
 }

tests/detectors/unprotected-upgrade/0.5.16/Fixed.sol

@@ -21,7 +21,6 @@ contract Fixed is Initializable{
     }
 }
 
-
 contract Not_Upgradeable{
 }
 
@@ -36,4 +35,20 @@ contract UpgradeableNoDestruct is Initializable{
         require(owner == address(0));
         owner = msg.sender;
     }
+}
+
+contract Fixed2 is Initializable {
+    address payable owner;
+
+    constructor() public initializer {}
+
+    function initialize() external initializer {
+        require(owner == address(0));
+        owner = msg.sender;
+    }
+
+    function kill() external {
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
 }

tests/detectors/unprotected-upgrade/0.6.11/Fixed.sol

@@ -36,4 +36,20 @@ contract UpgradeableNoDestruct is Initializable{
         require(owner == address(0));
         owner = msg.sender;
     }
+}
+
+contract Fixed2 is Initializable {
+    address payable owner;
+
+    constructor() public initializer {}
+
+    function initialize() external initializer {
+        require(owner == address(0));
+        owner = msg.sender;
+    }
+
+    function kill() external {
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
 }

tests/detectors/unprotected-upgrade/0.7.6/Fixed.sol

@@ -3,7 +3,7 @@ import "./Initializable.sol";
 contract Fixed is Initializable{
     address payable owner;
 
-    constructor() public{
+    constructor() {
         owner = msg.sender;
     }
 
@@ -21,14 +21,13 @@ contract Fixed is Initializable{
     }
 }
 
-
 contract Not_Upgradeable{
 }
 
 contract UpgradeableNoDestruct is Initializable{
     address payable owner;
 
-    constructor() public{
+    constructor() {
         owner = msg.sender;
     }
 
@@ -36,4 +35,20 @@ contract UpgradeableNoDestruct is Initializable{
         require(owner == address(0));
         owner = msg.sender;
     }
+}
+
+contract Fixed2 is Initializable {
+    address payable owner;
+
+    constructor() initializer {}
+
+    function initialize() external initializer {
+        require(owner == address(0));
+        owner = msg.sender;
+    }
+
+    function kill() external {
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
 }