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

fix: preserve empty tuple components during declaration-to-assignment conversion #2034

Merged
merged 1 commit into from
Mar 29, 2024

Conversation

kevinclancy
Copy link
Contributor

This fixes issue 1913.

Previously, when performing an assignment whose variables were originally declared in a tuple-based declaration, the projection components were reused:

    function test() external returns(int) {
        (int a, int b, int c) = threeRet(3);
        (a, c) = twoRet(b);
        return b;
    }
...
  Expression: (a,c) = twoRet(b)
  IRs:
    TUPLE_1(int256,int256) = INTERNAL_CALL, Test.twoRet(int256)(b)
    a(int256)= UNPACK TUPLE_1 index: 0 
    c(int256)= UNPACK TUPLE_1 index: 2 

This happened because of some code in the _post_assignment_operation function in expression_to_slithir.py:

                        # The following test is probably always true?
                        if (
                            isinstance(left[idx], LocalVariableInitFromTuple)
                            and left[idx].tuple_index is not None
                        ):
                            index = left[idx].tuple_index

We shouldn't be reusing the variable's original projection component, but there is a reason that code was written; namely, this code translates a multi-variable declaration into a multi-variable assignment.

It also removes padding None entries and records each component index in its corresponding variable instead. As we've seen, this approach doesn't work, because not all tuple assignments are generated from variable declarations. To solve this, this PR leaves the None components inside of the tuple in order to select the correct projection index for each variable.

…iable's original tuple component across multiple assignments
@kevinclancy kevinclancy changed the title fix: preserve empty tuple components, and don't reuse a variable's tuple component fix: preserve empty tuple components during declaration-to-assignment conversion Jul 7, 2023
@0xalpharush
Copy link
Contributor

Can you move the tests to tests/unit/slithir and run pytest tests/e2e/detectors/test_detectors.py --insta update please?

@kevinclancy
Copy link
Contributor Author

@0xalpharush When I run with --insta update-new, I get an error:

ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --insta update-new
  inifile: None
  rootdir: /Users/kevin.clancy/my-slither

@chenhuitao chenhuitao deleted the upstream-pr/tuple-assign-fix branch March 18, 2024 10:42
@0xalpharush 0xalpharush mentioned this pull request Mar 29, 2024
@0xalpharush 0xalpharush merged commit 70c2742 into crytic:dev Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants