Open source shadowing local and shadowing builtin detectors

scripts/tests_generate_expected_json_4.sh

@@ -40,3 +40,5 @@ generate_expected_json(){
 #generate_expected_json tests/shadowing_abstract.sol "shadowing-abstract"
 #generate_expected_json tests/timestamp.sol "timestamp"
 #generate_expected_json tests/multiple_calls_in_loop.sol "calls-loop"
+#generate_expected_json tests/shadowing_builtin_symbols.sol "shadowing-builtin"
+#generate_expected_json tests/shadowing_local_variable.sol "shadowing-local"

scripts/travis_test_4.sh

@@ -91,5 +91,6 @@ test_slither tests/shadowing_abstract.sol "shadowing-abstract"
 test_slither tests/shadowing_state_variable.sol "shadowing-state"
 test_slither tests/timestamp.sol "timestamp"
 test_slither tests/multiple_calls_in_loop.sol "calls-loop"
-
+test_slither tests/shadowing_builtin_symbols.sol "shadowing-builtin"
+test_slither tests/shadowing_local_variable.sol "shadowing-local"
 

slither/__main__.py

@@ -132,6 +132,8 @@ def get_detectors_and_printers():
     from slither.detectors.attributes.const_functions import ConstantFunctions
     from slither.detectors.shadowing.abstract import ShadowingAbstractDetection
     from slither.detectors.shadowing.state import StateShadowing
+    from slither.detectors.shadowing.local import LocalShadowing
+    from slither.detectors.shadowing.builtin_symbols import BuiltinSymbolShadowing
     from slither.detectors.operations.block_timestamp import Timestamp
     from slither.detectors.statements.calls_in_loop import MultipleCallsInLoop
     from slither.detectors.statements.incorrect_strict_equality import IncorrectStrictEquality
@@ -163,7 +165,9 @@ def get_detectors_and_printers():
                  StateShadowing,
                  Timestamp,
                  MultipleCallsInLoop,
-                 IncorrectStrictEquality]
+                 IncorrectStrictEquality,
+                 LocalShadowing,
+                 BuiltinSymbolShadowing]
 
     from slither.printers.summary.function import FunctionSummary
     from slither.printers.summary.contract import ContractSummary

slither/detectors/shadowing/builtin_symbols.py

@@ -0,0 +1,133 @@
+"""
+Module detecting reserved keyword shadowing
+"""
+
+from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
+
+
+class BuiltinSymbolShadowing(AbstractDetector):
+    """
+    Built-in symbol shadowing
+    """
+
+    ARGUMENT = 'shadowing-builtin'
+    HELP = 'Built-in symbol shadowing'
+    IMPACT = DetectorClassification.LOW
+    CONFIDENCE = DetectorClassification.HIGH
+
+    WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#builtin-symbol-shadowing'
+
+    vuln_name = 'ShadowingBuiltinSymbols'
+
+    SHADOWING_FUNCTION = "function"
+    SHADOWING_MODIFIER = "modifier"
+    SHADOWING_LOCAL_VARIABLE = "local variable"
+    SHADOWING_STATE_VARIABLE = "state variable"
+    SHADOWING_EVENT = "event"
+
+    # Reserved keywords reference: https://solidity.readthedocs.io/en/v0.5.2/units-and-global-variables.html
+    BUILTIN_SYMBOLS = ["assert", "require", "revert", "block", "blockhash",
+                       "gasleft", "msg", "now", "tx", "this", "addmod", "mulmod",
+                       "keccak256", "sha256", "sha3", "ripemd160", "ecrecover",
+                       "selfdestruct", "suicide", "abi"]
+
+    # https://solidity.readthedocs.io/en/v0.5.2/miscellaneous.html#reserved-keywords
+    RESERVED_KEYWORDS = ['abstract', 'after', 'alias', 'apply', 'auto', 'case', 'catch', 'copyof',
+                         'default', 'define', 'final', 'immutable', 'implements', 'in', 'inline',
+                         'let', 'macro', 'match', 'mutable', 'null', 'of', 'override', 'partial',
+                         'promise', 'reference', 'relocatable', 'sealed', 'sizeof', 'static',
+                         'supports', 'switch', 'try', 'type', 'typedef', 'typeof', 'unchecked']
+
+    def is_builtin_symbol(self, word):
+        """ Detects if a given word is a built-in symbol.
+
+        Returns:
+            boolean: True if the given word represents a built-in symbol."""
+
+        return word in self.BUILTIN_SYMBOLS or word in self.RESERVED_KEYWORDS
+
+    def detect_builtin_shadowing_locals(self, function_or_modifier):
+        """ Detects if local variables in a given function/modifier are named after built-in symbols.
+            Any such items are returned in a list.
+
+        Returns:
+            list of tuple: (type, definition, local variable parent)"""
+
+        results = []
+        for local in function_or_modifier.variables:
+            if self.is_builtin_symbol(local.name):
+                results.append((self.SHADOWING_LOCAL_VARIABLE, local, function_or_modifier))
+        return results
+
+    def detect_builtin_shadowing_definitions(self, contract):
+        """ Detects if functions, access modifiers, events, state variables, or local variables are named after built-in
+            symbols. Any such definitions are returned in a list.
+
+        Returns:
+            list of tuple: (type, definition, [local variable parent])"""
+
+        result = []
+
+        # Loop through all functions, modifiers, variables (state and local) to detect any built-in symbol keywords.
+        for function in contract.functions:
+            if function.contract == contract:
+                if self.is_builtin_symbol(function.name):
+                    result.append((self.SHADOWING_FUNCTION, function, None))
+                result += self.detect_builtin_shadowing_locals(function)
+        for modifier in contract.modifiers:
+            if modifier.contract == contract:
+                if self.is_builtin_symbol(modifier.name):
+                    result.append((self.SHADOWING_MODIFIER, modifier, None))
+                result += self.detect_builtin_shadowing_locals(modifier)
+        for variable in contract.variables:
+            if variable.contract == contract:
+                if self.is_builtin_symbol(variable.name):
+                    result.append((self.SHADOWING_STATE_VARIABLE, variable, None))
+        for event in contract.events:
+            if event.contract == contract:
+                if self.is_builtin_symbol(event.name):
+                    result.append((self.SHADOWING_EVENT, event, None))
+
+        return result
+
+    def detect(self):
+        """ Detect shadowing of built-in symbols
+
+        Recursively visit the calls
+        Returns:
+            list: {'vuln', 'filename,'contract','func', 'shadow'}
+
+        """
+
+        results = []
+        for contract in self.contracts:
+            shadows = self.detect_builtin_shadowing_definitions(contract)
+            if shadows:
+                for shadow in shadows:
+                    # Obtain components
+                    shadow_type = shadow[0]
+                    shadow_object = shadow[1]
+                    local_variable_parent = shadow[2]
+
+                    # Build the path for our info string
+                    local_variable_path = contract.name + "."
+                    if local_variable_parent is not None:
+                        local_variable_path += local_variable_parent.name + "."
+                    local_variable_path += shadow_object.name
+
+                    info = '{} ({} @ {}) shadows built-in symbol \"{}"\n'.format(local_variable_path,
+                                                                                 shadow_type,
+                                                                                 shadow_object.source_mapping_str,
+                                                                                 shadow_object.name)
+                    # Print relevant information to the log
+                    self.log(info)
+
+                    # Generate relevant JSON data for this shadowing definition.
+                    json = self.generate_json_result(info)
+                    if shadow_type in [self.SHADOWING_FUNCTION, self.SHADOWING_MODIFIER, self.SHADOWING_EVENT]:
+                        self.add_function_to_json(shadow_object, json)
+                    elif shadow_type in [self.SHADOWING_STATE_VARIABLE, self.SHADOWING_LOCAL_VARIABLE]:
+                        self.add_variable_to_json(shadow_object, json)
+                    results.append(json)
+
+        return results

slither/detectors/shadowing/local.py

@@ -0,0 +1,109 @@
+"""
+Module detecting local variable shadowing
+"""
+
+from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
+
+
+class LocalShadowing(AbstractDetector):
+    """
+    Local variable shadowing
+    """
+
+    ARGUMENT = 'shadowing-local'
+    HELP = 'Local variables shadowing'
+    IMPACT = DetectorClassification.LOW
+    CONFIDENCE = DetectorClassification.HIGH
+
+    WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#local-variable-shadowing'
+
+    vuln_name = 'ShadowingLocalVariable'
+
+    OVERSHADOWED_FUNCTION = "function"
+    OVERSHADOWED_MODIFIER = "modifier"
+    OVERSHADOWED_STATE_VARIABLE = "state variable"
+    OVERSHADOWED_EVENT = "event"
+
+    def detect_shadowing_definitions(self, contract):
+        """ Detects if functions, access modifiers, events, state variables, and local variables are named after
+        reserved keywords. Any such definitions are returned in a list.
+
+        Returns:
+            list of tuple: (type, contract name, definition)"""
+        result = []
+
+        # Loop through all functions + modifiers in this contract.
+        for function in contract.functions + contract.modifiers:
+            # We should only look for functions declared directly in this contract (not in a base contract).
+            if function.contract != contract:
+                continue
+
+            # This function was declared in this contract, we check what its local variables might shadow.
+            for variable in function.variables:
+                overshadowed = []
+                for scope_contract in [contract] + contract.inheritance:
+                    # Check functions
+                    for scope_function in scope_contract.functions:
+                        if variable.name == scope_function.name and scope_function.contract == scope_contract:
+                            overshadowed.append((self.OVERSHADOWED_FUNCTION, scope_contract.name, scope_function))
+                    # Check modifiers
+                    for scope_modifier in scope_contract.modifiers:
+                        if variable.name == scope_modifier.name and scope_modifier.contract == scope_contract:
+                            overshadowed.append((self.OVERSHADOWED_MODIFIER, scope_contract.name, scope_modifier))
+                    # Check events
+                    for scope_event in scope_contract.events:
+                        if variable.name == scope_event.name and scope_event.contract == scope_contract:
+                            overshadowed.append((self.OVERSHADOWED_EVENT, scope_contract.name, scope_event))
+                    # Check state variables
+                    for scope_state_variable in scope_contract.variables:
+                        if variable.name == scope_state_variable.name and scope_state_variable.contract == scope_contract:
+                            overshadowed.append((self.OVERSHADOWED_STATE_VARIABLE, scope_contract.name, scope_state_variable))
+
+                # If we have found any overshadowed objects, we'll want to add it to our result list.
+                if overshadowed:
+                    result.append((contract.name, function.name, variable, overshadowed))
+
+        return result
+
+    def detect(self):
+        """ Detect shadowing local variables
+
+        Recursively visit the calls
+        Returns:
+            list: {'vuln', 'filename,'contract','func', 'shadow'}
+
+        """
+
+        results = []
+        for contract in self.contracts:
+            shadows = self.detect_shadowing_definitions(contract)
+            if shadows:
+                for shadow in shadows:
+                    local_parent_name = shadow[1]
+                    local_variable = shadow[2]
+                    overshadowed = shadow[3]
+                    info = '{}.{}.{} (local variable @ {}) shadows:\n'.format(contract.name,
+                                                                              local_parent_name,
+                                                                              local_variable.name,
+                                                                              local_variable.source_mapping_str)
+                    for overshadowed_entry in overshadowed:
+                        info += "\t- {}.{} ({} @ {})\n".format(overshadowed_entry[1],
+                                                               overshadowed_entry[2],
+                                                               overshadowed_entry[0],
+                                                               overshadowed_entry[2].source_mapping_str)
+
+                    # Print relevant information to the log
+                    self.log(info)
+
+                    # Generate relevant JSON data for this shadowing definition.
+                    json = self.generate_json_result(info)
+                    self.add_variable_to_json(local_variable, json)
+                    for overshadowed_entry in overshadowed:
+                        if overshadowed_entry[0] in [self.OVERSHADOWED_FUNCTION, self.OVERSHADOWED_MODIFIER,
+                                                     self.OVERSHADOWED_EVENT]:
+                            self.add_function_to_json(overshadowed_entry[2], json)
+                        elif overshadowed_entry[0] == self.OVERSHADOWED_STATE_VARIABLE:
+                            self.add_variable_to_json(overshadowed_entry[2], json)
+                    results.append(json)
+
+        return results

tests/expected_json/shadowing_local_variable.shadowing-local.json

@@ -0,0 +1 @@
+[{"check": "shadowing-local", "impact": "Low", "confidence": "High", "description": "FurtherExtendedContract.shadowingParent.x (local variable @ tests/shadowing_local_variable.sol#25) shadows:\n\t- FurtherExtendedContract.x (state variable @ tests/shadowing_local_variable.sol#17)\n\t- ExtendedContract.x (state variable @ tests/shadowing_local_variable.sol#9)\n\t- BaseContract.x (state variable @ tests/shadowing_local_variable.sol#4)\n", "elements": [{"type": "variable", "name": "x", "source_mapping": {"start": 376, "length": 6, "filename": "tests/shadowing_local_variable.sol", "lines": [25]}}, {"type": "variable", "name": "x", "source_mapping": {"start": 256, "length": 10, "filename": "tests/shadowing_local_variable.sol", "lines": [17]}}, {"type": "variable", "name": "x", "source_mapping": {"start": 133, "length": 10, "filename": "tests/shadowing_local_variable.sol", "lines": [9]}}, {"type": "variable", "name": "x", "source_mapping": {"start": 54, "length": 10, "filename": "tests/shadowing_local_variable.sol", "lines": [4]}}]}, {"check": "shadowing-local", "impact": "Low", "confidence": "High", "description": "FurtherExtendedContract.shadowingParent.y (local variable @ tests/shadowing_local_variable.sol#25) shadows:\n\t- BaseContract.y (state variable @ tests/shadowing_local_variable.sol#5)\n", "elements": [{"type": "variable", "name": "y", "source_mapping": {"start": 398, "length": 5, "filename": "tests/shadowing_local_variable.sol", "lines": [25]}}, {"type": "variable", "name": "y", "source_mapping": {"start": 70, "length": 10, "filename": "tests/shadowing_local_variable.sol", "lines": [5]}}]}, {"check": "shadowing-local", "impact": "Low", "confidence": "High", "description": "FurtherExtendedContract.shadowingParent.z (local variable @ tests/shadowing_local_variable.sol#25) shadows:\n\t- ExtendedContract.z (function @ tests/shadowing_local_variable.sol#11)\n", "elements": [{"type": "variable", "name": "z", "source_mapping": {"start": 405, "length": 6, "filename": "tests/shadowing_local_variable.sol", "lines": [25]}}, {"type": "function", "name": "z", "source_mapping": {"start": 150, "length": 27, "filename": "tests/shadowing_local_variable.sol", "lines": [11]}, "contract": {"type": "contract", "name": "ExtendedContract", "source_mapping": {"start": 85, "length": 110, "filename": "tests/shadowing_local_variable.sol", "lines": [8, 9, 10, 11, 12, 13, 14]}}}]}, {"check": "shadowing-local", "impact": "Low", "confidence": "High", "description": "FurtherExtendedContract.shadowingParent.w (local variable @ tests/shadowing_local_variable.sol#25) shadows:\n\t- FurtherExtendedContract.w (modifier @ tests/shadowing_local_variable.sol#20-23)\n", "elements": [{"type": "variable", "name": "w", "source_mapping": {"start": 413, "length": 6, "filename": "tests/shadowing_local_variable.sol", "lines": [25]}}, {"type": "function", "name": "w", "source_mapping": {"start": 274, "length": 71, "filename": "tests/shadowing_local_variable.sol", "lines": [20, 21, 22, 23]}, "contract": {"type": "contract", "name": "FurtherExtendedContract", "source_mapping": {"start": 197, "length": 235, "filename": "tests/shadowing_local_variable.sol", "lines": [16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26]}}}]}, {"check": "shadowing-local", "impact": "Low", "confidence": "High", "description": "FurtherExtendedContract.shadowingParent.v (local variable @ tests/shadowing_local_variable.sol#25) shadows:\n\t- ExtendedContract.v (event @ tests/shadowing_local_variable.sol#13)\n", "elements": [{"type": "variable", "name": "v", "source_mapping": {"start": 421, "length": 6, "filename": "tests/shadowing_local_variable.sol", "lines": [25]}}, {"type": "function", "name": "v", "source_mapping": {"start": 183, "length": 10, "filename": "tests/shadowing_local_variable.sol", "lines": [13]}, "contract": {"type": "contract", "name": "ExtendedContract", "source_mapping": {"start": 85, "length": 110, "filename": "tests/shadowing_local_variable.sol", "lines": [8, 9, 10, 11, 12, 13, 14]}}}]}]

tests/shadowing_builtin_symbols.sol

@@ -0,0 +1,34 @@
+pragma solidity ^0.4.25;
+
+contract BaseContract {
+    uint blockhash;
+    uint now;
+
+    event revert(bool condition);
+}
+
+contract ExtendedContract is BaseContract {
+    uint ecrecover = 7;
+
+    function assert(bool condition) public {
+        uint msg;
+    }
+}
+
+contract FurtherExtendedContract is ExtendedContract {
+    uint blockhash = 7;
+    uint this = 5;
+    uint abi;
+
+    modifier require {
+        assert(msg.sender != address(0));
+        uint keccak256;
+        uint sha3;
+        _;
+    }
+}
+
+contract Reserved{
+    address mutable;
+
+}

tests/shadowing_local_variable.sol

@@ -0,0 +1,26 @@
+pragma solidity ^0.4.24;
+
+contract BaseContract {
+    uint x = 5;
+    uint y = 5;
+}
+
+contract ExtendedContract is BaseContract {
+    uint x = 7;
+
+    function z() public pure {}
+
+    event v();
+}
+
+contract FurtherExtendedContract is ExtendedContract {
+    uint x = 7;
+
+
+    modifier w {
+        assert(msg.sender != address(0));
+        _;
+    }
+
+    function shadowingParent(uint x) public pure { int y; uint z; uint w; uint v; }
+}