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

[False-Positive]: constable-states false positive introduced in 0.11.0 #2659

Open
arr00 opened this issue Feb 3, 2025 · 4 comments
Open

Comments

@arr00
Copy link

arr00 commented Feb 3, 2025

Describe the false alarm that Slither raise and how you know it's inaccurate:

See this action run. There is a new false positive introduced in 0.11.0 where the strings mentioned below are now tagged for being constable.

Frequency

Occasionally

Code example to reproduce the issue:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/19c2f2f5a5ea43e18dfff2b92b54b76815783d93/contracts/utils/cryptography/EIP712.sol#L51-L52

Version:

0.11.0

Relevant log output:

EIP712._nameFallback (contracts/utils/cryptography/EIP712.sol#51) should be constant 
EIP712._versionFallback (contracts/utils/cryptography/EIP712.sol#52) should be constant
@smonicas
Copy link
Collaborator

smonicas commented Feb 3, 2025

I'm not sure to understand the issue. If i run slither contracts/utils/cryptography/EIP712.sol --detect constable-states they are detected also in 0.10.4

@arr00
Copy link
Author

arr00 commented Feb 3, 2025

When running locally I do see that--but you can see that setting it back to that version in CI silences the error. Regardless, it is still a false positive but it is relatively hard to detect why. We conditionality set the value in the constructor if necessary (uses this lib https://github.com/OpenZeppelin/openzeppelin-contracts/blob/19c2f2f5a5ea43e18dfff2b92b54b76815783d93/contracts/utils/ShortStrings.sol#L88-L95).

@smonicas
Copy link
Collaborator

smonicas commented Feb 3, 2025

I see the pattern used and it's quite complicated. I will try to see if it is possible to remove this false positive. You can also disable the detector with a comment above each variable instead of changing the Slither version.

    // slither-disable-next-line constable-states
    string private _nameFallback;
    // slither-disable-next-line constable-states
    string private _versionFallback;

@arr00
Copy link
Author

arr00 commented Feb 3, 2025

Ok, we can do that if necessary. Do you know this is only being surfaced now with the new release?

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

No branches or pull requests

2 participants