Enhance delegate call (#1042)

* Enhance delegate call

* Slightly update report texts

* Black thou shalst use

* Upate tests

mythril/analysis/modules/delegatecall.py

@@ -1,19 +1,72 @@
 """This module contains the detection code for insecure delegate call usage."""
-import re
+import json
 import logging
-from typing import List
+from copy import copy
+from typing import List, cast
 
+from mythril.analysis import solver
 from mythril.analysis.swc_data import DELEGATECALL_TO_UNTRUSTED_CONTRACT
-from mythril.analysis.ops import get_variable, VarType, Call, Variable
 from mythril.analysis.report import Issue
-from mythril.analysis.call_helpers import get_call_from_state
 from mythril.analysis.modules.base import DetectionModule
+from mythril.exceptions import UnsatError
+from mythril.laser.ethereum.state.annotation import StateAnnotation
 from mythril.laser.ethereum.state.global_state import GlobalState
-
+from mythril.laser.smt import symbol_factory, UGT
+from mythril.laser.smt import symbol_factory, UGT
 
 log = logging.getLogger(__name__)
 
 
+class DelegateCallAnnotation(StateAnnotation):
+    def __init__(self, call_state: GlobalState) -> None:
+        """
+        Initialize DelegateCall Annotation
+        :param call_state: Call state
+        """
+        self.call_state = call_state
+        self.return_value = call_state.new_bitvec(
+            "retval_{}".format(call_state.get_current_instruction()["address"]), 256
+        )
+
+    def get_issue(self, global_state: GlobalState, transaction_sequence: str) -> Issue:
+        """
+        Returns Issue for the annotation
+        :param global_state: Global State
+        :param transaction_sequence: Transaction sequence
+        :return: Issue
+        """
+
+        address = self.call_state.get_current_instruction()["address"]
+        logging.debug(
+            "[DELEGATECALL] Detected delegatecall to a user-supplied address : {}".format(
+                address
+            )
+        )
+        description_head = "The contract delegates execution to another contract with a user-supplied address."
+        description_tail = (
+            "The smart contract delegates execution to a user-supplied address. Note that callers "
+            "can execute arbitrary contracts and that the callee contract "
+            "can access the storage of the calling contract. "
+        )
+
+        return Issue(
+            contract=self.call_state.environment.active_account.contract_name,
+            function_name=self.call_state.environment.active_function_name,
+            address=address,
+            swc_id=DELEGATECALL_TO_UNTRUSTED_CONTRACT,
+            title="Delegatecall Proxy To User-Supplied Address",
+            bytecode=global_state.environment.code.bytecode,
+            severity="Medium",
+            description_head=description_head,
+            description_tail=description_tail,
+            debug=transaction_sequence,
+            gas_used=(
+                global_state.mstate.min_gas_used,
+                global_state.mstate.max_gas_used,
+            ),
+        )
+
+
 class DelegateCallModule(DetectionModule):
     """This module detects calldata being forwarded using DELEGATECALL."""
 
@@ -24,7 +77,7 @@ class DelegateCallModule(DetectionModule):
             swc_id=DELEGATECALL_TO_UNTRUSTED_CONTRACT,
             description="Check for invocations of delegatecall(msg.data) in the fallback function.",
             entrypoint="callback",
-            pre_hooks=["DELEGATECALL"],
+            pre_hooks=["DELEGATECALL", "RETURN", "STOP"],
         )
 
     def execute(self, state: GlobalState) -> list:
@@ -43,59 +96,47 @@ def _analyze_states(state: GlobalState) -> List[Issue]:
     :param state: the current state
     :return: returns the issues for that corresponding state
     """
-    call = get_call_from_state(state)
-    if call is None:
-        return []
-    issues = []  # type: List[Issue]
+    issues = []
+    op_code = state.get_current_instruction()["opcode"]
+    annotations = cast(
+        List[DelegateCallAnnotation],
+        list(state.get_annotations(DelegateCallAnnotation)),
+    )
 
-    if call.type is not "DELEGATECALL":
-        return []
-    if state.environment.active_function_name is not "fallback":
+    if len(annotations) == 0 and op_code in ("RETURN", "STOP"):
         return []
 
-    state = call.state
-    address = state.get_current_instruction()["address"]
-    meminstart = get_variable(state.mstate.stack[-3])
+    if op_code == "DELEGATECALL":
+        gas = state.mstate.stack[-1]
+        to = state.mstate.stack[-2]
 
-    if meminstart.type == VarType.CONCRETE:
-        issues += _concrete_call(call, state, address, meminstart)
+        constraints = copy(state.mstate.constraints)
+        # Check whether we can also set the callee address
 
-    return issues
+        constraints += [
+            to == 0xDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEF,
+            UGT(gas, symbol_factory.BitVecVal(2300, 256)),
+        ]
+        try:
+            solver.get_model(constraints)
+            state.annotate(DelegateCallAnnotation(call_state=state))
 
+        except UnsatError:
+            log.debug("[DELEGATECALL] Annotation skipped.")
 
-def _concrete_call(
-    call: Call, state: GlobalState, address: int, meminstart: Variable
-) -> List[Issue]:
-    """
-    :param call: The current call's information
-    :param state: The current state
-    :param address: The PC address
-    :param meminstart: memory starting position
-    :return: issues
-    """
-    if not re.search(r"calldata.*\[0", str(state.mstate.memory[meminstart.val])):
         return []
-
-    issue = Issue(
-        contract=state.environment.active_account.contract_name,
-        function_name=state.environment.active_function_name,
-        address=address,
-        swc_id=DELEGATECALL_TO_UNTRUSTED_CONTRACT,
-        bytecode=state.environment.code.bytecode,
-        title="Delegatecall Proxy",
-        severity="Low",
-        description_head="The contract implements a delegatecall proxy.",
-        description_tail="The smart contract forwards the received calldata via delegatecall. Note that callers "
-        "can execute arbitrary functions in the callee contract and that the callee contract "
-        "can access the storage of the calling contract. "
-        "Make sure that the callee contract is audited properly.",
-        gas_used=(state.mstate.min_gas_used, state.mstate.max_gas_used),
+    else:
+        for annotation in annotations:
+            try:
+                transaction_sequence = solver.get_transaction_sequence(
+                    state, state.mstate.constraints + [annotation.return_value == 1]
                 )
+                debug = json.dumps(transaction_sequence, indent=4)
+                issues.append(annotation.get_issue(state, transaction_sequence=debug))
+            except UnsatError:
+                continue
 
-    target = hex(call.to.val) if call.to.type == VarType.CONCRETE else str(call.to)
-    issue.description += "DELEGATECALL target: {}".format(target)
-
-    return [issue]
+        return issues
 
 
 detector = DelegateCallModule()

mythril/laser/ethereum/instructions.py

@@ -2109,7 +2109,6 @@ class Instruction:
                 "retval_" + str(instr["address"]), 256
             )
             global_state.mstate.stack.append(return_value)
-            global_state.mstate.constraints.append(return_value == 0)
             return [global_state]
 
         try:

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

@@ -27,6 +27,19 @@
             "swc-id": "111",
             "title": "Use of callcode"
         },
+        {
+            "address": 849,
+            "contract": "Unknown",
+            "debug": "<DEBUG-DATA>",
+            "description": "The contract delegates execution to another contract with a user-supplied address.\nThe smart contract delegates execution to a user-supplied address. Note that callers can execute arbitrary contracts and that the callee contract can access the storage of the calling contract. ",
+            "function": "_function_0x9b58bc26",
+            "max_gas_used": 35928,
+            "min_gas_used": 1176,
+            "severity": "Medium",
+            "sourceMap": null,
+            "swc-id": "112",
+            "title": "Delegatecall Proxy To User-Supplied Address"
+        },
         {
             "address": 849,
             "contract": "Unknown",

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

@@ -1,6 +1,23 @@
 [
     {
         "issues": [
+            {
+                "description": {
+                    "head": "The contract delegates execution to another contract with a user-supplied address.",
+                    "tail": "The smart contract delegates execution to a user-supplied address. Note that callers can execute arbitrary contracts and that the callee contract can access the storage of the calling contract. "
+                },
+                "extra": {
+                    "discoveryTime": "<DISCOVERY-TIME-DATA>"
+                },
+                "locations": [
+                    {
+                        "sourceMap": "849:1:0"
+                    }
+                ],
+                "severity": "Medium",
+                "swcID": "SWC-112",
+                "swcTitle": "Delegatecall to Untrusted Callee"
+            },
             {
                 "description": {
                     "head": "Use of callcode is deprecated.",

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

@@ -26,6 +26,19 @@ External calls return a boolean value. If the callee contract halts with an exce
 Use of callcode is deprecated.
 The callcode method executes code of another contract in the context of the caller account. Due to a bug in the implementation it does not persist sender and value over the call. It was therefore deprecated and may be removed in the future. Use the delegatecall method instead.
 
+## Delegatecall Proxy To User-Supplied Address
+- SWC ID: 112
+- Severity: Medium
+- Contract: Unknown
+- Function name: `_function_0x9b58bc26`
+- PC address: 849
+- Estimated Gas Usage: 1176 - 35928
+
+### Description
+
+The contract delegates execution to another contract with a user-supplied address.
+The smart contract delegates execution to a user-supplied address. Note that callers can execute arbitrary contracts and that the callee contract can access the storage of the calling contract.
+
 ## Unchecked Call Return Value
 - SWC ID: 104
 - Severity: Low

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

@@ -20,6 +20,17 @@ Use of callcode is deprecated.
 The callcode method executes code of another contract in the context of the caller account. Due to a bug in the implementation it does not persist sender and value over the call. It was therefore deprecated and may be removed in the future. Use the delegatecall method instead.
 --------------------
 
+==== Delegatecall Proxy To User-Supplied Address ====
+SWC ID: 112
+Severity: Medium
+Contract: Unknown
+Function name: _function_0x9b58bc26
+PC address: 849
+Estimated Gas Usage: 1176 - 35928
+The contract delegates execution to another contract with a user-supplied address.
+The smart contract delegates execution to a user-supplied address. Note that callers can execute arbitrary contracts and that the callee contract can access the storage of the calling contract. 
+--------------------
+
 ==== Unchecked Call Return Value ====
 SWC ID: 104
 Severity: Low