Is a function prone to re-entrancy attack even if I set balance to zero within the same function (example included)

Question

Such as in this scenario, is the hackable via re-entrancy? thanks...

function withdraw(uint256 money) public {
    if(money <= balance[msg.sender]) {
        (bool success, ) = msg.sender.call.value(money)("");
        if(success) {
            balance[msg.sender] -= money;
        } 
    }

Answer 1

Yes.

You have to put the state in order before you transfer flow control to a contract you don't trust. msg.sender could be anyone/anything, so this applies.

You've sent all gas to the fallback function. It can loop around like this:

contract Attacker {

  // ... set up the victim contract and other details, then ...

  function () payable {
    // check available gas
    // if sufficient gas for another run
    if(sufficientGas) {
       victim.withdraw(amountToStealOnThisIteration); // nothing stops this and balance[msg.sender] is not updated yet
    } // only now that the attack is over will msg.sender's balance be adjusted in the victim contract
  }

}

I'm not sure it's especially clear, but every time your function is called, this fallback function in msg.sender takes over. It's a transfer of flow control. It can circle around and call your function again (give me my money again), and again. Of course, if it runs out of gas the attack will fail, but keep in mind that the attacker can practice this until it works smoothly.

Here's a very unsophisticated (and old) gist: https://gist.github.com/rob-Hitchens/7eab95883e1d30b7224f024304a1f713

1. Deploy Victim
2. Send the Victim some money, Victim.deposit()
3. Deploy Attacker with Victim's address
4. Attacker.attack()

It will take about 30x as much money as it's supposed to.

You can fix this easily with optimistic accounting. In other words, update the state as if success is assured. If !success, then revert (require() does it) and it will revert the state changes that came earier.

function withdraw(uint256 money) public {
    uint bal = balance[msg.sender];
    balance[msg.sender] =- money; // optimistic accounting
    require(money <= bal]); // fail hard
    require(msg.sender.call.value(money)("")); // If the returned bool isn't true then fail hard
    // we are done
}

Hope it helps.