crytic / crytic/slither

Make <Modifying storage array by value> detector more accurate

Open
#1,697 2 comments 0 reactions 0 assignees View on GitHub
false-positive
Dominant language
Python
Stars
6.4k
Forks
1.1k
PR merge metrics
No merged PRs in 30d

Description

### Describe the issue:

When I was reading this [detector](https://github.com/crytic/slither/wiki/Detector-Documentation#modifying-storage-array-by-value)'s source code, I found it will misreport the test case below:
```solidity
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`](https://github.com/crytic/slither/blob/master/slither/detectors/compiler_bugs/array_by_reference.py#L58) 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:

```solidity
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_

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.