diff --git a/.github/workflows/black.yml b/.github/workflows/black.yml index df51a3ddf..33a1666b2 100644 --- a/.github/workflows/black.yml +++ b/.github/workflows/black.yml @@ -32,7 +32,7 @@ jobs: fetch-depth: 0 - name: Set up Python 3.8 - uses: actions/setup-python@v3 + uses: actions/setup-python@v4 with: python-version: 3.8 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 76438ec73..b59aacd66 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -55,7 +55,7 @@ jobs: steps: - uses: actions/checkout@v3 - name: Set up Python ${{ matrix.python }} - uses: actions/setup-python@v3 + uses: actions/setup-python@v4 with: python-version: ${{ matrix.python }} - name: Install dependencies @@ -67,11 +67,11 @@ jobs: - name: Set up nix if: matrix.type == 'dapp' - uses: cachix/install-nix-action@v20 + uses: cachix/install-nix-action@v22 - name: Set up cachix if: matrix.type == 'dapp' - uses: cachix/cachix-action@v10 + uses: cachix/cachix-action@v12 with: name: dapp diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 7e6f88f1e..4cb1adcb1 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -47,7 +47,7 @@ jobs: password: ${{ secrets.GITHUB_TOKEN }} - name: Docker Build and Push - uses: docker/build-push-action@v3 + uses: docker/build-push-action@v4 with: platforms: linux/amd64,linux/arm64/v8,linux/arm/v7 target: final diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml index 20676e031..f6d66aa0a 100644 --- a/.github/workflows/docs.yml +++ b/.github/workflows/docs.yml @@ -43,4 +43,4 @@ jobs: path: './html/' - name: Deploy to GitHub Pages id: deployment - uses: actions/deploy-pages@v1 + uses: actions/deploy-pages@v2 diff --git a/.github/workflows/linter.yml b/.github/workflows/linter.yml index f972cbcd1..b352a8301 100644 --- a/.github/workflows/linter.yml +++ b/.github/workflows/linter.yml @@ -33,7 +33,7 @@ jobs: fetch-depth: 0 - name: Set up Python 3.8 - uses: actions/setup-python@v3 + uses: actions/setup-python@v4 with: python-version: 3.8 diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 529752f80..f7d9ff9e7 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -44,7 +44,7 @@ jobs: path: dist/ - name: publish - uses: pypa/gh-action-pypi-publish@v1.8.6 + uses: pypa/gh-action-pypi-publish@v1.8.7 - name: sign uses: sigstore/gh-action-sigstore-python@v1.2.3 diff --git a/.github/workflows/pylint.yml b/.github/workflows/pylint.yml index 983c176a9..207f98eac 100644 --- a/.github/workflows/pylint.yml +++ b/.github/workflows/pylint.yml @@ -27,7 +27,7 @@ jobs: fetch-depth: 0 - name: Set up Python 3.8 - uses: actions/setup-python@v3 + uses: actions/setup-python@v4 with: python-version: 3.8 diff --git a/slither/detectors/operations/cache_array_length.py b/slither/detectors/operations/cache_array_length.py index da73d3fd5..e4d8cf2c6 100644 --- a/slither/detectors/operations/cache_array_length.py +++ b/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,9 +215,10 @@ contract C ) for usage in non_optimal_array_len_usages: info = [ - "Loop condition at ", - usage, - " should use cached array length instead of referencing `length` member " + "Loop condition ", + f"`{usage.source_mapping.content}` ", + f"({usage.source_mapping}) ", + "should use cached array length instead of referencing `length` member " "of the storage array.\n ", ] res = self.generate_result(info) diff --git a/slither/detectors/operations/unchecked_low_level_return_values.py b/slither/detectors/operations/unchecked_low_level_return_values.py index 0537ebbf2..c1fb1a868 100644 --- a/slither/detectors/operations/unchecked_low_level_return_values.py +++ b/slither/detectors/operations/unchecked_low_level_return_values.py @@ -1,15 +1,24 @@ """ Module detecting unused return values from low level """ -from slither.detectors.abstract_detector import DetectorClassification -from slither.detectors.operations.unused_return_values import UnusedReturnValues +from typing import List + +from slither.core.cfg.node import Node from slither.slithir.operations import LowLevelCall -from slither.slithir.operations.operation import Operation + +from slither.core.declarations.function_contract import FunctionContract +from slither.core.variables.state_variable import StateVariable +from slither.detectors.abstract_detector import ( + AbstractDetector, + DetectorClassification, + DETECTOR_INFO, +) +from slither.utils.output import Output -class UncheckedLowLevel(UnusedReturnValues): +class UncheckedLowLevel(AbstractDetector): """ - If the return value of a send is not checked, it might lead to losing ether + If the return value of a low-level call is not checked, it might lead to losing ether """ ARGUMENT = "unchecked-lowlevel" @@ -38,5 +47,44 @@ If the low level is used to prevent blocking operations, consider logging failed WIKI_RECOMMENDATION = "Ensure that the return value of a low-level call is checked or logged." - def _is_instance(self, ir: Operation) -> bool: # pylint: disable=no-self-use - return isinstance(ir, LowLevelCall) + @staticmethod + def detect_unused_return_values(f: FunctionContract) -> List[Node]: + """ + Return the nodes where the return value of a call is unused + Args: + f (Function) + Returns: + list(Node) + """ + values_returned = [] + nodes_origin = {} + for n in f.nodes: + for ir in n.irs: + if isinstance(ir, LowLevelCall): + # 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) + nodes_origin[ir.lvalue] = ir + + for read in ir.read: + if read in values_returned: + values_returned.remove(read) + + return [nodes_origin[value].node for value in values_returned] + + def _detect(self) -> List[Output]: + """Detect low level calls where the success value is not checked""" + results = [] + for c in self.compilation_unit.contracts_derived: + for f in c.functions_and_modifiers: + unused_return = UncheckedLowLevel.detect_unused_return_values(f) + if unused_return: + + for node in unused_return: + info: DETECTOR_INFO = [f, " ignores return value by ", node, "\n"] + + res = self.generate_result(info) + + results.append(res) + + return results diff --git a/slither/detectors/operations/unused_return_values.py b/slither/detectors/operations/unused_return_values.py index 80be98b45..783a44807 100644 --- a/slither/detectors/operations/unused_return_values.py +++ b/slither/detectors/operations/unused_return_values.py @@ -101,10 +101,8 @@ contract MyConc{ def _detect(self) -> List[Output]: """Detect high level calls which return a value that are never used""" results = [] - for c in self.compilation_unit.contracts: - for f in c.functions + c.modifiers: - if f.contract_declarer != c: - continue + for c in self.compilation_unit.contracts_derived: + for f in c.functions_and_modifiers: unused_return = self.detect_unused_return_values(f) if unused_return: 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 63d4b883c..456c702a5 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,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. 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 704d6bed9..ea3c4120e 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 @@ -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(); + } } } \ 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 dafdfc431..c5cb9f283 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 diff --git a/tests/e2e/detectors/test_data/unchecked-lowlevel/0.7.6/unchecked_lowlevel.sol b/tests/e2e/detectors/test_data/unchecked-lowlevel/0.7.6/unchecked_lowlevel.sol index 96712b077..cd6f78e98 100644 --- a/tests/e2e/detectors/test_data/unchecked-lowlevel/0.7.6/unchecked_lowlevel.sol +++ b/tests/e2e/detectors/test_data/unchecked-lowlevel/0.7.6/unchecked_lowlevel.sol @@ -4,8 +4,14 @@ contract MyConc{ } function good(address payable dst) external payable{ - (bool ret, bytes memory _) = dst.call{value:msg.value}(""); + (bool ret, ) = dst.call{value:msg.value}(""); require(ret); } + function good2(address payable dst) external payable{ + (bool ret, ) = dst.call{value:msg.value}(""); + if (!ret) { + revert(); + } + } } diff --git a/tests/e2e/detectors/test_data/unchecked-lowlevel/0.7.6/unchecked_lowlevel.sol-0.7.6.zip b/tests/e2e/detectors/test_data/unchecked-lowlevel/0.7.6/unchecked_lowlevel.sol-0.7.6.zip index 530d247c4..bccb47182 100644 Binary files a/tests/e2e/detectors/test_data/unchecked-lowlevel/0.7.6/unchecked_lowlevel.sol-0.7.6.zip and b/tests/e2e/detectors/test_data/unchecked-lowlevel/0.7.6/unchecked_lowlevel.sol-0.7.6.zip differ