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

[zackcoburn]

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 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]

@jbrukh yep that is true

[sonic1111]

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]

@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]

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

[jbrukh]

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]

@jbrukh thanks we are in touch with them

[GriffGreen]

@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]

@GriffGreen Thanks Griff

[sonic1111]

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

[romanman]

@sonic1111 hopefully tomorrow

[sonic1111]

OK

[sonic1111]

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]

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]

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

[zackcoburn]

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]

@zackcoburn : do you have balance list?

[zackcoburn]

Yes, it's the list I sent earlier.

…

On Monday, January 16, 2017, Roman Mandeleil ***@***.***> wrote: @zackcoburn <https://github.com/zackcoburn> : do you have balance list? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APEqPXZJm_J8RZmWdWRrrjGIZ2pKBcgDks5rS0z9gaJpZM4LaVZt> .

[romanman]

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

[zackcoburn]

The list is of all accounts in the EtherDelta smart contract that have an (old) HKG balance. These balances weren't recovered on the new HKG contract anywhere since they were balances held in EtherDelta's internal structure, and presumably the recovery process didn't look for them.

…

On Monday, January 16, 2017, Roman Mandeleil ***@***.*** ***@***.***');>> wrote: @zackcoburn <https://github.com/zackcoburn> so is it just balance of the owners? aren't that recovered by us recovered they balance on the new contract anywhere ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APEqPQqWjbwDXWnDSWFUUNmfL7SC8k2Lks5rS10QgaJpZM4LaVZt> .

[romanman]

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

[zackcoburn]

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]

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

[zackcoburn]

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]

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

[zackcoburn]

I added the new HKG token to the EtherDelta UI. The new code looks fine to me. I would just go ahead and credit those EtherDelta users with their balances from the old HKG.

…

On Tue, Jan 17, 2017 at 8:43 PM, Roman Mandeleil ***@***.***> wrote: @zackcoburn <https://github.com/zackcoburn> just the simple trading eth/hkg I guess, if you want I can send you number of tokens for trying this — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APEqPSJh21Gvuykt0AS1VXTbvHelJ5UAks5rTMWOgaJpZM4LaVZt> .

[romanman]

@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]

Great!

…

On Wed, Jan 18, 2017 at 5:43 PM, Roman Mandeleil ***@***.***> wrote: @zackcoburn <https://github.com/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 ;) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APEqPatneXF3PNp7gMllGBA3PAVjZsQBks5rTezdgaJpZM4LaVZt> .

[zackcoburn]

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