Merge pull request #311 from crytic/dev-refactor-reentrancy

Refactor reentrancy + API changes

slither/core/cfg/node.py

@@ -200,6 +200,10 @@ def __init__(self, node_type, node_id):
         self._expression_vars_read = []
         self._expression_calls = []
 
+        # Computed on the fly, can be True of False
+        self._can_reenter = None
+        self._can_send_eth = None
+
     ###################################################################################
     ###################################################################################
     # region General's properties
@@ -402,6 +406,39 @@ def internal_calls_as_expressions(self):
     def calls_as_expression(self):
         return list(self._expression_calls)
 
+    def can_reenter(self, callstack=None):
+        '''
+        Check if the node can re-enter
+        Do not consider CREATE as potential re-enter, but check if the
+        destination's constructor can contain a call (recurs. follow nested CREATE)
+        For Solidity > 0.5, filter access to public variables and constant/pure/view
+        For call to this. check if the destination can re-enter
+        Do not consider Send/Transfer as there is not enough gas
+        :param callstack: used internally to check for recursion
+        :return bool:
+        '''
+        from slither.slithir.operations import Call
+        if self._can_reenter is None:
+            self._can_reenter = False
+            for ir in self.irs:
+                if isinstance(ir, Call) and ir.can_reenter(callstack):
+                    self._can_reenter = True
+                    return True
+        return self._can_reenter
+
+    def can_send_eth(self):
+        '''
+        Check if the node can send eth
+        :return bool:
+        '''
+        from slither.slithir.operations import Call
+        if self._can_send_eth is None:
+            for ir in self.all_slithir_operations():
+                if isinstance(ir, Call) and ir.can_send_eth():
+                    self._can_send_eth = True
+                    return True
+        return self._can_reenter
+
     # endregion
     ###################################################################################
     ###################################################################################

slither/core/children/child_node.py

@@ -18,3 +18,7 @@ def function(self):
     @property
     def contract(self):
         return self.node.function.contract
+
+    @property
+    def slither(self):
+        return self.contract.slither

slither/core/declarations/function.py

@@ -132,6 +132,10 @@ def __init__(self):
         self._function_type = None
         self._is_constructor = None
 
+        # Computed on the fly, can be True of False
+        self._can_reenter = None
+        self._can_send_eth = None
+
     ###################################################################################
     ###################################################################################
     # region General properties
@@ -169,11 +173,44 @@ def canonical_name(self):
         name, parameters, _ = self.signature
         return self.contract_declarer.name + '.' + name + '(' + ','.join(parameters) + ')'
 
-
     @property
     def contains_assembly(self):
         return self._contains_assembly
 
+    def can_reenter(self, callstack=None):
+        '''
+        Check if the function can re-enter
+        Follow internal calls.
+        Do not consider CREATE as potential re-enter, but check if the
+        destination's constructor can contain a call (recurs. follow nested CREATE)
+        For Solidity > 0.5, filter access to public variables and constant/pure/view
+        For call to this. check if the destination can re-enter
+        Do not consider Send/Transfer as there is not enough gas
+        :param callstack: used internally to check for recursion
+        :return bool:
+        '''
+        from slither.slithir.operations import Call
+        if self._can_reenter is None:
+            self._can_reenter = False
+            for ir in self.all_slithir_operations():
+                if isinstance(ir, Call) and ir.can_reenter(callstack):
+                    self._can_reenter = True
+                    return True
+        return self._can_reenter
+
+    def can_send_eth(self):
+        '''
+        Check if the function can send eth
+        :return bool:
+        '''
+        from slither.slithir.operations import Call
+        if self._can_send_eth is None:
+            for ir in self.all_slithir_operations():
+                if isinstance(ir, Call) and ir.can_send_eth():
+                    self._can_send_eth = True
+                    return True
+        return self._can_reenter
+
     @property
     def slither(self):
         return self.contract.slither
@@ -1184,8 +1221,6 @@ def _analyze_calls(self):
         external_calls_as_expressions = [item for sublist in external_calls_as_expressions for item in sublist]
         self._external_calls_as_expressions = list(set(external_calls_as_expressions))
 
-
-
     # endregion
     ###################################################################################
     ###################################################################################

slither/detectors/reentrancy/reentrancy.py

@@ -11,9 +11,8 @@
 from slither.detectors.abstract_detector import (AbstractDetector,
                                                  DetectorClassification)
 from slither.slithir.operations import (HighLevelCall, LowLevelCall,
-                                        LibraryCall,
+                                        Call,
                                         Send, Transfer)
-from slither.core.variables.variable import Variable
 
 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()))}
@@ -25,12 +24,6 @@ def dict_are_equal(d1, d2):
     return all(set(d1[k]) == set(d2[k]) for k in d1.keys())
 
 class Reentrancy(AbstractDetector):
-# This detector is not meant to be registered
-# It is inherited by reentrancy variantsœ
-#    ARGUMENT = 'reentrancy'
-#    HELP = 'Reentrancy vulnerabilities'
-#    IMPACT = DetectorClassification.HIGH
-#    CONFIDENCE = DetectorClassification.HIGH
 
     KEY = 'REENTRANCY'
 
@@ -43,27 +36,10 @@ def _can_callback(self, irs):
             - low level call
             - high level call
 
-            Do not consider Send/Transfer as there is not enough gas
+
         """
         for ir in irs:
-            if isinstance(ir, LowLevelCall):
-                return True
-            if isinstance(ir, HighLevelCall) and not isinstance(ir, LibraryCall):
-                # If solidity >0.5, STATICCALL is used
-                if self.slither.solc_version and self.slither.solc_version.startswith('0.5.'):
-                    if isinstance(ir.function, Function) and (ir.function.view or ir.function.pure):
-                        continue
-                    if isinstance(ir.function, Variable):
-                        continue
-                # If there is a call to itself
-                # We can check that the function called is
-                # reentrancy-safe
-                if ir.destination == SolidityVariable('this'):
-                    if isinstance(ir.function, Variable):
-                        continue
-                    if not ir.function.all_high_level_calls():
-                        if not ir.function.all_low_level_calls():
-                            continue
+            if isinstance(ir, Call) and ir.can_reenter():
                 return True
         return False
 
@@ -73,9 +49,11 @@ def _can_send_eth(irs):
             Detect if the node can send eth
         """
         for ir in irs:
-            if isinstance(ir, (HighLevelCall, LowLevelCall, Transfer, Send)):
-                if ir.call_value:
-                    return True
+            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):
@@ -174,7 +152,6 @@ def _explore(self, node, visited, skip_father=None):
                 self._explore(son, visited, node)
                 sons = [sons[0]]
 
-
         for son in sons:
             self._explore(son, visited)
 

slither/slithir/operations/call.py

@@ -15,3 +15,16 @@ def arguments(self):
     def arguments(self, v):
         self._arguments = v
 
+    def can_reenter(self, callstack=None):
+        '''
+        Must be called after slithIR analysis pass
+        :return: bool
+        '''
+        return False
+
+    def can_send_eth(self):
+        '''
+        Must be called after slithIR analysis pass
+        :return: bool
+        '''
+        return False

slither/slithir/operations/high_level_call.py

@@ -2,6 +2,7 @@
 from slither.slithir.operations.lvalue import OperationWithLValue
 from slither.core.variables.variable import Variable
 from slither.core.declarations.solidity_variables import SolidityVariable
+from slither.core.declarations.function import Function
 
 from slither.slithir.utils.utils import is_valid_lvalue
 from slither.slithir.variables.constant import Constant
@@ -86,6 +87,55 @@ def nbr_arguments(self):
     def type_call(self):
         return self._type_call
 
+    ###################################################################################
+    ###################################################################################
+    # region Analyses
+    ###################################################################################
+    ###################################################################################
+
+    def can_reenter(self, callstack=None):
+        '''
+        Must be called after slithIR analysis pass
+        For Solidity > 0.5, filter access to public variables and constant/pure/view
+        For call to this. check if the destination can re-enter
+        :param callstack: check for recursion
+        :return: bool
+        '''
+        # If solidity >0.5, STATICCALL is used
+        if self.slither.solc_version and self.slither.solc_version.startswith('0.5.'):
+            if isinstance(self.function, Function) and (self.function.view or self.function.pure):
+                return False
+            if isinstance(self.function, Variable):
+                return False
+        # If there is a call to itself
+        # We can check that the function called is
+        # reentrancy-safe
+        if self.destination == SolidityVariable('this'):
+            if isinstance(self.function, Variable):
+                return False
+            # In case of recursion, return False
+            callstack = [] if callstack is None else callstack
+            if self.function in callstack:
+                return False
+            callstack = callstack + [self.function]
+            if self.function.can_reenter(callstack):
+                return True
+        return True
+
+    def can_send_eth(self):
+        '''
+        Must be called after slithIR analysis pass
+        :return: bool
+        '''
+        return self._call_value is not None
+
+    # endregion
+    ###################################################################################
+    ###################################################################################
+    # region Built in
+    ###################################################################################
+    ###################################################################################
+
     def __str__(self):
         value = ''
         gas = ''

slither/slithir/operations/library_call.py

@@ -9,6 +9,18 @@ class LibraryCall(HighLevelCall):
     def _check_destination(self, destination):
         assert isinstance(destination, (Contract))
 
+    def can_reenter(self, callstack=None):
+        '''
+        Must be called after slithIR analysis pass
+        :return: bool
+        '''
+        # In case of recursion, return False
+        callstack = [] if callstack is None else callstack
+        if self.function in callstack:
+            return False
+        callstack = callstack + [self.function]
+        return self.function.can_reenter(callstack)
+
     def __str__(self):
         gas = ''
         if self.call_gas:

slither/slithir/operations/low_level_call.py

@@ -54,6 +54,20 @@ def read(self):
         # remove None
         return self._unroll([x for x in all_read if x])
 
+    def can_reenter(self, callstack=None):
+        '''
+        Must be called after slithIR analysis pass
+        :return: bool
+        '''
+        return True
+
+    def can_send_eth(self):
+        '''
+        Must be called after slithIR analysis pass
+        :return: bool
+        '''
+        return self._call_value is not None
+
     @property
     def destination(self):
         return self._destination

slither/slithir/operations/new_contract.py

@@ -31,16 +31,52 @@ def call_id(self):
     def call_id(self, c):
         self._callid = c
 
-
     @property
     def contract_name(self):
         return self._contract_name
 
-
     @property
     def read(self):
         return self._unroll(self.arguments)
 
+    @property
+    def contract_created(self):
+        contract_name = self.contract_name
+        contract_instance = self.slither.get_contract_from_name(contract_name)
+        return contract_instance
+
+    ###################################################################################
+    ###################################################################################
+    # region Analyses
+    ###################################################################################
+    ###################################################################################
+
+    def can_reenter(self, callstack=None):
+        '''
+        Must be called after slithIR analysis pass
+        For Solidity > 0.5, filter access to public variables and constant/pure/view
+        For call to this. check if the destination can re-enter
+        :param callstack: check for recursion
+        :return: bool
+        '''
+        callstack = [] if callstack is None else callstack
+        constructor = self.contract_created.constructor
+        if constructor is None:
+            return False
+        if constructor in callstack:
+            return False
+        callstack = callstack + [constructor]
+        return constructor.can_reenter(callstack)
+
+    def can_send_eth(self):
+        '''
+        Must be called after slithIR analysis pass
+        :return: bool
+        '''
+        return self._call_value is not None
+
+    # endregion
+
     def __str__(self):
         value = ''
         if self.call_value:

slither/utils/function.py

@@ -1,4 +1,3 @@
-import hashlib
 import sha3
 
 def get_function_id(sig):