Fix withdrawal logic in EtherStore contract

[luis-immunefi]

No description provided.

[immunefi-magnus]

🛡️ Immunefi PR Reviews

We’ve assigned 2 code reviewer(s) to this PR.

They’ll begin the review shortly and leave feedback directly in the pull request.

This review is based on the current state of your pull request. If you make changes after the review starts, they won’t be reflected here. To ensure the review includes your latest updates, you’ll need to open a new pull request.

[immunefi-magnus]

✅ AI Code Review Summary

I've completed reviewing this pull request using AI-powered analysis.

I found 1 issue that you may want to address. Please check the comments below for detailed explanations and suggested fixes.

📝 AI-generated suggestions are intended to assist and may be incomplete or inaccurate — please review them carefully before applying.

This review is based on the state of the pull request at the time it was opened. If you make changes after the review starts, they won’t be reflected here. To ensure the review includes your latest updates, you’ll need to open a new pull request.

[immunefi-magnus]

Re-entrancy in withdraw() Lets Single Attacker Drain Entire EtherStore Balance

tl;dr:
Re-entrancy in EtherStore.withdraw allows malicious contracts to recurse and withdraw repeatedly, resulting in complete theft of all stored ether.

Explanation

Impact

Calling withdraw() from a malicious contract can empty the entire EtherStore balance. The attacker repeatedly re-enters before its balances[msg.sender] entry is cleared, stealing every wei held by the contract and violating the core invariant that a user may withdraw only what they deposited.

Execution Flow

EtherStore.withdraw() performs:

1. uint256 bal = balances[msg.sender];
2. msg.sender.call{value: bal}("") ⟵ external call with control transfer.
3. balances[msg.sender] = 0;

Because step 2 executes before step 3, a fallback/receive function on msg.sender can invoke withdraw() again. Each re-entrant call reads the unchanged balances[msg.sender], passes the non-restrictive require(bal >= 0), and repeats the transfer. The loop continues until address(this).balance reaches zero. deposit() is permissionless, so the attacker only needs to seed a minimal initial balance; no other state or access restrictions block the path.

Exploitability

Trivial. Prerequisites: deploy a helper contract, deposit any positive amount, and call withdraw(). Gas cost is small; profit equals the full ether balance of EtherStore. The attack works on mainnet conditions with a single transaction and no race requirements.

Mitigation Steps:
Move balances[msg.sender] = 0; to before the external call in withdraw() to follow checks-effects-interactions.

• Add ReentrancyGuard and mark withdraw() with nonReentrant to provide a generic defense.

• Replace call with transfer or send, or explicitly forward a fixed gas stipend to minimise re-entrancy surface.

• Validate that bal > 0 instead of bal >= 0 to reject zero-value withdrawals.

• codexa