Approve method attack

[Rapso]

So it is possolible that attacker will spend old and new balance available to him in case of unfortunate transaction ordering. More about it you can find here https://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/edit#heading=h.m9fhqynw2xvt
You are using StandardToken so it is mentioned also in their code

/**

• @dev Approve the passed address to spend the specified amount of tokens on behalf of msg.sender.
• Beware that changing an allowance with this method brings the risk that someone may use both the old
• and the new allowance by unfortunate transaction ordering. One possible solution to mitigate this
• race condition is to first reduce the spender's allowance to 0 and set the desired value afterwards:
• ERC: Token standard ethereum/EIPs#20 (comment)
• @param _spender The address which will spend the funds.
• @param _value The amount of tokens to be spent.
  */

to solve this you can add such code to the approve function
require((_value == 0) || (allowed[msg.sender][_spender] == 0));

eth address - 0xE14f7A9AF3F75a4Ccd33909B0046b16d82c6EC0e

[artemii235]

Hello, @Rapso !
Thanks for opening an issue! We will check this information and apply fixes if needed. After we review the problem we will decide the amount of JCR you'll receive for this issue and comment it here.

[artemii235]

Hello @Rapso!

We have explored information about this attack and decided to not implement suggested fix. According to this comment ethereum/EIPs#738 (comment) require((_value == 0) || (allowed[msg.sender][_spender] == 0)); is not sufficient. Also as we can see there is no ECR20-compatible method to resolve it. Current suggestions are to add increaseApproval`decreaseApprovalor addsafeApprove(spender, new_value, expected_old_value)` function, but this is not included in standard and can't be used with tokens which are already deployed to Ethereum blockchain.

Thank you for creating the issue. We will be glad to consider new suggestions if you have any.

[ilejn]

Sorry to intervene, but I would say that assertion require((_value == 0) || (allowed[msg.sender][_spender] == 0)); IS sufficient. There is a fundamental difference between "the attack is not preventable" AND "the attack is preventable in a tedious way".
Anyway, the fix is not really important e.g. because many currently existing contracts with the issue.

[artemii235]

Hello @ilejn!

Thank you for your message. According to this ethereum/EIPs#738 (comment) and this OpenZeppelin/openzeppelin-contracts#438 (comment) comments we can see that in some case if Alice is not able to understand that Bob transferred N tokens Bob will be still able to transfer N+M tokens. In general this code require((_value == 0) || (allowed[msg.sender][_spender] == 0)); does not prevent Bob from transferring N allowed tokens even if Alice doesn't want it anymore. Users should consider that if they allow someone to transfer N tokens (even by mistake) and this is confirmed by Ethereum network - these is NO way to prevent the allowed side to transfer exactly N tokens - no matter if they set allowance to zero or just lower value.

[ilejn]

Hello @artemii235 , I am not sure that this is appropriate place for discussion, while comments your are referring to say opposite thing, namely that this check gives possibility to prevent this attack if contract provides proper traceability.

[nateawelch]

@artemii235 I've posted a possible backwards compatible solution to this problem ethereum/EIPs#738 (comment).

Short description is that it stores a bool used along with each allowance. used is true if a spender used some/all of their allowance since owner last updated it. If used for a given spender is true in approve, then it sets the allowance to 0 and returns false. This is preferable over require((_value == 0) || (allowed[msg.sender][_spender] == 0)); because in that case, spender could front run owner's tx changing the allowance and transfer all the allowance first, then once owner's tx goes through, the new allowance is available. In the solution I posted, this attack isn't possible because the allowance will default to 0 on approve if some/all of the previous allowance was used.

[artemii235]

Hello @flygoing!

Thank you for your suggestion, I have checked the example at https://gist.github.com/flygoing/836666010f0a5bf91abac211df938611. This solution is not backward-compatible and also might confuse the caller of approve method, example:

1. allowed[Alice][Bob].amount = 1 Ether and allowed[Alice][Bob].used = true
2. Alice calls approve(Bob, 2 Ether).
3. This code:

if (allowed[msg.sender][_spender].used) {
    allowed[msg.sender][_spender].amount = 0;
    allowed[msg.sender][_spender].used=false;
    Approval(msg.sender,_spender,0);
    return false;
}

breaks standard - https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20-token-standard.md#approve (If this function is called again it overwrites the current allowance with _value) - resulting allowance must be 2 Ether but it will be set to 0.

[nateawelch]

@artemii235 Thanks for the response. I see your point there, I didn't account for the fact that it'd break standard to do the automatic set to 0. Here's another, less automatic alternative that I don't believe breaks standard: https://gist.github.com/flygoing/2956f0d3b5e662a44b83b8e4bec6cca6

This fixes the issue with the common solution require((_value == 0) || (allowed[msg.sender][_spender] == 0)) that you discuss above. That solution makes it so Bob can still spend the full balance of the allowance before Alice's change of the allowance is mined. The slight alteration of added used to that check adds the security of failing it if it was spent, making the user reset is to 0 and then set it to what they want if they need to.

IMO there isn't a perfect solution to this issue that is within standard. The real solution is to implement a new token standard that has safe functions but also has backward compatibility with current ERC20 implementations.

[artemii235]

@flygoing
I agree with you. Even EIP (https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20-token-standard.md#approve) tells us that

NOTE: To prevent attack vectors like the one described here and discussed here, 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 compatilibilty with contracts deployed before

Seems like we have to wait for new standard. Until it's released developers of applications dependent on approve/transferFrom should keep in mind that they have to set allowance to 0 first and verify if it was used before setting new value.