Merge pull request #2019 from crytic/fix/issue-2017

fix(cache-array-length): handle when `HighLevelCall` is a `StateVariable`

slither/detectors/operations/cache_array_length.py

@@ -4,7 +4,6 @@ from slither.core.cfg.node import Node, NodeType
 from slither.core.declarations import Function
 from slither.core.expressions import BinaryOperation, Identifier, MemberAccess, UnaryOperation
 from slither.core.solidity_types import ArrayType
-from slither.core.source_mapping.source_mapping import SourceMapping
 from slither.core.variables import StateVariable
 from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
 from slither.slithir.operations import Length, Delete, HighLevelCall
@@ -134,7 +133,12 @@ contract C
                         and op.expression.expression.value == array
                     ):
                         return True
-                if isinstance(op, HighLevelCall) and not op.function.view and not op.function.pure:
+                if (
+                    isinstance(op, HighLevelCall)
+                    and isinstance(op.function, Function)
+                    and not op.function.view
+                    and not op.function.pure
+                ):
                     return True
 
         for son in node.sons:
@@ -144,7 +148,7 @@ contract C
         return False
 
     @staticmethod
-    def _handle_loops(nodes: List[Node], non_optimal_array_len_usages: List[SourceMapping]) -> None:
+    def _handle_loops(nodes: List[Node], non_optimal_array_len_usages: List[Node]) -> None:
         """
         For each loop, checks if it has a comparison with `length` array member and, if it has, checks whether that
         array size could potentially change in that loop.
@@ -180,21 +184,21 @@ contract C
                     non_optimal_array_len_usages.append(if_node)
 
     @staticmethod
-    def _get_non_optimal_array_len_usages_for_function(f: Function) -> List[SourceMapping]:
+    def _get_non_optimal_array_len_usages_for_function(f: Function) -> List[Node]:
         """
         Finds non-optimal usages of array length in loop conditions in a given function.
         """
-        non_optimal_array_len_usages: List[SourceMapping] = []
+        non_optimal_array_len_usages: List[Node] = []
         CacheArrayLength._handle_loops(f.nodes, non_optimal_array_len_usages)
 
         return non_optimal_array_len_usages
 
     @staticmethod
-    def _get_non_optimal_array_len_usages(functions: List[Function]) -> List[SourceMapping]:
+    def _get_non_optimal_array_len_usages(functions: List[Function]) -> List[Node]:
         """
         Finds non-optimal usages of array length in loop conditions in given functions.
         """
-        non_optimal_array_len_usages: List[SourceMapping] = []
+        non_optimal_array_len_usages: List[Node] = []
 
         for f in functions:
             non_optimal_array_len_usages += (
@@ -211,8 +215,9 @@ contract C
         )
         for usage in non_optimal_array_len_usages:
             info = [
-                "Loop condition at ",
+                "Loop condition ",
-                usage,
+                f"`{usage.source_mapping.content}` ",
+                f"({usage.source_mapping}) ",
                 "should use cached array length instead of referencing `length` member "
                 "of the storage array.\n ",
             ]

tests/e2e/detectors/snapshots/detectors__detector_CacheArrayLength_0_8_17_CacheArrayLength_sol__0.txt

@@ -1,18 +1,20 @@
-Loop condition at i < array.length (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#36) should use cached array length instead of referencing `length` member of the storage array.
+Loop condition `j < array.length` (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#109) should use cached array length instead of referencing `length` member of the storage array.
  
-Loop condition at i_scope_22 < array.length (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#166) should use cached array length instead of referencing `length` member of the storage array.
+Loop condition `i < array.length` (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#161) should use cached array length instead of referencing `length` member of the storage array.
  
-Loop condition at j_scope_11 < array.length (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#108) should use cached array length instead of referencing `length` member of the storage array.
+Loop condition `i < array.length` (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#172) should use cached array length instead of referencing `length` member of the storage array.
  
-Loop condition at i_scope_6 < array.length (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#79) should use cached array length instead of referencing `length` member of the storage array.
+Loop condition `j < array.length` (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#126) should use cached array length instead of referencing `length` member of the storage array.
  
-Loop condition at i_scope_21 < array.length (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#160) should use cached array length instead of referencing `length` member of the storage array.
+Loop condition `k < array2.length` (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#133) should use cached array length instead of referencing `length` member of the storage array.
  
-Loop condition at k_scope_9 < array2.length (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#98) should use cached array length instead of referencing `length` member of the storage array.
+Loop condition `i < array.length` (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#68) should use cached array length instead of referencing `length` member of the storage array.
  
-Loop condition at k_scope_17 < array2.length (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#132) should use cached array length instead of referencing `length` member of the storage array.
+Loop condition `k < array2.length` (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#99) should use cached array length instead of referencing `length` member of the storage array.
  
-Loop condition at j_scope_15 < array.length (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#125) should use cached array length instead of referencing `length` member of the storage array.
+Loop condition `i < array.length` (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#167) should use cached array length instead of referencing `length` member of the storage array.
  
-Loop condition at i_scope_4 < array.length (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#67) should use cached array length instead of referencing `length` member of the storage array.
+Loop condition `i < array.length` (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#37) should use cached array length instead of referencing `length` member of the storage array.
+ 
+Loop condition `i < array.length` (tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#80) should use cached array length instead of referencing `length` member of the storage array.
  

tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol

@@ -9,6 +9,7 @@ contract CacheArrayLength
 
     S[] array;
     S[] array2;
+    uint public x;
 
     function h() external
     {
@@ -167,5 +168,10 @@ contract CacheArrayLength
         {
             this.h_view();
         }
+        // array not modified and it cannot be changed in a function call since x is a public state variable
+        for (uint i = 0; i < array.length; i++) // warning should appear
+        {
+            this.x();
+        }
     }   
 }