[Bug-Candidate]: Function-local (memory) variable shadowing not detected by Slither

[mojtaba-eshghie]

Describe the issue:

In solidity, it is possible to specify the name of the returned variable from the function as well as its value type. The presence of this feature cancels the need to both declare the variable in the function body as well as having a return statement. However, if the developer mistakenly declares the variable in the function body again, the newly declared variable will not shadow the previously declared variable in the function declaration statement. This leads to the function returning the default initial value for the named return variable instead of the intended value that is computed in the function body since it pertains to another variable only with the same name. The current solidity compiler (0.8.15) does not raise an error, and only shows a warning: Warning: This declaration shadows an existing declaration for the declaration of the variable in the function body. This is not correct since the declared variable in the function declaration statement will shadow the variable in the function body, not the other way around. This shadowing is not detected by Slither. In the accompanied code the max variable in the getMax function is shadowed by the default value max is initialized within the named return statement in the function declaration.

Code example to reproduce the issue:

// SPDX-License-Identifier: MIT
pragma solidity >=0.6.0 <=0.9.0;

library ArrayUtils {
    function getMax(uint[] calldata a) public pure returns (uint max) {
        uint max = 0;
        uint max_index = 0;
        for (uint i = 0; i < a.length; i++) {
            if (a[i] >= max) {
                max_index = i;
                max = a[i];
            }
        }
    }

    function range(uint l) internal pure returns (uint[] memory r) {
        r = new uint[](l);      
        for (uint i = 0; i < l; i++) {
            r[i] = i;
        }
    }
}


contract ArraysUser {
    using ArrayUtils for *;

    function getMax(uint l) public pure returns (uint max) {
        max = ArrayUtils.range(l).getMax();
    }
}

Version:

0.8.3

Relevant log output:

No response

[montyly]

Hi mojtaba-eshghie. This is a great idea thanks. We should have a detector for it.

[AndyBarnard]

Hey guys, I wrote this detector, currently trying to test it and getting stuck here: $ python ./tests/test_detectors.py --generate Traceback (most recent call last): File "/home/andy/Dev/slither/./tests/test_detectors.py", line 399, in <module> all_detectors.ArbitrarySendEth, AttributeError: module 'slither.detectors.all_detectors' has no attribute 'ArbitrarySendEth'. Did you mean: 'ArbitrarySend'? Not sure what's going on here, I'm a bit new to the slither codebase. Can anyone illuminate this for me? Is this a bug?

[0xalpharush]

This was fixed by https://github.com/crytic/slither/pull/1510 and will be available in the upcoming release