From 9dfd2aa604e635aaab0f2bb2ce8c676fcf2ae648 Mon Sep 17 00:00:00 2001 From: Nikhil Parasaram Date: Sun, 26 May 2019 16:23:28 +0530 Subject: [PATCH] Enhance delegate call (#1042) * Enhance delegate call * Slightly update report texts * Black thou shalst use * Upate tests --- mythril/analysis/modules/delegatecall.py | 143 +++++++++++------- mythril/laser/ethereum/instructions.py | 1 - .../kinds_of_calls.sol.o.json | 13 ++ .../kinds_of_calls.sol.o.jsonv2 | 17 +++ .../kinds_of_calls.sol.o.markdown | 13 ++ .../kinds_of_calls.sol.o.text | 11 ++ 6 files changed, 146 insertions(+), 52 deletions(-) diff --git a/mythril/analysis/modules/delegatecall.py b/mythril/analysis/modules/delegatecall.py index 3ef4a338..aeb3e06f 100644 --- a/mythril/analysis/modules/delegatecall.py +++ b/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), - ) - - target = hex(call.to.val) if call.to.type == VarType.CONCRETE else str(call.to) - issue.description += "DELEGATECALL target: {}".format(target) - - return [issue] + 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 + + return issues detector = DelegateCallModule() diff --git a/mythril/laser/ethereum/instructions.py b/mythril/laser/ethereum/instructions.py index f17d4401..087b6421 100644 --- a/mythril/laser/ethereum/instructions.py +++ b/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: diff --git a/tests/testdata/outputs_expected/kinds_of_calls.sol.o.json b/tests/testdata/outputs_expected/kinds_of_calls.sol.o.json index b6bc99ac..72ac1e67 100644 --- a/tests/testdata/outputs_expected/kinds_of_calls.sol.o.json +++ b/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": "", + "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", diff --git a/tests/testdata/outputs_expected/kinds_of_calls.sol.o.jsonv2 b/tests/testdata/outputs_expected/kinds_of_calls.sol.o.jsonv2 index 4f0d13e0..cf80cc34 100644 --- a/tests/testdata/outputs_expected/kinds_of_calls.sol.o.jsonv2 +++ b/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": "" + }, + "locations": [ + { + "sourceMap": "849:1:0" + } + ], + "severity": "Medium", + "swcID": "SWC-112", + "swcTitle": "Delegatecall to Untrusted Callee" + }, { "description": { "head": "Use of callcode is deprecated.", diff --git a/tests/testdata/outputs_expected/kinds_of_calls.sol.o.markdown b/tests/testdata/outputs_expected/kinds_of_calls.sol.o.markdown index 4e222b18..e6f7f11e 100644 --- a/tests/testdata/outputs_expected/kinds_of_calls.sol.o.markdown +++ b/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 diff --git a/tests/testdata/outputs_expected/kinds_of_calls.sol.o.text b/tests/testdata/outputs_expected/kinds_of_calls.sol.o.text index 9e9761c3..1bb3abad 100644 --- a/tests/testdata/outputs_expected/kinds_of_calls.sol.o.text +++ b/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