diff --git a/slither/detectors/operations/cache_array_length.py b/slither/detectors/operations/cache_array_length.py index 1f8111bdb..da73d3fd5 100644 --- a/slither/detectors/operations/cache_array_length.py +++ b/slither/detectors/operations/cache_array_length.py @@ -7,7 +7,7 @@ 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 +from slither.slithir.operations import Length, Delete, HighLevelCall class CacheArrayLength(AbstractDetector): @@ -119,8 +119,10 @@ contract C # - 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.irs: + 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 @@ -132,6 +134,8 @@ contract C 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: @@ -173,7 +177,7 @@ contract C if not CacheArrayLength._is_loop_referencing_array_length( if_node, visited, array, 1 ): - non_optimal_array_len_usages.append(if_node.expression) + non_optimal_array_len_usages.append(if_node) @staticmethod def _get_non_optimal_array_len_usages_for_function(f: Function) -> List[SourceMapping]: @@ -207,8 +211,10 @@ contract C ) for usage in non_optimal_array_len_usages: info = [ - f"Loop condition at {usage.source_mapping} should use cached array length instead of referencing " - f"`length` member of the storage array.\n " + "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) 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 index a0ba35740..63d4b883c 100644 --- 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 @@ -1,14 +1,18 @@ -Loop condition at tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#47 should use cached array length instead of referencing `length` member of the storage array. +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 tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#78 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 tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#16 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 tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#88 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 tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#105 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 tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#112 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 tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol#59 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 index 79858d182..704d6bed9 100644 --- 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 @@ -10,6 +10,26 @@ contract CacheArrayLength 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 @@ -123,5 +143,29 @@ contract CacheArrayLength { } + + // 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 index ab3d81371..dafdfc431 100644 Binary files a/tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol-0.8.17.zip and b/tests/e2e/detectors/test_data/cache-array-length/0.8.17/CacheArrayLength.sol-0.8.17.zip differ