fix issue-1029, FP on unprotected-upgrade detector

slither/detectors/statements/unprotected_upgradeable.py

@@ -21,6 +21,23 @@ def _can_be_destroyed(contract) -> List[Function]:
     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):
 
     ARGUMENT = "unprotected-upgrade"
@@ -61,34 +78,40 @@ def _detect(self):
 
         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: ",
-                            ]
-                            + initiliaze_functions
-                            + [
-                                ". Anyone can delete the contract with: ",
-                            ]
-                            + functions_that_can_destroy
-                        )
-
-                        res = self.generate_result(info)
-                        results.append(res)
+                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
+                        ]
+                        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: ",
+                                ]
+                                + initiliaze_functions
+                                + [
+                                    ". Anyone can delete the contract with: ",
+                                ]
+                                + functions_that_can_destroy
+                            )
+
+                            res = self.generate_result(info)
+                            results.append(res)
 
         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,19 +21,34 @@ contract Fixed is Initializable{
     }
 }
 
-
 contract Not_Upgradeable{
 }
 
 contract UpgradeableNoDestruct is Initializable{
     address payable owner;
 
-    constructor() public{
+    constructor() {
         owner = msg.sender;
     }
 
     function initialize() external initializer{
         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);
+    }
 }