Breaking change in the way Slither handled codebase with name reused

- Add NameReused detector to report the name's reuse
- Slither will analyze the codebase that contains the issue, with partial context. As a result, the analyze might not be correct
pull/413/head
Josselin 5 years ago
parent d76bec9850
commit 1b6f50ce29
  1. 2
      slither/all_exceptions.py
  2. 17
      slither/core/declarations/contract.py
  3. 20
      slither/core/slither_core.py
  4. 1
      slither/detectors/all_detectors.py
  5. 0
      slither/detectors/slither/__init__.py
  6. 75
      slither/detectors/slither/name_reused.py
  7. 2
      slither/slither.py
  8. 286
      slither/solc_parsing/declarations/contract.py
  9. 4
      slither/solc_parsing/exceptions.py
  10. 83
      slither/solc_parsing/slitherSolc.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

@ -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
###################################################################################
###################################################################################

@ -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

@ -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
#
#

@ -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

@ -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))

@ -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
###################################################################################

@ -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

@ -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):

Loading…
Cancel
Save