1

I am trying to debug the following code and explain why such coding practice is risky and how it can be avoideded

function withdrawBalance()
{ uint amountToWithdraw = userBalances[msg.sender];
if (msg.sender.call.value(amountToWithdraw)() == false)
{
throw;
}
userBalances[msg.sender] = 0;
}
1

This is risky, because you update the balance of a user only after you used call on another address. The vulnerability in this code, called 're-entrancy', is that the a called contract could call withdrawBalance in their fallback function.

The attacking contract receives ether everytime they call withdrawBalance. But everytime they do, before their balance is checked, they do receive ether and just call withdrawBalance again, multiple times. Your contract's balance will probably be depleted by then.

This is the appropriate fix:

function withdrawBalance()
{
    uint amountToWithdraw = userBalances[msg.sender];
    userBalances[msg.sender] = 0;
    if (msg.sender.call.value(amountToWithdraw)() == false) {
        throw;
    }
}

Putting the balance deduction before call, makes the re-entrancy attack impossible. Now, before an attacking contract calls withdrawBalance, their balance will already have been deducted, and the function will throw.

| improve this answer | |
0

this might cause a Reentrancy attack. I advice you to read this post as it it explains thoroughly this scenario :

https://hackernoon.com/smart-contract-security-part-1-reentrancy-attacks-ddb3b2429302

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.