Skip to content

Fix withdrawal logic in EtherStore contract #29

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
luis-immunefi wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix withdrawal logic in EtherStore contract #29

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/EtherStore.sol
View file
Open in desktop

@immunefi-magnus immunefi-magnus Bot Nov 28, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,16 @@ contract EtherStore {
uint256 bal = balances[msg.sender];
require(bal >= 0);




(bool sent,) = msg.sender.call{value: bal}("");
require(sent, "Failed to send Ether");





balances[msg.sender] = 0;
}

Expand Down