The standarToken code is old, have attack vector on ERC20 API (approve/transferFrom methods)

[yuanaichi]

see ethereum/EIPs#20 (comment)

[maraoz]

This is correct. Please see how we fixed it in OpenZeppelin: https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/contracts/token/StandardToken.sol#L37

Note this is one of the problems covered by our recommendation here: https://medium.com/zeppelin-blog/basic-attention-token-bat-audit-88bf196df64b#e022

[colterj199]

@maraoz how does that fix the issue? Maybe I'm missing something....

if ((_value != 0) && (allowed[msg.sender][_spender] != 0)) throw;

if the attacker actually managed to win the race condition (ie. if they successfully detected that Alice wanted to reduce allowance, and successfully called transferFrom before Alice's new approve() tx is included in a block), then allowed[msg.sender][_spender] will be equal to 0...

IMO this is an issue with the API itself, and should be fixed in ERC22. In the meanwhile, it's up to whoever uses approve/transferFrom to first call approve to 0, once that tx is mined, check that the tokens were not transferredFrom before the approve(0) was done, then call approve with the new allowance.

[maraoz]

@colterj199 with this extra condition, the allower should set allowance to 0, then wait to see if the allowee used the previous allowance by racing him, and if not, set the allowance to the new value safely. If the allowee did spend the allowance, he didn't break the contract, as he had been given an allowance in the first place.

[colterj199]

@maraoz sorry I wasn't clear enough. I understand that this is the right way for the approver to change the allowance (aka first approve(0), see if allowee tried to race, if not, approve(newAllowance)). What I don't understand is how this new check really helps in preventing a racing attacker. It doesn't force the approver to actually check if the allowee raced and won or not. Let's take an example, maybe it'll be more clear.

Let's say Joe has approved Sarah to transfer 1000 tokens at block #100. Now Joe wants to change that allowance to 500. So Joe broadcast a transaction approve(Sarah, 500). Sarah intercept that transaction, and broadcast a transferFrom which makes it to block #110. So at block #110, allowed[Joe][Sarah]=0. Joe's transaction finally makes it to block #111, and the miner confirms that the approve is valid (aka allowed[Joe][Sarah]=0 so doesn't throw), and the new allowance is confirmed.

In other words, even if your proposed approve() function has that extra line of code, it didn't prevent Joe from fucking up. Sure, Joe should have first called approve(Sarah, 0) and checked that Sarah didn't race him, but that responsibility is on him.

What I'm trying to say is that adding that one line only gives the illusion that approve() is safe to use, but it has to come with a huge warning sign that the approver must do the check to see that no attackers has raced. And this must happen regardless of whether that line of code is in the contract or not. That extra check provides no "on-chain" security by itself. It's similar to a client-side input validation check without actual server-side validation. You can very well use this code and still be attacked if you don't follow the practice of setting to 0, checking, then setting to new allowance.

So what's the advantage of adding this check then?

To play devil's advocate, I could argue that adding this check increases the likelihood that Joe will do the proper steps. But the downside of false security is larger to me...

[locklin]

As stated above; this is a known ERC20 issue, and things which comply with ERC20, including the above check used in OpenZeppelin framework (and minime) don't really solve the issue.
I can think of a couple of possible replies to address this.

1. Maybe there should be some dire warnings along with the source code.
2. Maybe people who attempt to use allowance should be aware of this issue.
3. Maybe there should be some kind of Mutex in allowance type operations, perhaps with a multi-block handshake. Gassy, but so are IRL cheques compared to handing someone a roll of bills. People pay for checking accounts because the bank assumes risk that someone will steal the cheques, or add another zero to the amount. CAPM is practically a law of nature.

#3 wouldn't be ERC20 standard, but something like it seems like a reasonable departure point for future standards to address this issue.

There are a number of competing token standard proposals that look helpful for other reasons (ERC23/223), but as far as I can tell none of them have successfully addressed this issue.

I'm happy to entertain further discussion here, and to keep the issue open, since it's a real issue with allowances. I'm not going to add lines of code which don't fix the problem, or which break the standard.