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: set the reference to the popped element to the correct type #44

Merged
merged 1 commit into from
May 18, 2023

Conversation

kevinclancy
Copy link

@kevinclancy kevinclancy commented May 18, 2023

Notes

The IR generated for calls to pop (for dynamic array types and bytes) would obtain a reference to the last element of the array and then delete it. But the type of this reference was always uint256 even though it should have been the type of the array element.

Now, we set the reference's type to the array element type. For bytes, we set the reference's type to bytes1.

Trail o' Bits developed a fix for this, but I think they used the wrong element type for bytes. It should be bytes1 but they use bytes. I will file an issue with them about this.

Testing

  • Run make test and verify all tests succeed.
  • Run ./evaluate.sh run 100 in tool-eval and verify all projects succeed
  • Run pipenv run slither test.sol --print slithir, where test.sol contains the following:
pragma solidity ^0.8.13;

contract Poptest {
    struct S {
        int x;
        int y;
    }
    S[] a;
    function poptest() internal {
        a.pop();
    }
}

Examine the IR and verify that the deleted reference variable has the correct type. Also, try changing the array a's type to bytes.

Related Issue

https://github.com/CertiKProject/slither-task/issues/501)

Additional Comments

I commented in the issue I already filed in the Slither repo about handling bytes incorrectly.

@kevinclancy kevinclancy requested a review from duckki May 18, 2023 00:31
@kevinclancy kevinclancy changed the title set the reference to the popped element to the correct type fix: set the reference to the popped element to the correct type May 18, 2023
@duckki duckki merged commit daf96c3 into certik May 18, 2023
@chenhuitao chenhuitao deleted the popfix branch March 18, 2024 10:41
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