DEV Community

juan23z
juan23z

Posted on

5 false positives that fool naive Solidity scanners (and how to kill them at the root)

#blockchain #ethereum #security #tooling

In a paid audit, a false positive is not just noise — it's a credibility killer. Hand a founder a "CRITICAL: unprotected fund sweep" that turns out to be safe-by-design, and you've spent your only currency: trust.

We run a heuristic scanner over Solidity before a human ever looks at the code. The scanner's job is recall (surface everything suspicious); the human's job is precision (confirm what's real). But every false positive the scanner emits is time the human burns — and if one ever leaks into a report, it's reputation burned.

Here are five patterns that fool naive static heuristics, why each is a false positive, and how we fixed them at the root — not by muting the rule, but by teaching it what it was missing.

1. The donation "vuln" that is actually the defense

The classic donation/inflation check flags any balanceOf(address(this)) used in accounting. But look at this:

uint256 balanceBefore = token.balanceOf(address(this));
token.safeTransferFrom(msg.sender, address(this), amount);
uint256 balanceAfter  = token.balanceOf(address(this));
uint256 received = balanceAfter - balanceBefore;   // credited to the user
Enter fullscreen mode Exit fullscreen mode

This is the fee-on-transfer-safe pattern. A donation before the call inflates balanceBefore and balanceAfter equally, so received is unchanged. The balanceOf here is the mitigation, not the bug. A scanner that greps for balanceOf(this) without recognizing the before/after delta flags the exact code that defends against the attack it's warning about.

Root fix: recognize the balanceBefore/balanceAfter delta idiom as safe; only flag when the raw balance feeds a share-price or ratio directly.

2. corruptedReserve = is not "reserve accounting"

A subtler version of the same detector matched a share-accounting pattern via a loose regex like reserve\s*=. It fired on:

corruptedReserve = toSweep;   // matched "Reserve =" as a substring
Enter fullscreen mode Exit fullscreen mode

That's a variable name, not accounting. The lesson: word-boundary your patterns. \breserve\s*= doesn't match inside corruptedReserve, sharePrice, or collateralRatio. A single missing \b manufactures false criticals.

3. Permissionless ≠ unprotected, when the destination is fixed

Access-control detectors love to flag sweep/rescue/withdrawAll functions with no onlyOwner. But callable-by-anyone is only dangerous if the caller can redirect the funds:

function sweepUnclaimed() external {          // no modifier — flagged CRITICAL
    uint256 amount = token.balanceOf(address(this));
    token.safeTransfer(recoveryAddress, amount);   // fixed, set at init
}
Enter fullscreen mode Exit fullscreen mode

Whoever calls it, the money goes to a predetermined recoveryAddress. This is a keeper pattern — permissionless on purpose, safe by construction. It's a false positive.

Root fix: for a sweep, only flag when the transfer destination is msg.sender, tx.origin, or a function parameter (something the caller controls). A fixed state variable → safe. (And watch the error direction: when in doubt, keep the finding — better a reviewed false positive than a blinded real one.)

4. Reentrancy on a local variable is not reentrancy

"External call before state update" is a great heuristic — until it counts a local as state:

token.transfer(to, amount);
uint256 len = holders.length;   // "state written after external call" — but it's a local!
for (uint256 i; i < len; ++i) { ... }
Enter fullscreen mode Exit fullscreen mode

len is a stack variable. So is uint256 gasUsed = .... So is a write to a memory array (returnData[i] = ...). None of them is shared storage; none can be corrupted by re-entry. The detector was reading the assignment's variable name but not the type keyword in front of it, so uint256 len = looked like a storage write.

Root fix: classify the write target — declarations with a primitive/memory type, and names declared memory/calldata/new in the same function, are locals. Only a genuine storage write after an attacker-controllable call is reentrancy.

5. Interface declarations are not unprotected functions

Flattened files bundle interfaces and implementations together. A function declaration has no body:

function setDepositLimit(uint256 x) external;   // interface — no body, no modifier possible
Enter fullscreen mode Exit fullscreen mode

Naive brace-matching grabbed the { of some later function and reported this declaration as an "unprotected config function." Root fix: if the ; comes before the {, it's a declaration — skip it. And skip flatten files entirely; they duplicate code you'll analyze in its real location.

The meta-lesson

Every one of these was fixed with a control test that proves two things at once: the false positive is gone and the real bug still fires. That second half is the discipline that matters — it's easy to silence a noisy rule and go blind. Suppress in the safe direction: when the classifier is unsure, keep the finding for a human.

Because that's the real architecture: the scanner surfaces, the human confirms. The tooling exists to make a careful auditor faster, not to replace their judgment. A zero-false-positive report isn't a model output — it's a heuristic that casts a wide net plus a human who verifies every survivor before their name goes on it.

That's the standard we hold. If you're shipping a Solidity protocol and want a review that reads the code the way an attacker would — fast, honest, and without a wall of false positives — let's talk.

Top comments (0)