Merge pull request #301 from crytic/dev-fix-ternary-modifier-calls

Improvements on modifier/constructor calls support

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"

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"

scripts/travis_test_5.sh

@@ -69,6 +69,7 @@ test_slither(){
 }
 
 
+test_slither tests/void-cst.sol "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"

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

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

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

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

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:

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

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

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"

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)

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)

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

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()"
+                }
+              }
+            }
+          }
+        ]
+      }
+    ]
+  }
+}

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

tests/void-cst.sol

@@ -0,0 +1,14 @@
+
+
+contract C{
+
+
+}
+
+contract D is C{
+
+    constructor() public C(){
+
+    }
+
+}