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

Vulnerability in StandardToken.sol's implementation of transferFrom() #8

Open
zackcoburn opened this Issue Jan 4, 2017 · 29 comments

Comments

Projects
None yet
5 participants
@zackcoburn

zackcoburn commented Jan 4, 2017

Hi,

Per Roman's request, I'm submitting this vulnerability report here.

StandardToken.sol (https://github.com/ether-camp/virtual-accelerator/blob/master/contracts/StandardToken.sol) has a vulnerability in the transferFrom() function:

            // do the actual transfer
            balances[from] -= value;    
            balances[to] =+ value;    

The =+ should be +=.

The vulnerable code is used in the deployed HackerGold token (https://etherscan.io/token/HackerGold).

By using approve() followed by transferFrom(), it is possible to essentially reset the balance of any account.

For example, see these two transactions:
https://etherscan.io/tx/0x8cbc0975efe91a53777211968870a4a62eea2c27dda4e69fa1a1ff3c6cb43dcb
https://etherscan.io/tx/0xfb0b85b5cb46d427933952a4d839d6f4b0bcad9f71ba9696fc7fb6ad5d359a38

The effect is that 0x2ccc5a059a1bda4c3c3c594516e812a0b15799c9's balance has been reduced from 5,000,000 HKG to 0.001 HKG.

Recommended fix:

  • Create a new HKG contract that fixes the bug and initializes all balances to what they were before the above transactions.
  • Any dapps that keep track of HKG balances internally (i.e., EtherDelta) need to be taken into account so that people who were holding balances inside such smart contracts get their tokens back.
  • Exchanges and token users will need to be notified about the transition to a new token contract.

Thanks,
Zack

@jbrukh

This comment has been minimized.

jbrukh commented Jan 6, 2017

This vulnerability also affects every hack.ether.camp team token as well, since they all inherit from StandardToken. For example, here is the RSC (Etherisc) token contract exhibiting the same vulnerability in transferFrom():

https://etherscan.io/address/0x833882e76f4967b9b18f52d70640bfcc82aa91e9#code

@romanman

This comment has been minimized.

Contributor

romanman commented Jan 6, 2017

@jbrukh yep that is true

@federicobond federicobond referenced this issue Jan 6, 2017

Open

Rules Wishlist #44

4 of 9 tasks complete
@sonic1111

This comment has been minimized.

sonic1111 commented Jan 6, 2017

Hi Please help me!Hi i can not to find my HKG tokens i have 41000 HKG in my etherwallet i did not send out it at all, but now i can not to see it. Not in https://ethplorer.io/address/0x6934d7f82b6d57b630bae786c10a8651c639f7d7 not in https://hack.ether.camp/sale What happend??? also i can not to see out transaction HKD in https://etherscan.io/address/0x6934d7f82b6d57b630bae786c10a8651c639f7d7 I did not send out it it was in my etherwallet but now its not there!!!!

@romanman

This comment has been minimized.

Contributor

romanman commented Jan 6, 2017

@sonic1111 we found a flaw that we are going
to fix and swap for you, so no worries you will
see the tokens soon

@sonic1111

This comment has been minimized.

sonic1111 commented Jan 6, 2017

Ok thanks very much i was nerrows becouse i can not to see them nowhere!?

@jbrukh

This comment has been minimized.

jbrukh commented Jan 6, 2017

For team tokens, we recommend that team owners examine their token distributions and make a snapshot of the distribution in case someone decides to make use of the vulnerability.

@romanman

This comment has been minimized.

Contributor

romanman commented Jan 6, 2017

@jbrukh thanks we are in touch with them

@GriffGreen

This comment has been minimized.

GriffGreen commented Jan 8, 2017

@romanman If you are looking for a new token contract, Jordi Baylina wrote one that might fit what you are trying to do really well:

Solidity code:
https://github.com/Giveth/minime/blob/master/MiniMeToken.sol

Blog post:
https://medium.com/giveth/the-minime-token-open-sourced-by-giveth-2710c0210787

@romanman

This comment has been minimized.

Contributor

romanman commented Jan 8, 2017

@GriffGreen Thanks Griff

@sonic1111

This comment has been minimized.

sonic1111 commented Jan 8, 2017

Hi, will you tell me when i can see my ballance of HKG correct? Not 0.001?! When this bug will be fixed?

@romanman

This comment has been minimized.

Contributor

romanman commented Jan 8, 2017

@sonic1111 hopefully tomorrow

@sonic1111

This comment has been minimized.

sonic1111 commented Jan 8, 2017

OK

@sonic1111

This comment has been minimized.

sonic1111 commented Jan 10, 2017

hi i see my correct ballance in my etherwallet and i see correct ballance in https://ethplorer.io but i can not to see correct total out in https://ethplorer.io opposit my ballance https://ethplorer.io/address/0x6934d7f82b6d57b630bae786c10a8651c639f7d7 I see total out is 0 but total our must be 5000

@zackcoburn

This comment has been minimized.

zackcoburn commented Jan 11, 2017

Here is a list of addresses that were holding HKG on EtherDelta before the bug.

0xd7e04ebc1a4b6b24ed5bac6709a0d3f919ae93f2 5
0x4f2e07a59363c4da39cf763ddaf51ed94ca0c768 5000
0x8506d946cc63d1f1f3a303d68b0da64597cd64f3 20998.878
0x45bcd6f98b77e44a842a8cdbbf2942c9dc9403a2 44.755
0x7bd830110cc5455527fe459e8690ab765ae163e3 121.932
0x79c43e47632690e6fd38647683e52bdc11fa9946 0.011
0x6266ea5e0d11ec74f87650757335e55672bea18d 19.969
0x9975d072d4cdf7fc8dd3786f8279874609f9083a 0.002
0x12c3994388f6eb469a2ac775ac42712685f4f8be 1000

These balances should be credited to the account holders in the new token.

To the best of my knowledge, this hasn't happened yet, but I'm not sure if you still have transfers to make. I see the new token transfers happening here: https://etherscan.io/token/HackerGold?a=0x342e62732b76875da9305083ea8ae63125a4e667.

@romanman

This comment has been minimized.

Contributor

romanman commented Jan 12, 2017

@zackcoburn who is running EtherDelta and can update the contract there?

@zackcoburn

This comment has been minimized.

zackcoburn commented Jan 12, 2017

I'm running EtherDelta. I can't just update the contract because:

  • The EtherDelta smart contract didn't receive any New HKG. Since it used transferFrom to do deposits, its HKG balance ended up at zero prior to the bug. But that's fine because EtherDelta can't receive tokens directly like that anyway. It has no way of assigning tokens in its internal balance structure unless they are received via the deposit() method.
  • I can't produce the New HKG for these users.
  • Even if I had the New HKG, I have no way of assigning it to the internal balance structure in the EtherDelta smart contract.

The best solution is to just credit those users who had HKG balances on EtherDelta prior to the bug with New HKG sent to their addresses.

@romanman

This comment has been minimized.

Contributor

romanman commented Jan 16, 2017

@zackcoburn : do you have balance list?

@zackcoburn

This comment has been minimized.

zackcoburn commented Jan 16, 2017

@romanman

This comment has been minimized.

Contributor

romanman commented Jan 16, 2017

@zackcoburn so is it just balance of the owners?
aren't that recovered by us recovered they balance
on the new contract anyway ?

@zackcoburn

This comment has been minimized.

zackcoburn commented Jan 16, 2017

@romanman

This comment has been minimized.

Contributor

romanman commented Jan 16, 2017

@zackcoburn yes, but they have some parallel in the HackerGold balance
how is that reflected on the token structure?

@zackcoburn

This comment has been minimized.

zackcoburn commented Jan 16, 2017

Each of these users may have had a balance in old HKG outside of EtherDelta, which you would have already accounted for in the recovery process. The balances from this list need to be added to the amounts already recovered for these accounts.

@romanman

This comment has been minimized.

Contributor

romanman commented Jan 16, 2017

@zackcoburn : ok we have budget to cover that, can you
try the new contract on the system?

@zackcoburn

This comment has been minimized.

zackcoburn commented Jan 16, 2017

You actually shouldn't even need budget to cover this, assuming you initialized the master recovery account with the entire token supply.

What do you want me to try with the new contract?

@romanman

This comment has been minimized.

Contributor

romanman commented Jan 17, 2017

@zackcoburn just the simple trading
eth/hkg I guess, if you want I can
send you number of tokens for trying
this

@zackcoburn

This comment has been minimized.

zackcoburn commented Jan 18, 2017

@romanman

This comment has been minimized.

Contributor

romanman commented Jan 18, 2017

@zackcoburn I had double checked the list and refunded all the accounts there
I think we are ready to announce that EtherDelta is good to go again ;)

@zackcoburn

This comment has been minimized.

zackcoburn commented Jan 18, 2017

@zackcoburn

This comment has been minimized.

zackcoburn commented Jan 19, 2017

I think some of these addresses still need more transferred. If the number in the list above is A, and the balance before doing these new transfers was B, the new balance should be A+B, not B.

For example, this address should have received a new transfer of 21,000, but he only received 11,000: https://etherscan.io/token/HackerGold?a=0x8506d946cc63d1f1f3a303d68b0da64597cd64f3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment