Add totalDeposits tracking and minor improvements

[luis-immunefi]

This PR adds a new feature to track the total Ether deposited into the contract across all users. The main goal is to provide a quick way to see the total deposits without having to sum individual balances.

Changes include:

• Added a totalDeposits state variable.
• Updated the deposit function to increment totalDeposits whenever someone deposits Ether.
• Updated the withdraw function to decrement totalDeposits when someone withdraws.
• Minor comment and readability improvements.

No changes were made to the core deposit/withdraw logic aside from updating the totalDeposits variable. The contract’s functionality should remain the same, with the addition of the total deposits tracking.

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

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