For loop prevents reentrancy detection

[frangio]

This was reported before in #940 but wasn't answered, so I wanted to bring attention to it again and give simple reproduction instructions.

In the following code I expect reentrancy-no-eth to detect both r1 and r2 as errors, but only r1 is detected.

Is this a known limitation? Is it documented? Are there any plans to fix it? Thanks.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

contract ITarget {
    function exec() public {}
}

contract Reenter {
    uint x;

    function r1(ITarget target) public {
        if (x > 0) {
            target.exec();
            x = 0;
        }
    }

    function r2(ITarget target) public {
        if (x > 0) {
            for (uint i = 0; i < 1; i++) {
                target.exec();
            }
            x = 0;
        }
    }
}

[montyly]

Hi @frangio, thanks for pointing this out, somehow #940 was missed for the 0.8.2 release.

We will investigate why it happens

[Jaime-Iglesias]

Hi @frangio, thanks for pointing this out, somehow #940 was missed for the 0.8.2 release.

We will investigate why it happens

The issue also affects reentrancy-eth as the detector code is very similar.

    function bad3(ITarget target) public payable {
        if (balance >= 10) {
            for (uint256 i = 0; i < 10; i++) {
                target.exec{ value: 1 ether }();
            }
        }
        balance -= 10;
    }

[0xalpharush]

I found another instance of this. Slither detects that nftCount is updated after _safeMint, but it would miss it otherwise (It doesn't alert if a postfix operator is used so that may be a quirk to look into as well).
Test case: https://github.com/FrankieIsLost/smart-batched-auction/blob/dccadf0fdee5859f637ee29f46eb832e10a84b05/contracts/BatchAuction.sol#L102-L107
Patched: FrankieIsLost/smart-batched-auction#1