Merge branch 'dev' into dev-reworked-fail-on

slither/detectors/attributes/locked_ether.py

@@ -3,7 +3,7 @@
 """
 from typing import List
 
-from slither.core.declarations.contract import Contract
+from slither.core.declarations import Contract, SolidityFunction
 from slither.detectors.abstract_detector import (
     AbstractDetector,
     DetectorClassification,
@@ -17,7 +17,9 @@ from slither.slithir.operations import (
     NewContract,
     LibraryCall,
     InternalCall,
+    SolidityCall,
 )
+from slither.slithir.variables import Constant
 from slither.utils.output import Output
 
 
@@ -68,8 +70,28 @@ Every Ether sent to `Locked` will be lost."""
                         ):
                             if ir.call_value and ir.call_value != 0:
                                 return False
-                        if isinstance(ir, (LowLevelCall)):
-                            if ir.function_name in ["delegatecall", "callcode"]:
+                        if isinstance(ir, (LowLevelCall)) and ir.function_name in [
+                            "delegatecall",
+                            "callcode",
+                        ]:
+                            return False
+                        if isinstance(ir, SolidityCall):
+                            call_can_send_ether = ir.function in [
+                                SolidityFunction(
+                                    "delegatecall(uint256,uint256,uint256,uint256,uint256,uint256)"
+                                ),
+                                SolidityFunction(
+                                    "callcode(uint256,uint256,uint256,uint256,uint256,uint256,uint256)"
+                                ),
+                                SolidityFunction(
+                                    "call(uint256,uint256,uint256,uint256,uint256,uint256,uint256)"
+                                ),
+                            ]
+                            nonzero_call_value = call_can_send_ether and (
+                                not isinstance(ir.arguments[2], Constant)
+                                or ir.arguments[2].value != 0
+                            )
+                            if nonzero_call_value:
                                 return False
                         # If a new internal call or librarycall
                         # Add it to the list to explore

slither/detectors/variables/unchanged_state_variables.py

@@ -87,6 +87,8 @@ class UnchangedStateVariables:
     def detect(self) -> None:
         """Detect state variables that could be constant or immutable"""
         for c in self.compilation_unit.contracts_derived:
+            if c.is_signature_only():
+                continue
             variables = []
             functions = []
 

slither/detectors/variables/unused_state_variables.py

@@ -20,8 +20,6 @@ from slither.visitors.expression.export_values import ExportValues
 
 
 def detect_unused(contract: Contract) -> Optional[List[StateVariable]]:
-    if contract.is_signature_only():
-        return None
     # Get all the variables read in all the functions and modifiers
 
     all_functions = [
@@ -73,6 +71,8 @@ class UnusedStateVars(AbstractDetector):
         """Detect unused state variables"""
         results = []
         for c in self.compilation_unit.contracts_derived:
+            if c.is_signature_only():
+                continue
             unusedVars = detect_unused(c)
             if unusedVars:
                 for var in unusedVars:

slither/slithir/convert.py

@@ -1363,11 +1363,12 @@ def convert_to_pop(ir: HighLevelCall, node: "Node") -> List[Operation]:
     # TODO the following is equivalent to length.points_to = arr
     # Should it be removed?
     ir_length.lvalue.points_to = arr
-    # Note bytes is an ElementaryType not ArrayType so in that case we use ir.destination.type
+    # Note bytes is an ElementaryType not ArrayType and bytes1 should be returned
+    # since bytes is bytes1[] without padding between the elements
     # while in other cases such as uint256[] (ArrayType) we use ir.destination.type.type
     # in this way we will have the type always set to the corresponding ElementaryType
     element_to_delete.set_type(
-        ir.destination.type
+        ElementaryType("bytes1")
         if isinstance(ir.destination.type, ElementaryType)
         else ir.destination.type.type
     )
@@ -1583,7 +1584,9 @@ def _convert_to_structure_to_list(return_type: Type) -> List[Type]:
     # }
     if isinstance(return_type, (MappingType, ArrayType)):
         return []
-    return [return_type.type]
+
+    assert isinstance(return_type, (ElementaryType, UserDefinedType, TypeAlias))
+    return [return_type]
 
 
 def convert_type_of_high_and_internal_level_call(

slither/solc_parsing/declarations/function.py

@@ -660,6 +660,70 @@ class FunctionSolc(CallerContextExpression):
         link_underlying_nodes(node_condition, node_endDoWhile)
         return node_endDoWhile
 
+    # pylint: disable=no-self-use
+    def _construct_try_expression(self, externalCall: Dict, parameters_list: Dict) -> Dict:
+        # if the parameters are more than 1 we make the leftHandSide of the Assignment node
+        # a TupleExpression otherwise an Identifier
+
+        # case when there isn't returns(...)
+        # e.g. external call that doesn't have any return variable
+        if not parameters_list:
+            return externalCall
+
+        ret: Dict = {"nodeType": "Assignment", "operator": "=", "src": parameters_list["src"]}
+
+        parameters = parameters_list.get("parameters", None)
+
+        # if the name is "" it means the return variable is not used
+        if len(parameters) == 1:
+            if parameters[0]["name"] != "":
+                self._add_param(parameters[0])
+                ret["typeDescriptions"] = {
+                    "typeString": parameters[0]["typeName"]["typeDescriptions"]["typeString"]
+                }
+                leftHandSide = {
+                    "name": parameters[0]["name"],
+                    "nodeType": "Identifier",
+                    "src": parameters[0]["src"],
+                    "referencedDeclaration": parameters[0]["id"],
+                    "typeDescriptions": parameters[0]["typeDescriptions"],
+                }
+            else:
+                # we don't need an Assignment so we return only the external call
+                return externalCall
+        else:
+            ret["typeDescriptions"] = {"typeString": "tuple()"}
+            leftHandSide = {
+                "components": [],
+                "nodeType": "TupleExpression",
+                "src": parameters_list["src"],
+            }
+
+            for i, p in enumerate(parameters):
+                if p["name"] == "":
+                    continue
+
+                new_statement = {
+                    "nodeType": "VariableDefinitionStatement",
+                    "src": p["src"],
+                    "declarations": [p],
+                }
+                self._add_param_init_tuple(new_statement, i)
+
+                ident = {
+                    "name": p["name"],
+                    "nodeType": "Identifier",
+                    "src": p["src"],
+                    "referencedDeclaration": p["id"],
+                    "typeDescriptions": p["typeDescriptions"],
+                }
+                leftHandSide["components"].append(ident)
+
+        ret["leftHandSide"] = leftHandSide
+        ret["rightHandSide"] = externalCall
+
+        return ret
+
     def _parse_try_catch(self, statement: Dict, node: NodeSolc) -> NodeSolc:
         externalCall = statement.get("externalCall", None)
 
@@ -669,15 +733,28 @@ class FunctionSolc(CallerContextExpression):
             node.underlying_node.scope.is_checked, False, node.underlying_node.scope
         )
         new_node = self._new_node(NodeType.TRY, statement["src"], catch_scope)
-        new_node.add_unparsed_expression(externalCall)
+        clauses = statement.get("clauses", [])
+        # the first clause is the try scope
+        returned_variables = clauses[0].get("parameters", None)
+        constructed_try_expression = self._construct_try_expression(
+            externalCall, returned_variables
+        )
+        new_node.add_unparsed_expression(constructed_try_expression)
         link_underlying_nodes(node, new_node)
         node = new_node
 
-        for clause in statement.get("clauses", []):
-            self._parse_catch(clause, node)
+        for index, clause in enumerate(clauses):
+            # clauses after the first one are related to catch cases
+            # we set the parameters (e.g. data in this case. catch(string memory data) ...)
+            # to be initialized so they are not reported by the uninitialized-local-variables detector
+            if index >= 1:
+                self._parse_catch(clause, node, True)
+            else:
+                # the parameters for the try scope were already added in _construct_try_expression
+                self._parse_catch(clause, node, False)
         return node
 
-    def _parse_catch(self, statement: Dict, node: NodeSolc) -> NodeSolc:
+    def _parse_catch(self, statement: Dict, node: NodeSolc, add_param: bool) -> NodeSolc:
         block = statement.get("block", None)
 
         if block is None:
@@ -687,15 +764,16 @@ class FunctionSolc(CallerContextExpression):
         try_node = self._new_node(NodeType.CATCH, statement["src"], try_scope)
         link_underlying_nodes(node, try_node)
 
-        if self.is_compact_ast:
-            params = statement.get("parameters", None)
-        else:
-            params = statement[self.get_children("children")]
+        if add_param:
+            if self.is_compact_ast:
+                params = statement.get("parameters", None)
+            else:
+                params = statement[self.get_children("children")]
 
-        if params:
-            for param in params.get("parameters", []):
-                assert param[self.get_key()] == "VariableDeclaration"
-                self._add_param(param)
+            if params:
+                for param in params.get("parameters", []):
+                    assert param[self.get_key()] == "VariableDeclaration"
+                    self._add_param(param, True)
 
         return self._parse_statement(block, try_node, try_scope)
 
@@ -1162,7 +1240,7 @@ class FunctionSolc(CallerContextExpression):
                     visited.add(son)
                     self._fix_catch(son, end_node, visited)
 
-    def _add_param(self, param: Dict) -> LocalVariableSolc:
+    def _add_param(self, param: Dict, initialized: bool = False) -> LocalVariableSolc:
 
         local_var = LocalVariable()
         local_var.set_function(self._function)
@@ -1172,6 +1250,9 @@ class FunctionSolc(CallerContextExpression):
 
         local_var_parser.analyze(self)
 
+        if initialized:
+            local_var.initialized = True
+
         # see https://solidity.readthedocs.io/en/v0.4.24/types.html?highlight=storage%20location#data-location
         if local_var.location == "default":
             local_var.set_location("memory")
@@ -1179,6 +1260,17 @@ class FunctionSolc(CallerContextExpression):
         self._add_local_variable(local_var_parser)
         return local_var_parser
 
+    def _add_param_init_tuple(self, statement: Dict, index: int) -> LocalVariableInitFromTupleSolc:
+
+        local_var = LocalVariableInitFromTuple()
+        local_var.set_function(self._function)
+        local_var.set_offset(statement["src"], self._function.compilation_unit)
+
+        local_var_parser = LocalVariableInitFromTupleSolc(local_var, statement, index)
+
+        self._add_local_variable(local_var_parser)
+        return local_var_parser
+
     def _parse_params(self, params: Dict):
         assert params[self.get_key()] == "ParameterList"
 

tests/e2e/detectors/snapshots/detectors__detector_LockedEther_0_4_25_locked_ether_sol__0.txt

@@ -1,5 +1,10 @@
 Contract locking ether found:
-	Contract OnlyLocked (tests/e2e/detectors/test_data/locked-ether/0.4.25/locked_ether.sol#26) has payable functions:
+	Contract OnlyLocked (tests/e2e/detectors/test_data/locked-ether/0.4.25/locked_ether.sol#37) has payable functions:
+	 - Locked.receive() (tests/e2e/detectors/test_data/locked-ether/0.4.25/locked_ether.sol#4-6)
+	But does not have a function to withdraw the ether
+
+Contract locking ether found:
+	Contract UnlockedAssembly (tests/e2e/detectors/test_data/locked-ether/0.4.25/locked_ether.sol#27-35) has payable functions:
 	 - Locked.receive() (tests/e2e/detectors/test_data/locked-ether/0.4.25/locked_ether.sol#4-6)
 	But does not have a function to withdraw the ether
 

tests/e2e/detectors/snapshots/detectors__detector_LockedEther_0_5_16_locked_ether_sol__0.txt

@@ -1,5 +1,10 @@
 Contract locking ether found:
-	Contract OnlyLocked (tests/e2e/detectors/test_data/locked-ether/0.5.16/locked_ether.sol#26) has payable functions:
+	Contract OnlyLocked (tests/e2e/detectors/test_data/locked-ether/0.5.16/locked_ether.sol#37) has payable functions:
+	 - Locked.receive() (tests/e2e/detectors/test_data/locked-ether/0.5.16/locked_ether.sol#4-6)
+	But does not have a function to withdraw the ether
+
+Contract locking ether found:
+	Contract UnlockedAssembly (tests/e2e/detectors/test_data/locked-ether/0.5.16/locked_ether.sol#27-35) has payable functions:
 	 - Locked.receive() (tests/e2e/detectors/test_data/locked-ether/0.5.16/locked_ether.sol#4-6)
 	But does not have a function to withdraw the ether
 

tests/e2e/detectors/snapshots/detectors__detector_LockedEther_0_6_11_locked_ether_sol__0.txt

@@ -1,5 +1,5 @@
 Contract locking ether found:
-	Contract OnlyLocked (tests/e2e/detectors/test_data/locked-ether/0.6.11/locked_ether.sol#26) has payable functions:
+	Contract OnlyLocked (tests/e2e/detectors/test_data/locked-ether/0.6.11/locked_ether.sol#36) has payable functions:
 	 - Locked.receive_eth() (tests/e2e/detectors/test_data/locked-ether/0.6.11/locked_ether.sol#4-6)
 	But does not have a function to withdraw the ether
 

tests/e2e/detectors/snapshots/detectors__detector_LockedEther_0_7_6_locked_ether_sol__0.txt

@@ -1,5 +1,5 @@
 Contract locking ether found:
-	Contract OnlyLocked (tests/e2e/detectors/test_data/locked-ether/0.7.6/locked_ether.sol#26) has payable functions:
+	Contract OnlyLocked (tests/e2e/detectors/test_data/locked-ether/0.7.6/locked_ether.sol#36) has payable functions:
 	 - Locked.receive_eth() (tests/e2e/detectors/test_data/locked-ether/0.7.6/locked_ether.sol#4-6)
 	But does not have a function to withdraw the ether
 

tests/e2e/detectors/snapshots/detectors__detector_UninitializedLocalVars_0_6_11_uninitialized_local_variable_sol__0.txt

@@ -1,2 +1,2 @@
-Uninitialized.func().uint_not_init (tests/e2e/detectors/test_data/uninitialized-local/0.6.11/uninitialized_local_variable.sol#4) is a local variable never initialized
+Uninitialized.func().uint_not_init (tests/e2e/detectors/test_data/uninitialized-local/0.6.11/uninitialized_local_variable.sol#8) is a local variable never initialized
 

tests/e2e/detectors/snapshots/detectors__detector_UninitializedLocalVars_0_7_6_uninitialized_local_variable_sol__0.txt

@@ -1,2 +1,2 @@
-Uninitialized.func().uint_not_init (tests/e2e/detectors/test_data/uninitialized-local/0.7.6/uninitialized_local_variable.sol#4) is a local variable never initialized
+Uninitialized.func().uint_not_init (tests/e2e/detectors/test_data/uninitialized-local/0.7.6/uninitialized_local_variable.sol#8) is a local variable never initialized
 

tests/e2e/detectors/test_data/locked-ether/0.4.25/locked_ether.sol

@@ -23,4 +23,15 @@ contract Unlocked is Locked, Send{
 
 }
 
+// Still reported because solidity < 0.6.0 doesn't have assembly in the AST
+contract UnlockedAssembly is Locked{
+
+    function withdraw() public {
+        assembly {
+            let success := call(gas(), caller(),100,0,0,0,0)    
+        }
+    }
+
+}
+
 contract OnlyLocked is Locked{ }

tests/e2e/detectors/test_data/locked-ether/0.5.16/locked_ether.sol

@@ -23,4 +23,15 @@ contract Unlocked is Locked, Send{
 
 }
 
+// Still reported because solidity < 0.6.0 doesn't have assembly in the AST
+contract UnlockedAssembly is Locked{
+
+    function withdraw() public {
+        assembly {
+            let success := call(gas(), caller(),100,0,0,0,0)    
+        }
+    }
+
+}
+
 contract OnlyLocked is Locked{ }

tests/e2e/detectors/test_data/locked-ether/0.6.11/locked_ether.sol

@@ -23,4 +23,14 @@ contract Unlocked is Locked, Send{
 
 }
 
+contract UnlockedAssembly is Locked{
+
+    function withdraw() public {
+        assembly {
+            let success := call(gas(), caller(),100,0,0,0,0)    
+        }
+    }
+
+}
+
 contract OnlyLocked is Locked{ }

tests/e2e/detectors/test_data/locked-ether/0.7.6/locked_ether.sol

@@ -23,4 +23,14 @@ contract Unlocked is Locked, Send{
 
 }
 
+contract UnlockedAssembly is Locked{
+
+    function withdraw() public {
+        assembly {
+            let success := call(gas(), caller(),100,0,0,0,0)    
+        }
+    }
+
+}
+
 contract OnlyLocked is Locked{ }

tests/e2e/detectors/test_data/uninitialized-local/0.6.11/uninitialized_local_variable.sol

@@ -1,10 +1,22 @@
+interface I {
+    function a() external;
+}
+
 contract Uninitialized{
 
     function func() external returns(uint){
         uint uint_not_init;
         uint uint_init = 1;
         return uint_not_init + uint_init;
-    }    
+    }
+
+    function func_try_catch(I i) external returns(uint) {
+        try i.a() {
+            return 1;
+        } catch (bytes memory data) {
+            data;
+        }
+    }
 
     function noreportfor() public {
         for(uint i; i < 6; i++) { 

tests/e2e/detectors/test_data/uninitialized-local/0.7.6/uninitialized_local_variable.sol

@@ -1,10 +1,22 @@
+interface I {
+    function a() external;
+}
+
 contract Uninitialized{
 
     function func() external returns(uint){
         uint uint_not_init;
         uint uint_init = 1;
         return uint_not_init + uint_init;
-    }    
+    }
+
+    function func_try_catch(I i) external returns(uint) {
+        try i.a() {
+            return 1;
+        } catch (bytes memory data) {
+            data;
+        }
+    }
 
     function noreportfor() public {
         for(uint i; i < 6; i++) { 

tests/e2e/solc_parsing/test_ast_parsing.py

@@ -495,12 +495,9 @@ class TestASTParsing:
 
         actual = generate_output(sl)
 
-        try:
-            with open(expected, "r", encoding="utf8") as f:
-                expected = json.load(f)
-        except OSError:
-            pytest.xfail("the file for this test was not generated")
-            raise
+        assert os.path.isfile(expected), f"Expected file {expected} does not exist"
+        with open(expected, "r", encoding="utf8") as f:
+            expected = json.load(f)
 
         diff = DeepDiff(expected, actual, ignore_order=True, verbose_level=2, view="tree")
         if diff:

tests/e2e/solc_parsing/test_data/expected/yul-top-level-0.8.0.sol-0.8.0-compact.json

@@ -0,0 +1,5 @@
+{
+  "Test": {
+    "test()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: EXPRESSION 1\n\"];\n}\n"
+  }
+}