Detect State changes after external calls

mythril/analysis/call_helpers.py

@@ -29,7 +29,7 @@ def get_call_from_state(state: GlobalState) -> Union[Call, None]:
             get_variable(stack[-7]),
         )
 
-        if to.type == VarType.CONCRETE and to.val < 5:
+        if to.type == VarType.CONCRETE and 0 < to.val < 5:
             return None
 
         if meminstart.type == VarType.CONCRETE and meminsz.type == VarType.CONCRETE:

mythril/analysis/modules/external_calls.py

@@ -2,12 +2,17 @@
 calls."""
 
 from mythril.analysis import solver
+from mythril.analysis.ops import Call, Variable, VarType
 from mythril.analysis.swc_data import REENTRANCY
 from mythril.analysis.modules.base import DetectionModule
 from mythril.analysis.report import Issue
-from mythril.laser.smt import UGT, symbol_factory
+from mythril.analysis.call_helpers import get_call_from_state
+from mythril.laser.smt import UGT, symbol_factory, simplify
+from mythril.laser.ethereum.state.annotation import StateAnnotation
 from mythril.laser.ethereum.state.global_state import GlobalState
 from mythril.exceptions import UnsatError
+from copy import copy
+from typing import List, cast
 import logging
 import json
 
@@ -22,6 +27,71 @@ an informational issue.
 """
 
 
+class CallIssue:
+    def __init__(self, call: Call, user_defined_address: bool) -> None:
+        self.call = call
+        self.user_defined_address = user_defined_address
+
+
+class ExternalCallsAnnotation(StateAnnotation):
+    def __init__(self) -> None:
+        self.calls = []  # type: List[CallIssue]
+
+    def __copy__(self):
+        result = ExternalCallsAnnotation()
+        result.calls = copy(self.calls)
+        return result
+
+
+def _get_state_change_issues(
+    callissues: List[CallIssue], state: GlobalState, address: int
+) -> List[Issue]:
+    issues = []
+    for callissue in callissues:
+        severity = "Medium" if callissue.user_defined_address else "Low"
+        call = callissue.call
+        logging.debug(
+            "[EXTERNAL_CALLS] Detected state changes at addresses: {}".format(address)
+        )
+        description_head = (
+            "The contract account state is changed after an external call. "
+        )
+        description_tail = (
+            "Consider that the called contract could re-enter the function before this "
+            "state change takes place. This can lead to business logic vulnerabilities."
+        )
+
+        issue = Issue(
+            contract=call.node.contract_name,
+            function_name=call.node.function_name,
+            address=address,
+            title="State change after external call",
+            severity=severity,
+            description_head=description_head,
+            description_tail=description_tail,
+            swc_id=REENTRANCY,
+            bytecode=state.environment.code.bytecode,
+        )
+        issues.append(issue)
+    return issues
+
+
+def _handle_state_change(
+    state: GlobalState, address: int, annotation: ExternalCallsAnnotation
+) -> List[Issue]:
+    calls = annotation.calls
+    issues = _get_state_change_issues(calls, state, address)
+    return issues
+
+
+def _balance_change(value: Variable) -> bool:
+    if value.type == VarType.CONCRETE:
+        return value.val > 0
+    else:
+        zero = symbol_factory.BitVecVal(0, 256)
+        return simplify(value.val > zero)
+
+
 def _analyze_state(state):
     """
 
@@ -31,8 +101,29 @@ def _analyze_state(state):
     node = state.node
     gas = state.mstate.stack[-1]
     to = state.mstate.stack[-2]
-
+    issues = []
     address = state.get_current_instruction()["address"]
+    annotations = cast(
+        List[ExternalCallsAnnotation],
+        list(state.get_annotations(ExternalCallsAnnotation)),
+    )
+    if len(annotations) == 0:
+        log.debug("Creating annotation for state")
+        state.annotate(ExternalCallsAnnotation())
+        annotations = cast(
+            List[ExternalCallsAnnotation],
+            list(state.get_annotations(ExternalCallsAnnotation)),
+        )
+
+    if state.get_current_instruction()["opcode"] == "SSTORE":
+        return _handle_state_change(state, address=address, annotation=annotations[0])
+    call = get_call_from_state(state)
+
+    if call is None:
+        return []
+
+    if _balance_change(call.value):
+        return _handle_state_change(state, address=address, annotation=annotations[0])
 
     try:
         constraints = node.constraints
@@ -68,6 +159,7 @@ def _analyze_state(state):
                 debug=debug,
                 gas_used=(state.mstate.min_gas_used, state.mstate.max_gas_used),
             )
+            annotations[0].calls.append(CallIssue(call=call, user_defined_address=True))
 
         except UnsatError:
 
@@ -95,12 +187,15 @@ def _analyze_state(state):
                 debug=debug,
                 gas_used=(state.mstate.min_gas_used, state.mstate.max_gas_used),
             )
+            annotations[0].calls.append(
+                CallIssue(call=call, user_defined_address=False)
+            )
 
     except UnsatError:
         log.debug("[EXTERNAL_CALLS] No model found.")
         return []
-
-    return [issue]
+    issues.append(issue)
+    return issues
 
 
 class ExternalCalls(DetectionModule):
@@ -114,7 +209,7 @@ class ExternalCalls(DetectionModule):
             swc_id=REENTRANCY,
             description=DESCRIPTION,
             entrypoint="callback",
-            pre_hooks=["CALL"],
+            pre_hooks=["CALL", "SSTORE"],
         )
 
     def execute(self, state: GlobalState):

tests/testdata/outputs_expected/calls.sol.o.json

@@ -1 +1,123 @@
-{"error": null, "issues": [{"address": 661, "contract": "Unknown", "debug": "<DEBUG-DATA>", "description": "The contract executes an external message call.\nAn external function call to a fixed contract address is executed. Make sure that the callee contract has been reviewed carefully.", "function": "thisisfine()", "max_gas_used": 1254, "min_gas_used": 643, "severity": "Low", "sourceMap": null, "swc-id": "107", "title": "External Call To Fixed Address"}, {"address": 661, "contract": "Unknown", "debug": "<DEBUG-DATA>", "description": "The return value of a message call is not checked.\nExternal calls return a boolean value. If the callee contract halts with an exception, 'false' is returned and execution continues in the caller. It is usually recommended to wrap external calls into a require statement to prevent unexpected states.", "function": "thisisfine()", "max_gas_used": 35972, "min_gas_used": 1361, "severity": "Low", "sourceMap": null, "swc-id": "104", "title": "Unchecked Call Return Value"}, {"address": 779, "contract": "Unknown", "debug": "<DEBUG-DATA>", "description": "The contract executes an external message call.\nAn external function call to a fixed contract address is executed. Make sure that the callee contract has been reviewed carefully.", "function": "callstoredaddress()", "max_gas_used": 1298, "min_gas_used": 687, "severity": "Low", "sourceMap": null, "swc-id": "107", "title": "External Call To Fixed Address"}, {"address": 779, "contract": "Unknown", "debug": "<DEBUG-DATA>", "description": "The return value of a message call is not checked.\nExternal calls return a boolean value. If the callee contract halts with an exception, 'false' is returned and execution continues in the caller. It is usually recommended to wrap external calls into a require statement to prevent unexpected states.", "function": "callstoredaddress()", "max_gas_used": 36016, "min_gas_used": 1405, "severity": "Low", "sourceMap": null, "swc-id": "104", "title": "Unchecked Call Return Value"}, {"address": 858, "contract": "Unknown", "debug": "<DEBUG-DATA>", "description": "The contract executes an external message call.\nAn external function call to a fixed contract address is executed. Make sure that the callee contract has been reviewed carefully.", "function": "reentrancy()", "max_gas_used": 1320, "min_gas_used": 709, "severity": "Low", "sourceMap": null, "swc-id": "107", "title": "External Call To Fixed Address"}, {"address": 858, "contract": "Unknown", "debug": "<DEBUG-DATA>", "description": "The return value of a message call is not checked.\nExternal calls return a boolean value. If the callee contract halts with an exception, 'false' is returned and execution continues in the caller. It is usually recommended to wrap external calls into a require statement to prevent unexpected states.", "function": "reentrancy()", "max_gas_used": 61052, "min_gas_used": 6441, "severity": "Low", "sourceMap": null, "swc-id": "104", "title": "Unchecked Call Return Value"}, {"address": 912, "contract": "Unknown", "debug": "<DEBUG-DATA>", "description": "A call to a user-supplied address is executed.\nThe callee address of an external message call can be set by the caller. Note that the callee can contain arbitrary code and may re-enter any function in this contract. Review the business logic carefully to prevent averse effects on thecontract state.", "function": "calluseraddress(address)", "max_gas_used": 616, "min_gas_used": 335, "severity": "Medium", "sourceMap": null, "swc-id": "107", "title": "External Call To User-Supplied Address"}, {"address": 912, "contract": "Unknown", "debug": "<DEBUG-DATA>", "description": "The return value of a message call is not checked.\nExternal calls return a boolean value. If the callee contract halts with an exception, 'false' is returned and execution continues in the caller. It is usually recommended to wrap external calls into a require statement to prevent unexpected states.", "function": "calluseraddress(address)", "max_gas_used": 35336, "min_gas_used": 1055, "severity": "Low", "sourceMap": null, "swc-id": "104", "title": "Unchecked Call Return Value"}], "success": true}
+{
+   "error":null,
+   "issues":[
+      {
+         "address":661,
+         "contract":"Unknown",
+         "debug":"<DEBUG-DATA>",
+         "description":"The contract executes an external message call.\nAn external function call to a fixed contract address is executed. Make sure that the callee contract has been reviewed carefully.",
+         "function":"thisisfine()",
+         "max_gas_used":1254,
+         "min_gas_used":643,
+         "severity":"Low",
+         "sourceMap":null,
+         "swc-id":"107",
+         "title":"External Call To Fixed Address"
+      },
+      {
+         "address":661,
+         "contract":"Unknown",
+         "debug":"<DEBUG-DATA>",
+         "description":"The return value of a message call is not checked.\nExternal calls return a boolean value. If the callee contract halts with an exception, 'false' is returned and execution continues in the caller. It is usually recommended to wrap external calls into a require statement to prevent unexpected states.",
+         "function":"thisisfine()",
+         "max_gas_used":35972,
+         "min_gas_used":1361,
+         "severity":"Low",
+         "sourceMap":null,
+         "swc-id":"104",
+         "title":"Unchecked Call Return Value"
+      },
+      {
+         "address":779,
+         "contract":"Unknown",
+         "debug":"<DEBUG-DATA>",
+         "description":"The contract executes an external message call.\nAn external function call to a fixed contract address is executed. Make sure that the callee contract has been reviewed carefully.",
+         "function":"callstoredaddress()",
+         "max_gas_used":1298,
+         "min_gas_used":687,
+         "severity":"Low",
+         "sourceMap":null,
+         "swc-id":"107",
+         "title":"External Call To Fixed Address"
+      },
+      {
+         "address":779,
+         "contract":"Unknown",
+         "debug":"<DEBUG-DATA>",
+         "description":"The return value of a message call is not checked.\nExternal calls return a boolean value. If the callee contract halts with an exception, 'false' is returned and execution continues in the caller. It is usually recommended to wrap external calls into a require statement to prevent unexpected states.",
+         "function":"callstoredaddress()",
+         "max_gas_used":36016,
+         "min_gas_used":1405,
+         "severity":"Low",
+         "sourceMap":null,
+         "swc-id":"104",
+         "title":"Unchecked Call Return Value"
+      },
+      {
+         "address":858,
+         "contract":"Unknown",
+         "debug":"<DEBUG-DATA>",
+         "description":"The contract executes an external message call.\nAn external function call to a fixed contract address is executed. Make sure that the callee contract has been reviewed carefully.",
+         "function":"reentrancy()",
+         "max_gas_used":1320,
+         "min_gas_used":709,
+         "severity":"Low",
+         "sourceMap":null,
+         "swc-id":"107",
+         "title":"External Call To Fixed Address"
+      },
+      {
+         "address":858,
+         "contract":"Unknown",
+         "debug":"<DEBUG-DATA>",
+         "description":"The return value of a message call is not checked.\nExternal calls return a boolean value. If the callee contract halts with an exception, 'false' is returned and execution continues in the caller. It is usually recommended to wrap external calls into a require statement to prevent unexpected states.",
+         "function":"reentrancy()",
+         "max_gas_used":61052,
+         "min_gas_used":6441,
+         "severity":"Low",
+         "sourceMap":null,
+         "swc-id":"104",
+         "title":"Unchecked Call Return Value"
+      },
+      {
+         "address":869,
+         "contract":"Unknown",
+         "debug":"<DEBUG-DATA>",
+         "description":"The contract account state is changed after an external call. \nConsider that the called contract could re-enter the function before this state change takes place. This can lead to business logic vulnerabilities.",
+         "function":"reentrancy()",
+         "max_gas_used":null,
+         "min_gas_used":null,
+         "severity":"Low",
+         "sourceMap":null,
+         "swc-id":"107",
+         "title":"State change after external call"
+      },
+      {
+         "address":912,
+         "contract":"Unknown",
+         "debug":"<DEBUG-DATA>",
+         "description":"A call to a user-supplied address is executed.\nThe callee address of an external message call can be set by the caller. Note that the callee can contain arbitrary code and may re-enter any function in this contract. Review the business logic carefully to prevent averse effects on thecontract state.",
+         "function":"calluseraddress(address)",
+         "max_gas_used":616,
+         "min_gas_used":335,
+         "severity":"Medium",
+         "sourceMap":null,
+         "swc-id":"107",
+         "title":"External Call To User-Supplied Address"
+      },
+      {
+         "address":912,
+         "contract":"Unknown",
+         "debug":"<DEBUG-DATA>",
+         "description":"The return value of a message call is not checked.\nExternal calls return a boolean value. If the callee contract halts with an exception, 'false' is returned and execution continues in the caller. It is usually recommended to wrap external calls into a require statement to prevent unexpected states.",
+         "function":"calluseraddress(address)",
+         "max_gas_used":35336,
+         "min_gas_used":1055,
+         "severity":"Low",
+         "sourceMap":null,
+         "swc-id":"104",
+         "title":"Unchecked Call Return Value"
+      }
+   ],
+   "success":true
+}

tests/testdata/outputs_expected/calls.sol.o.jsonv2

@@ -6,7 +6,9 @@
                "head":"The contract executes an external message call.",
                "tail":"An external function call to a fixed contract address is executed. Make sure that the callee contract has been reviewed carefully."
             },
-                "extra": {},
+            "extra":{
+
+            },
             "locations":[
                {
                   "sourceMap":"661:1:0"
@@ -21,7 +23,9 @@
                "head":"The contract executes an external message call.",
                "tail":"An external function call to a fixed contract address is executed. Make sure that the callee contract has been reviewed carefully."
             },
-                "extra": {},
+            "extra":{
+
+            },
             "locations":[
                {
                   "sourceMap":"779:1:0"
@@ -36,7 +40,9 @@
                "head":"The contract executes an external message call.",
                "tail":"An external function call to a fixed contract address is executed. Make sure that the callee contract has been reviewed carefully."
             },
-                "extra": {},
+            "extra":{
+
+            },
             "locations":[
                {
                   "sourceMap":"858:1:0"
@@ -46,12 +52,31 @@
             "swcID":"SWC-107",
             "swcTitle":"Reentrancy"
          },
+         {
+            "description":{
+               "head":"The contract account state is changed after an external call. ",
+               "tail":"Consider that the called contract could re-enter the function before this state change takes place. This can lead to business logic vulnerabilities."
+            },
+            "extra":{
+
+            },
+            "locations":[
+               {
+                  "sourceMap":"869:1:0"
+               }
+            ],
+            "severity":"Low",
+            "swcID":"SWC-107",
+            "swcTitle":"Reentrancy"
+         },
          {
             "description":{
                "head":"A call to a user-supplied address is executed.",
                "tail":"The callee address of an external message call can be set by the caller. Note that the callee can contain arbitrary code and may re-enter any function in this contract. Review the business logic carefully to prevent averse effects on thecontract state."
             },
-                "extra": {},
+            "extra":{
+
+            },
             "locations":[
                {
                   "sourceMap":"912:1:0"
@@ -66,7 +91,9 @@
                "head":"The return value of a message call is not checked.",
                "tail":"External calls return a boolean value. If the callee contract halts with an exception, 'false' is returned and execution continues in the caller. It is usually recommended to wrap external calls into a require statement to prevent unexpected states."
             },
-                "extra": {},
+            "extra":{
+
+            },
             "locations":[
                {
                   "sourceMap":"661:1:0"
@@ -81,7 +108,9 @@
                "head":"The return value of a message call is not checked.",
                "tail":"External calls return a boolean value. If the callee contract halts with an exception, 'false' is returned and execution continues in the caller. It is usually recommended to wrap external calls into a require statement to prevent unexpected states."
             },
-                "extra": {},
+            "extra":{
+
+            },
             "locations":[
                {
                   "sourceMap":"779:1:0"
@@ -96,7 +125,9 @@
                "head":"The return value of a message call is not checked.",
                "tail":"External calls return a boolean value. If the callee contract halts with an exception, 'false' is returned and execution continues in the caller. It is usually recommended to wrap external calls into a require statement to prevent unexpected states."
             },
-                "extra": {},
+            "extra":{
+
+            },
             "locations":[
                {
                   "sourceMap":"858:1:0"
@@ -111,7 +142,9 @@
                "head":"The return value of a message call is not checked.",
                "tail":"External calls return a boolean value. If the callee contract halts with an exception, 'false' is returned and execution continues in the caller. It is usually recommended to wrap external calls into a require statement to prevent unexpected states."
             },
-                "extra": {},
+            "extra":{
+
+            },
             "locations":[
                {
                   "sourceMap":"912:1:0"
@@ -122,7 +155,9 @@
             "swcTitle":"Unchecked Call Return Value"
          }
       ],
-        "meta": {},
+      "meta":{
+
+      },
       "sourceFormat":"evm-byzantium-bytecode",
       "sourceList":[
          "0x7cbb77986c6b1bf6e945cd3fba06d3ea3d28cfc49cdfdc9571ec30703ac5862f"

tests/testdata/outputs_expected/calls.sol.o.markdown

@@ -78,6 +78,19 @@ An external function call to a fixed contract address is executed. Make sure tha
 The return value of a message call is not checked.
 External calls return a boolean value. If the callee contract halts with an exception, 'false' is returned and execution continues in the caller. It is usually recommended to wrap external calls into a require statement to prevent unexpected states.
 
+## State change after external call
+- SWC ID: 107
+- Severity: Low
+- Contract: Unknown
+- Function name: `reentrancy()`
+- PC address: 869
+- Estimated Gas Usage: None - None
+
+### Description
+
+The contract account state is changed after an external call. 
+Consider that the called contract could re-enter the function before this state change takes place. This can lead to business logic vulnerabilities.
+
 ## External Call To User-Supplied Address
 - SWC ID: 107
 - Severity: Medium

tests/testdata/outputs_expected/calls.sol.o.text

@@ -64,6 +64,17 @@ The return value of a message call is not checked.
 External calls return a boolean value. If the callee contract halts with an exception, 'false' is returned and execution continues in the caller. It is usually recommended to wrap external calls into a require statement to prevent unexpected states.
 --------------------
 
+==== State change after external call ====
+SWC ID: 107
+Severity: Low
+Contract: Unknown
+Function name: reentrancy()
+PC address: 869
+Estimated Gas Usage: None - None
+The contract account state is changed after an external call. 
+Consider that the called contract could re-enter the function before this state change takes place. This can lead to business logic vulnerabilities.
+--------------------
+
 ==== External Call To User-Supplied Address ====
 SWC ID: 107
 Severity: Medium