Smart contract good practice for reentrancy attacks

Question

I'm a rookie working with solidity and blockchain technologies and I was reading some good practices to improve my code.

And I have a question about a code that I'm not quite understanding very well:

Source: https://github.com/ConsenSys/smart-contract-best-practices/blob/master/docs/known_attacks.md

// INSECURE
mapping (address => uint) private userBalances;

function withdrawBalance() public {
    uint amountToWithdraw = userBalances[msg.sender];
    require(msg.sender.call.value(amountToWithdraw)()); // At this point, the caller's code is executed, and can call withdrawBalance again
    userBalances[msg.sender] = 0;
}

In the above code is said to be insecure because a malicious agent can call the step 2 require the amount of times we wants. My question regarding this is, how the malicious agent can call misuse this and call that line of code more than 1 time. I'm clearly missing something here.

Answer 1

This is known as a reentrancy attack.

This is insecure because the user's balance is set to 0 only after the withdrawal has been processed. Moreover, the withdrawal is processed by using evm's CALL opcode, which passes control to the receiving address.

If the receiving address is a contract, it can hijack this transfer using the fallback function. Inside this fallback function, it can check to see if the balance of the sending contract exceeds the amount that was transferred. If it does, it will call withdrawBalance again, until the withdrawal contract's balance has been depleted.

A simple attacker contract may look something like:

contract Attacker {

    function startattack() {
        victim.withdrawBalance();
    }

    function() payable {
        if (victim.balance > msg.value) {
            victim.withdrawBalance();
        }
    }
}

By calling startattack, you initiate a withdrawal. When the require(msg.sender.call.value(amountToWithdraw)()); line is executed, it runs the code in the fallback function. At this point, msg.value is userBalances[msg.sender]. The attacker can check if the victim's contract still has more ether available than userBalances[msg.sender], and run a withdrawal again (which will result in this process looping until the balance drops below userBalances[msg.sender]).

This can be avoided by switching the order of lines to:

function withdrawBalance() public {
    uint amountToWithdraw = userBalances[msg.sender];
    userBalances[msg.sender] = 0;
    require(msg.sender.call.value(amountToWithdraw)());
}

Now, even if the attacker calls withdrawBalance again, the user's balance has already been set to 0, and further withdrawal will not be possible.