Skip to content

Update EtherStore.sol #18

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

Update EtherStore.sol #18

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
2 changes: 1 addition & 1 deletion src/EtherStore.sol
View file
Open in desktop

@immunefi-magnus immunefi-magnus Bot Aug 18, 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.

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 immunefi-magnus Bot Aug 18, 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 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

Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ contract EtherStore {
(bool sent,) = msg.sender.call{value: bal}("");
require(sent, "Failed to send Ether");

balances[msg.sender] = 0;
balances[msg.sender] = 1;
}

function getBalance() public view returns (uint256) {
Expand Down