search

RGB-WG / rgb-core

RGB Core Library: consensus validation for private & scalable client-validated smart contracts on Bitcoin & Lightning
https://spec.rgb.tech
Apache License 2.0
207 stars 52 forks source link

Validation of damaged consignment panics instead of error #254

Closed zoedberg closed 2 months ago

zoedberg commented 3 months ago

I've manually edited a consignment and validation panicked.

In detail it panicked at panic!("provided {opid} is absent"); (https://github.com/RGB-WG/rgb-core/blob/master/src/validation/validator.rs#L224)

I think we should change this panic to an error and more in general we should avoid panicking during validation, otherwise an attacker can send invalid consignment on purpose to make a wallet panic.

dr-orlovsky commented 3 months ago

I agree on avoiding panics overall, but here It seems panic is necessary since it reveals a business-logic bug in validation code: the index of operations seems to be inconsistent. I need to debug: can you please provide details on how you have edited the consignment (or attach the consignment which doesn't work)?

St333p commented 3 months ago

edited_consignment.zip Here is the modified consignment that causes the panic (zipped since github does not support yaml). The chain entry of a correct consignment was changed to liquid.

Error message is:

thread 'validate_consignment_chain_fail' panicked at [...]rgb-core/src/validation/validator.rs:224:13:
provided 6952f2a77ae37c17742d80d1c0328539703258674aa815b7543d7eedb8d2f365 is absent
crisdut commented 2 months ago

Looking at the implementation:

In the validation step, we use the IndexedConsignment structure. If we look at the method, which is called in line, we can conclude that either this OpId is equal to Genesis or was not indexed correctly at structure initialization.

@zoedberg can you provide the IndexedConsignment, please?

Also, @zoedberg It's not related to the error, but I noticed that your transition af2e0914c57bd299b30a38739968c1fd7baf41f102d06c46aff6641b4c7e9bee has the transitionType equal to 65535 instead of 10000

PS: This validation was recently added, see here.

zoedberg commented 2 months ago

@zoedberg can you provide the IndexedConsignment, please?

You should be able to easily convert the provided yaml consignment to a IndexedConsignment

Also, @zoedberg It's not related to the error, but I noticed that your transition af2e0914c57bd299b30a38739968c1fd7baf41f102d06c46aff6641b4c7e9bee has the transitionType equal to 65535 instead of 10000

The consignment has been created using the upstream pay method, nothing custom has been done on my side. Do you think this is a bug?