strange setter fix

slitherin/detectors/strange_setter.py

@@ -5,6 +5,7 @@
 import slither.core.expressions.new_array as na
 import slither.core.expressions.new_contract as nc
 from slither.analyses.data_dependency.data_dependency import is_dependent
+from slither.slithir.operations.internal_call import InternalCall
 
 
 class StrangeSetter(AbstractDetector):
@@ -21,7 +22,9 @@ class StrangeSetter(AbstractDetector):
         "https://github.com/pessimistic-io/slitherin/blob/master/docs/strange_setter.md"
     )
     WIKI_TITLE = "Strange Setter"
-    WIKI_DESCRIPTION = "Setter must write to storage variables or pass arguments to external calls"
+    WIKI_DESCRIPTION = (
+        "Setter must write to storage variables or pass arguments to external calls"
+    )
     WIKI_EXPLOIT_SCENARIO = "-"
     WIKI_RECOMMENDATION = "Make sure that your setter actually sets something"
 
@@ -34,26 +37,26 @@ def _is_strange_setter(self, fun: Function) -> bool:
             # nothing is in the params, so we don't care
             return False
         used_params = set()
-        for (
-            fin
-        ) in fun.internal_calls:  # branch with for-loop for setters in internal calls
-            if isinstance(fin, Function):
-                for param in fin.parameters:
-                    for n in fin.nodes:
-                        if n.state_variables_written and str(param) in str(
-                            n
-                        ):  # check if there's a state variable setter using function parameters
-                            used_params.add(param)
+
+        for node in fun.nodes:
+            for ir in node.irs:
+                if isinstance(ir, InternalCall):
+                    for arg in ir.arguments:
+                        used_params.add(arg)
+
         for param in fun.parameters:
             if fun.state_variables_written:
                 for n in fun.nodes:
                     if str(param) in str(n):
                         used_params.add(param)
+
         for param in fun.parameters:
             for external in fun.external_calls_as_expressions:
                 if isinstance(external._called, na.NewArray):
                     continue
-                if isinstance(external._called, nc.NewContract): # skip new contract calls, idk how to get arguments passed to creation
+                if isinstance(
+                    external._called, nc.NewContract
+                ):  # skip new contract calls, idk how to get arguments passed to creation
                     continue
                 for arg in [*external.arguments, external._called._expression]:
                     if str(arg) == str(param):
@@ -76,7 +79,7 @@ def _detect(self) -> List[Output]:
         """Main function"""
         res = []
         for contract in self.compilation_unit.contracts_derived:
-            if not contract.is_interface:
+            if not contract.is_interface and not contract.is_library:
                 overriden_funtions = []  # functions that are overridden
                 for f in contract.functions:
                     if f.parameters:

tests/strange_setter_test.sol

@@ -44,18 +44,28 @@ contract StrangeSetter {
         vulnurable_internal(x);
     }
 
-    function setSwapEnabledExternal_ok(ExternalContract target, bool swapEnabled) external onlyOwner {
+    function setSwapEnabledExternal_ok(
+        ExternalContract target,
+        bool swapEnabled
+    ) external onlyOwner {
         target.set(swapEnabled);
     }
 
-    function setUseOnlyOneArg_vulnerable(uint256 arg1, bool isProtectedArg) external onlyOwner {
+    function setUseOnlyOneArg_vulnerable(
+        uint256 arg1,
+        bool isProtectedArg
+    ) external onlyOwner {
         isProtected = isProtectedArg;
     }
 
     function set_ok(uint256 setter) public onlyOwner {
         toSet = setter;
     }
 
+    function set_ok2(uint val) public onlyOwner {
+        vulnurable_internal(val);
+    }
+
     function set_ok_with_temp_war(uint256 setter) public onlyOwner {
         uint k = setter * 100;
         toSet = k;
@@ -88,12 +98,12 @@ contract OkConstructor {
     }
 }
 
-contract TestInheritance is StrangeSetter{
+contract TestInheritance is StrangeSetter {
     constructor(uint256 _toSet) StrangeSetter(_toSet) {}
 }
 
 contract TestNewContract {
     constructor(uint256 _toSet) {
         new TestInheritance(_toSet);
     }
-}
+}