From 1bb72898f65a1a4eac6d480394ae84ebb15578d9 Mon Sep 17 00:00:00 2001 From: Josselin Date: Wed, 20 Nov 2019 13:56:30 +0100 Subject: [PATCH 1/2] Reentrancies detectors: - Clean Reentrancy based class - Add ReentrancyEvent to detect reentrancies leading to out of order Events - Add ReentrancyNoGas to detect reentrancies based on send/transfer Fix #236 --- scripts/tests_generate_expected_json_4.sh | 1 + slither/detectors/all_detectors.py | 2 + slither/detectors/reentrancy/reentrancy.py | 60 +-- .../detectors/reentrancy/reentrancy_events.py | 112 +++++ .../detectors/reentrancy/reentrancy_no_gas.py | 138 ++++++ .../reentrancy.reentrancy-unlimited-gas.json | 410 ++++++++++++++++++ .../reentrancy.reentrancy-unlimited-gas.txt | 8 + .../right_to_left_override.rtlo.txt | 1 + tests/reentrancy.sol | 18 + 9 files changed, 725 insertions(+), 25 deletions(-) create mode 100644 slither/detectors/reentrancy/reentrancy_events.py create mode 100644 slither/detectors/reentrancy/reentrancy_no_gas.py create mode 100644 tests/expected_json/reentrancy.reentrancy-unlimited-gas.json create mode 100644 tests/expected_json/reentrancy.reentrancy-unlimited-gas.txt diff --git a/scripts/tests_generate_expected_json_4.sh b/scripts/tests_generate_expected_json_4.sh index 790763ffe..c44898d9f 100755 --- a/scripts/tests_generate_expected_json_4.sh +++ b/scripts/tests_generate_expected_json_4.sh @@ -31,6 +31,7 @@ generate_expected_json(){ #generate_expected_json tests/pragma.0.4.24.sol "pragma" #generate_expected_json tests/old_solc.sol.json "solc-version" #generate_expected_json tests/reentrancy.sol "reentrancy-eth" +generate_expected_json tests/reentrancy.sol "reentrancy-unlimited-gas" #generate_expected_json tests/uninitialized_storage_pointer.sol "uninitialized-storage" #generate_expected_json tests/tx_origin.sol "tx-origin" #generate_expected_json tests/unused_state.sol "unused-state" diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index 39822221b..e2f1dc368 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -11,6 +11,8 @@ from .functions.suicidal import Suicidal from .reentrancy.reentrancy_benign import ReentrancyBenign from .reentrancy.reentrancy_read_before_write import ReentrancyReadBeforeWritten from .reentrancy.reentrancy_eth import ReentrancyEth +from .reentrancy.reentrancy_no_gas import ReentrancyNoGas +from .reentrancy.reentrancy_events import ReentrancyEvent from .variables.unused_state_variables import UnusedStateVars from .variables.possible_const_state_variables import ConstCandidateStateVars from .statements.tx_origin import TxOrigin diff --git a/slither/detectors/reentrancy/reentrancy.py b/slither/detectors/reentrancy/reentrancy.py index bb5afef17..4109daf51 100644 --- a/slither/detectors/reentrancy/reentrancy.py +++ b/slither/detectors/reentrancy/reentrancy.py @@ -9,20 +9,21 @@ from slither.core.cfg.node import NodeType from slither.core.declarations import Function from slither.core.expressions import UnaryOperation, UnaryOperationType from slither.detectors.abstract_detector import AbstractDetector -from slither.slithir.operations import Call +from slither.slithir.operations import Call, EventCall def union_dict(d1, d2): d3 = {k: d1.get(k, set()) | d2.get(k, set()) for k in set(list(d1.keys()) + list(d2.keys()))} return d3 + def dict_are_equal(d1, d2): if set(list(d1.keys())) != set(list(d2.keys())): return False return all(set(d1[k]) == set(d2[k]) for k in d1.keys()) -class Reentrancy(AbstractDetector): +class Reentrancy(AbstractDetector): KEY = 'REENTRANCY' def _can_callback(self, irs): @@ -49,9 +50,6 @@ class Reentrancy(AbstractDetector): for ir in irs: if isinstance(ir, Call) and ir.can_send_eth(): return True - # if isinstance(ir, (HighLevelCall, LowLevelCall, Transfer, Send)): - # if ir.call_value: - # return True return False def _filter_if(self, node): @@ -65,8 +63,8 @@ class Reentrancy(AbstractDetector): This will work only on naive implementation """ - return isinstance(node.expression, UnaryOperation)\ - and node.expression.type == UnaryOperationType.BANG + return isinstance(node.expression, UnaryOperation) \ + and node.expression.type == UnaryOperationType.BANG def _explore(self, node, visited, skip_father=None): """ @@ -89,29 +87,37 @@ class Reentrancy(AbstractDetector): # calls returns the list of calls that can callback # read returns the variable read # read_prior_calls returns the variable read prior a call - fathers_context = {'send_eth':set(), 'calls':set(), 'read':set(), 'read_prior_calls':{}} + fathers_context = {'send_eth': set(), 'calls': set(), 'read': set(), 'read_prior_calls': {}, 'events': set()} for father in node.fathers: if self.KEY in father.context: - fathers_context['send_eth'] |= set([s for s in father.context[self.KEY]['send_eth'] if s!=skip_father]) - fathers_context['calls'] |= set([c for c in father.context[self.KEY]['calls'] if c!=skip_father]) + fathers_context['send_eth'] |= set( + [s for s in father.context[self.KEY]['send_eth'] if s != skip_father]) + fathers_context['calls'] |= set([c for c in father.context[self.KEY]['calls'] if c != skip_father]) fathers_context['read'] |= set(father.context[self.KEY]['read']) - fathers_context['read_prior_calls'] = union_dict(fathers_context['read_prior_calls'], father.context[self.KEY]['read_prior_calls']) + fathers_context['read_prior_calls'] = union_dict(fathers_context['read_prior_calls'], + father.context[self.KEY]['read_prior_calls']) + fathers_context['events'] |= set(father.context[self.KEY]['events']) # Exclude path that dont bring further information if node in self.visited_all_paths: - if all(call in self.visited_all_paths[node]['calls'] for call in fathers_context['calls']): - if all(send in self.visited_all_paths[node]['send_eth'] for send in fathers_context['send_eth']): - if all(read in self.visited_all_paths[node]['read'] for read in fathers_context['read']): - if dict_are_equal(self.visited_all_paths[node]['read_prior_calls'], fathers_context['read_prior_calls']): - return + if fathers_context['calls'].issubset(self.visited_all_paths[node]['calls']): + if fathers_context['send_eth'].issubset(self.visited_all_paths[node]['send_eth']): + if fathers_context['read'].issubset(self.visited_all_paths[node]['read']): + if dict_are_equal(self.visited_all_paths[node]['read_prior_calls'], + fathers_context['read_prior_calls']): + if fathers_context['events'].issubset(self.visited_all_paths[node]['events']): + return else: - self.visited_all_paths[node] = {'send_eth':set(), 'calls':set(), 'read':set(), 'read_prior_calls':{}} + self.visited_all_paths[node] = {'send_eth': set(), 'calls': set(), 'read': set(), + 'read_prior_calls': {}, 'events': set()} - self.visited_all_paths[node]['send_eth'] = set(self.visited_all_paths[node]['send_eth'] | fathers_context['send_eth']) - self.visited_all_paths[node]['calls'] = set(self.visited_all_paths[node]['calls'] | fathers_context['calls']) - self.visited_all_paths[node]['read'] = set(self.visited_all_paths[node]['read'] | fathers_context['read']) - self.visited_all_paths[node]['read_prior_calls'] = union_dict(self.visited_all_paths[node]['read_prior_calls'], fathers_context['read_prior_calls']) + self.visited_all_paths[node]['send_eth'] |= fathers_context['send_eth'] + self.visited_all_paths[node]['calls'] |= fathers_context['calls'] + self.visited_all_paths[node]['read'] |= fathers_context['read'] + self.visited_all_paths[node]['read_prior_calls'] = union_dict(self.visited_all_paths[node]['read_prior_calls'], + fathers_context['read_prior_calls']) + self.visited_all_paths[node]['events'] |= fathers_context['events'] node.context[self.KEY] = fathers_context @@ -131,13 +137,17 @@ class Reentrancy(AbstractDetector): contains_call = False node.context[self.KEY]['written'] = set(state_vars_written) if self._can_callback(node.irs + slithir_operations): - node.context[self.KEY]['calls'] = set(node.context[self.KEY]['calls'] | {node}) - node.context[self.KEY]['read_prior_calls'][node] = set(node.context[self.KEY]['read_prior_calls'].get(node, set()) | node.context[self.KEY]['read'] |state_vars_read) + node.context[self.KEY]['calls'] |= {node} + node.context[self.KEY]['read_prior_calls'][node] = set( + node.context[self.KEY]['read_prior_calls'].get(node, set()) | node.context[self.KEY][ + 'read'] | state_vars_read) contains_call = True if self._can_send_eth(node.irs + slithir_operations): - node.context[self.KEY]['send_eth'] = set(node.context[self.KEY]['send_eth'] | {node}) + node.context[self.KEY]['send_eth'] |= {node} + + node.context[self.KEY]['read'] |= state_vars_read - node.context[self.KEY]['read'] = set(node.context[self.KEY]['read'] | state_vars_read) + node.context[self.KEY]['events'] |= set([ir.name for ir in node.irs if isinstance(ir, EventCall)]) sons = node.sons if contains_call and node.type in [NodeType.IF, NodeType.IFLOOP]: diff --git a/slither/detectors/reentrancy/reentrancy_events.py b/slither/detectors/reentrancy/reentrancy_events.py new file mode 100644 index 000000000..e953eb7f2 --- /dev/null +++ b/slither/detectors/reentrancy/reentrancy_events.py @@ -0,0 +1,112 @@ +"""" + Re-entrancy detection + + Based on heuristics, it may lead to FP and FN + Iterate over all the nodes of the graph until reaching a fixpoint +""" +from slither.detectors.abstract_detector import DetectorClassification + + +from .reentrancy import Reentrancy +class ReentrancyEvent(Reentrancy): + ARGUMENT = 'reentrancy-events' + HELP = 'Reentrancy vulnerabilities leading to out-of-order Events' + IMPACT = DetectorClassification.LOW + CONFIDENCE = DetectorClassification.MEDIUM + + WIKI = 'https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-4' + + WIKI_TITLE = 'Reentrancy vulnerabilities' + WIKI_DESCRIPTION = ''' +Detection of the [re-entrancy bug](https://github.com/trailofbits/not-so-smart-contracts/tree/master/reentrancy). +Only report reentrancies leading to out-of-order Events''' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity + function bug(Called d){ + counter += 1; + d.f(); + emit Counter(counter); + } +``` + +If `d.()` reenters, the `Counter` events will be showed in an incorrect order, which might lead to issues for third-parties.''' + + + WIKI_RECOMMENDATION = 'Apply the [check-effects-interactions pattern](http://solidity.readthedocs.io/en/v0.4.21/security-considerations.html#re-entrancy).' + + STANDARD_JSON = False + + def find_reentrancies(self): + result = {} + for contract in self.contracts: + for f in contract.functions_and_modifiers_declared: + for node in f.nodes: + # dead code + if not self.KEY in node.context: + continue + if node.context[self.KEY]['calls']: + if not any(n != node for n in node.context[self.KEY]['calls']): + continue + + # calls are ordered + finding_key = (node.function, + tuple(sorted(list(node.context[self.KEY]['calls']), key=lambda x: x.node_id)), + tuple(sorted(list(node.context[self.KEY]['send_eth']), key=lambda x: x.node_id))) + finding_vars = [(v, node) for v in node.context[self.KEY]['events']] + if finding_vars: + if finding_key not in result: + result[finding_key] = set() + result[finding_key] = set(result[finding_key] | set(finding_vars)) + return result + + def _detect(self): + """ + """ + super()._detect() + + reentrancies = self.find_reentrancies() + + results = [] + + result_sorted = sorted(list(reentrancies.items()), key=lambda x:x[0][0].name) + for (func, calls, send_eth), events in result_sorted: + calls = sorted(list(set(calls)), key=lambda x: x.node_id) + send_eth = sorted(list(set(send_eth)), key=lambda x: x.node_id) + + info = ['Reentrancy in ', func, ':\n'] + info += ['\tExternal calls:\n'] + for call_info in calls: + info += ['\t- ' , call_info, '\n'] + if calls != send_eth and send_eth: + info += ['\tExternal calls sending eth:\n'] + for call_info in send_eth: + info += ['\t- ', call_info, '\n'] + info += ['\tEvent emitted after the call(s):\n'] + for (e, node) in sorted(events, key=lambda x: (x[0], x[1].node_id)): + info += ['\t- ', e, ' in ', node, '\n'] + + # Create our JSON result + res = self.generate_result(info) + + # Add the function with the re-entrancy first + res.add(func) + + # Add all underlying calls in the function which are potentially problematic. + for call_info in calls: + res.add(call_info, { + "underlying_type": "external_calls" + }) + + # + + # If the calls are not the same ones that send eth, add the eth sending nodes. + if calls != send_eth: + for call_info in send_eth: + res.add(call_info, { + "underlying_type": "external_calls_sending_eth" + }) + + # Append our result + results.append(res) + + return results diff --git a/slither/detectors/reentrancy/reentrancy_no_gas.py b/slither/detectors/reentrancy/reentrancy_no_gas.py new file mode 100644 index 000000000..71d274e24 --- /dev/null +++ b/slither/detectors/reentrancy/reentrancy_no_gas.py @@ -0,0 +1,138 @@ +"""" + Re-entrancy detection + + Based on heuristics, it may lead to FP and FN + Iterate over all the nodes of the graph until reaching a fixpoint +""" +from slither.core.variables.variable import Variable +from slither.detectors.abstract_detector import DetectorClassification +from slither.slithir.operations import Send, Transfer +from .reentrancy import Reentrancy + + +class ReentrancyNoGas(Reentrancy): + KEY = 'REENTRANCY_NO_GAS' + + ARGUMENT = 'reentrancy-unlimited-gas' + HELP = 'Reentrancy vulnerabilities through send and transfer' + IMPACT = DetectorClassification.INFORMATIONAL + CONFIDENCE = DetectorClassification.MEDIUM + + WIKI = 'https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-3' + + WIKI_TITLE = 'Reentrancy vulnerabilities' + WIKI_DESCRIPTION = ''' +Detection of the [re-entrancy bug](https://github.com/trailofbits/not-so-smart-contracts/tree/master/reentrancy). +Only report reentrancy that are based on `transfer` or `send`.''' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity + function callme(){ + msg.sender.transfer(balances[msg.sender]): + balances[msg.sender] = 0; + } +``` + +`send` and `transfer` does not protect from reentrancies in case of gas-price change.''' + + WIKI_RECOMMENDATION = 'Apply the [check-effects-interactions pattern](http://solidity.readthedocs.io/en/v0.4.21/security-considerations.html#re-entrancy).' + + def _can_callback(self, irs): + """ + Same as Reentrancy, but also consider Send and Transfer + + """ + return any((isinstance(ir, (Send, Transfer)) for ir in irs)) + + STANDARD_JSON = False + + def find_reentrancies(self): + result = {} + for contract in self.contracts: + for f in contract.functions_and_modifiers_declared: + for node in f.nodes: + # dead code + if not self.KEY in node.context: + continue + if node.context[self.KEY]['calls']: + if not any(n != node for n in node.context[self.KEY]['calls']): + continue + + # calls are ordered + finding_key = (node.function, + tuple(sorted(list(node.context[self.KEY]['calls']), key=lambda x: x.node_id)), + tuple(sorted(list(node.context[self.KEY]['send_eth']), key=lambda x: x.node_id))) + finding_vars = [(v, node) for v in node.context[self.KEY]['written']] + finding_vars += [(v, node) for v in node.context[self.KEY]['events']] + if finding_vars: + if finding_key not in result: + result[finding_key] = set() + result[finding_key] = set(result[finding_key] | set(finding_vars)) + return result + + def _detect(self): + """ + """ + + super()._detect() + reentrancies = self.find_reentrancies() + + results = [] + + result_sorted = sorted(list(reentrancies.items()), key=lambda x: x[0][0].name) + for (func, calls, send_eth), varsWrittenOrEvent in result_sorted: + calls = sorted(list(set(calls)), key=lambda x: x.node_id) + send_eth = sorted(list(set(send_eth)), key=lambda x: x.node_id) + info = ['Reentrancy in ', func, ':\n'] + + info += ['\tExternal calls:\n'] + for call_info in calls: + info += ['\t- ', call_info, '\n'] + if calls != send_eth and send_eth: + info += ['\tExternal calls sending eth:\n'] + for call_info in send_eth: + info += ['\t- ', call_info, '\n'] + + varsWritten = [(v, node) for (v, node) in varsWrittenOrEvent if isinstance(v, Variable)] + if varsWritten: + info += ['\tState variables written after the call(s):\n'] + for (v, node) in sorted(varsWritten, key=lambda x: (x[0].name, x[1].node_id)): + info += ['\t- ', v, ' in ', node, '\n'] + + events = [(e, node) for (e, node) in varsWrittenOrEvent if isinstance(e, str)] + if events: + info += ['\tEvent emitted after the call(s):\n'] + for (e, node) in sorted(events, key=lambda x: (x[0], x[1].node_id)): + info += ['\t- ', e, ' in ', node, '\n'] + + # Create our JSON result + res = self.generate_result(info) + + # Add the function with the re-entrancy first + res.add(func) + + # Add all underlying calls in the function which are potentially problematic. + for call_info in calls: + res.add(call_info, { + "underlying_type": "external_calls" + }) + + # + + # If the calls are not the same ones that send eth, add the eth sending nodes. + if calls != send_eth: + for call_info in send_eth: + res.add(call_info, { + "underlying_type": "external_calls_sending_eth" + }) + + # Add all variables written via nodes which write them. + for (v, node) in varsWritten: + res.add(node, { + "underlying_type": "variables_written", + "variable_name": v.name + }) + + # Append our result + results.append(res) + + return results diff --git a/tests/expected_json/reentrancy.reentrancy-unlimited-gas.json b/tests/expected_json/reentrancy.reentrancy-unlimited-gas.json new file mode 100644 index 000000000..53537ca09 --- /dev/null +++ b/tests/expected_json/reentrancy.reentrancy-unlimited-gas.json @@ -0,0 +1,410 @@ +{ + "success": true, + "error": null, + "results": { + "detectors": [ + { + "elements": [ + { + "type": "function", + "name": "withdrawBalance_fixed_2", + "source_mapping": { + "start": 951, + "length": 386, + "filename_used": "/home/travis/build/crytic/slither/tests/reentrancy.sol", + "filename_relative": "tests/reentrancy.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/reentrancy.sol", + "filename_short": "tests/reentrancy.sol", + "is_dependency": false, + "lines": [ + 33, + 34, + 35, + 36, + 37, + 38, + 39, + 40 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "Reentrancy", + "source_mapping": { + "start": 26, + "length": 2334, + "filename_used": "/home/travis/build/crytic/slither/tests/reentrancy.sol", + "filename_relative": "tests/reentrancy.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/reentrancy.sol", + "filename_short": "tests/reentrancy.sol", + "is_dependency": false, + "lines": [ + 3, + 4, + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21, + 22, + 23, + 24, + 25, + 26, + 27, + 28, + 29, + 30, + 31, + 32, + 33, + 34, + 35, + 36, + 37, + 38, + 39, + 40, + 41, + 42, + 43, + 44, + 45, + 46, + 47, + 48, + 49, + 50, + 51, + 52, + 53, + 54, + 55, + 56, + 57, + 58, + 59, + 60, + 61, + 62, + 63, + 64, + 65, + 66, + 67, + 68, + 69, + 70, + 71, + 72 + ], + "starting_column": 1, + "ending_column": 2 + } + }, + "signature": "withdrawBalance_fixed_2()" + } + }, + { + "type": "node", + "name": "msg.sender.transfer(userBalance[msg.sender])", + "source_mapping": { + "start": 1249, + "length": 44, + "filename_used": "/home/travis/build/crytic/slither/tests/reentrancy.sol", + "filename_relative": "tests/reentrancy.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/reentrancy.sol", + "filename_short": "tests/reentrancy.sol", + "is_dependency": false, + "lines": [ + 38 + ], + "starting_column": 9, + "ending_column": 53 + }, + "type_specific_fields": { + "parent": { + "type": "function", + "name": "withdrawBalance_fixed_2", + "source_mapping": { + "start": 951, + "length": 386, + "filename_used": "/home/travis/build/crytic/slither/tests/reentrancy.sol", + "filename_relative": "tests/reentrancy.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/reentrancy.sol", + "filename_short": "tests/reentrancy.sol", + "is_dependency": false, + "lines": [ + 33, + 34, + 35, + 36, + 37, + 38, + 39, + 40 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "Reentrancy", + "source_mapping": { + "start": 26, + "length": 2334, + "filename_used": "/home/travis/build/crytic/slither/tests/reentrancy.sol", + "filename_relative": "tests/reentrancy.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/reentrancy.sol", + "filename_short": "tests/reentrancy.sol", + "is_dependency": false, + "lines": [ + 3, + 4, + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21, + 22, + 23, + 24, + 25, + 26, + 27, + 28, + 29, + 30, + 31, + 32, + 33, + 34, + 35, + 36, + 37, + 38, + 39, + 40, + 41, + 42, + 43, + 44, + 45, + 46, + 47, + 48, + 49, + 50, + 51, + 52, + 53, + 54, + 55, + 56, + 57, + 58, + 59, + 60, + 61, + 62, + 63, + 64, + 65, + 66, + 67, + 68, + 69, + 70, + 71, + 72 + ], + "starting_column": 1, + "ending_column": 2 + } + }, + "signature": "withdrawBalance_fixed_2()" + } + } + }, + "additional_fields": { + "underlying_type": "external_calls" + } + }, + { + "type": "node", + "name": "userBalance[msg.sender] = 0", + "source_mapping": { + "start": 1303, + "length": 27, + "filename_used": "/home/travis/build/crytic/slither/tests/reentrancy.sol", + "filename_relative": "tests/reentrancy.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/reentrancy.sol", + "filename_short": "tests/reentrancy.sol", + "is_dependency": false, + "lines": [ + 39 + ], + "starting_column": 9, + "ending_column": 36 + }, + "type_specific_fields": { + "parent": { + "type": "function", + "name": "withdrawBalance_fixed_2", + "source_mapping": { + "start": 951, + "length": 386, + "filename_used": "/home/travis/build/crytic/slither/tests/reentrancy.sol", + "filename_relative": "tests/reentrancy.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/reentrancy.sol", + "filename_short": "tests/reentrancy.sol", + "is_dependency": false, + "lines": [ + 33, + 34, + 35, + 36, + 37, + 38, + 39, + 40 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "Reentrancy", + "source_mapping": { + "start": 26, + "length": 2334, + "filename_used": "/home/travis/build/crytic/slither/tests/reentrancy.sol", + "filename_relative": "tests/reentrancy.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/reentrancy.sol", + "filename_short": "tests/reentrancy.sol", + "is_dependency": false, + "lines": [ + 3, + 4, + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21, + 22, + 23, + 24, + 25, + 26, + 27, + 28, + 29, + 30, + 31, + 32, + 33, + 34, + 35, + 36, + 37, + 38, + 39, + 40, + 41, + 42, + 43, + 44, + 45, + 46, + 47, + 48, + 49, + 50, + 51, + 52, + 53, + 54, + 55, + 56, + 57, + 58, + 59, + 60, + 61, + 62, + 63, + 64, + 65, + 66, + 67, + 68, + 69, + 70, + 71, + 72 + ], + "starting_column": 1, + "ending_column": 2 + } + }, + "signature": "withdrawBalance_fixed_2()" + } + } + }, + "additional_fields": { + "underlying_type": "variables_written", + "variable_name": "userBalance" + } + } + ], + "description": "Reentrancy in Reentrancy.withdrawBalance_fixed_2() (tests/reentrancy.sol#33-40):\n\tExternal calls:\n\t- msg.sender.transfer(userBalance[msg.sender]) (tests/reentrancy.sol#38)\n\tState variables written after the call(s):\n\t- Reentrancy.userBalance (tests/reentrancy.sol#4) in userBalance[msg.sender] = 0 (tests/reentrancy.sol#39)\n", + "markdown": "Reentrancy in [Reentrancy.withdrawBalance_fixed_2()](tests/reentrancy.sol#L33-L40):\n\tExternal calls:\n\t- [msg.sender.transfer(userBalance[msg.sender])](tests/reentrancy.sol#L38)\n\tState variables written after the call(s):\n\t- [Reentrancy.userBalance](tests/reentrancy.sol#L4) in [userBalance[msg.sender] = 0](tests/reentrancy.sol#L39)\n", + "id": "ff12b17dedf35fcd1f5b1286653a53430009f3689c46c06c1aba1f2d4b17d713", + "check": "reentrancy-unlimited-gas", + "impact": "Informational", + "confidence": "Medium" + } + ] + } +} \ No newline at end of file diff --git a/tests/expected_json/reentrancy.reentrancy-unlimited-gas.txt b/tests/expected_json/reentrancy.reentrancy-unlimited-gas.txt new file mode 100644 index 000000000..2c29b8225 --- /dev/null +++ b/tests/expected_json/reentrancy.reentrancy-unlimited-gas.txt @@ -0,0 +1,8 @@ + +Reentrancy in Reentrancy.withdrawBalance_fixed_2() (tests/reentrancy.sol#33-40): + External calls: + - msg.sender.transfer(userBalance[msg.sender]) (tests/reentrancy.sol#38) + State variables written after the call(s): + - Reentrancy.userBalance (tests/reentrancy.sol#4) in userBalance[msg.sender] = 0 (tests/reentrancy.sol#39) +Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-3 +tests/reentrancy.sol analyzed (1 contracts with 1 detectors), 1 result(s) found diff --git a/tests/expected_json/right_to_left_override.rtlo.txt b/tests/expected_json/right_to_left_override.rtlo.txt index 96fd1dd4c..b94be4d0e 100644 --- a/tests/expected_json/right_to_left_override.rtlo.txt +++ b/tests/expected_json/right_to_left_override.rtlo.txt @@ -3,3 +3,4 @@ tests/right_to_left_override.sol contains a unicode right-to-left-override chara - b' test1(/*A\xe2\x80\xae/*B*/2 , 1/*\xe2\x80\xad' Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#right-to-left-override-character tests/right_to_left_override.sol analyzed (1 contracts with 1 detectors), 1 result(s) found +INFO:Slither:/home/travis/build/crytic/slither/scripts/../tests/expected_json/right_to_left_override.rtlo.json exists already, the overwrite is prevented diff --git a/tests/reentrancy.sol b/tests/reentrancy.sol index 077dd57fc..1b17ec4b2 100644 --- a/tests/reentrancy.sol +++ b/tests/reentrancy.sol @@ -70,3 +70,21 @@ contract Reentrancy { } } + + +contract Called{ + function f() public; +} + +contract ReentrancyEvent { + + event E(); + + function test(Called c) public{ + + c.f(); + emit E(); + + } +} + From 51e7a149f6b072403f95e7348c148c5aff7d4e7e Mon Sep 17 00:00:00 2001 From: Josselin Date: Wed, 20 Nov 2019 14:24:48 +0100 Subject: [PATCH 2/2] ReentrancyEvent: improve output --- scripts/travis_test_etherscan.sh | 2 +- slither/detectors/reentrancy/reentrancy.py | 2 +- slither/detectors/reentrancy/reentrancy_events.py | 6 +++--- slither/detectors/reentrancy/reentrancy_no_gas.py | 10 +++++----- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/scripts/travis_test_etherscan.sh b/scripts/travis_test_etherscan.sh index 57b026cc5..3bdb49107 100755 --- a/scripts/travis_test_etherscan.sh +++ b/scripts/travis_test_etherscan.sh @@ -18,7 +18,7 @@ fi slither rinkeby:0xFe05820C5A92D9bc906D4A46F662dbeba794d3b7 --solc "./solc-0.4.25" -if [ $? -ne 70 ] +if [ $? -ne 71 ] then echo "Etherscan test failed" exit -1 diff --git a/slither/detectors/reentrancy/reentrancy.py b/slither/detectors/reentrancy/reentrancy.py index 4109daf51..d46510f9d 100644 --- a/slither/detectors/reentrancy/reentrancy.py +++ b/slither/detectors/reentrancy/reentrancy.py @@ -147,7 +147,7 @@ class Reentrancy(AbstractDetector): node.context[self.KEY]['read'] |= state_vars_read - node.context[self.KEY]['events'] |= set([ir.name for ir in node.irs if isinstance(ir, EventCall)]) + node.context[self.KEY]['events'] |= set([ir for ir in node.irs if isinstance(ir, EventCall)]) sons = node.sons if contains_call and node.type in [NodeType.IF, NodeType.IFLOOP]: diff --git a/slither/detectors/reentrancy/reentrancy_events.py b/slither/detectors/reentrancy/reentrancy_events.py index e953eb7f2..3961b8495 100644 --- a/slither/detectors/reentrancy/reentrancy_events.py +++ b/slither/detectors/reentrancy/reentrancy_events.py @@ -52,7 +52,7 @@ If `d.()` reenters, the `Counter` events will be showed in an incorrect order, w finding_key = (node.function, tuple(sorted(list(node.context[self.KEY]['calls']), key=lambda x: x.node_id)), tuple(sorted(list(node.context[self.KEY]['send_eth']), key=lambda x: x.node_id))) - finding_vars = [(v, node) for v in node.context[self.KEY]['events']] + finding_vars = list(node.context[self.KEY]['events']) if finding_vars: if finding_key not in result: result[finding_key] = set() @@ -82,8 +82,8 @@ If `d.()` reenters, the `Counter` events will be showed in an incorrect order, w for call_info in send_eth: info += ['\t- ', call_info, '\n'] info += ['\tEvent emitted after the call(s):\n'] - for (e, node) in sorted(events, key=lambda x: (x[0], x[1].node_id)): - info += ['\t- ', e, ' in ', node, '\n'] + for event in sorted(events, key=lambda x: x.node.node_id): + info += ['\t- ', event.node, '\n'] # Create our JSON result res = self.generate_result(info) diff --git a/slither/detectors/reentrancy/reentrancy_no_gas.py b/slither/detectors/reentrancy/reentrancy_no_gas.py index 71d274e24..4ecc3e6be 100644 --- a/slither/detectors/reentrancy/reentrancy_no_gas.py +++ b/slither/detectors/reentrancy/reentrancy_no_gas.py @@ -6,7 +6,7 @@ """ from slither.core.variables.variable import Variable from slither.detectors.abstract_detector import DetectorClassification -from slither.slithir.operations import Send, Transfer +from slither.slithir.operations import Send, Transfer, EventCall from .reentrancy import Reentrancy @@ -62,7 +62,7 @@ Only report reentrancy that are based on `transfer` or `send`.''' tuple(sorted(list(node.context[self.KEY]['calls']), key=lambda x: x.node_id)), tuple(sorted(list(node.context[self.KEY]['send_eth']), key=lambda x: x.node_id))) finding_vars = [(v, node) for v in node.context[self.KEY]['written']] - finding_vars += [(v, node) for v in node.context[self.KEY]['events']] + finding_vars += [(e, e.node) for e in node.context[self.KEY]['events']] if finding_vars: if finding_key not in result: result[finding_key] = set() @@ -98,11 +98,11 @@ Only report reentrancy that are based on `transfer` or `send`.''' for (v, node) in sorted(varsWritten, key=lambda x: (x[0].name, x[1].node_id)): info += ['\t- ', v, ' in ', node, '\n'] - events = [(e, node) for (e, node) in varsWrittenOrEvent if isinstance(e, str)] + events = [(e, node) for (e, node) in varsWrittenOrEvent if isinstance(e, EventCall)] if events: info += ['\tEvent emitted after the call(s):\n'] - for (e, node) in sorted(events, key=lambda x: (x[0], x[1].node_id)): - info += ['\t- ', e, ' in ', node, '\n'] + for (_, node) in sorted(events, key=lambda x: x[1].node_id): + info += ['\t- ', node, '\n'] # Create our JSON result res = self.generate_result(info)