Merge pull request #122 from pessimistic-io/reduce-fps-pess-strange-s…

…etter

Reduce number of FPs of pess strange setter detector

docs/strange_setter.md

@@ -6,15 +6,15 @@
 * Confidence: `Medium`
 
 ## Description
-The detector sees if a contract contains a setter (also constructor) that does not change contract storage variables.
-Setter functions MUST change the values of storage variables.
-Setter functions that do not modify storage variables may lead to contract misfunctions.
+The detector sees if a contract contains a setter (also constructor) that does not change contract storage variables or does not perform external calls using provided arguments.
+Setter functions MUST change the values of storage variables or perform external calls using provided parameters.
+Setter functions that do not use provided variables may lead to contract misfunctions.
 
 ### Potential Improvement
-Remove highlights of mappings.
+Detect shadowing before storage update/external call
 
 ## Vulnerable Scenario
 [test scenario](../tests/strange_setter_test.sol)
 
 ## Recommendation
-Make sure that setter functions modify the states of storage variables.
+Make sure that setter functions modify the states of storage variables or performs external call using provided arguments.

slitherin/detectors/strange_setter.py

@@ -2,11 +2,12 @@
 from slither.utils.output import Output
 from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
 from slither.core.declarations import Function
+from slither.analyses.data_dependency.data_dependency import is_dependent, is_tainted
 
 
 class StrangeSetter(AbstractDetector):
     """
-    Sees if contract contains a setter, that does not change contract storage variables.
+    Sees if contract contains a setter, that does not change contract storage variables or that does not use arguments for an external call.
     """
 
     ARGUMENT = "pess-strange-setter"  # slither will launch the detector with slither.py --detect mydetector
@@ -18,7 +19,7 @@ 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"
+    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"
 
@@ -30,6 +31,7 @@ def _is_strange_setter(self, fun: Function) -> bool:
         if not fun.parameters:
             # 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
@@ -39,13 +41,19 @@ def _is_strange_setter(self, fun: Function) -> bool:
                         if n.state_variables_written and str(param) in str(
                             n
                         ):  # check if there's a state variable setter using function parameters
-                            return False
+                            used_params.add(param)
         for param in fun.parameters:
             if fun.state_variables_written:
                 for n in fun.nodes:
                     if str(param) in str(n):
-                        return False
-        return True
+                        used_params.add(param)
+        for param in fun.parameters:
+            for external in fun.external_calls_as_expressions:
+                for arg in [*external.arguments, external._called._expression]:
+                    if str(arg) == str(param):
+                        used_params.add(param)
+        intersection_len = len(set(fun.parameters) & used_params)
+        return intersection_len != len(fun.parameters)
 
     def _is_strange_constructor(self, fun: Function) -> bool:
         """Checks if constructor sets nothing"""

tests/strange_setter_test.sol

@@ -1,5 +1,8 @@
 pragma solidity ^0.8.0;
 
+interface ExternalContract {
+    function set(bool arg) external;
+}
 // What it should detect:
 // If smth is set in the function, and the function contains parameters,
 // and this parameters were not uset to set.
@@ -37,10 +40,18 @@ contract StrangeSetter {
     }
 
     function setWithInt(bytes32 nameHash, address builder) public onlyOwner {
-        uint256 x = 0; //TODO this is not detected. There are params which are not used
+        uint256 x = 0;
         vulnurable_internal(x);
     }
 
+    function setSwapEnabledExternal_ok(ExternalContract target, bool swapEnabled) external onlyOwner {
+        target.set(swapEnabled);
+    }
+
+    function setUseOnlyOneArg_vulnerable(uint256 arg1, bool isProtectedArg) external onlyOwner {
+        isProtected = isProtectedArg;
+    }
+
     function set_ok(uint256 setter) public onlyOwner {
         toSet = setter;
     }