Solidity – Checking Reentrancy Vulnerability in Smart Contracts

go-ethereumreentrant-attackssolidity

According to the Ethereum Smart Contract Best Practices, race conditions can occur across multiple functions, and even multiple contracts, any solution aimed at preventing reentry will not be sufficient and they said to focus on the finishing internal work first. Few posts from Ethereum Stack Exchange says Race condition is not a much problem as system is designed that way.

So basically, does race condition really a problem here or its just a wrong term which is being used to describe the system.

Also if I want to develop some tool to check whether my program has reentrancy vulnerability, what would be the best possible solution to this? Do I really need to scan the code to find if we are allowing calls to external contracts?

Thanks in advance.

Best Answer

In case the danger and the pattern isn't clear, a contract is vulnerable if it calls, sends or transfers to an untrusted address (contract with fallback function?) and then updates the state afterward. This is because the state is implicitly incomplete when flow control is transferred to a (potentially) hostile contract. Flow control may not return as the caller expects. Instead, the callee might do any number of unexpected things such as calling the function again, calling another function or even calling another contract (possibly it relies on your contract for vital information - information that isn't updated yet).

A simple heuristic for dealing with this safely is to do "optimistic accounting" first. That is, put the whole state in order before invoking another contract. If something goes wrong, revert everything to the original state. A complete transaction abort suffices in most cases (failed transfer, failed require, explicit revert), but should you wish to carry on, just manually undo the state changes.

Vulnerable (Bad):

function forwardFunds(address receiver, uint amount) public {
  require(balances[msg.sender] >= amount);
  receiver.transfer(amount); // <= flow control transferred before the sender's balance is updated and before an event is emitted. Potentially the start of trouble. 
  balances[receiver] -= amount;
  LogFundsForwarded(msg.sender, receiver, amount);
}

Finishing internal work first (Good)

function forwardFunds(address receiver, uint amount) public {
  require(balances[msg.sender] >= amount);
  balances[msg.sender] -= amount;
  LogFundsForwarded(msg.sender, receiver, amount);
  receiver.transfer(amount); // <== on fail, this will revert all of the above.
}

I should mention that the above example contains an unrelated flaw in that it "talks" to two untrusted contracts at a time, and that is a bad thing to do. Different issue. It would be better to use the withdrawal pattern instead of the forwarding method in this contrived example.

Hope it helps.

Related Topic