diff --git a/scripts/tests_generate_expected_json_4.sh b/scripts/tests_generate_expected_json_4.sh index e748e4f42..4e1def80c 100755 --- a/scripts/tests_generate_expected_json_4.sh +++ b/scripts/tests_generate_expected_json_4.sh @@ -21,7 +21,7 @@ generate_expected_json(){ } -generate_expected_json tests/deprecated_calls.sol "deprecated-standards" +#generate_expected_json tests/deprecated_calls.sol "deprecated-standards" #generate_expected_json tests/erc20_indexed.sol "erc20-indexed" #generate_expected_json tests/incorrect_erc20_interface.sol "erc20-interface" #generate_expected_json tests/incorrect_erc721_interface.sol "erc721-interface" diff --git a/scripts/tests_generate_expected_json_5.sh b/scripts/tests_generate_expected_json_5.sh index fb9552437..78cc2b3ef 100755 --- a/scripts/tests_generate_expected_json_5.sh +++ b/scripts/tests_generate_expected_json_5.sh @@ -20,6 +20,7 @@ generate_expected_json(){ sed "s|$CURRENT_PATH|$TRAVIS_PATH|g" "$output_filename_txt" -i } +generate_expected_json tests/void-cst.sol "void-cst" #generate_expected_json tests/solc_version_incorrect_05.ast.json "solc-version" #generate_expected_json tests/uninitialized-0.5.1.sol "uninitialized-state" #generate_expected_json tests/backdoor.sol "backdoor" diff --git a/scripts/travis_test_5.sh b/scripts/travis_test_5.sh index 7224638d7..bb598ac1f 100755 --- a/scripts/travis_test_5.sh +++ b/scripts/travis_test_5.sh @@ -69,6 +69,7 @@ test_slither(){ } +test_slither tests/void-cst.json "void-cst" test_slither tests/solc_version_incorrect_05.ast.json "solc-version" test_slither tests/unchecked_lowlevel-0.5.1.sol "unchecked-lowlevel" test_slither tests/unchecked_send-0.5.1.sol "unchecked-send" diff --git a/slither/core/cfg/node.py b/slither/core/cfg/node.py index 8a94a7caa..4251b0bad 100644 --- a/slither/core/cfg/node.py +++ b/slither/core/cfg/node.py @@ -64,7 +64,7 @@ class NodeType: # Node not related to the CFG # Use for state variable declaration, or modifier calls - STANDALONE = 0x50 + OTHER_ENTRYPOINT = 0x50 # @staticmethod @@ -111,6 +111,23 @@ def link_nodes(n1, n2): n1.add_son(n2) n2.add_father(n1) +def recheable(node): + ''' + Return the set of nodes reacheable from the node + :param node: + :return: set(Node) + ''' + nodes = node.sons + visited = set() + while nodes: + next = nodes[0] + nodes = nodes[1:] + if not next in visited: + visited.add(next) + for son in next.sons: + if not son in visited: + nodes.append(son) + return visited # endregion diff --git a/slither/core/declarations/function.py b/slither/core/declarations/function.py index 7243a7c03..c0f5b2687 100644 --- a/slither/core/declarations/function.py +++ b/slither/core/declarations/function.py @@ -22,7 +22,33 @@ logger = logging.getLogger("Function") ReacheableNode = namedtuple('ReacheableNode', ['node', 'ir']) -ModifierStatements = namedtuple('Modifier', ['modifier', 'node']) +class ModifierStatements: + + def __init__(self, modifier, entry_point, nodes): + self._modifier = modifier + self._entry_point = entry_point + self._nodes = nodes + + + @property + def modifier(self): + return self._modifier + + @property + def entry_point(self): + return self._entry_point + + @entry_point.setter + def entry_point(self, entry_point): + self._entry_point = entry_point + + @property + def nodes(self): + return self._nodes + + @nodes.setter + def nodes(self, nodes): + self._nodes = nodes class FunctionType(Enum): NORMAL = 0 @@ -403,7 +429,7 @@ class Function(ChildContract, ChildInheritance, SourceMapping): """ # This is a list of contracts internally, so we convert it to a list of constructor functions. - return [c for c in self._explicit_base_constructor_calls if c.modifier.constructors_declared] + return list(self._explicit_base_constructor_calls) # endregion @@ -1266,10 +1292,12 @@ class Function(ChildContract, ChildInheritance, SourceMapping): node.slithir_generation() for modifier_statement in self.modifiers_statements: - modifier_statement.node.slithir_generation() + for node in modifier_statement.nodes: + node.slithir_generation() for modifier_statement in self.explicit_base_constructor_calls_statements: - modifier_statement.node.slithir_generation() + for node in modifier_statement.nodes: + node.slithir_generation() self._analyze_read_write() self._analyze_calls() diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index 9fd21ac83..39822221b 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -36,5 +36,6 @@ from .source.rtlo import RightToLeftOverride from .statements.too_many_digits import TooManyDigits from .operations.unchecked_low_level_return_values import UncheckedLowLevel from .operations.unchecked_send_return_value import UncheckedSend +from .operations.void_constructor import VoidConstructor # # diff --git a/slither/detectors/operations/void_constructor.py b/slither/detectors/operations/void_constructor.py new file mode 100644 index 000000000..6f2097e75 --- /dev/null +++ b/slither/detectors/operations/void_constructor.py @@ -0,0 +1,46 @@ + +from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.slithir.operations import Nop + +class VoidConstructor(AbstractDetector): + + ARGUMENT = 'void-cst' + HELP = 'Constructor called not implemented' + IMPACT = DetectorClassification.LOW + CONFIDENCE = DetectorClassification.HIGH + + WIKI = 'https://github.com/crytic/slither/wiki/Detector-Documentation#void-constructor' + + WIKI_TITLE = 'Void Constructor' + WIKI_DESCRIPTION = 'Detect the call to a constructor not implemented' + WIKI_RECOMMENDATION = 'Remove the constructor call.' + WIKI_EXPLOIT_SCENARIO = ''' + ```solidity +contract A{} +contract B is A{ + constructor() public A(){} +} +``` +By reading B's constructor definition, the reader might assume that `A()` initiate the contract, while no code is executed.''' + + + def _detect(self): + """ + """ + results = [] + for c in self.contracts: + cst = c.constructor + if cst: + + for constructor_call in cst.explicit_base_constructor_calls_statements: + for node in constructor_call.nodes: + if any(isinstance(ir, Nop) for ir in node.irs): + info = "Void constructor called in {} ({}):\n" + info = info.format(cst.canonical_name, cst.source_mapping_str) + info += "\t-{} {}\n".format(str(node.expression), node.source_mapping_str) + + json = self.generate_json_result(info) + self.add_function_to_json(cst, json) + self.add_nodes_to_json([node], json) + results.append(json) + return results diff --git a/slither/printers/summary/slithir.py b/slither/printers/summary/slithir.py index c6e30073f..4e936e038 100644 --- a/slither/printers/summary/slithir.py +++ b/slither/printers/summary/slithir.py @@ -35,13 +35,9 @@ class PrinterSlithIR(AbstractPrinter): for ir in node.irs: print('\t\t\t{}'.format(ir)) for modifier_statement in function.modifiers_statements: - print(f'\t\tModifier Call {modifier_statement.node.expression}') - for ir in modifier_statement.node.irs: - print('\t\t\t{}'.format(ir)) + print(f'\t\tModifier Call {modifier_statement.entry_point.expression}') for modifier_statement in function.explicit_base_constructor_calls_statements: - print(f'\t\tConstructor Call {modifier_statement.node.expression}') - for ir in modifier_statement.node.irs: - print('\t\t\t{}'.format(ir)) + print(f'\t\tConstructor Call {modifier_statement.entry_point.expression}') for modifier in contract.modifiers: print('\tModifier {}'.format(modifier.canonical_name)) for node in modifier.nodes: diff --git a/slither/slithir/convert.py b/slither/slithir/convert.py index 101604d55..c50a4f8a6 100644 --- a/slither/slithir/convert.py +++ b/slither/slithir/convert.py @@ -21,7 +21,7 @@ from slither.slithir.operations import (Assignment, Balance, Binary, NewElementaryType, NewStructure, OperationWithLValue, Push, Return, Send, SolidityCall, Transfer, - TypeConversion, Unary, Unpack) + TypeConversion, Unary, Unpack, Nop) from slither.slithir.tmp_operations.argument import Argument, ArgumentType from slither.slithir.tmp_operations.tmp_call import TmpCall from slither.slithir.tmp_operations.tmp_new_array import TmpNewArray @@ -587,6 +587,9 @@ def extract_tmp_call(ins, contract): return EventCall(ins.called.name) if isinstance(ins.called, Contract): + # Called a base constructor, where there is no constructor + if ins.called.constructor is None: + return Nop() internalcall = InternalCall(ins.called.constructor, ins.nbr_arguments, ins.lvalue, ins.type_call) internalcall.call_id = ins.call_id diff --git a/slither/slithir/operations/__init__.py b/slither/slithir/operations/__init__.py index b20df950d..cccc812e5 100644 --- a/slither/slithir/operations/__init__.py +++ b/slither/slithir/operations/__init__.py @@ -30,3 +30,4 @@ from .length import Length from .balance import Balance from .phi import Phi from .phi_callback import PhiCallback +from .nop import Nop diff --git a/slither/slithir/operations/nop.py b/slither/slithir/operations/nop.py new file mode 100644 index 000000000..fb26813c1 --- /dev/null +++ b/slither/slithir/operations/nop.py @@ -0,0 +1,14 @@ +from .operation import Operation + +class Nop(Operation): + + @property + def read(self): + return [] + + @property + def used(self): + return [] + + def __str__(self): + return "NOP" \ No newline at end of file diff --git a/slither/solc_parsing/declarations/contract.py b/slither/solc_parsing/declarations/contract.py index c7a9a7793..eb6021966 100644 --- a/slither/solc_parsing/declarations/contract.py +++ b/slither/solc_parsing/declarations/contract.py @@ -373,7 +373,7 @@ class ContractSolc04(Contract): def _create_node(self, func, counter, variable): # Function uses to create node for state variable declaration statements - node = Node(NodeType.STANDALONE, counter) + node = Node(NodeType.OTHER_ENTRYPOINT, counter) node.set_offset(variable.source_mapping, self.slither) node.set_function(func) func.add_node(node) diff --git a/slither/solc_parsing/declarations/function.py b/slither/solc_parsing/declarations/function.py index bfe0d4ea2..9ec13cef8 100644 --- a/slither/solc_parsing/declarations/function.py +++ b/slither/solc_parsing/declarations/function.py @@ -2,7 +2,7 @@ """ import logging -from slither.core.cfg.node import NodeType, link_nodes +from slither.core.cfg.node import NodeType, link_nodes, recheable from slither.core.declarations.contract import Contract from slither.core.declarations.function import Function, ModifierStatements, FunctionType @@ -220,13 +220,13 @@ class FunctionSolc(Function): for node in self.nodes: node.analyze_expressions(self) - for modifier_statement in self.modifiers_statements: - modifier_statement.node.analyze_expressions(self) + if self._filter_ternary(): + for modifier_statement in self.modifiers_statements: + modifier_statement.nodes = recheable(modifier_statement.entry_point) - for modifier_statement in self.explicit_base_constructor_calls_statements: - modifier_statement.node.analyze_expressions(self) + for modifier_statement in self.explicit_base_constructor_calls_statements: + modifier_statement.nodes = recheable(modifier_statement.entry_point) - self._filter_ternary() self._remove_alone_endif() @@ -903,15 +903,21 @@ class FunctionSolc(Function): self._expression_modifiers.append(m) for m in ExportValues(m).result(): if isinstance(m, Function): - node = self._new_node(NodeType.STANDALONE, modifier['src']) + entry_point = self._new_node(NodeType.OTHER_ENTRYPOINT, modifier['src']) + node = self._new_node(NodeType.EXPRESSION, modifier['src']) node.add_unparsed_expression(modifier) + link_nodes(entry_point, node) self._modifiers.append(ModifierStatements(modifier=m, - node=node)) + entry_point=entry_point, + nodes=[entry_point, node])) elif isinstance(m, Contract): - node = self._new_node(NodeType.STANDALONE, modifier['src']) + entry_point = self._new_node(NodeType.OTHER_ENTRYPOINT, modifier['src']) + node = self._new_node(NodeType.EXPRESSION, modifier['src']) node.add_unparsed_expression(modifier) + link_nodes(entry_point, node) self._explicit_base_constructor_calls.append(ModifierStatements(modifier=m, - node=node)) + entry_point=entry_point, + nodes=[entry_point, node])) # endregion ################################################################################### @@ -971,9 +977,10 @@ class FunctionSolc(Function): def _filter_ternary(self): ternary_found = True + updated = False while ternary_found: ternary_found = False - for node in self.nodes: + for node in self._nodes: has_cond = HasConditional(node.expression) if has_cond.result(): st = SplitTernaryExpression(node.expression) @@ -982,11 +989,13 @@ class FunctionSolc(Function): raise ParsingError(f'Incorrect ternary conversion {node.expression} {node.source_mapping_str}') true_expr = st.true_expression false_expr = st.false_expression - self.split_ternary_node(node, condition, true_expr, false_expr) + self._split_ternary_node(node, condition, true_expr, false_expr) ternary_found = True + updated = True break + return updated - def split_ternary_node(self, node, condition, true_expr, false_expr): + def _split_ternary_node(self, node, condition, true_expr, false_expr): condition_node = self._new_node(NodeType.IF, node.source_mapping) condition_node.add_expression(condition) condition_node.analyze_expressions(self) diff --git a/slither/visitors/slithir/expression_to_slithir.py b/slither/visitors/slithir/expression_to_slithir.py index 753bd82eb..f4ffd91a1 100644 --- a/slither/visitors/slithir/expression_to_slithir.py +++ b/slither/visitors/slithir/expression_to_slithir.py @@ -256,5 +256,5 @@ class ExpressionToSlithIR(ExpressionVisitor): self._result.append(operation) set_val(expression, lvalue) else: - raise Exception('Unary operation to IR not supported {}'.format(expression)) + raise SlithIRError('Unary operation to IR not supported {}'.format(expression)) diff --git a/tests/expected_json/void-cst.void-cst.json b/tests/expected_json/void-cst.void-cst.json new file mode 100644 index 000000000..ab97b9d7d --- /dev/null +++ b/tests/expected_json/void-cst.void-cst.json @@ -0,0 +1,130 @@ +{ + "success": true, + "error": null, + "results": { + "detectors": [ + { + "check": "void-cst", + "impact": "Low", + "confidence": "High", + "description": "Void constructor called in D.constructor() (tests/void-cst.sol#10-12):\n\t-C() tests/void-cst.sol#10\n", + "elements": [ + { + "type": "function", + "name": "constructor", + "source_mapping": { + "start": 41, + "length": 32, + "filename_used": "/home/travis/build/crytic/slither/tests/void-cst.sol", + "filename_relative": "tests/void-cst.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/void-cst.sol", + "filename_short": "tests/void-cst.sol", + "is_dependency": false, + "lines": [ + 10, + 11, + 12 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "D", + "source_mapping": { + "start": 19, + "length": 57, + "filename_used": "/home/travis/build/crytic/slither/tests/void-cst.sol", + "filename_relative": "tests/void-cst.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/void-cst.sol", + "filename_short": "tests/void-cst.sol", + "is_dependency": false, + "lines": [ + 8, + 9, + 10, + 11, + 12, + 13, + 14 + ], + "starting_column": 1, + "ending_column": 2 + } + }, + "signature": "constructor()" + } + }, + { + "type": "node", + "name": "C()", + "source_mapping": { + "start": 62, + "length": 3, + "filename_used": "/home/travis/build/crytic/slither/tests/void-cst.sol", + "filename_relative": "tests/void-cst.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/void-cst.sol", + "filename_short": "tests/void-cst.sol", + "is_dependency": false, + "lines": [ + 10 + ], + "starting_column": 26, + "ending_column": 29 + }, + "type_specific_fields": { + "parent": { + "type": "function", + "name": "constructor", + "source_mapping": { + "start": 41, + "length": 32, + "filename_used": "/home/travis/build/crytic/slither/tests/void-cst.sol", + "filename_relative": "tests/void-cst.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/void-cst.sol", + "filename_short": "tests/void-cst.sol", + "is_dependency": false, + "lines": [ + 10, + 11, + 12 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "D", + "source_mapping": { + "start": 19, + "length": 57, + "filename_used": "/home/travis/build/crytic/slither/tests/void-cst.sol", + "filename_relative": "tests/void-cst.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/void-cst.sol", + "filename_short": "tests/void-cst.sol", + "is_dependency": false, + "lines": [ + 8, + 9, + 10, + 11, + 12, + 13, + 14 + ], + "starting_column": 1, + "ending_column": 2 + } + }, + "signature": "constructor()" + } + } + } + } + ] + } + ] + } +} \ No newline at end of file diff --git a/tests/expected_json/void-cst.void-cst.txt b/tests/expected_json/void-cst.void-cst.txt new file mode 100644 index 000000000..cc3473967 --- /dev/null +++ b/tests/expected_json/void-cst.void-cst.txt @@ -0,0 +1,5 @@ +INFO:Detectors: +Void constructor called in D.constructor() (tests/void-cst.sol#10-12): + -C() tests/void-cst.sol#10 +Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#void-constructor +INFO:Slither:tests/void-cst.sol analyzed (2 contracts), 1 result(s) found diff --git a/tests/void-cst.sol b/tests/void-cst.sol new file mode 100644 index 000000000..43edc9d9a --- /dev/null +++ b/tests/void-cst.sol @@ -0,0 +1,14 @@ + + +contract C{ + + +} + +contract D is C{ + + constructor() public C(){ + + } + +}