Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detector for array length caching added #1694

Merged
merged 6 commits into from
Jun 23, 2023

Conversation

bart1e
Copy link
Contributor

@bart1e bart1e commented Feb 26, 2023

This detector checks for loops that compare loop index against array.length and doesn't modify it (where array is a state array). Caching array.length in a local variable will save gas in such cases.

It is done by parsing each loop individually using function nodes. For each loop, it is checked whether it modifies array.length in some way (it can be done using push, pop or delete on the entire array).

For simplicity, the check for push and pop is done in one step, by iterating over irs of each node in the loop. I only check whether array.length is referenced there - the reason for that is explained in the code.

@bart1e bart1e force-pushed the detector_for_array_len_caching branch from 74eb62c to d1804f3 Compare May 3, 2023 17:20
@bart1e
Copy link
Contributor Author

bart1e commented May 15, 2023

@0xalpharush I have made the changes you requested.

In case of external function calls, I am also checking if the function called is pure or internal and if this is the case, I'm assuming that they don't modify an array. I have also added several new test cases to cover it.

@montyly
Copy link
Member

montyly commented Jun 22, 2023

This looks great, thanks @bart1e.

One caveat is that internal function call, or storage pointer will lead to false positive. For example

contract CacheArrayLength
{

    uint[] public array;

    function f() public
    {

        for (uint i = 0; i < array.length; i++)
        {
	        change(array);
        }
    }

    function change(uint[] storage array) internal{
	    array.pop();
    }


}

With pointer aliases it can even be more difficult. Maybe we should:

  • Disable the result if the array is given as an internal function call argument
  • Disable the result if a storage pointer is created with the array

Most likely both case are uncommon (and could actually be bugs - for a separate detector?), so I am ok to merge this PR for this week's release if we don't have the time to include them. What do you think @0xalpharush ?

@0xalpharush
Copy link
Contributor

@montyly I think we can merge and follow up in a separate PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants