Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SoHoAudit] Race Condition via “ERC20 API Attack” on ERC20Token.sol #850

Closed
JonHaas opened this issue Jul 11, 2018 · 0 comments
Closed

[SoHoAudit] Race Condition via “ERC20 API Attack” on ERC20Token.sol #850

JonHaas opened this issue Jul 11, 2018 · 0 comments

Comments

@JonHaas
Copy link

@JonHaas JonHaas commented Jul 11, 2018

A known race condition exists within the present implementation of the ERC20 standard. Due to the nature of this vulnerability being an inherent flaw in the ERC20 standard, considerations must be made for any divergence (as modifications made while no longer be ERC20 compliant).

The scenario for exploitation is as follows:

  1. Alice calls approve(Bob, 1000), allocating 1000 tokens for Bob to spend
  2. Alice opts to change the amount approved for Bob to spend to a lesser amount via approve(Bob, 500). Once mined, this decreases the number of tokens that Bob can spend to 500.
  3. Bob sees the transaction and calls transferFrom(Alice, X, 1000) before approve(Bob, 500) has been mined.
  4. If Bob’s transaction is mined prior to Alice’s, 1000 tokens will be transferred by Bob.
  5. However, once Alice’s transaction is mined, Bob can call transferFrom(Alice, X, 500), transferring a total of 1500 tokens despite Alice attempting to limit the total token allowance to 500.

The particular exploit requires the usage of both the transferFrom and approve functions. As demonstrated above, the race condition occurs when one calls approve a second time on a spender that has already been allowed. If the spender sees the transaction containing the call before it has been mined, then the spender can call transferFrom to transfer the previous value and still receive the authorization to transfer the new value. While a multitude of approaches do exist to prevent this particular behavior from being exploited (one example of such is included below), they unfortunately may cause downstream functionality issues with those who implement such changes.

A potential fix includes preventing a call to approve if all the previous tokens are not spent through adding a check that the allowed balance is 0:

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

At this point, this is more or less a consideration of whether or not the team determines this to be an acceptable risk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.