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.
