Merge pull request #1861 from crytic/dev-unused-return-tuple

Improve tuple analysis for unused-return detector

slither/detectors/operations/unused_return_values.py

@@ -3,7 +3,7 @@ Module detecting unused return values from external calls
 """
 from typing import List
 
-from slither.core.cfg.node import Node
+from slither.core.cfg.node import Node, NodeType
 from slither.core.declarations import Function
 from slither.core.declarations.function_contract import FunctionContract
 from slither.core.variables.state_variable import StateVariable
@@ -12,8 +12,8 @@ from slither.detectors.abstract_detector import (
     DetectorClassification,
     DETECTOR_INFO,
 )
-from slither.slithir.operations import HighLevelCall
-from slither.slithir.operations.operation import Operation
+from slither.slithir.operations import HighLevelCall, Assignment, Unpack, Operation
+from slither.slithir.variables import TupleVariable
 from slither.utils.output import Output
 
 
@@ -50,13 +50,18 @@ contract MyConc{
     WIKI_RECOMMENDATION = "Ensure that all the return values of the function calls are used."
 
     def _is_instance(self, ir: Operation) -> bool:  # pylint: disable=no-self-use
-        return isinstance(ir, HighLevelCall) and (
-            (
-                isinstance(ir.function, Function)
-                and ir.function.solidity_signature
-                not in ["transfer(address,uint256)", "transferFrom(address,address,uint256)"]
+        return (
+            isinstance(ir, HighLevelCall)
+            and (
+                (
+                    isinstance(ir.function, Function)
+                    and ir.function.solidity_signature
+                    not in ["transfer(address,uint256)", "transferFrom(address,address,uint256)"]
+                )
+                or not isinstance(ir.function, Function)
             )
-            or not isinstance(ir.function, Function)
+            or ir.node.type == NodeType.TRY
+            and isinstance(ir, (Assignment, Unpack))
         )
 
     def detect_unused_return_values(
@@ -71,18 +76,27 @@ contract MyConc{
         """
         values_returned = []
         nodes_origin = {}
+        # pylint: disable=too-many-nested-blocks
         for n in f.nodes:
             for ir in n.irs:
                 if self._is_instance(ir):
                     # if a return value is stored in a state variable, it's ok
                     if ir.lvalue and not isinstance(ir.lvalue, StateVariable):
-                        values_returned.append(ir.lvalue)
+                        values_returned.append((ir.lvalue, None))
                         nodes_origin[ir.lvalue] = ir
+                        if isinstance(ir.lvalue, TupleVariable):
+                            # we iterate the number of elements the tuple has
+                            # and add a (variable, index) in values_returned for each of them
+                            for index in range(len(ir.lvalue.type)):
+                                values_returned.append((ir.lvalue, index))
                 for read in ir.read:
-                    if read in values_returned:
-                        values_returned.remove(read)
-
-        return [nodes_origin[value].node for value in values_returned]
+                    remove = (read, ir.index) if isinstance(ir, Unpack) else (read, None)
+                    if remove in values_returned:
+                        # this is needed to remove the tuple variable when the first time one of its element is used
+                        if remove[1] is not None and (remove[0], None) in values_returned:
+                            values_returned.remove((remove[0], None))
+                        values_returned.remove(remove)
+        return [nodes_origin[value].node for (value, _) in values_returned]
 
     def _detect(self) -> List[Output]:
         """Detect high level calls which return a value that are never used"""

tests/e2e/detectors/snapshots/detectors__detector_UnusedReturnValues_0_4_25_unused_return_sol__0.txt

@@ -1,4 +1,8 @@
-User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.4.25/unused_return.sol#17-29) ignores return value by t.f() (tests/e2e/detectors/test_data/unused-return/0.4.25/unused_return.sol#18)
+User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.4.25/unused_return.sol#18-37) ignores return value by t.g() (tests/e2e/detectors/test_data/unused-return/0.4.25/unused_return.sol#31)
 
-User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.4.25/unused_return.sol#17-29) ignores return value by a.add(0) (tests/e2e/detectors/test_data/unused-return/0.4.25/unused_return.sol#22)
+User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.4.25/unused_return.sol#18-37) ignores return value by t.f() (tests/e2e/detectors/test_data/unused-return/0.4.25/unused_return.sol#19)
+
+User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.4.25/unused_return.sol#18-37) ignores return value by a.add(0) (tests/e2e/detectors/test_data/unused-return/0.4.25/unused_return.sol#23)
+
+User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.4.25/unused_return.sol#18-37) ignores return value by (e) = t.g() (tests/e2e/detectors/test_data/unused-return/0.4.25/unused_return.sol#36)
 

tests/e2e/detectors/snapshots/detectors__detector_UnusedReturnValues_0_5_16_unused_return_sol__0.txt

@@ -1,4 +1,8 @@
-User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.5.16/unused_return.sol#17-29) ignores return value by t.f() (tests/e2e/detectors/test_data/unused-return/0.5.16/unused_return.sol#18)
+User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.5.16/unused_return.sol#18-37) ignores return value by (e) = t.g() (tests/e2e/detectors/test_data/unused-return/0.5.16/unused_return.sol#36)
 
-User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.5.16/unused_return.sol#17-29) ignores return value by a.add(0) (tests/e2e/detectors/test_data/unused-return/0.5.16/unused_return.sol#22)
+User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.5.16/unused_return.sol#18-37) ignores return value by a.add(0) (tests/e2e/detectors/test_data/unused-return/0.5.16/unused_return.sol#23)
+
+User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.5.16/unused_return.sol#18-37) ignores return value by t.g() (tests/e2e/detectors/test_data/unused-return/0.5.16/unused_return.sol#31)
+
+User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.5.16/unused_return.sol#18-37) ignores return value by t.f() (tests/e2e/detectors/test_data/unused-return/0.5.16/unused_return.sol#19)
 

tests/e2e/detectors/snapshots/detectors__detector_UnusedReturnValues_0_6_11_unused_return_sol__0.txt

@@ -1,4 +1,8 @@
-User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.6.11/unused_return.sol#17-29) ignores return value by a.add(0) (tests/e2e/detectors/test_data/unused-return/0.6.11/unused_return.sol#22)
+User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.6.11/unused_return.sol#18-37) ignores return value by t.f() (tests/e2e/detectors/test_data/unused-return/0.6.11/unused_return.sol#19)
 
-User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.6.11/unused_return.sol#17-29) ignores return value by t.f() (tests/e2e/detectors/test_data/unused-return/0.6.11/unused_return.sol#18)
+User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.6.11/unused_return.sol#18-37) ignores return value by a.add(0) (tests/e2e/detectors/test_data/unused-return/0.6.11/unused_return.sol#23)
+
+User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.6.11/unused_return.sol#18-37) ignores return value by t.g() (tests/e2e/detectors/test_data/unused-return/0.6.11/unused_return.sol#31)
+
+User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.6.11/unused_return.sol#18-37) ignores return value by (e) = t.g() (tests/e2e/detectors/test_data/unused-return/0.6.11/unused_return.sol#36)
 

tests/e2e/detectors/snapshots/detectors__detector_UnusedReturnValues_0_7_6_unused_return_sol__0.txt

@@ -1,4 +1,8 @@
-User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.7.6/unused_return.sol#17-29) ignores return value by a.add(0) (tests/e2e/detectors/test_data/unused-return/0.7.6/unused_return.sol#22)
+User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.7.6/unused_return.sol#18-37) ignores return value by t.g() (tests/e2e/detectors/test_data/unused-return/0.7.6/unused_return.sol#31)
 
-User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.7.6/unused_return.sol#17-29) ignores return value by t.f() (tests/e2e/detectors/test_data/unused-return/0.7.6/unused_return.sol#18)
+User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.7.6/unused_return.sol#18-37) ignores return value by a.add(0) (tests/e2e/detectors/test_data/unused-return/0.7.6/unused_return.sol#23)
+
+User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.7.6/unused_return.sol#18-37) ignores return value by t.f() (tests/e2e/detectors/test_data/unused-return/0.7.6/unused_return.sol#19)
+
+User.test(Target) (tests/e2e/detectors/test_data/unused-return/0.7.6/unused_return.sol#18-37) ignores return value by (e) = t.g() (tests/e2e/detectors/test_data/unused-return/0.7.6/unused_return.sol#36)
 

tests/e2e/detectors/test_data/unused-return/0.4.25/unused_return.sol

@@ -8,6 +8,7 @@ library SafeMath{
 
 contract Target{
     function f() public returns(uint);
+    function g() public returns(uint, uint);
 }
 
 contract User{
@@ -26,5 +27,12 @@ contract User{
         // As the value returned by the call is stored
         // (unused local variable should be another issue) 
         uint b = a.add(1);
+
+        t.g();
+
+        (uint c, uint d) = t.g();
+
+        // Detected as unused return
+        (uint e,) = t.g();
     }
 }

tests/e2e/detectors/test_data/unused-return/0.5.16/unused_return.sol

@@ -8,6 +8,7 @@ library SafeMath{
 
 contract Target{
     function f() public returns(uint);
+    function g() public returns(uint, uint);
 }
 
 contract User{
@@ -26,5 +27,12 @@ contract User{
         // As the value returned by the call is stored
         // (unused local variable should be another issue) 
         uint b = a.add(1);
+
+        t.g();
+
+        (uint c, uint d) = t.g();
+
+        // Detected as unused return
+        (uint e,) = t.g();
     }
 }

tests/e2e/detectors/test_data/unused-return/0.6.11/unused_return.sol

@@ -8,6 +8,7 @@ library SafeMath{
 
 abstract contract Target{
     function f() public virtual returns(uint);
+    function g() public virtual returns(uint, uint);
 }
 
 contract User{
@@ -26,5 +27,12 @@ contract User{
         // As the value returned by the call is stored
         // (unused local variable should be another issue) 
         uint b = a.add(1);
+
+        t.g();
+
+        (uint c, uint d) = t.g();
+
+        // Detected as unused return
+        (uint e,) = t.g();
     }
 }

tests/e2e/detectors/test_data/unused-return/0.7.6/unused_return.sol

@@ -8,6 +8,7 @@ library SafeMath{
 
 abstract contract Target{
     function f() public virtual returns(uint);
+    function g() public virtual returns(uint, uint);
 }
 
 contract User{
@@ -26,5 +27,12 @@ contract User{
         // As the value returned by the call is stored
         // (unused local variable should be another issue) 
         uint b = a.add(1);
+
+        t.g();
+
+        (uint c, uint d) = t.g();
+
+        // Detected as unused return
+        (uint e,) = t.g();
     }
 }