Resolution on the EIP20 API Approve / TransferFrom multiple withdrawal attack

[HaleTom]

#I raise this as a separate issue as I can't see a definitive resolution after it was initially posted here. The thread of the issue is hard to follow because of the intermingled comments about non-related parts of the ERC20 standard.

Please close this issue if you believe that it should be posted as a comment on EIP20 itself and I will take that as a hint to move the discussion there.

But perhaps keeping separate issues for security related issues is warranted so that a single thread can be followed, months down the track. For example, the issue was raised on 29 Nov 2016, and the latest comment is on 17 June 2017 -- it's nigh on impossible to follow the thread between those two dates with the interspersed other comments and references.

Prevent EIP20 API Approve / TransferFrom multiple withdrawal attack

I seek clarity on how to effectively prevent the double withdrawal attack described in ERC20 API: An Attack Vector on Approve/TransferFrom Methods

Summary / example of the attack:

1. Alice allows Bob to transfer N of Alice's tokens (N>0) by calling approve method on Token smart contract passing Bob's address and N as method arguments
2. After some time, Alice decides to change from N to M (M>0) the number of Alice's tokens Bob is allowed to transfer, so she calls approve method again, this time passing Bob's address and M as method arguments
3. Bob notices Alice's second transaction before it was mined and quickly sends another transaction that calls transferFrom method to transfer N Alice's tokens somewhere
4. If Bob's transaction will be executed before Alice's transaction, then Bob will successfully transfer N Alice's tokens and will gain an ability to transfer another M tokens
5. Before Alice noticed that something went wrong, Bob calls transferFrom method again, this time to transfer M Alice's tokens.

So, Alice's attempt to change Bob's allowance from N to M (N>0 and M>0) made it possible for Bob to transfer N+M of Alice's tokens, while Alice never wanted to allow so many of her tokens to be transferred by Bob.

EIP20 says concerning approve():

... clients SHOULD make sure to create user interfaces in such a way that they set the allowance first to 0 before setting it to another value for the same spender. THOUGH The contract itself shouldn't enforce it, to allow backwards compatibility with contracts deployed before

I'm confused as to why this check should be passed to the UI, and the contract shouldn't enforce it.

How is the UI to know that the approve(spender, 0) transaction will be processed before the subsequent non-zero approval, and not the other way around? It seems it is not possible to do this check via the Web3 API.

If the contract doesn't enforce this, then isn't anyone not using a UI still able to effect the attack?

I also note the lowercase shouldn't in "[t]he contract itself shouldn't enforce it", from which one may infer a different meaning to an RFC2119 "SHOULD NOT". I've raised my issues with the wording of this paragraph here.

EIP20 refers readers to the MiniMeToken implementation regarding this issue which works around it by adding the line:

require((_amount == 0) || (allowed[msg.sender][_spender] == 0));

Interestingly the nimiq-exchange-token breaks the standard with this line:

require(_value != 0 || allowed[msg.sender][_spender] == 0);

This prevents fully cancelling an allowance after it is established (the only way would be via a transferFrom() of the total allowance amount)

EIP20 refers readers to two example token implementations (I've linked to the relevant line numbers):

• OpenZeppelin - Brief issue description and link to the original issue comment. Mentions "[o]ne possible solution to mitigate this race condition", but doesn't implement it.
• ConsenSys - no mention or attempt to work around the issue

The MiniMeToken function comment says:

This is a modified version of the ERC20 approve function to be a little bit safer

Given this is the reference implementation, I would like to be a lot more confident than "a little bit safer".

I'm also a bit confused as to why a contract implementation is given as an exemplar when the text of EIP20 says that this should be implemented in the UI. This seems inconsistent.

Regarding ERC223, @Dexaran asked:

Can someone name advantages of approve + transferFrom usage or even one thing that can not be done with fallback instead of approval.

I hope that the lack of reply (AFACT) means that fallback and the current ERC223 recommended implementation resolves this issue, but frankly I don't know enough about RFC223.

Questions:

1. Should the resolution to this issue be implemented in both UI and contract?
   1. What exemplar can we point to for the UI?
2. Is it possible to completely prevent this attack (and if not, why)?
3. Is the MiniMeToken approach sufficient? There was some challenge to the approach and some rebuttal, but I couldn't work out a definitive answer.
   1. If insufficient, recommendations were made in the document describing the issue, and also in the EIP20 comments. What is the clear direction that we should follow?
4. Could ERC223 be the point to break EIP20's attempt to "allow backwards compatilibilty with contracts deployed before"?
5. The situations in which approve() MAY / SHOULD / MUST return false are not currently mentioned. Does this issue necessitate the return value to be specified?

^{Please note that I am looking for Ethereum related work starting January 2018.}

[Qqwy]

I know of two possible alternative approaches, one of which is used in the wild:

1. Add functions that increase/decrease the allowance relative to its current value, such as the increaseApproval and decreaseApproval functions in the MonolithDAO Token.
2. Add an alternate approval function safeApprove(spender, new_value, expected_old_value) that takes the current expected approval amount as input parameter. This would stop steps 3..5 in the mentioned attack from executing properly: When Bob's transferFrom(Alice, Bob, N) transaction occurs before Alice's safeApprove(Bob, M, N) transaction then the latter will fail.

[HaleTom]

It seems that the approach of adding:

require((_amount == 0) || (allowed[msg.sender][_spender] == 0));

is not sufficient:

1. Bob is allowed to transfer N Alice's tokens
2. Alice publishes transaction that changes Bob's allowance to zero
3. Bob front runs Alice's transaction and transfers N Alice's tokens
4. Alice's transaction is mined
5. Alice sees that her transaction was mined successfully, that Bob's allowance is now zero and that proper Approval event was logged. This is exactly what she would see if Bob would not transfer any tokens from her, so she has no reason to think that Bob actually used his allowance before it was revoked
6. Now Alice publishes transaction that changes Bob's allowance to M
7. Alice's second transaction is mined so now Bob is allowed to transfer M Alice's tokens
8. Bob transfers M Alice's tokens

Again. Bob got N+M tokens which is more that Alice ever wanted to allow him to transfer.

One may argue that at step 5 Alice should notice that her token balance was decreased and Transfer event was logged. This is true, but if Bob had transferred tokens not to himself but to somebody else, then Transfer event will not be linked to Bob, and, if Alice's account is busy and many people are allowed to transfer from it, Alice may think that this transfer was probably performed by somebody else, not by Bob.

[HaleTom]

@Qqwy I like your safeApprove approach. All approaches are non backward compatible with ERC20 AFAICT.

I probably should tag some previously involved people: @mikhail-vladimirov @frozeman @chriseth @Dexaran @GriffGreen @yupasik @koeppelmann @PeterBorah @jbaylina @khovratovich @brudaswen @paberr

[3sGgpQ8H]

@HaleTom

All approaches are non backward compatible with ERC20

If you just add new (safeApprove) method, this is backward compatible because all existing code will still be able to use your smart contract.

[nateawelch]

@mikhail-vladimirov I think what @HaleTom meant by backwards compatible was that adding a new method means that contracts that interact with ERC20 tokens wouldn't be able to use this safety feature without being replaced.

@HaleTom I get your issue here, and a valid solution to that is to store another bool with each allowance keeping track if any of the allowance has been used since the owner last set allowance. Here's a quick example.

I believe this offers a few benefits over the other solutions:

1. Unless I misunderstand expectations around returning true/false from approve, it's backwards compatible.
2. If the spender hasn't used transferFrom since approve last succeeded, then the 1 transaction is all that's needed. The owner can tell it worked because an Approval event is broadcast with the proper amount. If it didn't work then it'll create an Approval event with 0 amount, since it set the allowance to back to 0.
3. The UI doesn't have to send a 0 approve and then the amount approve with order that matters. Now it can just send 1 if it knows the allowance hasn't been touched yet (again, if the spender managed to get call transferFrom before this has been mined, the approve will just set the allowance to 0) or it can just send 2 transactions setting the allowance, in which case the 1st to be mined will reset the allowance to 0/used to false and the 2nd will actually set the allowance.

Please give feedback, I'm sure I've missed something.

[3sGgpQ8H]

The idea with boolean variable looks odd for me because it makes possible the situation when approve method changes the allowance but returns false, while currently false return value means that allowance and contract's state in general were not changed.

Also this does not solve original problem. Lets assume the following scenario:

1. Bob is allowed to transfer 100 tokens from Alice and his used flag is already true
2. Alice wants to change his allowance to 101
3. Alice calls approve method to set Bob's allowance to zero (or to 101, does not matter because Bob's used flag is already true)
4. Bob frontruns Alice's transaction and get 100 tokens from Alice
5. Alice sees that her transaction was mined and she has no clues to know that Bob did frontrun it
6. Now Alice calls approve method again to set bob's allowance to 101
7. After Alice's transaction is mined, Bob gets another 101 tokens from Alice

So Bob got 201 tokens from Alice in total while Alice never intended to give him more than 101 token.

[nateawelch]

Yeah, as I pointed out, my one concern was the expectations around returning true/false from approve. I suppose you could just return true no matter what. Preferably, in any other programming language, I'd update the used state and then throw an error, but there's no non-reverting way to raise errors in solidity yet, if ever.

Can Alice not listen to the event emitted from her first tx? There's 3 possible scenarios in that case:

1. Alice called approve with a non-zero _value & the event _value matches hers. This means the previous allowance, if there was one, was never used
2. Alice called approve with a non-zero _value & the event _value is zero. This means the previous allowance was used.
3. Alice called approve with a zero _value. In this case the event _value will match her approve _value and she wont know whether or not the spender used the previous allowance. It doesn't really matter since she was setting it to zero anyway.

Edit: An alternative is to just involve the used flag in require((_amount == 0) || (allowed[msg.sender][_spender] == 0)), i.e. require((_amount == 0) || (allowed[msg.sender][_spender].amount == 0 && !allowed[msg.sender][_spender].used)), meaning you must reset the allowance to 0 if the spender has touched it, and this enforces it in the contract instead of just in the UI.

[maurelian]

Came here from your email to consensys-diligence @HaleTom. Thanks for addressing this safety issue.

ConsenSys - no mention or attempt to work around the issue

It seems we're going to need to start adapting that to multiple token standards. We'll monitor, and if something resembling consensys is reached be happy to provide an implementation alongside our EIP20 implementation.

[HaleTom]

Has anyone written a truffle (or other) test which illustrates this issue?

[outofgas]

Hello,
if higher gas costs were acceptable one could keep track of the so-far spent allowance. Sketch:

    struct Allowance {
        uint initial;
        uint residual;
    }
    
    mapping(address => mapping(address => Allowance)) public allowances;
    
    function approve(address spender, uint amount) public returns (bool) {
        Allowance storage _allowance = allowances[msg.sender][spender];
        
        // This test should not be necessary.
        uint spent = _allowance.initial > _allowance.residual
                   ? _allowance.initial - _allowance.residual
                   : 0;
                   
        _allowance.initial = amount;
        _allowance.residual = spent < amount ? amount - spent : 0;
        
        Approval(msg.sender, spender, _allowance.residual);
        
        return true;
    }
    
    function allowance(address holder, address spender) public view returns (uint) {
        return allowances[holder][spender].residual;
    }
    
    function transferFrom(address holder, uint amount) public returns (bool) {
        uint residual = allowance(holder, msg.sender);
        
        require(amount <= residual);
        
        allowances[holder][msg.sender].residual = residual - amount;
        
        // ... do the token transfer
        
        return true;
    }

This approach would still require the allowance to be set to zero prior to setting it to a new value.

[xhyumiracle]

I think the fix mentioned here might be helpful. Just FYI.
Personally I like the idea and it might be a great way to fix the problem, cuz I think the user who updates the allowance value should be responsible for it, (s)he should provide the latest state (tx state or allowance value) when they decide to update, otherwise the best we can do is to notice them.
What the implementation should do is to provide a handful way for users to announce the state.

[github-actions]

There has been no activity on this issue for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

[github-actions]

This issue was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.