[False-Positive]: Function marked as re-entrant

[0xSwego]

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

I have a function F in contract A, that can only be called from contract B. Slither marks some lines of F as possible re-entrant, while this is not possible.

Is it possible to disable the check for the whole function, or do I have to add //slither-disable-next-line reentrancy-no-eth over every re-entrant line?

Frequency

Occasionally

Code example to reproduce the issue:

modifier onlyOtherContract () {
    require(msg.sender == otherContract, "Only otherContract can call this");
    _;
}

function F() external onlyOtherContract {
   ...
   bool successfulApproval = _token.approve(address(_gToken), _amount);   // <<-- one of the lines that is marked as possibly re-entrant
   ...
}

Version:

I use it in a github action, with crytic/[email protected]

Relevant log output:

No response

[0xalpharush]

You can use // slither-disable-start reentrancy-no-eth and // slither-disable-end reentrancy-no-eth to accomplish this

[valle-xyz]

And what about the onlyOtherContract? My rational was: this modifier ensures this is called from a trusted source. So the warning could be loosened.

[EmbededLee]

Hi, @0xalpharush. I am also interested in this issue. So, is it hard for Slither now to detect this modifier usage in this scenario? I've also noticed that the SAST tool for smart contracts usually has a lot of false positives, is this just due to the nature of SAST, or is it a solidity language feature, such as the modifier usage mentioned above? Looking forward to your professional reply. Thanks advance.

[0xalpharush]

False positives are a frequent criticism of many static analysis tools. Solidity presents some unique challenges as it's not always clear what behavior will be of code that is called or if this code is considered trusted (for external calls to contracts we many not have the source code of).

We could probably filter reentrancies where the entry points and any state variables updated in the function only allow a single address to call it, but we'd still need to know if the user trusts this contract (which requires annotations). It would be great to have everything automated of course but sometimes it's easier for a developer who has this context to throw out the result themself.

[EmbededLee]

Thanks @0xalpharush for the detailed explanation. It's clear that handling Solidity's nuances is no small feat for SAST tools, and your perspective on the need for developer context to sift through false positives is quite valuable. The idea of using annotations to improve the accuracy of automated tools seems like a promising approach. Looking forward to seeing how this evolves. Keep up the great work!