Depreacte the support of Omnilock Bitcoin authentication's P2SH address

[homura]

In the past, Omnilock supported Bitcoin, assuming all Bitcoin dApp users used UniSat wallet as their dApp entry. This meant that every P2SH address was assumed to be spendable with only one public key. However, this approach was not secure enough since P2SH outputs can be spent with any kind of scripts in Bitcoin.

How to

To address this issue, the decodeAddress function in omnilock/bitcoin will be deprecated. Instead, a new function called parseBtcAddressToPubkeyHash will be created to provide more accurate results

This change will also affect the createOmnilockScript function, which will also be deprecated. Another function called createSimplePubkeyBasedOmnilockScript

[Keith-CY]

Now the createOmnilock method is marked as deprecated by comment at https://github.com/ckb-js/lumos/blob/40e12a95e95e9d0a052a4829679351ee3c6384bf/packages/common-scripts/src/omnilock.ts#L112-L163

which is not strong enough for me, because it relies on teams/developers' perceptions, or communication to make sure they are informed.

However, there are numerous developers out of our sight and may not be connected.

IMO, it's potentially a security risk that a fix should be actively enforced in all applications.

Here I would suggest as follows,

1. use deprecate to mark this method in a proper way;
2. break the API of createOmnilockScript so that all developers have to handle it explicitly.

If the p2sh should be disabled in any case, the bitcoin branch(https://github.com/ckb-js/lumos/blob/40e12a95e95e9d0a052a4829679351ee3c6384bf/packages/common-scripts/src/omnilock.ts#L148-L160) in createOmnilock should be thrown as follows

if (omnilockInfo.auth.flag === 'BITCOIN') {
  throw new Error(`... please use createSimplePublicKeyBasedOmnilockScript for bitcoin address`)
}

If the p2sh => omnilock is still necessary in some cases, a whitelist could be added to disable p2sh by default. Developers who really need it can enable p2sh in the whitelist.

createOmnilockScript({
  auth: {
    flag: 'BITCOIN',
    content: 'a bitcoin address',
    allowed: ['p2pkwh', 'p2sh']
  }
})

[Hanssen0]

In worst cases, allowing P2SH addresses may freeze users' assets permanently. It's worth explicitly warning users & developers about such a severe security issue, and we should even consider breaking compatibility to force developers to deal with it.

[Keith-CY]

@homura please check

[homura]

It does make sense to introduce a breaking change for a risky API.

As mentioned above, Lumos could introduce a field called allow to ensure that developers understand the risk associated with P2SH. However, the introduction of allow should not impact non-P2SH addresses to avoid disrupting existing features, for example

declare Options {
  config: Config;
  _dangerousAllowBitcoinP2shAddress?: boolean;
}

declare function createOmnilock(omnilockInfo: OmnilockInfo, options?: Options): Script

---

Additionally, I am worried that the default ban on P2SH addresses may be too restrictive. Similar to Ethereum addresses, dApps typically do not verify if an address is an EOA or contract address for individual users because dApps assume that all users connect to dApps using a standard Ethereum wallet. For BTC dApp users, UniSat has become the standard solution, and it only provides P2SH-P2WPKH addresses

Do you think we should ban P2SH addresses by default, considering that users may connect using a non-standard wallet?

[Keith-CY]

It does make sense to introduce a breaking change for a risky API.

As mentioned above, Lumos could introduce a field called allow to ensure that developers understand the risk associated with P2SH. However, the introduction of allow should not impact non-P2SH addresses to avoid disrupting existing features, for example

declare Options {
  config: Config;
  _dangerousAllowBitcoinP2shAddress?: boolean;
}

declare function createOmnilock(omnilockInfo: OmnilockInfo, options?: Options): Script

Additionally, I am worried that the default ban on P2SH addresses may be too restrictive. Similar to Ethereum addresses, dApps typically do not verify if an address is an EOA or contract address for individual users because dApps assume that all users connect to dApps using a standard Ethereum wallet. For BTC dApp users, UniSat has become the standard solution, and it only provides P2SH-P2WPKH addresses

Do you think we should ban P2SH addresses by default, considering that users may connect using a non-standard wallet?

When the new p2sh address is supported by UniSat, it can be difficult to promptly upgrade all applications to prevent unsupported p2sh addresses from being used by default. Therefore, developers should understand the risks before deciding to support any p2sh address, and then make a decision:

1. disable all p2sh addresses; Or
2. allow all p2sh addresses(trust UniSat); Or
3. regenerate p2sh-p2wpkh addresses(via public key)

[Hanssen0]

Do you think we should ban P2SH addresses by default, considering that users may connect using a non-standard wallet?

Developers can use Lumos to decode addresses from not only users' wallets but also some recipient addresses. That means our users can potentially transfer assets to any BTC address, including an arbitrary P2SH address.

The situation is different from that of EVM contract addresses. Contract owners do not publish them to receive assets, but P2SH's owners do. Contract owners who didn't add assets managing logic won't expect to be able to unlock their assets, but P2SH's owners do.

Considering the above, the problem is that even the Omnilock should NOT support P2SH addresses. Upgrading Omnilock now is risky, but that does not mean we should keep it in Lumos.

[homura]

developers should understand the risks before deciding to support any p2sh address, and then make a decision:

1. disable all p2sh addresses; Or
2. allow all p2sh addresses(trust UniSat); Or
3. regenerate p2sh-p2wpkh addresses(via public key)

Developers can use Lumos to decode addresses from not only users' wallets but also some recipient addresses. That means our users can potentially transfer assets to any BTC address, including an arbitrary P2SH address.

It makes sense, I'll make a breaking change to address the risky feature and remove the API createSimplePubkeyBasedOmnilockScript

export type IdentityBitcoin = {
  flag: "BITCOIN";
  /**
   * a Bitcoin address, such as
   * `P2PKH(17VZNX1SN5NtKa8UQFxwQbFeFc3iqRYhem)`,
   * `P2SH(3EktnHQD7RiAE6uzMj2ZifT9YgRrkSgzQX)`,
   * `Bech32(bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4)`
   */
  content: string;
+ // defaults to false.
+ // When it is true, the P2SH address will be allowed for an Omnilock script.
+ // make sure that the address is a P2SH-P2WPKH, or the script CANNOT be unlocked.
+ allowP2SH?: boolean;
};

declare function createOmnilock(omnilockInfo: OmnilockInfo, options?: Options): Script

[Keith-CY]

developers should understand the risks before deciding to support any p2sh address, and then make a decision:

1. disable all p2sh addresses; Or
2. allow all p2sh addresses(trust UniSat); Or
3. regenerate p2sh-p2wpkh addresses(via public key)

Developers can use Lumos to decode addresses from not only users' wallets but also some recipient addresses. That means our users can potentially transfer assets to any BTC address, including an arbitrary P2SH address.

It makes sense, I'll make a breaking change to address the risky feature and remove the API createSimplePubkeyBasedOmnilockScript

export type IdentityBitcoin = {
  flag: "BITCOIN";
  /**
   * a Bitcoin address, such as
   * `P2PKH(17VZNX1SN5NtKa8UQFxwQbFeFc3iqRYhem)`,
   * `P2SH(3EktnHQD7RiAE6uzMj2ZifT9YgRrkSgzQX)`,
   * `Bech32(bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4)`
   */
  content: string;
+ // defaults to false.
+ // When it is true, the P2SH address will be allowed for an Omnilock script.
+ // make sure that the address is a P2SH-P2WPKH, or the script CANNOT be unlocked.
+ allowP2SH?: boolean;
};

declare function createOmnilock(omnilockInfo: OmnilockInfo, options?: Options): Script

I prefer to add a field for the whitelist. By doing so, this API can be updated smoothly when other address types are supported.

If the flag allowP2SH is added, when the p2sh address is supported in the future, the flag allowP2SH should be removed and apps have to update for the API change.

[homura]

I prefer to add a field for the whitelist.

Consider using the blacklist mechanism instead of a whitelist because:

• blacklist mechanism hides the complexity from developers, so they don't need to worry about whether something is a p2wpkh or p2pkh.
• whitelist mechanism would require developers who are using createOmniLockScript with non-p2sh addresses to make changes

[Keith-CY]

I prefer to add a field for the whitelist.

Consider using the blacklist mechanism instead of a whitelist because:

• blacklist mechanism hides the complexity from developers, so they don't need to worry about whether something is a p2wpkh or p2pkh.

• whitelist mechanism would require developers who are using createOmniLockScript with non-p2sh addresses to make changes

Both work for me, but whitelist doesn't require extra work of developers if a default one is set.

[homura]

I modified the allowP2SH to an allowlist allow to provide more precise control. You can check the change here