How to fix a reentrancy bug in 5 lines — a real PR walkthrough

Last week I scanned a staking vault contract submitted by an indie team building on Base. Slither flagged a HIGH-severity reentrancy in withdraw(). Mythril confirmed it. The fix was 5 lines moved around. The team merged the PR in 12 minutes.

Here's the exact diff, and why every part of it matters.

The vulnerable code

function withdraw() external {
    uint256 amount = balances[msg.sender];
    require(amount > 0, "nothing to withdraw");
    (bool ok, ) = msg.sender.call{value: amount}('');
    require(ok, "send failed");
    balances[msg.sender] = 0;
}

If you've never seen this before: withdraw() sends ETH to msg.sender and only then sets the user's balance to zero. If msg.sender is a contract with a receive() function that calls withdraw() again, the second call sees the same balances[msg.sender] (still non-zero, we haven't reached the = 0 line yet) and sends another payout.

In the wild this would drain the vault until it's empty or runs out of gas. The attacker only deposits 1 ETH, gets paid 10 ETH.

The fix

  function withdraw() external {
      uint256 amount = balances[msg.sender];
      require(amount > 0, "nothing to withdraw");
+     balances[msg.sender] = 0;
      (bool ok, ) = msg.sender.call{value: amount}('');
      require(ok, "send failed");
-     balances[msg.sender] = 0;
  }

Five lines changed. One added, one removed, three context.

That's it. That's the whole fix.

Why it works

The pattern is called Checks-Effects-Interactions (CEI). Three steps in this order, always:

1. Checks — validate inputs and preconditions. require(amount > 0).
2. Effects — update your contract's state. balances[msg.sender] = 0.
3. Interactions — call external contracts. msg.sender.call{...}().

In the buggy version, Effects come after Interactions. By the time the recursive call lands, our state is stale. By moving the state update before the external call, the recursive call sees the new state (balances[msg.sender] == 0), fails the require(amount > 0), and reverts.

Why we didn't add nonReentrant

You might wonder why the fix isn't just import ReentrancyGuard and slap nonReentrant on withdraw(). Two reasons:

1. CEI is free. nonReentrant costs ~2,300 gas per call (one SSTORE for the lock, one for the unlock). On a high-throughput vault, that's real money.
2. CEI is composable. If you ever call this function internally (e.g., from a withdrawAll() helper), nonReentrant would block it. CEI doesn't.

The right rule of thumb: CEI by default. nonReentrant as a belt-and-braces for functions that share state across multiple external entry points.

What about cross-function reentrancy?

A sneakier variant: instead of re-entering withdraw(), the attacker re-enters a different function that reads balances[msg.sender] (which still hasn't been zeroed in the original-vulnerable version).

CEI doesn't protect against this on its own. If the vault also had a getRewards() function that read balances and paid out a proportional reward, the attacker could exploit the inconsistency.

If your contract has multiple functions that mutate shared user balance state, add nonReentrant to all of them, including the view functions other contracts might use as oracles (read-only reentrancy).

The PR description we wrote

fix(vault): apply checks-effects-interactions in withdraw()

Reorders state update to before the external call, eliminating the
single-function reentrancy that Slither/Mythril flagged.

Detector: reentrancy-eth (slither, HIGH confidence)
Confirmed by: mythril symbolic exec
CWE: CWE-841 (Improper Enforcement of Behavioral Workflow)
SWC: SWC-107

This is a one-line move with no logic change. No additional gas overhead.
Reviewed by AEDSC pre-audit.

The team's reviewer approved it in 4 minutes.

How to find this in your own code

Three options:

1. Free: slither . and look for HIGH severity findings tagged reentrancy-eth, reentrancy-no-eth, reentrancy-events, reentrancy-unlimited-gas.
2. AEDSC (free first scan): we merge Slither + Mythril + Aderyn and present the result with this exact kind of fix walk-through. Drop your contract →.
3. Manual review: search your codebase for every .call{value: ...}(, .transfer(, and .send(. For each, verify the state mutation that should happen sits before the call.

If you find anything you're unsure about, send the contract and I'll review it manually within 24h. Free.

Want to see the full report we generated for this team? It's here. Different contract, same staking-vault patterns.