Merge pull request #71 from pessimistic-io/develop

Slitherin 0.3.0

docs/arbitrary_call.md

@@ -0,0 +1,23 @@
+# Arbitrary Call
+
+## Configuration
+
+- Check: `pess-arbitrary-call`
+- Severity: `High`
+- Confidence: `Low`
+
+## Description
+
+The detector iterates over all low-level calls, checks if the destination or calldata could be tainted(manipulated).
+
+### Potential Improvement
+
+Filter out role protected calls, divide detector to multiple detectors with different severity and confidence
+
+## Vulnerable Scenario
+
+[test scenarios](../tests/arbitrary_call_test.sol)
+
+## Recommendation
+
+Do not allow users to make arbitrary calls.

docs/unprotected_setter.md

@@ -9,7 +9,7 @@
 The detector sees if a contract contains a setter that changes the contract parameters without modifier protection or access control inside the function.
 
 ## Vulnerable Scenario
-The exploit scenario is in progress.
+[test scenario](../tests/unprotected_setter_test.sol)
 
 ## Recommendation
 Add access control and make sure that setter functions are protected. 

package.json

@@ -1,5 +1,5 @@
 {
   "dependencies": {
-    "@openzeppelin/contracts": "^4.9.2"
+    "@openzeppelin/contracts": "^4.9.3"
   }
 }

setup.py

@@ -10,7 +10,7 @@
     long_description_content_type="text/markdown",
     url="https://github.com/pessimistic-io/slitherin",
     author="Pessimistic.io",
-    version="0.2.1",
+    version="0.3.0",
     package_dir={"":"."},
     packages=find_packages(),
     license="AGPL-3.0",

slither_pess/__init__.py

@@ -1,3 +1,4 @@
+from slither_pess.detectors.arbitrary_call import ArbitraryCall
 from slither_pess.detectors.double_entry_token_possibility import (
     DoubleEntryTokenPossiblity,
 )
@@ -42,6 +43,7 @@ def make_plugin():
         UniswapV2,
         TokenFallback,
         ForContinueIncrement,
+        ArbitraryCall,
     ]
     plugin_printers = []
 

slither_pess/detectors/arbitrary_call.py

@@ -0,0 +1,135 @@
+from collections import namedtuple
+from dataclasses import dataclass
+from typing import Dict, List, Optional, Tuple, Set
+
+from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
+from slither.slithir.operations import TypeConversion, Operation
+from slither.core.declarations import (
+    Contract,
+    SolidityVariableComposed,
+    FunctionContract,
+)
+from slither.core.cfg.node import Node
+from slither.slithir.operations import LowLevelCall
+from slither.analyses.data_dependency.data_dependency import is_dependent, is_tainted
+
+
+# TODO/Possible improvements:
+# look for transferFroms if there is any transferFrom and contract contains whole arbitrary call - it is screwed
+# Filter out role protected
+
+
+class ArbitraryCall(AbstractDetector):
+    """
+    Detects arbitrary and dangerous calls.
+    (only detects low-level calls)
+    """
+
+    ARGUMENT = "pess-arbitrary-call"  # slither will launch the detector with slither.py --detect mydetector
+    HELP = "someaddress.call(somedata)"
+    IMPACT = DetectorClassification.HIGH
+    CONFIDENCE = DetectorClassification.LOW
+
+    WIKI = (
+        "https://github.com/pessimistic-io/slitherin/blob/master/docs/arbitrary_call.md"
+    )
+    WIKI_TITLE = "Arbitrary calls"
+    WIKI_DESCRIPTION = "Check docs"
+    WIKI_EXPLOIT_SCENARIO = "Attacker can manipulate on inputs"
+    WIKI_RECOMMENDATION = "Do not allow arbitrary calls"
+
+    def analyze_function(
+        self, function: FunctionContract
+    ) -> List[Tuple[FunctionContract, Node, LowLevelCall, bool, bool]]:
+        results = []
+        for node in function.nodes:
+            for ir in node.irs:
+                try:
+                    if isinstance(ir, LowLevelCall):
+                        destination_tainted = is_tainted(ir.destination, node, True)
+                        if (
+                            destination_tainted
+                            and ir.destination == SolidityVariableComposed("msg.sender")
+                        ):
+                            # We don't care about msg.sender, since will be only reentrancy issue
+                            destination_tainted = False
+                        args_tainted = any(
+                            is_tainted(arg, node, True) for arg in ir.arguments
+                        )  # seems like ir.arguments = [data] for all low-level calls
+                        if args_tainted or destination_tainted:
+                            results.append(
+                                (
+                                    function,
+                                    node,
+                                    ir,
+                                    args_tainted,
+                                    destination_tainted,
+                                )
+                            )
+
+                except Exception as e:
+                    print("ArbitraryCall:Failed to check types", e)
+                    print(ir)
+
+        return results
+
+    def analyze_contract(self, contract: Contract):
+        stores_approve = False
+        all_tainted_calls: List[
+            Tuple[FunctionContract, Node, LowLevelCall, bool, bool]
+        ] = []
+        results = []
+        for f in contract.functions:
+            res = self.analyze_function(f)
+            if res:
+                all_tainted_calls.extend(res)
+
+        for call_fn, node, ir, args_tainted, destination_tainted in all_tainted_calls:
+            info = ["Manipulated call found: ", node, " in ", call_fn, "\n"]
+            if args_tainted and destination_tainted:
+                text = "Both destination and calldata could be manipulated"
+            else:
+                part = "calldata" if args_tainted else "destination"
+                text = f"Only the {part} could be manipulated"
+            info += [f"{text}\n"]
+
+            for f in contract.functions:
+                if f.visibility not in ["external", "public"]:
+                    continue
+
+                fn_taints_args = False
+                fn_taints_destination = False
+
+                if args_tainted and any(
+                    is_dependent(ir.arguments[0], fn_arg, node)
+                    for fn_arg in f.variables
+                ):
+                    fn_taints_args = True
+
+                if destination_tainted and any(
+                    is_dependent(ir.destination, fn_arg, node) for fn_arg in f.variables
+                ):
+                    fn_taints_destination = True
+
+                if not (fn_taints_args or fn_taints_destination):
+                    continue
+
+                if fn_taints_args and fn_taints_destination:
+                    text = "The call could be fully manipulated (arbitrary call)"
+                else:
+                    part = "calldata" if fn_taints_args else "destination"
+                    text = f"The {part} could be manipulated"
+                info += [f"\t{text} through ", f, "\n"]
+
+            res = self.generate_result(info)
+            res.add(node)
+            results.append(res)
+        return results
+
+    def _detect(self):
+        results = []
+        for contract in self.compilation_unit.contracts_derived:
+            r = self.analyze_contract(contract)
+            if r:
+                results.extend(r)
+        return results

slither_pess/detectors/strange_setter.py

@@ -9,64 +9,79 @@ class StrangeSetter(AbstractDetector):
     Sees if contract contains a setter, that does not change contract storage variables.
     """
 
-    ARGUMENT = 'pess-strange-setter' # slither will launch the detector with slither.py --detect mydetector
-    HELP = 'Contract storage parameter is not changed by setter'
+    ARGUMENT = "pess-strange-setter"  # slither will launch the detector with slither.py --detect mydetector
+    HELP = "Contract storage parameter is not changed by setter"
     IMPACT = DetectorClassification.HIGH
     CONFIDENCE = DetectorClassification.MEDIUM
 
-    WIKI = 'https://github.com/pessimistic-io/slitherin/blob/master/docs/strange_setter.md'
-    WIKI_TITLE = 'Strange Setter'
+    WIKI = (
+        "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_EXPLOIT_SCENARIO = '-'
-    WIKI_RECOMMENDATION = 'Make sure that your setter actually sets something'
+    WIKI_EXPLOIT_SCENARIO = "-"
+    WIKI_RECOMMENDATION = "Make sure that your setter actually sets something"
 
-
-    def _is_strange_setter(self, fun: Function) -> bool: 
+    def _is_strange_setter(self, fun: Function) -> bool:
         """Checks if setter sets smth to a storage variable and if function parameters are used when setting"""
-        for fin in fun.internal_calls:  # branch with for-loop for setters in internal calls
-            if isinstance(fin, Function):   # check for a correct function type
+        if not isinstance(fun, Function):
+            return True
+
+        if not fun.parameters:
+            # nothing is in the params, so we don't care
+            return False
+        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
-                                return False
-        if isinstance(fun, Function):   # check for a correct function type
-            for param in fun.parameters: 
-                if fun.state_variables_written:
-                    for n in fun.nodes:
-                        if str(param) in str(n):
+                        if n.state_variables_written and str(param) in str(
+                            n
+                        ):  # check if there's a state variable setter using function parameters
                             return False
+        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
 
     def _is_strange_constructor(self, fun: Function) -> bool:
         """Checks if constructor sets nothing"""
-        if isinstance(fun, Function):   # check for a correct function type
-            state_var_internal = {}
-            for fin in fun.internal_calls:
-                if isinstance(fin, Function):
-                    state_var_internal = fin.state_variables_written
-            if not fun.state_variables_written and not state_var_internal: # checks for the state variables written in constructor and via internal calls
-                return True
-        return False
-
+        return self._is_strange_setter(fun)
 
     def _detect(self) -> List[Output]:
         """Main function"""
         res = []
         for contract in self.compilation_unit.contracts_derived:
             if not contract.is_interface:
-                overriden_funtions = [] # functions that are overridden
+                overriden_funtions = []  # functions that are overridden
                 for f in contract.functions:
                     if f.parameters:
                         x = False
-                        overriden_funtions.append(contract.get_functions_overridden_by(f))  # adding functions to an overridden list
+                        overriden_funtions.append(
+                            contract.get_functions_overridden_by(f)
+                        )  # adding functions to an overridden list
                         if f.name == "constructor":
                             x = self._is_strange_constructor(f)
-                        if f.name.startswith("set") and not f in overriden_funtions and len(f.nodes) != 0:  # check if setter starts with 'set', is not overriden and is not empty 
+                        elif (
+                            f.name.startswith("set")
+                            and not f in overriden_funtions
+                            and len(f.nodes) != 0
+                        ):  # check if setter starts with 'set', is not overriden and is not empty
                             x = self._is_strange_setter(f)
                         if x:
-                            res.append(self.generate_result([
-                                "Function", ' ',
-                                f, ' is a strange setter. ',
-                                'Nothing is set in constructor or set in a function without using function parameters'
-                                '\n']))
+                            res.append(
+                                self.generate_result(
+                                    [
+                                        "Function",
+                                        " ",
+                                        f,
+                                        " is a strange setter. ",
+                                        "Nothing is set in constructor or set in a function without using function parameters"
+                                        "\n",
+                                    ]
+                                )
+                            )
         return res