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]

Re-entrancy and underflow in withdraw enable balance inflation and Ether theft in EtherStore

Description:

TL;DR:
The withdraw function in EtherStore is exposed to both reentrancy and underflow vulnerabilities, allowing an attacker to recursively withdraw funds while inflating their balance.

The vulnerability arises because the withdraw function first sends Ether to msg.sender and only then updates the user's balance to -1, which, due to underflow in an unsigned integer, becomes 2^256 - 1. This mis-sequencing permits reentrant calls, enabling an attacker to repeatedly withdraw funds.

Details

The function retrieves the balance from the balances mapping and checks that it is greater than zero. It then transfers the balance to the caller using a low-level call:

(bool sent, ) = msg.sender.call{value: bal}("");

After the transfer, instead of zeroing out the balance, the contract sets balances[msg.sender] to -1. Since balances is of type uint256, this assignment underflows, converting -1 to 2^256 - 1. Consequently, when the attacker invokes the function, the sequence of an external call before state update allows them to recursively call withdraw several times. This breaks the intended invariant, as the balance becomes artificially inflated and the attacker can drain the contract's funds.

Impact

Exploitation of this vulnerability can lead to complete depletion of the contract's Ether reserves. The attacker can repeatedly call withdraw by taking advantage of the underflowed large balance, effectively compromising the integrity of the contract's accounting and resulting in significant financial loss.

Mitigation Steps:
Update the state by setting balances[msg.sender] to 0 before making the external call to prevent reentrancy.
-Implement a reentrancy guard to restrict recursive calls to the withdraw function.
-Remove any erroneous assignments that lead to underflow, ensuring that balance updates accurately reflect withdrawals.

• codexa

[immunefi-magnus]

Unchecked underflow in withdraw() grants users maximum balance

Description:

TL;DR:
The withdraw() function in the EtherStore contract sets balances[msg.sender] to -1, which underflows to 2^256-1, granting the caller an effectively unlimited balance.

The withdraw() function improperly assigns -1 to balances[msg.sender] after transferring the user's balance. Since balances is a uint256 mapping, this underflow corrupts the user's balance to the maximum possible value, allowing repeated withdrawals far exceeding the legitimate balance.

Details

In the EtherStore contract, the function withdraw() reads the balance of the caller from the balances mapping and enforces that it is greater than zero. It then transfers the value specified by this balance using a low-level .call(). After successfully transferring the Ether, the function assigns -1 to balances[msg.sender]. However, since balances is declared as a uint256, assigning -1 results in an underflow, causing the value to wrap around to 2^256 - 1. This means that the user now has a balance so large that it can be repeatedly withdrawn, which effectively gives the attacker an unlimited balance. The code snippet illustrating the vulnerability 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; // Underflow occurs here
}

Impact

Exploitation of this vulnerability allows any user with a nonzero balance to repeatedly call the withdraw() function and drain the contract's funds. The underflow in balances[msg.sender] corrupts the accounting mechanism, meaning that even after withdrawing funds, the user’s balance remains effectively unlimited. This can lead to the complete depletion of Ether from the contract, thus having critical financial implications for the protocol.

Mitigation Steps:
Correct the balance update in withdraw() by setting balances[msg.sender] to 0 instead of -1.
-Implement input validation to prevent assignments that result in underflow/overflow.
-Utilize Solidity version 0.8 or later, which has built-in overflow and underflow checks, or incorporate libraries such as SafeMath.
-Reorder state changes and external calls appropriately to avoid known vulnerability patterns.

• codexa