diff --git a/slither/all_exceptions.py b/slither/all_exceptions.py index 397ac1508..eacb8b56f 100644 --- a/slither/all_exceptions.py +++ b/slither/all_exceptions.py @@ -2,6 +2,6 @@ This module import all slither exceptions """ from slither.slithir.exceptions import SlithIRError -from slither.solc_parsing.exceptions import ParsingError, ParsingContractNotFound, ParsingNameReuse, VariableNotFound +from slither.solc_parsing.exceptions import ParsingError, VariableNotFound from slither.core.exceptions import SlitherCoreError from slither.exceptions import SlitherException \ No newline at end of file diff --git a/slither/core/declarations/contract.py b/slither/core/declarations/contract.py index a7a8bbfcf..3eb8164f6 100644 --- a/slither/core/declarations/contract.py +++ b/slither/core/declarations/contract.py @@ -50,6 +50,8 @@ class Contract(ChildSlither, SourceMapping): self._initial_state_variables = [] # ssa + self._is_incorrectly_parsed = False + ################################################################################### ################################################################################### # region General's properties @@ -874,6 +876,21 @@ class Contract(ChildSlither, SourceMapping): return self._is_upgradeable_proxy return self._is_upgradeable_proxy + # endregion + ################################################################################### + ################################################################################### + # region Internals + ################################################################################### + ################################################################################### + + @property + def is_incorrectly_constructed(self): + """ + Return true if there was an internal Slither's issue when analyzing the contract + :return: + """ + return self._is_incorrectly_parsed + # endregion ################################################################################### ################################################################################### diff --git a/slither/core/slither_core.py b/slither/core/slither_core.py index 9fb1eebda..e73132e9e 100644 --- a/slither/core/slither_core.py +++ b/slither/core/slither_core.py @@ -5,6 +5,8 @@ import os import logging import json import re +from collections import defaultdict + from slither.core.context.context import Context from slither.slithir.operations import InternalCall from slither.utils.colors import red @@ -43,6 +45,9 @@ class Slither(Context): self._markdown_root = "" + self._contract_name_collisions = defaultdict(list) + self._contract_with_missing_inheritance = set() + ################################################################################### ################################################################################### # region Source code @@ -300,4 +305,19 @@ class Slither(Context): def generate_patches(self, p): self._generate_patches = p + + # endregion + ################################################################################### + ################################################################################### + # region Internals + ################################################################################### + ################################################################################### + + @property + def contract_name_collisions(self): + return self._contract_name_collisions + + @property + def contracts_with_missing_inheritance(self): + return self._contract_with_missing_inheritance # endregion diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index 0a4c0acd1..78147ebd4 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -44,5 +44,6 @@ from .statements.type_based_tautology import TypeBasedTautology from .statements.boolean_constant_equality import BooleanEquality from .statements.boolean_constant_misuse import BooleanConstantMisuse from .statements.divide_before_multiply import DivideBeforeMultiply +from .slither.name_reused import NameReused # # diff --git a/slither/detectors/slither/__init__.py b/slither/detectors/slither/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/slither/detectors/slither/name_reused.py b/slither/detectors/slither/name_reused.py new file mode 100644 index 000000000..92e037afa --- /dev/null +++ b/slither/detectors/slither/name_reused.py @@ -0,0 +1,75 @@ +from collections import defaultdict + +from slither.detectors.abstract_detector import (AbstractDetector, + DetectorClassification) + + +def _find_missing_inheritance(slither): + missings = slither.contracts_with_missing_inheritance + + ret = [] + for b in missings: + is_most_base = True + for inheritance in b.immediate_inheritance: + if inheritance in missings: + is_most_base = False + if is_most_base: + ret.append(b) + + return ret + + +class NameReused(AbstractDetector): + ARGUMENT = 'name-reused' + HELP = "Contract's name reused" + IMPACT = DetectorClassification.HIGH + CONFIDENCE = DetectorClassification.HIGH + + WIKI = 'https://github.com/crytic/slither/wiki/Detector-Documentation#name-reused' + + WIKI_TITLE = 'Name reused' + WIKI_DESCRIPTION = 'todo' + WIKI_EXPLOIT_SCENARIO = 'todo' + WIKI_RECOMMENDATION = 'Rename the contract.' + + def _detect(self): + results = [] + + names_reused = self.slither.contract_name_collisions + + incorrectly_constructed = [contract for contract in self.contracts + if contract.is_incorrectly_constructed] + + inheritance_corrupted = defaultdict(list) + for contract in incorrectly_constructed: + for father in contract.inheritance: + inheritance_corrupted[father.name].append(contract) + + for contract_name, files in names_reused.items(): + info = [contract_name, ' is re-used:\n'] + for file in files: + if file is None: + info += ['\t- In an file not found, most likely in\n'] + else: + info += ['\t- ', file, '\n'] + + if contract_name in inheritance_corrupted: + info += ['\tAs a result, the inherited contracts are not correctly analyzed:\n'] + for corrupted in inheritance_corrupted[contract_name]: + info += ["\t\t- ", corrupted, '\n'] + res = self.generate_result(info) + results.append(res) + + most_base_with_missing_inheritance = _find_missing_inheritance(self.slither) + + for b in most_base_with_missing_inheritance: + info = [b, ' inherits from a contract for which the name is reused.\n', + 'Slither could not determine the contract, but it is either:\n'] + for inheritance in b.immediate_inheritance: + info += ['\t-', inheritance, '\n'] + info += [b, ' and all the contracts inheriting from it are not correctly analyzed:\n'] + for derived in b.derived_contracts: + info += ['\t-', derived, '\n'] + res = self.generate_result(info) + results.append(res) + return results diff --git a/slither/slither.py b/slither/slither.py index 55698de74..514c1ae1f 100644 --- a/slither/slither.py +++ b/slither/slither.py @@ -10,6 +10,7 @@ from crytic_compile import CryticCompile, InvalidCompilation from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification from slither.printers.abstract_printer import AbstractPrinter +from .solc_parsing.exceptions import VariableNotFound from .solc_parsing.slitherSolc import SlitherSolc from .exceptions import SlitherError @@ -84,6 +85,7 @@ class Slither(SlitherSolc): self._analyze_contracts() + def _init_from_raw_json(self, filename): if not os.path.isfile(filename): raise SlitherError('{} does not exist (are you in the correct directory?)'.format(filename)) diff --git a/slither/solc_parsing/declarations/contract.py b/slither/solc_parsing/declarations/contract.py index b667d51f1..0bbfb0347 100644 --- a/slither/solc_parsing/declarations/contract.py +++ b/slither/solc_parsing/declarations/contract.py @@ -12,15 +12,15 @@ from slither.solc_parsing.declarations.modifier import ModifierSolc from slither.solc_parsing.declarations.structure import StructureSolc from slither.solc_parsing.solidity_types.type_parsing import parse_type from slither.solc_parsing.variables.state_variable import StateVariableSolc -from slither.solc_parsing.exceptions import ParsingError +from slither.solc_parsing.exceptions import ParsingError, VariableNotFound logger = logging.getLogger("ContractSolcParsing") -class ContractSolc04(Contract): +class ContractSolc04(Contract): def __init__(self, slitherSolc, data): - #assert slitherSolc.solc_version.startswith('0.4') + # assert slitherSolc.solc_version.startswith('0.4') super(ContractSolc04, self).__init__() self.set_slither(slitherSolc) @@ -54,7 +54,6 @@ class ContractSolc04(Contract): self._parse_contract_info() self._parse_contract_items() - ################################################################################### ################################################################################### # region General Properties @@ -168,7 +167,7 @@ class ContractSolc04(Contract): self.baseConstructorContractsCalled.append(referencedDeclaration) def _parse_contract_items(self): - if not self.get_children() in self._data: # empty contract + if not self.get_children() in self._data: # empty contract return for item in self._data[self.get_children()]: if item[self.get_key()] == 'FunctionDefinition': @@ -190,7 +189,7 @@ class ContractSolc04(Contract): elif item[self.get_key()] == 'UsingForDirective': self._usingForNotParsed.append(item) else: - raise ParsingError('Unknown contract item: '+item[self.get_key()]) + raise ParsingError('Unknown contract item: ' + item[self.get_key()]) return def _parse_struct(self, struct): @@ -208,7 +207,7 @@ class ContractSolc04(Contract): if self.get_children('members') in struct: children = struct[self.get_children('members')] else: - children = [] # empty struct + children = [] # empty struct st = StructureSolc(name, canonicalName, children) st.set_contract(self) st.set_offset(struct['src'], self.slither) @@ -263,7 +262,6 @@ class ContractSolc04(Contract): for function in self._functionsNotParsed: self._parse_function(function) - self._functionsNotParsed = None return @@ -275,41 +273,51 @@ class ContractSolc04(Contract): ################################################################################### ################################################################################### + def log_incorrect_parsing(self): + self._is_incorrectly_parsed = True + def analyze_content_modifiers(self): - for modifier in self.modifiers: - modifier.analyze_content() + try: + for modifier in self.modifiers: + modifier.analyze_content() + except (VariableNotFound, KeyError): + self.log_incorrect_parsing() return def analyze_content_functions(self): - for function in self.functions: - function.analyze_content() - + try: + for function in self.functions: + function.analyze_content() + except (VariableNotFound, KeyError): + self.log_incorrect_parsing() return def analyze_params_modifiers(self): - elements_no_params = self._modifiers_no_params - getter = lambda f: f.modifiers - getter_available = lambda f: f.modifiers_declared - Cls = ModifierSolc - self._modifiers = self._analyze_params_elements(elements_no_params, getter, getter_available, Cls) - + try: + elements_no_params = self._modifiers_no_params + getter = lambda f: f.modifiers + getter_available = lambda f: f.modifiers_declared + Cls = ModifierSolc + self._modifiers = self._analyze_params_elements(elements_no_params, getter, getter_available, Cls) + except (VariableNotFound, KeyError): + self.log_incorrect_parsing() self._modifiers_no_params = [] return def analyze_params_functions(self): - - elements_no_params = self._functions_no_params - getter = lambda f: f.functions - getter_available = lambda f: f.functions_declared - Cls = FunctionSolc - self._functions = self._analyze_params_elements(elements_no_params, getter, getter_available, Cls) - + try: + elements_no_params = self._functions_no_params + getter = lambda f: f.functions + getter_available = lambda f: f.functions_declared + Cls = FunctionSolc + self._functions = self._analyze_params_elements(elements_no_params, getter, getter_available, Cls) + except (VariableNotFound, KeyError): + self.log_incorrect_parsing() self._functions_no_params = [] return - def _analyze_params_elements(self, elements_no_params, getter, getter_available, Cls): """ Analyze the parameters of the given elements (Function or Modifier). @@ -323,55 +331,53 @@ class ContractSolc04(Contract): """ all_elements = {} - for father in self.inheritance: - for element in getter(father): - elem = Cls(element._functionNotParsed, self, element.contract_declarer) - elem.set_offset(element._functionNotParsed['src'], self.slither) - elem.analyze_params() - self.slither.add_function(elem) - all_elements[elem.canonical_name] = elem - - accessible_elements = self.available_elements_from_inheritances(all_elements, getter_available) - - # If there is a constructor in the functions - # We remove the previous constructor - # As only one constructor is present per contracts - # - # Note: contract.all_functions_called returns the constructors of the base contracts - has_constructor = False - for element in elements_no_params: - element.analyze_params() - if element.is_constructor: - has_constructor = True - - if has_constructor: - _accessible_functions = {k: v for (k, v) in accessible_elements.items() if not v.is_constructor} - - for element in elements_no_params: - accessible_elements[element.full_name] = element - all_elements[element.canonical_name] = element - - for element in all_elements.values(): - if accessible_elements[element.full_name] != all_elements[element.canonical_name]: - element.is_shadowed = True - accessible_elements[element.full_name].shadows = True - + try: + for father in self.inheritance: + for element in getter(father): + elem = Cls(element._functionNotParsed, self, element.contract_declarer) + elem.set_offset(element._functionNotParsed['src'], self.slither) + elem.analyze_params() + self.slither.add_function(elem) + all_elements[elem.canonical_name] = elem + + accessible_elements = self.available_elements_from_inheritances(all_elements, getter_available) + + # If there is a constructor in the functions + # We remove the previous constructor + # As only one constructor is present per contracts + # + # Note: contract.all_functions_called returns the constructors of the base contracts + has_constructor = False + for element in elements_no_params: + element.analyze_params() + if element.is_constructor: + has_constructor = True + + if has_constructor: + _accessible_functions = {k: v for (k, v) in accessible_elements.items() if not v.is_constructor} + + for element in elements_no_params: + accessible_elements[element.full_name] = element + all_elements[element.canonical_name] = element + + for element in all_elements.values(): + if accessible_elements[element.full_name] != all_elements[element.canonical_name]: + element.is_shadowed = True + accessible_elements[element.full_name].shadows = True + except (VariableNotFound, KeyError): + self.log_incorrect_parsing() return all_elements - - def analyze_constant_state_variables(self): - from slither.solc_parsing.expressions.expression_parsing import VariableNotFound for var in self.variables: if var.is_constant: # cant parse constant expression based on function calls try: var.analyze(self) - except VariableNotFound: + except (VariableNotFound, KeyError): pass return - def _create_node(self, func, counter, variable): # Function uses to create node for state variable declaration statements node = Node(NodeType.OTHER_ENTRYPOINT, counter) @@ -379,9 +385,9 @@ class ContractSolc04(Contract): node.set_function(func) func.add_node(node) expression = AssignmentOperation(Identifier(variable), - variable.expression, - AssignmentOperationType.ASSIGN, - variable.type) + variable.expression, + AssignmentOperationType.ASSIGN, + variable.type) expression.set_offset(variable.source_mapping, self.slither) node.add_expression(expression) @@ -405,7 +411,7 @@ class ContractSolc04(Contract): prev_node = self._create_node(constructor_variable, 0, variable_candidate) variable_candidate.node_initialization = prev_node counter = 1 - for v in self.state_variables[idx+1:]: + for v in self.state_variables[idx + 1:]: if v.expression and not v.is_constant: next_node = self._create_node(constructor_variable, counter, v) v.node_initialization = next_node @@ -430,7 +436,7 @@ class ContractSolc04(Contract): prev_node = self._create_node(constructor_variable, 0, variable_candidate) variable_candidate.node_initialization = prev_node counter = 1 - for v in self.state_variables[idx+1:]: + for v in self.state_variables[idx + 1:]: if v.expression and v.is_constant: next_node = self._create_node(constructor_variable, counter, v) v.node_initialization = next_node @@ -440,54 +446,58 @@ class ContractSolc04(Contract): break - - - - def analyze_state_variables(self): - for var in self.variables: - var.analyze(self) - return + try: + for var in self.variables: + var.analyze(self) + return + except (VariableNotFound, KeyError): + self.log_incorrect_parsing() def analyze_using_for(self): - for father in self.inheritance: - self._using_for.update(father.using_for) + try: + for father in self.inheritance: + self._using_for.update(father.using_for) - if self.is_compact_ast: - for using_for in self._usingForNotParsed: - lib_name = parse_type(using_for['libraryName'], self) - if 'typeName' in using_for and using_for['typeName']: - type_name = parse_type(using_for['typeName'], self) - else: - type_name = '*' - if not type_name in self._using_for: - self.using_for[type_name] = [] - self._using_for[type_name].append(lib_name) - else: - for using_for in self._usingForNotParsed: - children = using_for[self.get_children()] - assert children and len(children) <= 2 - if len(children) == 2: - new = parse_type(children[0], self) - old = parse_type(children[1], self) - else: - new = parse_type(children[0], self) - old = '*' - if not old in self._using_for: - self.using_for[old] = [] - self._using_for[old].append(new) - self._usingForNotParsed = [] + if self.is_compact_ast: + for using_for in self._usingForNotParsed: + lib_name = parse_type(using_for['libraryName'], self) + if 'typeName' in using_for and using_for['typeName']: + type_name = parse_type(using_for['typeName'], self) + else: + type_name = '*' + if not type_name in self._using_for: + self.using_for[type_name] = [] + self._using_for[type_name].append(lib_name) + else: + for using_for in self._usingForNotParsed: + children = using_for[self.get_children()] + assert children and len(children) <= 2 + if len(children) == 2: + new = parse_type(children[0], self) + old = parse_type(children[1], self) + else: + new = parse_type(children[0], self) + old = '*' + if not old in self._using_for: + self.using_for[old] = [] + self._using_for[old].append(new) + self._usingForNotParsed = [] + except (VariableNotFound, KeyError): + self.log_incorrect_parsing() def analyze_enums(self): - - for father in self.inheritance: - self._enums.update(father.enums_as_dict()) - - for enum in self._enumsNotParsed: - # for enum, we can parse and analyze it - # at the same time - self._analyze_enum(enum) - self._enumsNotParsed = None + try: + for father in self.inheritance: + self._enums.update(father.enums_as_dict()) + + for enum in self._enumsNotParsed: + # for enum, we can parse and analyze it + # at the same time + self._analyze_enum(enum) + self._enumsNotParsed = None + except (VariableNotFound, KeyError): + self.log_incorrect_parsing() def _analyze_enum(self, enum): # Enum can be parsed in one pass @@ -517,24 +527,28 @@ class ContractSolc04(Contract): struct.analyze() def analyze_structs(self): - for struct in self.structures: - self._analyze_struct(struct) + try: + for struct in self.structures: + self._analyze_struct(struct) + except (VariableNotFound, KeyError): + self.log_incorrect_parsing() def analyze_events(self): - for father in self.inheritance_reverse: - self._events.update(father.events_as_dict()) - - for event_to_parse in self._eventsNotParsed: - event = EventSolc(event_to_parse, self) - event.analyze(self) - event.set_contract(self) - event.set_offset(event_to_parse['src'], self.slither) - self._events[event.full_name] = event + try: + for father in self.inheritance_reverse: + self._events.update(father.events_as_dict()) + + for event_to_parse in self._eventsNotParsed: + event = EventSolc(event_to_parse, self) + event.analyze(self) + event.set_contract(self) + event.set_offset(event_to_parse['src'], self.slither) + self._events[event.full_name] = event + except (VariableNotFound, KeyError): + self.log_incorrect_parsing() self._eventsNotParsed = None - - # endregion ################################################################################### ################################################################################### @@ -578,6 +592,28 @@ class ContractSolc04(Contract): for func in self.functions + self.modifiers: func.fix_phi(last_state_variables_instances, initial_state_variables_instances) + # endregion + ################################################################################### + ################################################################################### + # region Internal + ################################################################################### + ################################################################################### + + def delete_content(self): + """ + Remove everything not parsed from the contract + This is used only if something went wrong with the inheritance parsing + :return: + """ + self._functionsNotParsed = [] + self._modifiersNotParsed = [] + self._functions_no_params = [] + self._modifiers_no_params = [] + self._eventsNotParsed = [] + self._variablesNotParsed = [] + self._enumsNotParsed = [] + self._structuresNotParsed = [] + self._usingForNotParsed = [] # endregion ################################################################################### diff --git a/slither/solc_parsing/exceptions.py b/slither/solc_parsing/exceptions.py index dcf576509..b9797a65c 100644 --- a/slither/solc_parsing/exceptions.py +++ b/slither/solc_parsing/exceptions.py @@ -2,8 +2,4 @@ from slither.exceptions import SlitherException class ParsingError(SlitherException): pass -class ParsingNameReuse(SlitherException): pass - -class ParsingContractNotFound(SlitherException): pass - class VariableNotFound(SlitherException): pass diff --git a/slither/solc_parsing/slitherSolc.py b/slither/solc_parsing/slitherSolc.py index 8a695ae6a..a0947ae52 100644 --- a/slither/solc_parsing/slitherSolc.py +++ b/slither/solc_parsing/slitherSolc.py @@ -13,8 +13,6 @@ from slither.core.declarations.pragma_directive import Pragma from slither.core.declarations.import_directive import Import from slither.analyses.data_dependency.data_dependency import compute_dependency -from slither.utils.colors import red -from .exceptions import ParsingNameReuse, ParsingContractNotFound class SlitherSolc(Slither): @@ -27,7 +25,6 @@ class SlitherSolc(Slither): self._is_compact_ast = False - ################################################################################### ################################################################################### # region AST @@ -126,7 +123,6 @@ class SlitherSolc(Slither): import_directive.set_offset(contract_data['src'], self) self._import_directives.append(import_directive) - def _parse_source_unit(self, data, filename): if data[self.get_key()] != 'SourceUnit': return -1 # handle solc prior 0.3.6 @@ -175,7 +171,7 @@ class SlitherSolc(Slither): def _analyze_contracts(self): if not self._contractsNotParsed: - logger.info(f'No contract were found in {self.filename}, check the correct compilation') + logger.info(f'No contract were found in {self.filename}, check the correct compilation') if self._analyzed: raise Exception('Contract analysis can be run only once!') @@ -184,51 +180,57 @@ class SlitherSolc(Slither): for contract in self._contractsNotParsed: if contract.name in self._contracts: if contract.id != self._contracts[contract.name].id: - info = 'Slither does not handle projects with contract names re-use' - info += '\n{} is defined in:'.format(contract.name) - info += '\n- {}\n- {}'.format(contract.source_mapping_str, - self._contracts[contract.name].source_mapping_str) - raise ParsingNameReuse(info) + self._contract_name_collisions[contract.name].append(contract.source_mapping_str) + self._contract_name_collisions[contract.name].append( + self._contracts[contract.name].source_mapping_str) else: self._contracts_by_id[contract.id] = contract self._contracts[contract.name] = contract - # Update of the inheritance + # Update of the inheritance for contract in self._contractsNotParsed: # remove the first elem in linearizedBaseContracts as it is the contract itself ancestors = [] fathers = [] father_constructors = [] - try: - # Resolve linearized base contracts. - for i in contract.linearizedBaseContracts[1:]: - if i in contract.remapping: - ancestors.append(self.get_contract_from_name(contract.remapping[i])) - else: - ancestors.append(self._contracts_by_id[i]) - - # Resolve immediate base contracts - for i in contract.baseContracts: - if i in contract.remapping: - fathers.append(self.get_contract_from_name(contract.remapping[i])) - else: - fathers.append(self._contracts_by_id[i]) - - # Resolve immediate base constructor calls - for i in contract.baseConstructorContractsCalled: - if i in contract.remapping: - father_constructors.append(self.get_contract_from_name(contract.remapping[i])) - else: - father_constructors.append(self._contracts_by_id[i]) - - except KeyError as e: - txt = f'A contract was not found (id {e}), it is likely that your codebase contains muliple contracts with the same name. ' - txt += 'Truffle does not handle this case during compilation. ' - txt += 'Please read https://github.com/trailofbits/slither/wiki#keyerror-or-nonetype-error, ' - txt += 'and update your code to remove the duplicate' - raise ParsingContractNotFound(txt) + # try: + # Resolve linearized base contracts. + missing_inheritance = False + + for i in contract.linearizedBaseContracts[1:]: + if i in contract.remapping: + ancestors.append(self.get_contract_from_name(contract.remapping[i])) + elif i in self._contracts_by_id: + ancestors.append(self._contracts_by_id[i]) + else: + missing_inheritance = True + + # Resolve immediate base contracts + for i in contract.baseContracts: + if i in contract.remapping: + fathers.append(self.get_contract_from_name(contract.remapping[i])) + elif i in self._contracts_by_id: + fathers.append(self._contracts_by_id[i]) + else: + missing_inheritance = True + + # Resolve immediate base constructor calls + for i in contract.baseConstructorContractsCalled: + if i in contract.remapping: + father_constructors.append(self.get_contract_from_name(contract.remapping[i])) + elif i in self._contracts_by_id: + father_constructors.append(self._contracts_by_id[i]) + else: + missing_inheritance = True + contract.setInheritance(ancestors, fathers, father_constructors) + if missing_inheritance: + self._contract_with_missing_inheritance.add(contract) + contract.log_incorrect_parsing() + contract.set_is_analyzed(True) + contract.delete_content() + contracts_to_be_analyzed = self.contracts # Any contract can refer another contract enum without need for inheritance @@ -257,7 +259,6 @@ class SlitherSolc(Slither): compute_dependency(self) - def _analyze_all_enums(self, contracts_to_be_analyzed): while contracts_to_be_analyzed: contract = contracts_to_be_analyzed[0] @@ -370,8 +371,6 @@ class SlitherSolc(Slither): contract.analyze_content_modifiers() contract.analyze_content_functions() - - contract.set_is_analyzed(True) def _convert_to_slithir(self):