z-classic / zclassic

Zclassic is financial freedom. ZK-SNARKs, and no founder's fee.
https://www.reddit.com/r/Zclassic/
Other
189 stars 81 forks source link

zclassicjs: Unspendable Outputs because OP_NOP5 is NOT implemented in the reference client #194

Open ghost opened 5 years ago

ghost commented 5 years ago

I was made aware of an issue where a user had used a ZenCash library against ZCL which rendered the Change output unspendable. As you can see, the transaction has been mined. Here's the transaction;

http://explorer.zclmine.pro/tx/678364244bffd778ba4659d657892f51c57db077813873fbaedd35d720c9991a

And here's the transaction script;

OP_DUP OP_HASH160 2352a7109c0e5db56d03c0cb7ea60f2e9b05c747 OP_EQUALVERIFY OP_CHECKSIG 0 0 OP_NOP5 Had this transaction been on ZenCash, the OP_NOP5 was implemented as OP_CHECKBLOCKATHEIGHT and the previous 2 values (0 , 0) would have been evaluated as the parameters ParamHeight and ParamBlockHash.

Since OP_NOP's are basically ignored when not implemented, as is the case with ZCL, then the last item on the script stack would have been 0, and therefore, I believe the output rendered unspendable.

I have asked around a little, and people seem to agree with my conclusion above. However, it has been raised that the outputs could possibly become spendable in the future. for example, if ZCL was to implement the replay protection (OP_CHECKBLOCKATHEIGHT) however, looking back through the history of ZCL it was actually removed in order to mirror ZCASH more closely.

There are obviously other changes or forks that can be done to make them spendable too.

Spending of the outputs theories could be tested by running a 1 node testnet.

It could be wise to analyse the chain to see how many of these transactions exist

The ideal outcome and closure of this ticket would be at minimum a proposal of how to spend the outputs in the future or now.

nimbosa commented 5 years ago

I am reading this just now, and it sounds like an interesting corner case scenario. @thedevworks

I have a few questions at the top of my head:

  1. is this a shielded transaction?

  2. why would someone use ZenCash software for spending on Zclassic network? was this for dev testing/ experimentation? or on production? this is an inherent risk in unimplemented features such as the one cited here, it would have been better to run the tx on testnet FIRST or write a test for it..

I will look more closely when I have the time. Feel free to contact me at the usual places, I am restoring my crypto communications channels one at a time (Telegram, Discord, etc..)

If anyone has more insight on this, please feel free to chime in and share your view. Thanks!

ghost commented 5 years ago

@nimbosa

  1. Nope, Clear
  2. They used this repo; https://github.com/z-classic/zclassicjs I guess it was never finished off

You are correct that they are entirely in the wrong, there should be at least minimum automated testing of making a transaction in a wallet before releasing to customers.. However, I was wondering if the outputs would ever become spendable again.

The wallet in question took the hit refunding the customer!

I suggest we remove that

ghost commented 5 years ago

Can you get back on keybase? I've been trying to get hold of you for weeks!

nimbosa commented 5 years ago

Thanks for the details @thedevworks I will get back on keybase later. ;)

I am not aware of the repo above and it is not a clear fork from ZenCashJS, so we have to manually check code change and its coverage. I would prefer making a public fork of a ZenCash/Zcash repo and start work from there so the difference would be clear and the results a little more predictable and easier to compare.

I hope this becomes a lesson for everybody. 🎉 🥂

nimbosa commented 5 years ago

found the source: https://github.com/ZencashOfficial/zencashjs

we can work two step backwards on this and maybe one step forward here in the reference client

I am inclined to move this to the right repo (https://github.com/z-classic/zclassicjs) as soon as a new issue or pull request comes up to resolve or implement this with specifics