4 ms·
what went wrong: TLDR probably Ctrl-C,Ctrl-V. (Just to be clear, this is about Zcoin, not Zcash/Zerocash. The two are completely different) The fix is here. h
by ianmiers 10y ago
what went wrong: TLDR probably Ctrl-C,Ctrl-V.
(Just to be clear, this is about Zcoin, not Zcash/Zerocash. The two are completely different)
The fix is here. https://github.com/zcoinofficial/zcoin/commit/33796c839f7d4df4fb89c2775ec971982cfc8996 https://github.com/zcoinofficial/zcoin/commit/33796c839f7d4d...
What happened?
First, some stylized facts about ZCoin:
0) ZCoin is a fork of Bitcoin that uses a 4 year old academic research library, libzerocoin, to make anonymous payments using the Zerocoin protocol.
1) Unlike Zcash/Zerocash, the Zerocoin protocol has only fixed value coins.
2) To get multiple denominations, you have completely separate instances of the anonymous currency that just happen to live on the same blockchain as the other denominations.
3) Zerocoin has its own bitcoin like non anonymous base currency. Call it basecoin.
4) You spend basecoins to get zerocoins.
5) When you spend zerocoins, you get basecoins.
6) ZQ_WILLIAMSON and ZQ_PEDERSEN are denominations, worth 100 and 50 respectively, defined in libzerocoin.
So what went wrong?
When you convert a zerocoin into 100 basecoin, the ZCoin code forked from bitcoin checked if the coin was a valid instance of ZQ_PEDERSEN (worth 50 ) not ZQ_WILLIAMSON (worth 100). So you paid 50 for the zcoin,got it into the instance for ZQ_PEDERSEN, but got back 100. Free money.
Why did this happen? Well, it looks like in order to support the multiple denominations libzerocoin offers, the ZCoin developers wrote some code for one denomination and then duplicated it for each remaining denomination. There are five in total, ZQ_LOVELACE=1,ZQ_GOLDWASSER=10, ZQ_RACKOFF = 25, ZQ_PEDERSEN = 50,ZQ_WILLIAMSON = 100.
But on the last one, ZQ_PEDERSEN was not changed to ZQ_WILLIAMSON in a few places. This caused the bug.
Caveat: I have nothing to do with ZCoin. However, I am an author of the zerocoin protocol, libzerocoin, the zerocash protocol, and am involved with Zcash.
- ballenf 10y agoAny idea why they would describe the code error as "a single additional character in code"? It looks like about 10 characters or so based on your link. There are also some other code changes associated with that commit
- ianmiers 10y agoI have no idea. If you can find a single character edit in the commit history, I will look at it. But this certainly is a bug. And it would allow you to steal funds.
- taspeotis 10y agoI think the single character fix is this [1], GP seems to be describing [2]. [1] https://github.com/zcoinofficial/zcoin/commit/b20c177032de3c4bfae62b5ada768a5dc2b4fa67 https://github.com/zcoinofficial/zcoin/commit/b20c177032de3c... [2] https://github.com/zcoinofficial/zcoin/commit/33796c839f7d4df4fb89c2775ec971982cfc8996 https://github.com/zcoinofficial/zcoin/commit/33796c839f7d4d...
- ianmiers 10y agoThat is a one character change. And it is labeled "urgent fix". So it's certainly possible. But that change appears to do exactly what the variable name suggests setting it to zero would to : stop zcoin tx's from being included in a block. That strongly suggests someone attempted to fix the issue by simply disabled all private transactions.
- skolsuper 10y agoAnother major bug caused by copy+paste. I seem to remember a security researcher article months (years?) ago that identified this theme, showed a way to grep a codebase for likely c+p errors and found a load of bugs in real production code that had remained hidden for years. I think I landed there from HN, but my google-fu is failing me now, can anyone else remember it?
- yorwba 10y agoProbably not what you mean, but this (https://news.ycombinator.com/item?id=12853211 https://news.ycombinator.com/item?id=12853211) submission about the PVS-Studio static analyzer also shows a bunch of copy+paste errors being found.
- wuschel 10y agoInteresting. How would you minimize these type of errors by design or best practice? I guess languages with a lack of higher order abstractions and no strong type system might be more prone to this type of errors.
- daira 10y agoThis isn't a subtle or difficult-to-find case. It's a case of "why the heck would anyone write code like that, in any language, in the first place?" The only language-level abstraction needed to avoid this particular kind of duplicated code, is a loop.
- stevenroose 10y agoI want to know..
- skolsuper 10y agoI went looking for this again today. It's not the article I was talking about, but you might find this interesting: http://pages.cs.wisc.edu/~shanlu/paper/TSE-CPMiner.pdf http://pages.cs.wisc.edu/~shanlu/paper/TSE-CPMiner.pdf
- daira 10y agoJust to clarify, the code that was duplicated per denomination is not part of libzerocoin itself, it's in main.cpp. I'm not sure who wrote it; it may or may not have been part of the academic prototype Ian refers to. In any case, this amount of duplication (in security-critical code, no less) should never have passed the necessary code review to release a cryptocurrency. Also note that there are still unexplained differences between the copied code branches after the security fix. (In contrast, Zcash did have duplicated code in the prototype we inherited, but we rewrote that entirely well before the Zcash launch.) [Edit: I confirmed that the duplicated validation code in main.cpp was not present in libzerocoin. Some of the code in main.cpp including some stale comments, appears to have been pasted from https://github.com/Zerocoin/libzerocoin/blob/master/Tutorial.cpp https://github.com/Zerocoin/libzerocoin/blob/master/Tutorial... , but that tutorial code does not have the bug. So it appears that it was introduced by the Moneta/Zcoin developers.] Disclosure of interest: I am a Zcash developer.