crytic / crytic/slither

incorrect-equality: gate on callee mutability to suppress view/pure helper FPs

Open
#3,041 1 comment 0 reactions 0 assignees View on GitHub
enhancement
Dominant language
Python
Stars
6.4k
Forks
1.1k
PR merge metrics
No merged PRs in 30d

Description

### Describe the desired feature

`incorrect-equality` fires on strict equalities inside `view`/`pure` callees, but the WIKI's exploit (attacker grieves a state transition by manipulating `block.timestamp` / `block.number` / a balance) needs the comparison to actually gate a state write.

`tainted_equality_nodes` at `slither/detectors/statements/incorrect_strict_equality.py:147-150` only filters `FunctionTopLevel`:

```python
for func in funcs:
if isinstance(func, FunctionTopLevel):
continue
for node in func.nodes:
...
```

No mutability check, so a side-effect-free callee still gets reported.

Repro:

```solidity
contract C {
uint256 public immutable deployBlock;
constructor() { deployBlock = block.number; }

function isDeployBlock() external view returns (bool) {
return block.number == deployBlock; // reported; no state to grief
}
}
```

### The possible solution

Add a mutability gate after the `FunctionTopLevel` check:

```python
if getattr(func, "view", False) or getattr(func, "pure", False):
continue
```

That kills the FP above. But it also kills a real case: an `internal view` helper called inside a state-modifying caller, like

```solidity
function _isExpired() internal view returns (bool) {
return block.timestamp == deadline;
}
function withdraw() external {
if (_isExpired()) { /* release funds */ }
}
```

slither's taint doesn't cross function boundaries, so the warning won't transfer to `withdraw`. That's a real recall loss on a pattern that's pretty common.

### Alternatives we've considered

1. Only skip `public` / `external view`. Narrower, but `_isExpired()` is `internal` so still missed.
2. Call-graph reachability. Skip a `view`/`pure` callee only if no state-modifying function in the contract transitively reaches it. One pass from state-modifying entry points.
3. No code change. Document the boundary in the WIKI, let users `// slither-disable-next-line` on confirmed FPs.

### Additional context

The strict-equality findings in #2425 (`CreateX._parseSalt`, `_requireSuccessfulContractCreation`, `_guard`, all `internal pure` / `internal view`) are this FP class.

#2759 is a different sub-class: magic-number compares (`== 0`, `== type(uint256).max`) inside stateful functions. Not about callee mutability, needs a separate value-class filter.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.