Merge pull request #743 from JoranHonig/bugfix/callback_fixes

Bugfix: Some fixes around the callback refactor

mythril/analysis/modules/suicide.py

@@ -5,16 +5,61 @@ from mythril.analysis.report import Issue
 from mythril.analysis.swc_data import UNPROTECTED_SELFDESTRUCT
 from mythril.exceptions import UnsatError
 from mythril.analysis.modules.base import DetectionModule
-from mythril.laser.ethereum.transaction import ContractCreationTransaction
-import re
+from mythril.laser.ethereum.state.global_state import GlobalState
 import logging
 
 
-"""
-MODULE DESCRIPTION:
+def _analyze_state(state):
+    logging.info("Suicide module: Analyzing suicide instruction")
+    node = state.node
+    instruction = state.get_current_instruction()
+    to = state.mstate.stack[-1]
 
+    logging.debug("[UNCHECKED_SUICIDE] suicide in function " + node.function_name)
 
-"""
+    description = "A reachable SUICIDE instruction was detected. "
+
+    if "caller" in str(to):
+        description += "The remaining Ether is sent to the caller's address.\n"
+    elif "storage" in str(to):
+        description += "The remaining Ether is sent to a stored address.\n"
+    elif "calldata" in str(to):
+        description += "The remaining Ether is sent to an address provided as a function argument.\n"
+    elif type(to) == BitVecNumRef:
+        description += "The remaining Ether is sent to: " + hex(to.as_long()) + "\n"
+    else:
+        description += "The remaining Ether is sent to: " + str(to) + "\n"
+
+    not_creator_constraints, constrained = get_non_creator_constraints(state)
+
+    if constrained:
+        return []
+    try:
+        solver.get_model(node.constraints + not_creator_constraints)
+
+        debug = "Transaction Sequence: " + str(
+            solver.get_transaction_sequence(
+                state, node.constraints + not_creator_constraints
+            )
+        )
+
+        issue = Issue(
+            contract=node.contract_name,
+            function_name=node.function_name,
+            address=instruction["address"],
+            swc_id=UNPROTECTED_SELFDESTRUCT,
+            bytecode=state.environment.code.bytecode,
+            title="Unchecked SUICIDE",
+            _type="Warning",
+            description=description,
+            debug=debug,
+            gas_used=(state.mstate.min_gas_used, state.mstate.max_gas_used),
+        )
+        return [issue]
+    except UnsatError:
+        logging.info("[UNCHECKED_SUICIDE] no model found")
+
+    return []
 
 
 class SuicideModule(DetectionModule):
@@ -28,77 +73,17 @@ class SuicideModule(DetectionModule):
                 "or where msg.sender is checked against a tainted storage index (i.e. "
                 "there's a write to that index is unconstrained by msg.sender)."
             ),
+            entrypoint="callback",
         )
+        self._issues = []
 
-    def execute(self, state_space):
-
-        logging.debug("Executing module: UNCHECKED_SUICIDE")
-
-        issues = []
-
-        for k in state_space.nodes:
-            node = state_space.nodes[k]
-
-            for state in node.states:
-                issues += self._analyze_state(state, node)
-
-        return issues
-
-    def _analyze_state(self, state, node):
-        issues = []
-        instruction = state.get_current_instruction()
-
-        if instruction["opcode"] != "SUICIDE":
-            return []
-
-        to = state.mstate.stack[-1]
-
-        logging.debug("[UNCHECKED_SUICIDE] suicide in function " + node.function_name)
-
-        description = "A reachable SUICIDE instruction was detected. "
-
-        if "caller" in str(to):
-            description += "The remaining Ether is sent to the caller's address.\n"
-        elif "storage" in str(to):
-            description += "The remaining Ether is sent to a stored address.\n"
-        elif "calldata" in str(to):
-            description += "The remaining Ether is sent to an address provided as a function argument.\n"
-        elif type(to) == BitVecNumRef:
-            description += "The remaining Ether is sent to: " + hex(to.as_long()) + "\n"
-        else:
-            description += "The remaining Ether is sent to: " + str(to) + "\n"
-
-        not_creator_constraints, constrained = get_non_creator_constraints(state)
-
-        if constrained:
-            return []
-
-        try:
-            model = solver.get_model(node.constraints + not_creator_constraints)
-
-            debug = "Transaction Sequence: " + str(
-                solver.get_transaction_sequence(
-                    state, node.constraints + not_creator_constraints
-                )
-            )
-
-            issue = Issue(
-                contract=node.contract_name,
-                function_name=node.function_name,
-                address=instruction["address"],
-                swc_id=UNPROTECTED_SELFDESTRUCT,
-                bytecode=state.environment.code.bytecode,
-                title="Unchecked SUICIDE",
-                _type="Warning",
-                description=description,
-                debug=debug,
-                gas_used=(state.mstate.min_gas_used, state.mstate.max_gas_used),
-            )
-            issues.append(issue)
-        except UnsatError:
-            logging.debug("[UNCHECKED_SUICIDE] no model found")
+    def execute(self, state: GlobalState):
+        self._issues.extend(_analyze_state(state))
+        return self.issues
 
-        return issues
+    @property
+    def issues(self):
+        return self._issues
 
 
 detector = SuicideModule()

mythril/analysis/security.py

@@ -9,6 +9,12 @@ import logging
 OPCODE_LIST = [c[0] for _, c in opcodes.items()]
 
 
+def reset_callback_modules():
+    modules = get_detection_modules("callback")
+    for module in modules:
+        module.detector._issues = []
+
+
 def get_detection_module_hooks():
     hook_dict = defaultdict(list)
     _modules = get_detection_modules(entrypoint="callback")
@@ -35,17 +41,18 @@ def get_detection_modules(entrypoint, include_modules=()):
     _modules = []
 
     if not include_modules:
-
-        for loader, name, _ in pkgutil.walk_packages(modules.__path__):
-            module = loader.find_module(name).load_module(name)
-            if module.__name__ != "base" and module.detector.entrypoint == entrypoint:
-                _modules.append(module)
-
+        for loader, module_name, _ in pkgutil.walk_packages(modules.__path__):
+            if module_name != "base":
+                module = importlib.import_module(
+                    "mythril.analysis.modules." + module_name
+                )
+                if module.detector.entrypoint == entrypoint:
+                    _modules.append(module)
     else:
         for module_name in include_modules:
-            module = importlib.import_module(module_name, modules)
-
-            _modules.append(module)
+            module = importlib.import_module("mythril.analysis.modules." + module_name)
+            if module.__name__ != "base" and module.detector.entrypoint == entrypoint:
+                _modules.append(module)
 
     logging.info("Found %s detection modules", len(_modules))
     return _modules
@@ -61,4 +68,10 @@ def fire_lasers(statespace, module_names=()):
         logging.info("Executing " + module.detector.name)
         issues += module.detector.execute(statespace)
 
+    for module in get_detection_modules(
+        entrypoint="callback", include_modules=module_names
+    ):
+        logging.debug("Retrieving results for " + module.detector.name)
+        issues += module.detector.issues
+
     return issues

mythril/analysis/symbolic.py

@@ -60,7 +60,7 @@ class SymExecWrapper:
             transaction_count=transaction_count,
         )
         self.laser.register_hooks(
-            hook_type="post", hook_dict=get_detection_module_hooks()
+            hook_type="pre", hook_dict=get_detection_module_hooks()
         )
 
         if isinstance(contract, SolidityContract):

mythril/laser/ethereum/state/global_state.py

@@ -21,6 +21,7 @@ class GlobalState:
         machine_state=None,
         transaction_stack=None,
         last_return_data=None,
+        annotations=None,
     ):
         """ Constructor for GlobalState"""
         self.node = node
@@ -32,6 +33,7 @@ class GlobalState:
         self.transaction_stack = transaction_stack if transaction_stack else []
         self.op_code = ""
         self.last_return_data = last_return_data
+        self.annotations = annotations or []
 
     def __copy__(self) -> "GlobalState":
         world_state = copy(self.world_state)
@@ -45,6 +47,7 @@ class GlobalState:
             mstate,
             transaction_stack=transaction_stack,
             last_return_data=self.last_return_data,
+            annotations=self.annotations,
         )
 
     @property

tests/report_test.py

@@ -1,5 +1,5 @@
 from mythril.analysis.report import Report
-from mythril.analysis.security import fire_lasers
+from mythril.analysis.security import fire_lasers, reset_callback_modules
 from mythril.analysis.symbolic import SymExecWrapper
 from mythril.ethereum import util
 from mythril.solidity.soliditycontract import EVMContract
@@ -42,6 +42,7 @@ def _generate_report(input_file):
 @pytest.fixture(scope="module")
 def reports():
     """ Fixture that analyses all reports"""
+    reset_callback_modules()
     pool = Pool(cpu_count())
     input_files = sorted(
         [f for f in TESTDATA_INPUTS.iterdir() if f.name != "environments.sol.o"]