Malformed asset transactions are accepted and propogated

[kralverde]

Describe the issue

After an OP_RVN_ASSET, the asset information should be pushed to the stack as per Script OP_PUSHs. However, some transactions are using standard variable length integer formats to denote the length of the asset information.

For example at block height 1658946 (timestamp is after the most recent release of core nodes), txid 0ce152e4437ebb3a8e71e8000bc3ff2b469ff33e6095c3810cd66af587a1f89c, vout 3: The script is 76a91438b7e8b4e3441e01958ba7b6086914bd1f4bf93888acc05072766e711e524156454e434f494e5f3236302350524f4a4543543333415254554e495400e1f505000000000000011220bd775bb9c62587341f6107b8bb25fb014e0548129d11cf692351c41a436ebfa875

The hex value directly after c0 is 50 which is greater than the maximum OP_PUSH. This value, 80 in base 10, does however denote how long the asset portion is.

https://rvn.cryptoscope.io/api/getrawtransaction/?txid=0ce152e4437ebb3a8e71e8000bc3ff2b469ff33e6095c3810cd66af587a1f89c&decode=1

A example of a(n assumed) proper asset transaction can be found at block 1785288 txid 892477ac125ca490b2ca9c31681bfaed1ec12321f8151bac0991e6b83ca4efcb vout 3 The script is 76a914a16de7791a594085e7c5cce3e02801df9cb6005288acc04c4f72766e711d5348494e524123544849525445454e5f4e494748544d415245535f303400e1f505000000000000011220919c561f88b12575cf3386a0803162ca3770d94e88739835e6cf1bc6e8169d0375

The hex after c0 is 4c4f; a proper OP_PUSH denoting the length of the asset portion.

These serializations formats are in conflict with one another. One or the other should be properly checked for.

A brief look through the code leads me to believe this may stem from this portion of code: https://github.com/RavenProject/Ravencoin/blob/f31e43d7a0f9dcadab9db1a9f1bc6016609e738e/src/script/script.cpp#L245 check hardchecks if an b'r' is in one of two positions instead of properly decoding what comes after an OP_RVN_ASSET.

I believe using an iterative parse of a script to find an instance of an OP_RVN_ASSET and get the data afterwards would fix this issue and future proof this method for non-p2pkh (and soon p2sh) asset scripts. Maybe something similar to this? https://github.com/Electrum-RVN-SIG/electrumx-ravencoin/blob/d8fa7600334e136ab5bfb73b84d816665eae1460/electrumx/lib/script.py#L241

EDIT: Incorrect asset script lengths are also accepted as seen in https://rvn.cryptoscope.io/api/getrawtransaction/?txid=71b08204ba41d82cf60a6926ed833da38d01fbb520dd67acf542b04a6655f0b6&decode=1 (All of these scripts are one byte longer than the OP_PUSH says they are)

Invalid scripts in general are also accepted if the OP_RVN_ASSET is in the correct place as seen in VOUT1 from https://rvn.cryptoscope.io/api/getrawtransaction/?txid=bccb8458babe041a5845a1052c17d9c70a9eb7c2fe288dd483d37c3179181f52&decode=1

[hans-schmidt]

Thank you for this excellent documentation. This is exactly what I was looking for. I think we can understand why these have been permitted to happen by the current code. I am running a matrix of tests to check how the revised code responds so that we can decide how to proceed.

[HyperPeek]

Excellent finding. I looked into properly checking the size of the script when looking for a solution to our p2sh issue, but that turned out to be not easy as there are various "wrong" tx already on the chain. In numerous discussions we ultimately came to the conclusion that it is probably better to not restrict tx scripts (thus do changes to the existing code) but rather allow whatever was allowed before the fork. In light of the above we should, however, recheck the ramnifications of the existing loose checks and if they pose any risk beyond creating non-standard tx's.

EDIT: Looking at the asset code in script.cpp once more, there actually does not seem to be any place where the size of the script (at (*this)[26][27] or (*this)[24][25] for p2sh) is checked. Whenever the size is required, this->size() is used, which is the explicit size of the script as read in.

I thus wonder if this may have been done on purpose to allow for more flexible usage. There does not appear any risk with this to me, as even a valid tx can get pretty big and fee is per kb of data on the chain. We should, however, document the supposed behaviour, especially if the size element of the tx is unused in scripts.

[fdoving]

If this needs to change, it should change on AreP2SHAssetsDeployed - as it will already be a hardfork.

[kralverde]

I find this fairly problematic since these are technically invalid, un-parsable scripts that are being accepted.

I'm not sure about the changes in the next update, but if its the isAssetScript method that is used to check if an asset is good or not, wouldn't it make sense to put a length check behind a AreP2SHAssetsDeployed check in there?

Or maybe some form of check when receiving a transaction whether it be by broadcast or sync to see if a script is actually parsable after AreP2SHAssetsDeployed

EDIT: While this isn't crazy important, I see this as something that needs to be taken care of at some point.

[fdoving]

What are the negative side effects of allowing this?

What is the worst thing that could happen?

If this is just garbage on the chain, without any other negative side effects, I don't care.

[kralverde]

From what I can tell, the worst is garbage on the chain.

[TronBlack]

I agree with fdov, it would be best to restrict non-compliant transactions and activate when P2SH activates.

[HyperPeek]

I am not sure how difficult it will be with the usage of the bytes for restricted assets / qualifiers. We can not simply enforce the size bytes and suddenly invalidating a previous valid output may lead to unexpected behaviour.