diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index f7f1efaa5e..57f44bc56e 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -89,5 +89,6 @@ 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 diff --git a/slither/detectors/operations/cache_array_length.py b/slither/detectors/operations/cache_array_length.py new file mode 100644 index 0000000000..da73d3fd5a --- /dev/null +++ b/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 diff --git a/tests/e2e/detectors/snapshots/detectors__detector_CacheArrayLength_0_8_17_CacheArrayLength_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_CacheArrayLength_0_8_17_CacheArrayLength_sol__0.txt new file mode 100644 index 0000000000..63d4b883c3 --- /dev/null +++ b/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. + diff --git a/tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol b/tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol new file mode 100644 index 0000000000..704d6bed9e --- /dev/null +++ b/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(); + } + } +} \ No newline at end of file diff --git a/tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol-0.8.17.zip b/tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol-0.8.17.zip new file mode 100644 index 0000000000..dafdfc4316 Binary files /dev/null and b/tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol-0.8.17.zip differ diff --git a/tests/e2e/detectors/test_detectors.py b/tests/e2e/detectors/test_detectors.py index d003e7ce00..e497dea725 100644 --- a/tests/e2e/detectors/test_detectors.py +++ b/tests/e2e/detectors/test_detectors.py @@ -1639,6 +1639,11 @@ def id_test(test_item: Test): "LowCyclomaticComplexity.sol", "0.8.16", ), + Test( + all_detectors.CacheArrayLength, + "CacheArrayLength.sol", + "0.8.17", + ), Test( all_detectors.IncorrectUsingFor, "IncorrectUsingForTopLevel.sol",