diff --git a/mythril/analysis/call_helpers.py b/mythril/analysis/call_helpers.py index 50285d3f..6cb796df 100644 --- a/mythril/analysis/call_helpers.py +++ b/mythril/analysis/call_helpers.py @@ -15,10 +15,9 @@ def get_call_from_state(state: GlobalState) -> Union[Call, None]: instruction = state.get_current_instruction() op = instruction["opcode"] - stack = state.mstate.stack - if op in ("CALL", "CALLCODE"): + if op in ("CALL", "CALLCODE", "STATICCALL"): gas, to, value, meminstart, meminsz, memoutstart, memoutsz = ( get_variable(stack[-1]), get_variable(stack[-2]), @@ -29,7 +28,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: diff --git a/mythril/analysis/modules/external_calls.py b/mythril/analysis/modules/external_calls.py index 95eb428e..554cbb93 100644 --- a/mythril/analysis/modules/external_calls.py +++ b/mythril/analysis/modules/external_calls.py @@ -36,6 +36,7 @@ def _analyze_state(state): try: constraints = copy(state.mstate.constraints) + transaction_sequence = solver.get_transaction_sequence( state, constraints + [UGT(gas, symbol_factory.BitVecVal(2300, 256))] ) diff --git a/mythril/analysis/modules/state_change_external_calls.py b/mythril/analysis/modules/state_change_external_calls.py new file mode 100644 index 00000000..0f4cb6f5 --- /dev/null +++ b/mythril/analysis/modules/state_change_external_calls.py @@ -0,0 +1,171 @@ +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 symbol_factory, UGT, BitVec, Or +from mythril.laser.ethereum.state.global_state import GlobalState +from mythril.laser.ethereum.state.annotation import StateAnnotation +from mythril.analysis import solver +from mythril.exceptions import UnsatError +from typing import List, cast, Optional +from copy import copy + +import logging + +log = logging.getLogger(__name__) + +DESCRIPTION = """ + +Check whether there is a state change of the contract after the execution of an external call +""" + + +class StateChangeCallsAnnotation(StateAnnotation): + def __init__(self, call_state: GlobalState, user_defined_address: bool) -> None: + self.call_state = call_state + self.state_change_states = [] # type: List[GlobalState] + self.user_defined_address = user_defined_address + + def __copy__(self): + new_annotation = StateChangeCallsAnnotation( + self.call_state, self.user_defined_address + ) + new_annotation.state_change_states = self.state_change_states[:] + return new_annotation + + def get_issue(self, global_state: GlobalState) -> Optional[Issue]: + if not self.state_change_states: + return None + + severity = "Medium" if self.user_defined_address else "Low" + address = global_state.get_current_instruction()["address"] + 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." + ) + + return Issue( + contract=global_state.environment.active_account.contract_name, + function_name=global_state.environment.active_function_name, + address=address, + title="State change after external call", + severity=severity, + description_head=description_head, + description_tail=description_tail, + swc_id=REENTRANCY, + bytecode=global_state.environment.code.bytecode, + ) + + +class StateChange(DetectionModule): + """This module searches for state change after low level calls (e.g. call.value()) that + forward gas to the callee.""" + + def __init__(self): + """""" + super().__init__( + name="State Change After External calls", + swc_id=REENTRANCY, + description=DESCRIPTION, + entrypoint="callback", + pre_hooks=[ + "CALL", + "SSTORE", + "DELEGATECALL", + "STATICCALL", + "CREATE", + "CALLCODE", + ], + ) + + def execute(self, state: GlobalState): + self._issues.extend(self._analyze_state(state)) + return self.issues + + @staticmethod + def _add_external_call(global_state: GlobalState) -> None: + gas = global_state.mstate.stack[-1] + to = global_state.mstate.stack[-2] + try: + constraints = copy(global_state.mstate.constraints) + solver.get_model( + constraints + + [ + UGT(gas, symbol_factory.BitVecVal(2300, 256)), + Or( + to > symbol_factory.BitVecVal(16, 256), + to == symbol_factory.BitVecVal(0, 256), + ), + ] + ) + + # Check whether we can also set the callee address + try: + constraints += [to == 0xDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEF] + solver.get_model(constraints) + + global_state.annotate(StateChangeCallsAnnotation(global_state, True)) + except UnsatError: + global_state.annotate(StateChangeCallsAnnotation(global_state, False)) + except UnsatError: + pass + + @staticmethod + def _analyze_state(global_state: GlobalState) -> List[Issue]: + + annotations = cast( + List[StateChangeCallsAnnotation], + list(global_state.get_annotations(StateChangeCallsAnnotation)), + ) + op_code = global_state.get_current_instruction()["opcode"] + + if len(annotations) == 0: + if op_code in ("SSTORE", "CREATE", "CREATE2"): + return [] + if op_code in ("SSTORE", "CREATE", "CREATE2"): + for annotation in annotations: + annotation.state_change_states.append(global_state) + + # Record state changes following from a transfer of ether + if op_code in ("CALL", "DELEGATECALL", "CALLCODE"): + value = global_state.mstate.stack[-3] # type: BitVec + if StateChange._balance_change(value, global_state): + for annotation in annotations: + annotation.state_change_states.append(global_state) + + # Record external calls + if op_code in ("CALL", "DELEGATECALL", "CALLCODE"): + StateChange._add_external_call(global_state) + + # Check for vulnerabilities + vulnerabilities = [] + for annotation in annotations: + if not annotation.state_change_states: + continue + vulnerabilities.append(annotation.get_issue(global_state)) + return vulnerabilities + + @staticmethod + def _balance_change(value: BitVec, global_state: GlobalState) -> bool: + if not value.symbolic: + assert value.value is not None + return value.value > 0 + + else: + constraints = copy(global_state.mstate.constraints) + + try: + solver.get_model( + constraints + [value > symbol_factory.BitVecVal(0, 256)] + ) + return True + except UnsatError: + return False + + +detector = StateChange() diff --git a/mythril/laser/ethereum/call.py b/mythril/laser/ethereum/call.py index bb86b7a8..da63928d 100644 --- a/mythril/laser/ethereum/call.py +++ b/mythril/laser/ethereum/call.py @@ -49,7 +49,6 @@ def get_call_parameters( callee_account = None call_data = get_call_data(global_state, memory_input_offset, memory_input_size) - if int(callee_address, 16) >= 5 or int(callee_address, 16) == 0: callee_account = get_callee_account( global_state, callee_address, dynamic_loader diff --git a/tests/testdata/outputs_expected/calls.sol.o.json b/tests/testdata/outputs_expected/calls.sol.o.json index 40f93d02..624cf510 100644 --- a/tests/testdata/outputs_expected/calls.sol.o.json +++ b/tests/testdata/outputs_expected/calls.sol.o.json @@ -1,110 +1,123 @@ { - "error": null, - "issues": [ - { - "address": 661, - "contract": "Unknown", - "debug": "", - "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": "", - "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": "", - "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": "", - "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": "", - "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": "", - "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": "", - "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 the contract 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": "", - "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 -} \ No newline at end of file + "error":null, + "issues":[ + { + "address":661, + "contract":"Unknown", + "debug":"", + "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":"", + "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":"", + "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":"", + "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":"", + "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":"", + "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":"", + "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":"", + "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 the contract 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":"", + "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 +} diff --git a/tests/testdata/outputs_expected/calls.sol.o.jsonv2 b/tests/testdata/outputs_expected/calls.sol.o.jsonv2 index da380624..44d5bc89 100644 --- a/tests/testdata/outputs_expected/calls.sol.o.jsonv2 +++ b/tests/testdata/outputs_expected/calls.sol.o.jsonv2 @@ -69,6 +69,23 @@ "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": { + "discoveryTime": "" + }, + "locations": [ + { + "sourceMap": "869:1:0" + } + ], + "severity": "Low", + "swcID": "SWC-107", + "swcTitle": "Reentrancy" + }, { "description": { "head": "The return value of a message call is not checked.", @@ -145,4 +162,4 @@ ], "sourceType": "raw-bytecode" } -] \ No newline at end of file +] diff --git a/tests/testdata/outputs_expected/calls.sol.o.markdown b/tests/testdata/outputs_expected/calls.sol.o.markdown index 3ad3e237..b45544be 100644 --- a/tests/testdata/outputs_expected/calls.sol.o.markdown +++ b/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 diff --git a/tests/testdata/outputs_expected/calls.sol.o.text b/tests/testdata/outputs_expected/calls.sol.o.text index dfbfa338..27706fd1 100644 --- a/tests/testdata/outputs_expected/calls.sol.o.text +++ b/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 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..f7da0197 100644 --- a/tests/testdata/outputs_expected/kinds_of_calls.sol.o.jsonv2 +++ b/tests/testdata/outputs_expected/kinds_of_calls.sol.o.jsonv2 @@ -94,4 +94,4 @@ ], "sourceType": "raw-bytecode" } -] \ No newline at end of file +]