1

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.

6

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.

2

"Race condition" is probably not the right term, since there's no concurrency involved.

The Solidity compiler emits a warning when it looks like you're not following the "checks-effects-interactions" pattern. It's a bit noisy, but it's helpful in pointing out places where you may be vulnerable.

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.