Merge pull request #2326 from crytic/fix/model-named-returns

Fix/model named returns:

Resolves issues #1450 and #1719, and includes the previously failing test from #1818, which now passes.

Creates an artificial return node iff a function has a named return variable declared in its signature. Finds all leaf nodes in the CFG which are not return nodes, and links them to the artificial return node. The return node is given an Identifier expression with a referencedDeclaration pointing to the variable declaration in the signature.

Once this return node is in place and correctly linked, subsequent analyses resolve the data dependency issue for named variables without any further changes, i.e., to Slither's IR generation. The goal being to implement the simplest solution to that problem.

slither/solc_parsing/declarations/function.py

@@ -68,6 +68,7 @@ def __init__(
         else:
             self._function.name = function_data["attributes"][self.get_key()]
         self._functionNotParsed = function_data
+        self._returnsNotParsed: List[dict] = []
         self._params_was_analyzed = False
         self._content_was_analyzed = False
 
@@ -280,6 +281,7 @@ def analyze_content(self) -> None:
 
         if self.is_compact_ast:
             body = self._functionNotParsed.get("body", None)
+            return_params = self._functionNotParsed.get("returnParameters", None)
 
             if body and body[self.get_key()] == "Block":
                 self._function.is_implemented = True
@@ -290,6 +292,7 @@ def analyze_content(self) -> None:
 
         else:
             children = self._functionNotParsed[self.get_children("children")]
+            return_params = children[1]
             self._function.is_implemented = False
             for child in children[2:]:
                 if child[self.get_key()] == "Block":
@@ -315,6 +318,9 @@ def analyze_content(self) -> None:
 
         self._remove_alone_endif()
 
+        if return_params:
+            self._fix_implicit_return(return_params)
+
         if self._function.entry_point:
             self._update_reachability(self._function.entry_point)
 
@@ -1229,6 +1235,13 @@ def _fix_continue_node(self, node: Node) -> None:
         node.set_sons([if_loop_node])
         if_loop_node.add_father(node)
 
+    # endregion
+    ###################################################################################
+    ###################################################################################
+    # region Try-Catch
+    ###################################################################################
+    ###################################################################################
+
     def _fix_try(self, node: Node) -> None:
         end_node = next((son for son in node.sons if son.type != NodeType.CATCH), None)
         if end_node:
@@ -1245,6 +1258,13 @@ def _fix_catch(self, node: Node, end_node: Node, visited: Set[Node]) -> None:
                     visited.add(son)
                     self._fix_catch(son, end_node, visited)
 
+    # endregion
+    ###################################################################################
+    ###################################################################################
+    # region Params, Returns, Modifiers
+    ###################################################################################
+    ###################################################################################
+
     def _add_param(self, param: Dict, initialized: bool = False) -> LocalVariableSolc:
 
         local_var = LocalVariable()
@@ -1298,11 +1318,11 @@ def _parse_returns(self, returns: Dict):
         self._function.returns_src().set_offset(returns["src"], self._function.compilation_unit)
 
         if self.is_compact_ast:
-            returns = returns["parameters"]
+            self._returnsNotParsed = returns["parameters"]
         else:
-            returns = returns[self.get_children("children")]
+            self._returnsNotParsed = returns[self.get_children("children")]
 
-        for ret in returns:
+        for ret in self._returnsNotParsed:
             assert ret[self.get_key()] == "VariableDeclaration"
             local_var = self._add_param(ret)
             self._function.add_return(local_var.underlying_variable)
@@ -1356,6 +1376,138 @@ def _parse_modifier(self, modifier: Dict):
                     )
                 )
 
+    def _fix_implicit_return(self, return_params: Dict) -> None:
+        """
+        Creates an artificial return node iff a function has a named return variable declared in its signature.
+        Finds all leaf nodes in the CFG which are not return nodes, and links them to the artificial return node.
+        """
+        does_not_have_return_params = len(self.underlying_function.returns) == 0
+        does_not_have_named_returns = all(
+            ret.name == "" for ret in self.underlying_function.returns
+        )
+        not_implemented = not self._function.is_implemented
+
+        if does_not_have_return_params or does_not_have_named_returns or not_implemented:
+            return
+
+        return_node = self._new_node(
+            NodeType.RETURN, return_params["src"], self.underlying_function
+        )
+        for node, node_solc in self._node_to_nodesolc.items():
+            if len(node.sons) == 0 and node.type not in [NodeType.RETURN, NodeType.THROW]:
+                link_underlying_nodes(node_solc, return_node)
+
+        for _, yul_block in self._node_to_yulobject.items():
+            for yul_node in yul_block.nodes:
+                node = yul_node.underlying_node
+                if len(node.sons) == 0 and node.type not in [NodeType.RETURN, NodeType.THROW]:
+                    link_underlying_nodes(yul_node, return_node)
+
+        if self.is_compact_ast:
+            self._add_return_exp_compact(return_node, return_params)
+        else:
+            self._add_return_exp_legacy(return_node, return_params)
+
+        return_node.analyze_expressions(self)
+
+    def _add_return_exp_compact(self, return_node: NodeSolc, return_params: Dict) -> None:
+        if len(self.underlying_function.returns) == 1:
+            return_arg = self.underlying_function.returns[0]
+            if return_arg.name != "":
+                (refId, refSrc, refType) = next(
+                    (ret["id"], ret["src"], ret["typeDescriptions"])
+                    for ret in self._returnsNotParsed
+                    if ret["name"] == return_arg.name
+                )
+                return_node.add_unparsed_expression(
+                    {
+                        "name": return_arg.name,
+                        "nodeType": "Identifier",
+                        "overloadedDeclarations": [],
+                        "referencedDeclaration": refId,
+                        "src": refSrc,
+                        "typeDescriptions": refType,
+                    }
+                )
+        else:
+            expression = {
+                "components": [],
+                "isConstant": False,
+                "isInlineArray": False,
+                "isLValue": False,
+                "isPure": False,
+                "lValueRequested": False,
+                "nodeType": "TupleExpression",
+                "src": return_params["src"],
+                "typeDescriptions": {},
+            }
+            type_ids = []
+            type_strs = []
+            for return_arg in self.underlying_function.returns:
+                # For each named return variable, we add an identifier to the tuple.
+                if return_arg.name != "":
+                    (refId, refSrc, refType) = next(
+                        (ret["id"], ret["src"], ret["typeDescriptions"])
+                        for ret in self._returnsNotParsed
+                        if ret["name"] == return_arg.name
+                    )
+                    type_ids.append(refType["typeIdentifier"])
+                    type_strs.append(refType["typeString"])
+                    expression["components"].append(
+                        {
+                            "name": return_arg.name,
+                            "nodeType": "Identifier",
+                            "overloadedDeclarations": [],
+                            "referencedDeclaration": refId,
+                            "src": refSrc,
+                            "typeDescriptions": refType,
+                        }
+                    )
+            expression["typeDescriptions"]["typeIdentifier"] = (
+                "t_tuple$_" + "_$_".join(type_ids) + "_$"
+            )
+            expression["typeDescriptions"]["typeString"] = "tuple(" + ",".join(type_strs) + ")"
+            return_node.add_unparsed_expression(expression)
+
+    def _add_return_exp_legacy(self, return_node: NodeSolc, return_params: Dict) -> None:
+        if len(self.underlying_function.returns) == 1:
+            return_arg = self.underlying_function.returns[0]
+            if return_arg.name != "":
+                (refSrc, refType) = next(
+                    (ret["src"], ret["attributes"]["type"])
+                    for ret in self._returnsNotParsed
+                    if ret["attributes"]["name"] == return_arg.name
+                )
+                return_node.add_unparsed_expression(
+                    {
+                        "attributes": {"type": refType, "value": return_arg.name},
+                        "name": "Identifier",
+                        "src": refSrc,
+                    }
+                )
+        else:
+            expression = {
+                "children": [],
+                "name": "TupleExpression",
+                "src": return_params["src"],
+            }
+            for return_arg in self.underlying_function.returns:
+                # For each named return variable, we add an identifier to the tuple.
+                if return_arg.name != "":
+                    (refSrc, refType) = next(
+                        (ret["src"], ret["attributes"]["type"])
+                        for ret in self._returnsNotParsed
+                        if ret["attributes"]["name"] == return_arg.name
+                    )
+                    expression["children"].append(
+                        {
+                            "attributes": {"type": refType, "value": return_arg.name},
+                            "name": "Identifier",
+                            "src": refSrc,
+                        }
+                    )
+            return_node.add_unparsed_expression(expression)
+
     # endregion
     ###################################################################################
     ###################################################################################

slither/solc_parsing/yul/parse_yul.py

@@ -321,6 +321,10 @@ def entrypoint(self) -> YulNode:
     def function(self) -> Function:
         return self._parent_func
 
+    @property
+    def nodes(self) -> List[YulNode]:
+        return self._nodes
+
     def new_node(self, node_type: NodeType, src: Union[str, Dict]) -> YulNode:
         if self._parent_func:
             node = self._parent_func.new_node(node_type, src, self.node_scope)

slither/utils/upgradeability.py

@@ -60,7 +60,6 @@ def compare(
     List[Function],
     List[Function],
     List[Function],
-    List[TaintedExternalContract],
 ]:
     """
     Compares two versions of a contract. Most useful for upgradeable (logic) contracts,
@@ -392,7 +391,7 @@ def get_proxy_implementation_var(proxy: Contract) -> Optional[Variable]:
         try:
             delegate = next(var for var in dependencies if isinstance(var, StateVariable))
         except StopIteration:
-            # TODO: Handle cases where get_dependencies doesn't return any state variables.
+            # TODO: Handle case where get_dependencies does not return any state variables.
             return delegate
     return delegate
 

tests/conftest.py

@@ -61,7 +61,7 @@ def inner(version):
 @pytest.fixture
 def slither_from_solidity_source(solc_binary_path):
     @contextmanager
-    def inner(source_code: str, solc_version: str = "0.8.19"):
+    def inner(source_code: str, solc_version: str = "0.8.19", legacy: bool = False):
         """Yields a Slither instance using source_code string and solc_version.
         Creates a temporary file and compiles with solc_version.
         """
@@ -72,7 +72,7 @@ def inner(source_code: str, solc_version: str = "0.8.19"):
                 fname = f.name
                 f.write(source_code)
             solc_path = solc_binary_path(solc_version)
-            yield Slither(fname, solc=solc_path)
+            yield Slither(fname, solc=solc_path, force_legacy=legacy)
         finally:
             Path(fname).unlink()
 

tests/e2e/solc_parsing/test_ast_parsing.py

@@ -631,7 +631,7 @@ def _generate_compile(test_item: Test, skip_existing=False):
             "To re-generate all the json artifacts run\n\tpython tests/test_ast_parsing.py --overwrite"
         )
         print("To compile json artifacts run\n\tpython tests/test_ast_parsing.py --compile")
-        print("\tThis will overwrite the previous json files")
+        print("\tThis will overwrite the previous json files.")
     elif sys.argv[1] == "--generate":
         for next_test in ALL_TESTS:
             _generate_test(next_test, skip_existing=True)

tests/e2e/solc_parsing/test_data/expected/custom_error-0.8.4.sol-0.8.10-compact.json

@@ -20,6 +20,6 @@
   "ContractArgCustomError": {
     "f()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: EXPRESSION 1\n\"];\n}\n",
     "g()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: NEW VARIABLE 1\n\"];\n1->2;\n2[label=\"Node Type: IF 2\n\"];\n2->3[label=\"True\"];\n2->4[label=\"False\"];\n3[label=\"Node Type: EXPRESSION 3\n\"];\n3->4;\n4[label=\"Node Type: END_IF 4\n\"];\n}\n",
-    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n}\n"
+    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: RETURN 1\n\"];\n}\n"
   }
 }

tests/e2e/solc_parsing/test_data/expected/custom_error-0.8.4.sol-0.8.11-compact.json

@@ -20,6 +20,6 @@
   "ContractArgCustomError": {
     "f()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: EXPRESSION 1\n\"];\n}\n",
     "g()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: NEW VARIABLE 1\n\"];\n1->2;\n2[label=\"Node Type: IF 2\n\"];\n2->3[label=\"True\"];\n2->4[label=\"False\"];\n3[label=\"Node Type: EXPRESSION 3\n\"];\n3->4;\n4[label=\"Node Type: END_IF 4\n\"];\n}\n",
-    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n}\n"
+    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: RETURN 1\n\"];\n}\n"
   }
 }

tests/e2e/solc_parsing/test_data/expected/custom_error-0.8.4.sol-0.8.12-compact.json

@@ -20,6 +20,6 @@
   "ContractArgCustomError": {
     "f()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: EXPRESSION 1\n\"];\n}\n",
     "g()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: NEW VARIABLE 1\n\"];\n1->2;\n2[label=\"Node Type: IF 2\n\"];\n2->3[label=\"True\"];\n2->4[label=\"False\"];\n3[label=\"Node Type: EXPRESSION 3\n\"];\n3->4;\n4[label=\"Node Type: END_IF 4\n\"];\n}\n",
-    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n}\n"
+    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: RETURN 1\n\"];\n}\n"
   }
 }

tests/e2e/solc_parsing/test_data/expected/custom_error-0.8.4.sol-0.8.13-compact.json

@@ -20,6 +20,6 @@
   "ContractArgCustomError": {
     "f()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: EXPRESSION 1\n\"];\n}\n",
     "g()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: NEW VARIABLE 1\n\"];\n1->2;\n2[label=\"Node Type: IF 2\n\"];\n2->3[label=\"True\"];\n2->4[label=\"False\"];\n3[label=\"Node Type: EXPRESSION 3\n\"];\n3->4;\n4[label=\"Node Type: END_IF 4\n\"];\n}\n",
-    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n}\n"
+    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: RETURN 1\n\"];\n}\n"
   }
 }

tests/e2e/solc_parsing/test_data/expected/custom_error-0.8.4.sol-0.8.14-compact.json

@@ -20,6 +20,6 @@
   "ContractArgCustomError": {
     "f()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: EXPRESSION 1\n\"];\n}\n",
     "g()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: NEW VARIABLE 1\n\"];\n1->2;\n2[label=\"Node Type: IF 2\n\"];\n2->3[label=\"True\"];\n2->4[label=\"False\"];\n3[label=\"Node Type: EXPRESSION 3\n\"];\n3->4;\n4[label=\"Node Type: END_IF 4\n\"];\n}\n",
-    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n}\n"
+    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: RETURN 1\n\"];\n}\n"
   }
 }

tests/e2e/solc_parsing/test_data/expected/custom_error-0.8.4.sol-0.8.15-compact.json

@@ -20,6 +20,6 @@
   "ContractArgCustomError": {
     "f()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: EXPRESSION 1\n\"];\n}\n",
     "g()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: NEW VARIABLE 1\n\"];\n1->2;\n2[label=\"Node Type: IF 2\n\"];\n2->3[label=\"True\"];\n2->4[label=\"False\"];\n3[label=\"Node Type: EXPRESSION 3\n\"];\n3->4;\n4[label=\"Node Type: END_IF 4\n\"];\n}\n",
-    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n}\n"
+    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: RETURN 1\n\"];\n}\n"
   }
 }

tests/e2e/solc_parsing/test_data/expected/custom_error-0.8.4.sol-0.8.4-compact.json

@@ -20,6 +20,6 @@
   "ContractArgCustomError": {
     "f()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: EXPRESSION 1\n\"];\n}\n",
     "g()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: NEW VARIABLE 1\n\"];\n1->2;\n2[label=\"Node Type: IF 2\n\"];\n2->3[label=\"True\"];\n2->4[label=\"False\"];\n3[label=\"Node Type: EXPRESSION 3\n\"];\n3->4;\n4[label=\"Node Type: END_IF 4\n\"];\n}\n",
-    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n}\n"
+    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: RETURN 1\n\"];\n}\n"
   }
 }

tests/e2e/solc_parsing/test_data/expected/custom_error-0.8.4.sol-0.8.5-compact.json

@@ -20,6 +20,6 @@
   "ContractArgCustomError": {
     "f()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: EXPRESSION 1\n\"];\n}\n",
     "g()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: NEW VARIABLE 1\n\"];\n1->2;\n2[label=\"Node Type: IF 2\n\"];\n2->3[label=\"True\"];\n2->4[label=\"False\"];\n3[label=\"Node Type: EXPRESSION 3\n\"];\n3->4;\n4[label=\"Node Type: END_IF 4\n\"];\n}\n",
-    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n}\n"
+    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: RETURN 1\n\"];\n}\n"
   }
 }

tests/e2e/solc_parsing/test_data/expected/custom_error-0.8.4.sol-0.8.6-compact.json

@@ -20,6 +20,6 @@
   "ContractArgCustomError": {
     "f()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: EXPRESSION 1\n\"];\n}\n",
     "g()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: NEW VARIABLE 1\n\"];\n1->2;\n2[label=\"Node Type: IF 2\n\"];\n2->3[label=\"True\"];\n2->4[label=\"False\"];\n3[label=\"Node Type: EXPRESSION 3\n\"];\n3->4;\n4[label=\"Node Type: END_IF 4\n\"];\n}\n",
-    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n}\n"
+    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: RETURN 1\n\"];\n}\n"
   }
 }

tests/e2e/solc_parsing/test_data/expected/custom_error-0.8.4.sol-0.8.7-compact.json

@@ -20,6 +20,6 @@
   "ContractArgCustomError": {
     "f()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: EXPRESSION 1\n\"];\n}\n",
     "g()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: NEW VARIABLE 1\n\"];\n1->2;\n2[label=\"Node Type: IF 2\n\"];\n2->3[label=\"True\"];\n2->4[label=\"False\"];\n3[label=\"Node Type: EXPRESSION 3\n\"];\n3->4;\n4[label=\"Node Type: END_IF 4\n\"];\n}\n",
-    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n}\n"
+    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: RETURN 1\n\"];\n}\n"
   }
 }

tests/e2e/solc_parsing/test_data/expected/custom_error-0.8.4.sol-0.8.8-compact.json

@@ -20,6 +20,6 @@
   "ContractArgCustomError": {
     "f()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: EXPRESSION 1\n\"];\n}\n",
     "g()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: NEW VARIABLE 1\n\"];\n1->2;\n2[label=\"Node Type: IF 2\n\"];\n2->3[label=\"True\"];\n2->4[label=\"False\"];\n3[label=\"Node Type: EXPRESSION 3\n\"];\n3->4;\n4[label=\"Node Type: END_IF 4\n\"];\n}\n",
-    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n}\n"
+    "h()": "digraph{\n0[label=\"Node Type: ENTRY_POINT 0\n\"];\n0->1;\n1[label=\"Node Type: RETURN 1\n\"];\n}\n"
   }
 }