Update EtherStore.sol

[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 2 issues 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]

Incorrect balance reset after withdrawal locks 1 wei per user

Description:

TL;DR:
The EtherStore contract’s withdraw function resets the sender's balance to 1 wei instead of 0, resulting in permanently locked funds.

The vulnerability in the EtherStore contract occurs when after a successful withdrawal, the user's balance is incorrectly set to 1 wei instead of zero. This error leads to a persistent lock of 1 wei per user and deviates from expected full withdrawal behavior.

Details

The issue is present in the withdraw function of the EtherStore contract. The function first retrieves the user’s balance, ensures it is greater than zero, and proceeds to send the entire balance using a low-level call. The intended behavior is to reset the user's balance to zero after a successful withdrawal; however, the code sets it to 1. This minor error means that after each withdrawal, users retain a balance of 1 wei that cannot be effectively withdrawn due to gas costs and minimum transfer constraints. The relevant snippet is shown below:

function withdraw() public {
    uint256 bal = balances[msg.sender];
    require(bal > 0);

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

    balances[msg.sender] = 1;
}

This logic flaw breaks the invariant that a full withdrawal should leave the user's balance at zero.

Impact

The impact of this bug is minimal in terms of monetary loss as it only locks 1 wei per user. However, it can lead to user confusion and does not align with the expected behavior of the contract. The issue does not create an avenue for fund theft or unauthorized control, but it implies a design oversight that may affect user trust.

Mitigation Steps:
Set balances[msg.sender] to 0 after a successful withdrawal to correctly reflect the withdrawal of funds.
-Implement unit tests to verify that the balance is reset to zero after withdrawal, ensuring the correct behavior of the function.

• codexa

[immunefi-magnus]

Re-entrancy vulnerability and incorrect balance reset in EtherStore.withdraw enable Ether theft and state corruption

Description:

TL;DR:
The EtherStore.withdraw function suffers from a reentrancy vulnerability and an incorrect balance update (setting to 1 instead of 0) which could lead to repeated withdrawals and state corruption.

In the EtherStore contract, the withdraw function transfers the entire balance using a low-level call without a reentrancy guard, and then erroneously sets the user's balance to 1 instead of 0. This can be exploited to recursively call the function and repeatedly withdraw funds, leading to Ether theft and corrupt accounting.

Details

The vulnerability originates in the order of operations in the withdraw function. Initially, the function retrieves the user’s balance with uint256 bal = balances[msg.sender] and requires it to be greater than zero. It then sends the Ether using a low-level call: (bool sent,) = msg.sender.call{value: bal}(''), which forwards all available gas and enables reentrancy. Critically, the balance reset is performed after the Ether transfer and is incorrect—balances[msg.sender] is set to 1 rather than being reset to 0. This flawed update allows an attacker to recursively invoke withdraw before the balance is properly cleared, potentially draining the contract's Ether. The affected code section is:

function withdraw() public {
    uint256 bal = balances[msg.sender];
    require(bal > 0);

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

    balances[msg.sender] = 1;
}

Impact

An attacker exploiting this vulnerability can perform repeated, nested calls to withdraw, depleting the contract's Ether reserves. Additionally, the balance mismanagement may result in corrupted state and inaccurate accounting for users' balances. This directly compromises both the financial integrity and the operational correctness of the protocol, allowing for theft and potential downstream logic failures.

Mitigation Steps:
Reset balances[msg.sender] to 0 before transferring Ether to prevent recursive re-entries.
-Implement a reentrancy guard (e.g., using a nonReentrant modifier or mutex) to prevent recursive calls.
-Follow the Checks-Effects-Interactions pattern by updating state before making external calls.

• codexa