Merge pull request #2030 from crytic/dev

prepare for 0.9.6 release

.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
 

.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
 

.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

.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

.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
 

.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

.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
 

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 ",
-                usage,
+                "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 ",
             ]

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

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:
 

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();
+        }
     }   
 }

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();
+        }
+    }
 }