Ignore reentrancy inexecuteBatch and update Slither config

[0xalpharush]

@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

• Tests
• Documentation
• Changelog entry

[frangio]

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 nonReentrant modifier?

[0xalpharush]

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 nonReentrant), those functions are analyzed to see if they would be impacted i.e. they read from the variable whose value is non-deterministic. In this case, I believe it's discovering that _beforeCall and _afterCall read/ write to _timestamp with an external call in between.

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?

[frangio]

how you would expect this case to be filtered out?

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.

[gitpoap-bot]

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.

[frangio]

@0xalpharush I had to add rm foundry.toml again because it was failing installation. The failure is due to foundry-rs/foundry#3942, so it's not exactly a Slither issue, but it does show that "compile_force_framework": "hardhat" isn't disabling Foundry installation.

[Amxx]

Hello @0xalpharush

We recently noticed that the "compile_force_framework": "hardhat" line in the slither config (that was introduced in this PR) is causing issues in the community repo: https://github.com/OpenZeppelin/openzeppelin-community-contracts

On that repo, slither appears to work just fine without this line. We would like to understand waht the consequences would be if we removed it here as well.

[0xalpharush]

I believe it will probably just default to building with Foundry. If it's working, then there's no reason to force the usage of hardhat. It should be apparent from the build command that's run what framework is used.

[Amxx]

FYI, we get an issue with the hardhat option because this script, which our hardhat.config.js is apparently not processed by slither ... and the remappings are unaccessible