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
- Deploy Victim
- Send the Victim some money,
Victim.deposit()
- Deploy Attacker with Victim's address
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.