Make <Modifying storage array by value> detector more accurate

[bossjoker1]

Describe the issue:

When I was reading this detector's source code, I found it will misreport the test case below:

contract Memory {
    uint[1] public x; // storage
    uint[1] public y; // storage

    function f() public {
        uint[1] memory temp;
        f1(x); // update x
        f2(temp, y); // update temp & y 
    }

    function f1(uint[1] storage arr) internal { // by reference
        arr[0] = 1;
    }

    function f2(uint[1] memory arr, uint[1] storage arr2) internal { // by value
        arr[0] = 2;
        arr2[0] = 3; 
    }
}

For this case, it will not modify storage array y (also memory array temp) by value. But the output is

Memory.f() (arrayTest.sol#5-9) passes array Memory.y (arrayTest.sol#3)by reference to Memory.f2(uint256[1],uint256[1]) (arrayTest.sol#15-18)which only takes arrays by value
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#modifying-storage-array-by-value
arrayTest.sol analyzed (1 contracts with 1 detectors), 1 result(s) found

I think the function get_funcs_modifying_array_params also needs to record the indexes of array parameters (storage or not) in the parameter list, which would help it avoid this misreport.

And I wish I could try to help fix the problem.

Thanks a lot.

Code example to reproduce the issue:

contract Memory {
    uint[1] public x; // storage
    uint[1] public y; // storage

    function f() public {
        uint[1] memory temp;
        f1(x); // update x
        f2(temp, y); // update temp & y 
    }

    function f1(uint[1] storage arr) internal { // by reference
        arr[0] = 1;
    }

    function f2(uint[1] memory arr, uint[1] storage arr2) internal { // by value
        arr[0] = 2;
        arr2[0] = 3; 
    }
}

Version:

0.4.24

Relevant log output:

No response

[bossjoker1]

we can modify func detect_calls_passing_ref_to_function like this:

# Verify one of these parameters is an array in storage.
  for (param, arg) in zip(ir.function.parameters, ir.arguments):
      # Verify this argument is a variable that is an array type.
      if not isinstance(arg, (StateVariable, LocalVariable)):
          continue
      if not isinstance(arg.type, ArrayType):
          continue

      # If it is a state variable OR a local variable referencing storage, we add it to the list.
      if (isinstance(arg, StateVariable) or (
          isinstance(arg, LocalVariable) and arg.location == "storage"
      )) and (isinstance(param.type, ArrayType) and param.location != "storage" ):
          results.append((node, arg, ir.function))

[0xalpharush]

I think it would make sense to verify the parameter and argument position to prevent this false positive. It'd be great if you could submit a PR and add the example you've provided as a test case here https://github.com/crytic/slither/blob/dev/tests/detectors/array-by-reference/. Then, running python tests/test_detectors.py --overwrite will overwrite the expected results for this detector