Method decreaseApproval in unsafe

[3sGgpQ8H]

Method decreaseApproval in StandardToken.sol is unsafe. Here is the scenario.

1. Bob is allowed to transfer zero Alice's tokens
2. Alice allows Bob to transfer 100 of here tokens via approve or increaseApproval method and transaction is executed successfully
3. Alice sees that Bob is now allowed to transfer 100 of her tokens
4. After some time, Alice uses decreaseApproval method to decrease by 100 the number of her tokens Bob is allowed to transfer and transaction is executed successfully and proper Approval event was logged
5. Alice sees that Bob is allowed to transfer 0 of her tokens
6. Now Alice may think that once decreaseApproval call was executed successfully, then Bob didn't manage to transfer any of her tokens before the allowance was decreased, but this assumption is wrong.
   Actually, Bob may or may not had transferred Alice's tokens before allowance was decreased, and Alice has no easy way to know for sure whether Bob transferred her tokens or not

Method decreaseApproval should fail in case current allowance is lower than requested decrease.

[frangio]

Hi @mikhail-vladimirov, thanks for reporting! (And thanks for the overall thorough review.)

I thought about this and I'm not sure I agree with either the risk or the proposed mitigation. At point 4, once Alice sees her call succeeded, she would be able to see any Transfer events if Bob had managed to make a transfer, and in fact she would see her own reduced balance.

Furthermore, suppose we changed decreaseApproval to fail if allowance was lower than the request decrease. If Alice attempts to decrease Bob's allowance to zero, and Bob manages to transfer half of his allowance before the decrease, the call to decreaseApproval would fail and then Bob would be able to spend the other half! This is in my opinion worse than the current risk. What do you think?

[3sGgpQ8H]

she would be able to see any Transfer events if Bob had managed to make a transfer

If Bob had transferred Alice's tokens to Carol, not to himself. it would be very non-trivial to know that this transfer was done by Bob, not Carol.

Bob would be able to spend the other half!

Bob anyway had a chance to get all the allowed tokens, and he had this ability for a long time, nothing bad if he will be able to do this for little more time. Bad thing is not that Bob is able to get tokens, this is actually normal, because Alice herself allowed him to do this. Bad thing is that after changing the allowance, Alice does not know how much of the original allowance was actually used by Bob, and thus does not know how much tokens she owes Bob, or Bob owes her; so Bob may actually cheat Alice by pretending that he didn't use any allowance (thus Alice still owes some tokens to Bob) while he actually did use it.

So safe decreaseApproval method should succeed only if allowance was actually decreased exactly by desired number of token, not by some other number. If Alice just wants to revoke all the allowance from Bob as fast as possible, she will use approve method to set allowance to zero.

[frangio]

If Bob had transferred Alice's tokens to Carol, not to himself. it would be very non-trivial to know that this transfer was done by Bob, not Carol.

This is a different problem that I completely agree with, which is the lack of a spender parameter in Transfer events. We could add a new event that allows to keep track of this, it's a pity that there isn't a standard one.

Bob anyway had a chance to get all the allowed tokens, and he had this ability for a long time, nothing bad if he will be able to do this for little more time.

Suppose "Bob" is a smart contract that is programmed to make a transfer with a certain frequency and you want to stop it immediatey. I agree that approve with zero is the correct thing to do here, but it may not be immediately apparent from the function signatures and someone might use decreaseApproval.

I share some of the concerns but I think most of the risk can be mitigated by logging how much an approved spender has transferred. Do you agree?

I'm open to hearing more opinions though. Have you reported this elsewhere?

[3sGgpQ8H]

There are absolutely no problems with Bob being able to transfer as many tokens as Alice approved to him until Alice revoked the approval. This is how token contracts are supposed to work. Regardless of what Bob does, once Alice approved some tokens to Bob, she approved, and it is her full responsibility to make sure Bob is a good guy before approving anything to him.

The real problems with ERC20 are:

1. In some cases Bob may transfer MORE Alice's tokens than Alice ever intended to approve to him. For example, Alice approved 100 token, then she wanted to change this to 101. While Alice never intended to allow Bob to transfer more than 101 of her tokens, Bob may actually transfer 201 which is really bad.
2. In some cases Bob may transfer as many Alice's tokens as Alice approved to him (which is fine) but may make it look like he didn't transfer them (which is really bad). Thus Bob may, for example, avoid paying Alice for tokens he actually got from her.

In current form, decreaseApproval addresses the first problem but didn't address the second one which is only slightly less dangerous.

[jakub-wojciechowski]

Hi Guys, I've enjoyed reading the discussion. Thx, for pointing out the vulnerability @mikhail-vladimirov!

Currently we have a soft condition requiring that the deduction is lower than allowance or the allowance is reduced to zero. How about making it stronger and requesting that the current allowance is equal to a state known by transaction sender.

function decreaseApproval (address _spender, uint _decreaseFrom, uint _decreaseBy)
...
require(allowed[msg.sender][_spender] == _decreaseFrom);

Using this approach we will make it sure that the operation exactly satisfies the intentions of the sender and the current state of the contract matches state know by the sender. It will also make the whole process of allowance update atomic and prevent race conditions. Actually, the proposed design will be a form of Compare and Swap pattern which is widely used in concurrency programming.

[3sGgpQ8H]

This should work, but in this case you probably need to remove "decrease" from the method name and make it able to change allowance in either direction.

[frangio]

I like @jakub-wojciechowski's proposal because it does make the semantics quite apparent from the function signature. I feel like I've seen the Compare and Swap function proposed before as a solution to the approve race condition.

[jakub-wojciechowski]

I'm really glad that you liked it. I'm certainly not the one to take credit for the Compare and Swap pattern as it's one of the most widely used lock-free synchronisation strategy and probably older than me :)

I agree with @mikhail-vladimirov that it'd be better to merge both decreaseApproval and increaseApproval in single method. What's your view on that @frangio ?

[3sGgpQ8H]

Here is an example of compare and set approve function in ERC20 token smart contract: https://github.com/abdk-consulting/icos-token-contract/blob/master/src/sol/ICOSToken.sol#L106

[SylTi]

@mikhail-vladimirov that example is not ERC20 compliant.

This should also be taken into account #438

[3sGgpQ8H]

that example is not ERC20 compliant

It is compliant with original ERC20. It does not strictly comply with EIP-20 though, because it was created before EIP-20 was finalized.

[SylTi]

it's been a while that approve only took 2 parameters in the draft

[3sGgpQ8H]

Oh, now I see what did you mean when said that contract I referred to is not ERC20 compliant. Of cause that contract has standard two-argument approve method as well, you probably just didn't noticed it because it is defined in base contract that contract inherits from: https://github.com/abdk-consulting/icos-token-contract/blob/master/src/sol/AbstractToken.sol#L83

[frangio]

I'd go ahead with a CAS function. What should we name it? It's technically possible to call it approve with three parameters but it could be confusing.

[SylTi]

since safeApprove is taken, maybe secureApprove?

[Neurone]

Interesting topic. I agree that decreaseApproval is currently unsafe, but it seems to me it suffers exactly the same problem of the approve method, so I still think that the safest approach is the set-0,check-balanche,think-how-much-allowance-you-want-to-set,set-allowance-again one.

I don't feel it is complicated to understand what happens to tokens, and in any case I believe that any possible difficulty in understanding what has happened in the past, it is surely to be preferred to the uncertainty of what will happen in the future.

When I send a TX, I pay for that, and I want an effect. On the contrary, with implementation of CAS the spender has the power to completely stop the effect of my TX. Moreover, if the check is implemented with require I pay the maximum of gas because the contract throws an exception.

The CAS approach is an interesting solution in concurrency programming but in the execution of Ethereum transactions there's no concurrency at all, so it think that pattern does not fit very well.

[3sGgpQ8H]

@Neurone

I still think that the safest approach is the set-0,check-balanche,think-how-much-allowance-you-want-to-set,set-allowance-again one.

The problem here is with 'check-balance' step. If your account is quite busy, tokens are constantly going in and out and you approved transfers from it to many people, then your balance is constantly changing even without your intervention and checking balance will tell you nothing about whether the one whose allowance you just tried to revoke managed to use it in the very last moment or not; but you need to know this for sure to perform the next step: think-how-much-allowance-you-want-to-set.

[Neurone]

Correct, but the difficulty to understand what is happening on your balance is not related to your intention to modify the allowance at all, that problem is there in any case. To monitor your token balance you need to establish an app that reads all blocks and transactions related to your token contract, and then you can understand who moved the tokens looking at the sender of the transacion.

So, let's say you have implemented an app that monitor your tokens, do you still need a CAS approach? If the answer is no, as I think, then I disagree to define the CAS approach as a best practice for setting allowance.

[3sGgpQ8H]

@Neurone

difficulty to understand what is happening on your balance is not related to your intention to modify the allowance at all

This relation exists and you described it in the schema you suggested as the safest way to change allowance: set-0,check-balance,think-how-much-allowance-you-want-to-set,set-allowance-again

Difficulty to understand what is happening on your balance directly affects you ability to perform check-balance step and thus directly affects your ability to execute the whole procedure.

[SylTi]

So, let's say you have implemented an app that monitor your tokens, do you still need a CAS approach? If the answer is no, as I think, then I disagree to define the CAS approach as a best practice for setting allowance.

The answer is yes, you still do. Because the balance could still change between the moment you check your balance on your app and the moment your transaction to set the new allowance is recorded.

[Neurone]

I'm not convinced yet :)

I'm thinking about this workflow:

1. Bob can move my tokens because I allowed him to do so, let's say for 200 tokens
2. During allowance period Bob moved 20 tokens, so now the remaining allowance is 180 tokens
3. Now I want to change the allowance to 50, so I send an approve(Bob, 0)
4. I wait some blocks, monitoring my tokens and Bob's transactions
5. I understand what happend and I calculate if some tokens was moved, and I have three options:
   a) Bob moved more tokens that I wanted him to move with the new limit, let's say 80: sorry, better luck next time. But at least now Bob can't move the remaining 100 tokens anymore
   b) Bob moved less tokens then I want him with the new limit, let's say 20 tokens. But now Bob can't move tokens anymore, so I choose the new limit, let's say 30 tokens, and I submit an approve(Bob, 30) transaction
   c) Bob don't moved any tokens and now the Bob's allowance is set to 0. So I send a approve(Bob, 50)

At step 3, if I send a decreaseApproval(Bob, 180, 130) transaction with a CAS approach, the differences in the above workflow are at step 4.a and 4.b. In both cases:

1. My TX is rejected
2. I pay the maximum gas cost
3. Bob is still able to move all remaining previously allowed tokens

So, why decreaseApproval(Bob, 180, 130) with CAS should be considered safer then approve(Bob, 0)?

[3sGgpQ8H]

@Neurone

I wait some blocks, monitoring my tokens ...

If your account is busy, tokens are constantly going in and out, and many people are allowed to transfer from it, then monitoring your tokens will not help because your token balance will be changing both ways all the time even without Bob's interventions.

... and Bob's transactions

Assuming that Bob is a contract (reasonable assumption, because approve/transferFrom is often used to send tokens to contracts), you cannot do this using standard tools and Web3 API. You have to either run modified node that is able to capture internal transactions performed by contracts, or to run sophisticated script inside non-modified node, which script will capture and trace all transactions trying to find internal transactions performed by Bob. Both ways are too complicated to be recommended as a general way to increase safety of token operations.

[Neurone]

Yes, again, I already agreed with you on this point: monitoring tokens movements is difficult (not impossible). This monitoring is difficult in any case, and using decreaseApproval (with or without CAS approach) does not solve the necessity of monitoring.

Another point is this: if you implement a CAS approach inside the decreaseApproval(Bob, 180, 130) function you don't have any security advantages compared to the approve(Bob, 0) function, only drawbacks.

[3sGgpQ8H]

@Neurone

This monitoring is difficult in any case, and using decreaseApproval (with or without CAS approach) does not solve the necessity of monitoring.

CAS approve solves problems mentioned in original article as well as the problem mentioned in original description of this issue. Could you please describe scenario where monitoring is still needed for safe management of tokens and allowances even if CAS approve would be available?

[SylTi]

@Neurone that depends on how you use approve.
In my opinion, you should only approve the exact amount of tokens that should be withdrawn by the other party. Not more.
And the CAS approach (with a single function), in this case, guarantees that the other party didn't withdraw anything before you change the approved amount.
And in the case that the approver is a smart contract, it reduce the gas cost (you don't have to call allowance, and a single call to CASapprove())

[Neurone]

@mikhail-vladimirov Sorry but I can't :-) basically because I think the CAS approach in the decreaseApproval does not solve the problem mentioned in the original description of this issue, that was step 6:

Now Alice may think that once decreaseApproval call was executed successfully, then Bob didn't manage to transfer any of her tokens before the allowance was decreased, but this assumption is wrong.
Actually, Bob may or may not had transferred Alice's tokens before allowance was decreased, and Alice has no easy way to know for sure whether Bob transferred her tokens or not. Actually, Bob may or may not had transferred Alice's tokens before allowance was decreased, and Alice has no easy way to know for sure whether Bob transferred her tokens or not

I simply don't think is a good approach blocking a TX that want to modify the state, just because I can't easily understand that state.

[SylTi]

I simply don't think is a good approach blocking a TX that want to modify the state, just because I can't easily understand that state.
@Neurone I think it's better in terms of security and gas efficiency if the approver is a contract.

[Neurone]

@SylTi once approved, no subsequent TX can guarantee to block the other party to widthdraw that amount. If CAS approach is implemented, you are enslaving the result of your TX to the behaviour of someone else. You can't control that behaviour, so you don't know if TX will be executed or it will throw an exception. I believe that is not a good approach having this kind of uncertainty when calling a smart contract's function.

@mikhail-vladimirov @SylTi Clearly we have different visions about smart contract design. It's not a problem for me, let's have a separated decreaseApproval function with CAS approach, but I'll continue to recommend the approve(Bob,0) one :)

[jakub-wojciechowski]

We've got a very interesting discussion in here. There is no silver bullet to solve all of the problems with the approve method. Imho, the best solution is to have 2 separate methods:

1. The old decreaseApproval with the small improvement of returning (instead of the boolean success flag) the value of the final subtractedValue. That will help to mitigate the original problem when the current approval is less than the intended modification by signalling to the caller what the effective reduction was. What do you think about it @mikhail-vladimirov ?

2. The new CAS approach which has the advantage of an atomic update that rigorously checks for entry conditions.

It will be up to the users whether they want their transaction to be effective immediately with the risk of inconsistency with their expectations or to perform the update only under strict preconditions. It slightly reminds me the battle between optimistic and pessimistic locking in relational databases.

[3sGgpQ8H]

@jakub-wojciechowski

of returning (instead of the boolean success flag) the value of the final subtractedValue

It is not possible via standard tools and Web3 API to get return value of contact call executed non-locally (i.e. mined as a transaction), so this will not help a lot. This probably will be changed in future (ethereum/EIPs#658), but as for now, this is not a solution from my point of view.

[Neurone]

Good news. EIP685 was merged in August and it was released few days ago with Geth 1.7.

[frangio]

Thanks for the discussion guys, after some consideration we've decided to go with the original proposal by @3sGgpQ8H.