Allow users to withdraw funds from address where ScriptHashType was accidentally set to Data.

[mucks-cdc]

Problem Description:

While implementing my own CKB SDK to meet specific requirements, I encountered an issue related to the ScriptHashType when parsing addresses. Regrettably, I mistakenly set the ScriptHashType to Data instead of Type, leading to funds being sent to an address with the correct code_hash and argument but an incorrect hash type. example address link.

Issue:

This inadvertent error renders it impossible to withdraw funds from addresses with the specified code_hash ('9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8'), hash_type set to Data, and the correct argument ('0x16b41b933fafd7be72f84daf661a09bf8179164e'). This oversight poses a potential vulnerability as similar mistakes might occur when implementing custom SDKs, resulting in users being unable to access their funds. I've also scanned the Mainnet for cases like this and found that this has already happened a couple times.

Proposed Solution:

Several potential solutions can address this issue:

Transaction Flexibility:

Allow transactions with the known code_hash ('9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8'), HashType set to Data, and the blake160 hash of the pubkey as args to withdraw funds by enabling the use of the corresponding Type script.

Cell Dependency Enhancement:

Create a cell containing the SECP256K1_BLAKE160_SIGHASH_ALL script with code_hash ('9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8') and hash_type set to Data. Users can include this cell in their cell_deps to facilitate withdrawals. However, further clarification on the selection of this code_hash is needed – is it the code hash of the actual script?

These solutions aim to mitigate the risk of users unintentionally locking funds due to common errors in SDK implementation. Your consideration of these options would greatly enhance the user experience and security of custom CKB SDKs.

[janx]

I'm really sorry to hear what has happened :( However what you described is the intended behavior not a bug, personally I don't think a fix is needed for this CKB implementation.

It also appears that proposals to make CKB work as you expect would be consensus changes and requires a hard-fork, no matter if it's done by the maintainer of this implementation or yourself. As you may already know a mere code commit won't make it effective on mainnet. A hard-fork is basically a community vote with the participation of all full nodes including mining nodes/pools. A controversial and/or uncoordinated hard-fork will cause a network split which could endanger the whole ecosystem, so the community is usually very cautious on such changes. For a change like this my suggestion is to solicit majority support before implementing it.

[mucks-cdc]

Thank you for your prompt response. Before advancing to garner majority support, I aim to ensure alignment with Ckb engineers regarding this matter. I'm particularly interested in understanding their stance—whether they remain neutral or have reservations against the proposal.

Additionally, as we consider implementing this logic, a key concern is potential security implications. While the parameters seem highly specific, I would appreciate insights into any possible security vulnerabilities that could arise from its integration.

Regarding the hash '9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8' associated with SECP256K1_BLAKE160_SIGHASH_ALL, I seek clarity on the methodology behind its derivation. Specifically, I'd like to understand the source data or process that was hashed to produce this value. If the hashing involves the actual code, I'm curious whether creating a cell with the code and subsequently adding it to the cell_deps could potentially enable fund withdrawal.

Your insights into these inquiries would greatly assist in charting our next steps.

Thank you for your time and attention to these critical details. I look forward to your guidance.

[jordanmack]

Create a cell containing the SECP256K1_BLAKE160_SIGHASH_ALL script with code_hash ('9bd7e06f3ecf4be0f2fcd2188b23f1b9fcc88e5d4b65a8637b17723bbda3cce8') and hash_type set to Data. Users can include this cell in their cell_deps to facilitate withdrawals. However, further clarification on the selection of this code_hash is needed – is it the code hash of the actual script?

When hash_type is set to data or data1, the code_hash must match the hash of the binary executable. It is impossible to create a binary to match a specific hash.

For reference, here is the section about hash_type in the L1 Developer Training Course: https://nervos.gitbook.io/developer-training-course/scripting-basics/using-scripts#hash-type

Additionally, as we consider implementing this logic, a key concern is potential security implications. While the parameters seem highly specific, I would appreciate insights into any possible security vulnerabilities that could arise from its integration.

I see two cases where it could create a small issue if this was implemented for any script. The first is that it could create a misleading scenario when attempting to verify implementation. A malicious developer could claim the smart contracts are immutable (data or data1) but then upgrade them later since they are actually hash_type=type. The second is that this is a deoptimization since CKB-VM would always have to check against type and data/data1 during every execution.

If it was limited to a single well-known default lock script, the above noted are no longer concerns, but this now leads to another potential issue with misguiding developers because it is training them on incorrect behavior. Say a developer mistakenly uses data/data1 for their dapp that relies on the well-known default lock script. Everything will always work fine and they may not learn proper usage. Later on, they switch from the default lock to something else and make the mistake again because they don't understand it. In the worst case, this could lead to lost user funds. Inconsistent behavior should be avoided.

At this point, I would have reservations about supporting such a change. It would fix the noted mistake, but then it opens the door for several other mistakes. I see it as a lateral move that potentially introduces new inconsistencies.

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[github-actions[bot]]

This issue was closed because it has been stalled for 5 days with no activity.