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
Ignore reentrancy inexecuteBatch
and update Slither config
#3955
Conversation
Thanks @0xalpharush! I was planning to report this upstream soon. 🙂 Funny that the new false positive is due to the fix for a bug that I reported. I was hoping there would be a way to get Slither to detect that this is safe without having to disable the check for the function, though I recognize that it's a fairly complex pattern. Are the only things that Slither detects as safe the CEI pattern and |
Slither collects variables read prior to external calls and variables written after. If any one those are in common and there are functions that could be called back into (no call to We have added ways to indicate that a smart contract is non-reentrant (trusted), but that's currently limited to state variables crytic/slither#1089. Do you have suggestions on how you would expect this case to be filtered out? Or, a UX for a user to specify properties that the detector can use to provide more finely tuned results? |
executeBatch
and update slither configexecuteBatch
and update Slither config
Just more advanced static analysis. 🙂 It probably exceeds the scope of Slither. Those features you point out are neat though. I will keep them in mind. |
Congrats, your important contribution to this open-source project has earned you a GitPOAP! GitPOAP: 2023 OpenZeppelin Contracts Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
@0xalpharush I had to add |
Co-authored-by: Francisco <fg@frang.io> (cherry picked from commit a5af0ad)
@frangio indicated this reentrancy isn't a vulnerability since the callbacks are guarded, so I've added a disable comment to
executeBatch
This result was detected in slither 0.9.2 because this bug was fixed and now reentrancy in for loops is considered properly.Additionally, I've configured slither to compile with hardhat and upgraded its version in the GitHub action to 0.9.2.
ref #3949
PR Checklist