From 8e46c9be42fbb6a05d0b59c2f47ddf82095de27e Mon Sep 17 00:00:00 2001 From: Nikhil Parasaram Date: Mon, 11 Feb 2019 17:21:31 +0530 Subject: [PATCH] Detect State changes after external calls --- mythril/analysis/call_helpers.py | 2 +- mythril/analysis/modules/external_calls.py | 105 ++++++- .../outputs_expected/calls.sol.o.json | 124 +++++++- .../outputs_expected/calls.sol.o.jsonv2 | 297 ++++++++++-------- .../outputs_expected/calls.sol.o.markdown | 13 + .../outputs_expected/calls.sol.o.text | 11 + 6 files changed, 414 insertions(+), 138 deletions(-) diff --git a/mythril/analysis/call_helpers.py b/mythril/analysis/call_helpers.py index 50285d3f..293613da 100644 --- a/mythril/analysis/call_helpers.py +++ b/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: diff --git a/mythril/analysis/modules/external_calls.py b/mythril/analysis/modules/external_calls.py index 09828267..ae427bf3 100644 --- a/mythril/analysis/modules/external_calls.py +++ b/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): diff --git a/tests/testdata/outputs_expected/calls.sol.o.json b/tests/testdata/outputs_expected/calls.sol.o.json index 1342a258..225562aa 100644 --- a/tests/testdata/outputs_expected/calls.sol.o.json +++ b/tests/testdata/outputs_expected/calls.sol.o.json @@ -1 +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 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": "", "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 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":"", + "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 431b4cd4..fb6916d4 100644 --- a/tests/testdata/outputs_expected/calls.sol.o.jsonv2 +++ b/tests/testdata/outputs_expected/calls.sol.o.jsonv2 @@ -1,132 +1,167 @@ [ - { - "issues": [ - { - "description": { - "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": {}, - "locations": [ - { - "sourceMap": "661:1:0" - } - ], - "severity": "Low", - "swcID": "SWC-107", - "swcTitle": "Reentrancy" - }, - { - "description": { - "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": {}, - "locations": [ - { - "sourceMap": "779:1:0" - } - ], - "severity": "Low", - "swcID": "SWC-107", - "swcTitle": "Reentrancy" - }, - { - "description": { - "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": {}, - "locations": [ - { - "sourceMap": "858: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": {}, - "locations": [ - { - "sourceMap": "912:1:0" - } - ], - "severity": "Medium", - "swcID": "SWC-107", - "swcTitle": "Reentrancy" - }, - { - "description": { - "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": {}, - "locations": [ - { - "sourceMap": "661:1:0" - } - ], - "severity": "Low", - "swcID": "SWC-104", - "swcTitle": "Unchecked Call Return Value" - }, - { - "description": { - "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": {}, - "locations": [ - { - "sourceMap": "779:1:0" - } - ], - "severity": "Low", - "swcID": "SWC-104", - "swcTitle": "Unchecked Call Return Value" - }, - { - "description": { - "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": {}, - "locations": [ - { - "sourceMap": "858:1:0" - } - ], - "severity": "Low", - "swcID": "SWC-104", - "swcTitle": "Unchecked Call Return Value" - }, - { - "description": { - "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": {}, - "locations": [ - { - "sourceMap": "912:1:0" - } - ], - "severity": "Low", - "swcID": "SWC-104", - "swcTitle": "Unchecked Call Return Value" - } - ], - "meta": {}, - "sourceFormat": "evm-byzantium-bytecode", - "sourceList": [ - "0x7cbb77986c6b1bf6e945cd3fba06d3ea3d28cfc49cdfdc9571ec30703ac5862f" - ], - "sourceType": "raw-bytecode" - } -] \ No newline at end of file + { + "issues":[ + { + "description":{ + "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":{ + + }, + "locations":[ + { + "sourceMap":"661:1:0" + } + ], + "severity":"Low", + "swcID":"SWC-107", + "swcTitle":"Reentrancy" + }, + { + "description":{ + "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":{ + + }, + "locations":[ + { + "sourceMap":"779:1:0" + } + ], + "severity":"Low", + "swcID":"SWC-107", + "swcTitle":"Reentrancy" + }, + { + "description":{ + "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":{ + + }, + "locations":[ + { + "sourceMap":"858:1:0" + } + ], + "severity":"Low", + "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":{ + + }, + "locations":[ + { + "sourceMap":"912:1:0" + } + ], + "severity":"Medium", + "swcID":"SWC-107", + "swcTitle":"Reentrancy" + }, + { + "description":{ + "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":{ + + }, + "locations":[ + { + "sourceMap":"661:1:0" + } + ], + "severity":"Low", + "swcID":"SWC-104", + "swcTitle":"Unchecked Call Return Value" + }, + { + "description":{ + "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":{ + + }, + "locations":[ + { + "sourceMap":"779:1:0" + } + ], + "severity":"Low", + "swcID":"SWC-104", + "swcTitle":"Unchecked Call Return Value" + }, + { + "description":{ + "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":{ + + }, + "locations":[ + { + "sourceMap":"858:1:0" + } + ], + "severity":"Low", + "swcID":"SWC-104", + "swcTitle":"Unchecked Call Return Value" + }, + { + "description":{ + "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":{ + + }, + "locations":[ + { + "sourceMap":"912:1:0" + } + ], + "severity":"Low", + "swcID":"SWC-104", + "swcTitle":"Unchecked Call Return Value" + } + ], + "meta":{ + + }, + "sourceFormat":"evm-byzantium-bytecode", + "sourceList":[ + "0x7cbb77986c6b1bf6e945cd3fba06d3ea3d28cfc49cdfdc9571ec30703ac5862f" + ], + "sourceType":"raw-bytecode" + } +] diff --git a/tests/testdata/outputs_expected/calls.sol.o.markdown b/tests/testdata/outputs_expected/calls.sol.o.markdown index 2edd13d7..2bd645a0 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 9e53ca4e..425c189b 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