Skip to content

Add totalDeposits tracking and minor improvements #27

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

Add totalDeposits tracking and minor improvements #27

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

@immunefi-magnus immunefi-magnus Bot Sep 16, 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.

Reentrancy vulnerability and inconsistent accounting in EtherStore's withdraw function

Description:

TL;DR:
The EtherStore contract's withdraw function calls msg.sender externally before updating state, enabling a reentrancy attack that drains funds and corrupts the totalDeposits accounting.

The withdraw function in the EtherStore contract is vulnerable because it performs an external call to msg.sender before resetting the caller's balance and updating the totalDeposits variable. This ordering allows an attacker to reenter the function and repeatedly drain Ether, while also causing inconsistencies in internal accounting.

Details

In the EtherStore contract, the withdraw function first retrieves the caller's balance and then verifies that it is greater than zero. However, the function then performs an external call via msg.sender.call{value: bal}('') before updating the user's balance to zero and decrementing the totalDeposits counter. As a result, if an attacker is able to reenter the function during the external call, the balance would not have been set to zero yet, allowing additional withdrawals. This reentrancy vulnerability is exacerbated by the inconsistent state update, where the totalDeposits is only modified after the external call, leading to potential discrepancies in any protocol logic relying on this variable. The core issue stems from the absence of a reentrancy guard and the incorrect ordering of state updates relative to external interactions.

Example snippet from EtherStore.sol:

function withdraw() public {
    uint256 bal = balances[msg.sender];
    require(bal > 0, "No balance to withdraw");

    // Vulnerable external call occurs before state update
    (bool sent,) = msg.sender.call{value: bal}("");
    require(sent, "Failed to send Ether");

    // State updates occur after the external call
    balances[msg.sender] = 0;
    totalDeposits -= bal;
}

Impact

Exploitation of this vulnerability could allow an attacker to repeatedly invoke the withdraw function and drain the contract's ETH balance. Moreover, the delayed update to totalDeposits introduces an inconsistency that could compromise any logic depending on that variable, potentially affecting protocol invariants and resulting in financial losses.

Mitigation Steps:
Implement a reentrancy guard to prevent recursive calls to the withdraw function.
-Reorder state updates to occur before any external calls, following the checks-effects-interactions pattern.
-Eliminate or properly manage the redundant totalDeposits variable to prevent future accounting discrepancies.

  • codexa

Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,28 @@ pragma solidity ^0.8.26;
contract EtherStore {
mapping(address => uint256) public balances;

// Engineer tried to optimize but did something weird here
uint256 public totalDeposits; // <-- unnecessary, just causes confusion

function deposit() public payable {
balances[msg.sender] += msg.value;

// Engineer thinks they are keeping track of total deposits
totalDeposits += msg.value;
}

function withdraw() public {
uint256 bal = balances[msg.sender];
require(bal >= 0);
require(bal > 0, "No balance to withdraw");

// 👀 Vulnerable point: external call happens BEFORE state update
(bool sent,) = msg.sender.call{value: bal}("");
require(sent, "Failed to send Ether");

balances[msg.sender] = 0;

// Engineer added extra logging but forgot to check for reentrancy
totalDeposits -= bal;
}

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