Merge pull request #1694 from bart1e/detector_for_array_len_caching

Detector for array length caching added

slither/detectors/all_detectors.py

@@ -89,5 +89,6 @@ from .functions.protected_variable import ProtectedVariables
 from .functions.permit_domain_signature_collision import DomainSeparatorCollision
 from .functions.codex import Codex
 from .functions.cyclomatic_complexity import CyclomaticComplexity
+from .operations.cache_array_length import CacheArrayLength
 from .statements.incorrect_using_for import IncorrectUsingFor
 from .operations.encode_packed import EncodePackedCollision

slither/detectors/operations/cache_array_length.py

@@ -0,0 +1,221 @@
+from typing import List, Set
+
+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
+
+
+class CacheArrayLength(AbstractDetector):
+    """
+    Detects `for` loops that use `length` member of some storage array in their loop condition and don't modify it.
+    """
+
+    ARGUMENT = "cache-array-length"
+    HELP = (
+        "Detects `for` loops that use `length` member of some storage array in their loop condition and don't "
+        "modify it. "
+    )
+    IMPACT = DetectorClassification.OPTIMIZATION
+    CONFIDENCE = DetectorClassification.HIGH
+
+    WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#cache-array-length"
+
+    WIKI_TITLE = "Cache array length"
+    WIKI_DESCRIPTION = (
+        "Detects `for` loops that use `length` member of some storage array in their loop condition "
+        "and don't modify it. "
+    )
+    WIKI_EXPLOIT_SCENARIO = """
+```solidity
+contract C
+{
+    uint[] array;
+    
+    function f() public 
+    {
+        for (uint i = 0; i < array.length; i++)
+        {
+            // code that does not modify length of `array`
+        }
+    }
+}
+```
+Since the `for` loop in `f` doesn't modify `array.length`, it is more gas efficient to cache it in some local variable and use that variable instead, like in the following example:
+
+```solidity
+contract C
+{
+    uint[] array;
+    
+    function f() public 
+    {
+        uint array_length = array.length;
+        for (uint i = 0; i < array_length; i++)
+        {
+            // code that does not modify length of `array`
+        }
+    }
+}
+```
+    """
+    WIKI_RECOMMENDATION = (
+        "Cache the lengths of storage arrays if they are used and not modified in `for` loops."
+    )
+
+    @staticmethod
+    def _is_identifier_member_access_comparison(exp: BinaryOperation) -> bool:
+        """
+        Checks whether a BinaryOperation `exp` is an operation on Identifier and MemberAccess.
+        """
+        return (
+            isinstance(exp.expression_left, Identifier)
+            and isinstance(exp.expression_right, MemberAccess)
+        ) or (
+            isinstance(exp.expression_left, MemberAccess)
+            and isinstance(exp.expression_right, Identifier)
+        )
+
+    @staticmethod
+    def _extract_array_from_length_member_access(exp: MemberAccess) -> StateVariable:
+        """
+        Given a member access `exp`, it returns state array which `length` member is accessed through `exp`.
+        If array is not a state array or its `length` member is not referenced, it returns `None`.
+        """
+        if exp.member_name != "length":
+            return None
+        if not isinstance(exp.expression, Identifier):
+            return None
+        if not isinstance(exp.expression.value, StateVariable):
+            return None
+        if not isinstance(exp.expression.value.type, ArrayType):
+            return None
+        return exp.expression.value
+
+    @staticmethod
+    def _is_loop_referencing_array_length(
+        node: Node, visited: Set[Node], array: StateVariable, depth: int
+    ) -> True:
+        """
+        For a given loop, checks if it references `array.length` at some point.
+        Will also return True if `array.length` is referenced but not changed.
+        This may potentially generate false negatives in the detector, but it was done this way because:
+        - situations when array `length` is referenced but not modified in loop are rare
+        - checking if `array.length` is indeed modified would require much more work
+        """
+        visited.add(node)
+        if node.type == NodeType.STARTLOOP:
+            depth += 1
+        if node.type == NodeType.ENDLOOP:
+            depth -= 1
+        if depth == 0:
+            return False
+
+        # Array length may change in the following situations:
+        # - when `push` is called
+        # - when `pop` is called
+        # - when `delete` is called on the entire array
+        # - when external function call is made (instructions from internal function calls are already in
+        #   `node.all_slithir_operations()`, so we don't need to handle internal calls separately)
+        if node.type == NodeType.EXPRESSION:
+            for op in node.all_slithir_operations():
+                if isinstance(op, Length) and op.value == array:
+                    # op accesses array.length, not necessarily modifying it
+                    return True
+                if isinstance(op, Delete):
+                    # take into account only delete entire array, since delete array[i] doesn't change `array.length`
+                    if (
+                        isinstance(op.expression, UnaryOperation)
+                        and isinstance(op.expression.expression, Identifier)
+                        and op.expression.expression.value == array
+                    ):
+                        return True
+                if isinstance(op, HighLevelCall) and not op.function.view and not op.function.pure:
+                    return True
+
+        for son in node.sons:
+            if son not in visited:
+                if CacheArrayLength._is_loop_referencing_array_length(son, visited, array, depth):
+                    return True
+        return False
+
+    @staticmethod
+    def _handle_loops(nodes: List[Node], non_optimal_array_len_usages: List[SourceMapping]) -> 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.
+        If it cannot, the loop condition is added to `non_optimal_array_len_usages`.
+        There may be some false negatives here - see docs for `_is_loop_referencing_array_length` for more information.
+        """
+        for node in nodes:
+            if node.type == NodeType.STARTLOOP:
+                if_node = node.sons[0]
+                if if_node.type != NodeType.IFLOOP:
+                    continue
+                if not isinstance(if_node.expression, BinaryOperation):
+                    continue
+                exp: BinaryOperation = if_node.expression
+                if not CacheArrayLength._is_identifier_member_access_comparison(exp):
+                    continue
+                array: StateVariable
+                if isinstance(exp.expression_right, MemberAccess):
+                    array = CacheArrayLength._extract_array_from_length_member_access(
+                        exp.expression_right
+                    )
+                else:  # isinstance(exp.expression_left, MemberAccess) == True
+                    array = CacheArrayLength._extract_array_from_length_member_access(
+                        exp.expression_left
+                    )
+                if array is None:
+                    continue
+
+                visited: Set[Node] = set()
+                if not CacheArrayLength._is_loop_referencing_array_length(
+                    if_node, visited, array, 1
+                ):
+                    non_optimal_array_len_usages.append(if_node)
+
+    @staticmethod
+    def _get_non_optimal_array_len_usages_for_function(f: Function) -> List[SourceMapping]:
+        """
+        Finds non-optimal usages of array length in loop conditions in a given function.
+        """
+        non_optimal_array_len_usages: List[SourceMapping] = []
+        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]:
+        """
+        Finds non-optimal usages of array length in loop conditions in given functions.
+        """
+        non_optimal_array_len_usages: List[SourceMapping] = []
+
+        for f in functions:
+            non_optimal_array_len_usages += (
+                CacheArrayLength._get_non_optimal_array_len_usages_for_function(f)
+            )
+
+        return non_optimal_array_len_usages
+
+    def _detect(self):
+        results = []
+
+        non_optimal_array_len_usages = CacheArrayLength._get_non_optimal_array_len_usages(
+            self.compilation_unit.functions
+        )
+        for usage in non_optimal_array_len_usages:
+            info = [
+                "Loop condition at ",
+                usage,
+                " should use cached array length instead of referencing `length` member "
+                "of the storage array.\n ",
+            ]
+            res = self.generate_result(info)
+            results.append(res)
+        return results

tests/e2e/detectors/snapshots/detectors__detector_CacheArrayLength_0_8_17_CacheArrayLength_sol__0.txt

@@ -0,0 +1,18 @@
+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 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 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 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 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 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 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 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 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.
+ 

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

@@ -0,0 +1,171 @@
+pragma solidity 0.8.17;
+
+contract CacheArrayLength
+{
+    struct S
+    {
+        uint s;
+    }
+
+    S[] array;
+    S[] array2;
+
+    function h() external
+    {
+        
+    }
+
+    function g() internal
+    {
+        this.h();
+    }
+
+    function h_view() external view
+    {
+
+    }
+
+    function g_view() internal view
+    {
+        this.h_view();
+    }
+
+    function f() public
+    {
+        // array accessed but length doesn't change
+        for (uint i = 0; i < array.length; i++) // warning should appear
+        {
+            array[i] = S(0);
+        }
+
+        // array.length doesn't change, but array.length not used in loop condition
+        for (uint i = array.length; i >= 0; i--)
+        {
+
+        }
+
+        // array.length changes in the inner loop
+        for (uint i = 0; i < array.length; i++)
+        {
+            for (uint j = i; j < 2 * i; j++)
+                array.push(S(j));
+        }
+
+        // array.length changes
+        for (uint i = 0; i < array.length; i++)
+        {
+            array.pop();
+        }
+
+        // array.length changes
+        for (uint i = 0; i < array.length; i++)
+        {
+            delete array;
+        }
+
+        // array.length doesn't change despite using delete
+        for (uint i = 0; i < array.length; i++) // warning should appear
+        {
+            delete array[i];
+        }
+
+        // array.length changes; push used in more complex expression
+        for (uint i = 0; i < array.length; i++)
+        {
+            array.push() = S(i);
+        }
+
+        // array.length doesn't change
+        for (uint i = 0; i < array.length; i++) // warning should appear
+        {
+            array2.pop();
+            array2.push();
+            array2.push(S(i));
+            delete array2;
+            delete array[0];
+        }
+
+        // array.length changes; array2.length doesn't change
+        for (uint i = 0; i < 7; i++)
+        {
+            for (uint j = i; j < array.length; j++)
+            {
+                for (uint k = 0; k < j; k++)
+                {
+
+                }
+
+                for (uint k = 0; k < array2.length; k++) // warning should appear
+                {
+                    array.pop();
+                }
+            }
+        }
+
+        // array.length doesn't change; array2.length changes
+        for (uint i = 0; i < 7; i++)
+        {
+            for (uint j = i; j < array.length; j++) // warning should appear
+            {
+                for (uint k = 0; k < j; k++)
+                {
+
+                }
+
+                for (uint k = 0; k < array2.length; k++)
+                {
+                    array2.pop();
+                }
+            }
+        }
+
+        // none of array.length and array2.length changes
+        for (uint i = 0; i < 7; i++)
+        {
+            for (uint j = i; j < array.length; j++) // warning should appear
+            {
+                for (uint k = 0; k < j; k++)
+                {
+
+                }
+
+                for (uint k = 0; k < array2.length; k++) // warning should appear
+                {
+                    
+                }
+            }
+        }
+
+        S[] memory array3;
+
+        // array3 not modified, but it's not a storage array
+        for (uint i = 0; i < array3.length; i++)
+        {
+
+        }
+
+        // array not modified, but it may potentially change in an internal function call
+        for (uint i = 0; i < array.length; i++)
+        {
+            g();
+        }
+
+        // array not modified, but it may potentially change in an external function call
+        for (uint i = 0; i < array.length; i++)
+        {
+            this.h();
+        }
+
+        // array not modified and it cannot be changed in a function call since g_view is a view function
+        for (uint i = 0; i < array.length; i++) // warning should appear
+        {
+            g_view();
+        }
+
+        // array not modified and it cannot be changed in a function call since h_view is a view function
+        for (uint i = 0; i < array.length; i++) // warning should appear
+        {
+            this.h_view();
+        }
+    }   
+}

tests/e2e/detectors/test_detectors.py

@@ -1639,6 +1639,11 @@ ALL_TESTS = [
         "LowCyclomaticComplexity.sol",
         "0.8.16",
     ),
+    Test(
+        all_detectors.CacheArrayLength,
+        "CacheArrayLength.sol",
+        "0.8.17",
+    ),
     Test(
         all_detectors.IncorrectUsingFor,
         "IncorrectUsingForTopLevel.sol",