BREAKING CHANGE: disallow p2sh converting by default since risky (#699)

.changeset/green-pumpkins-build.md

@@ -0,0 +1,5 @@
+---
+"@ckb-lumos/common-scripts": minor
+---
+
+**BREAKING CHANGE**: `createOmnilockScript` uses the `allows` option to restrict allowed btc addresses

.eslintrc.next.js

@@ -23,6 +23,7 @@ module.exports = {
               -1, // index -1 is not found
               0, // first element of an array
               1, // common for i + 1 in a loop
+              2, // slice(2) for string that starts with "0x" is common to work with 3rd-party libs
               16, // toString(16)
               1000, // second to millisecond
             ],

packages/common-scripts/src/omnilock-bitcoin.ts

@@ -1,46 +1,67 @@
-// TODO the magic number eslint will be resolved in 0.24 by recovering https://github.com/ckb-js/lumos/pull/682
-/*eslint-disable @typescript-eslint/no-magic-numbers*/
-
 import { bytes, BytesLike } from "@ckb-lumos/codec";
 import { bech32 } from "bech32";
 import bs58 from "bs58";
 
+export type SupportedBtcAddressType = "P2SH-P2WPKH" | "P2WPKH" | "P2PKH";
+
 // https://github.com/cryptape/omnilock/blob/9419b7795641da0ade25a04127e25d8a0b709077/c/ckb_identity.h#L28
 const BTC_PREFIX = "CKB (Bitcoin Layer) transaction: 0x";
 
 /**
  * Decode bitcoin address to public key hash in bytes
  * @see https://en.bitcoin.it/wiki/List_of_address_prefixes
  * @param address
+ * @param allows
  */
-export function decodeAddress(address: string): ArrayLike<number> {
-  try {
-    // Bech32
-    if (address.startsWith("bc1q")) {
-      return bech32.fromWords(bech32.decode(address).words.slice(1));
-    }
+export function decodeAddress(
+  address: string,
+  allows: SupportedBtcAddressType[] = ["P2WPKH", "P2PKH"]
+): ArrayLike<number> {
+  const btcAddressFlagSize = 1;
+  const hashSize = 20;
 
-    // P2PKH
-    if (address.startsWith("1")) {
-      return bs58.decode(address).slice(1, 21);
-    }
+  if (isP2wpkhAddress(address)) {
+    assertAddressType(allows, "P2WPKH");
+    return bech32.fromWords(bech32.decode(address).words.slice(1));
+  }
 
-    // P2SH
-    if (address.startsWith("3")) {
-      return bs58.decode(address).slice(1, 21);
-    }
-  } catch {
-    // https://bitcoin.design/guide/glossary/address/#taproot-address---p2tr
-    if (address.startsWith("bc1p")) {
-      throw new Error("Taproot address is not supported yet.");
-    }
+  if (isP2pkhAddress(address)) {
+    assertAddressType(allows, "P2PKH");
+    return bs58
+      .decode(address)
+      .slice(btcAddressFlagSize, btcAddressFlagSize + hashSize);
+  }
+
+  if (isP2shAddress(address)) {
+    assertAddressType(allows, "P2SH-P2WPKH");
+    return bs58
+      .decode(address)
+      .slice(btcAddressFlagSize, btcAddressFlagSize + hashSize);
+  }
+
+  // https://bitcoin.design/guide/glossary/address/#taproot-address---p2tr
+  if (address.startsWith("bc1p")) {
+    throw new Error("Taproot address is not supported yet.");
   }
 
   throw new Error(
-    `Unsupported bitcoin address ${address}, only 1...(P2PKH) 3...(P2SH), and bc1...(Bech32) are supported.`
+    "Unsupported address: " +
+      address +
+      "Only Native SegWit(P2WPKH) and Legacy(P2PKH) addresses are supported"
   );
 }
 
+function assertAddressType(
+  allows: SupportedBtcAddressType[],
+  usingAddressType: SupportedBtcAddressType
+): void {
+  if (!allows.includes(usingAddressType)) {
+    throw new Error(
+      `'${usingAddressType}' must be included in the 'allows' for the address`
+    );
+  }
+}
+
 export interface Provider {
   requestAccounts(): Promise<string[]>;
   signMessage(message: string, type?: "ecdsa"): Promise<string>;
@@ -82,30 +103,58 @@ export async function signMessage(
   const signature = bytes.bytify(base64ToHex(signatureBase64));
 
   const address = accounts[0];
+  /* eslint-disable @typescript-eslint/no-magic-numbers */
+
   // a secp256k1 private key can be used to sign various types of messages
   // the first byte of signature used as a recovery id to identify the type of message
   // https://github.com/XuJiandong/omnilock/blob/4e9fdb6ca78637651c8145bb7c5b82b4591332fb/c/ckb_identity.h#L249-L266
-  if (address.startsWith("bc1q")) {
+  if (isP2wpkhAddress(address)) {
     signature[0] = 39 + ((signature[0] - 27) % 4);
-  } else if (address.startsWith("3")) {
+  } else if (isP2shAddress(address)) {
     signature[0] = 35 + ((signature[0] - 27) % 4);
-  } else if (address.startsWith("1")) {
+  } else if (isP2pkhAddress(address)) {
     signature[0] = 31 + ((signature[0] - 27) % 4);
   } else {
     throw new Error(
-      `Unsupported bitcoin address ${address}, only 1...(P2PKH) 3...(P2SH), and bc1...(Bech32) are supported.`
+      `Unsupported bitcoin address ${address}. Only supports SegWit, P2SH-P2WPKH, P2PKH`
     );
   }
 
+  /* eslint-enable @typescript-eslint/no-magic-numbers */
+
   return bytes.hexify(signature);
 }
 
 function base64ToHex(str: string) {
   const raw = atob(str);
   let result = "";
   for (let i = 0; i < raw.length; i++) {
-    const hex = raw.charCodeAt(i).toString(16);
-    result += hex.length === 2 ? hex : "0" + hex;
+    // eslint-disable-next-line @typescript-eslint/no-magic-numbers
+    result += raw.charCodeAt(i).toString(16).padStart(2, "0");
   }
   return "0x" + result;
 }
+
+/* https://en.bitcoin.it/wiki/List_of_address_prefixes */
+
+function isP2wpkhAddress(address: string): boolean {
+  return (
+    address.startsWith("bc1") || // mainnet
+    address.startsWith("tb1") // testnet
+  );
+}
+
+function isP2shAddress(address: string): boolean {
+  return (
+    address.startsWith("3") || // mainnet
+    address.startsWith("2") // testnet
+  );
+}
+
+function isP2pkhAddress(address: string): boolean {
+  return (
+    address.startsWith("1") || // mainnet
+    address.startsWith("m") || // testnet
+    address.startsWith("n") // testnet
+  );
+}

packages/common-scripts/src/omnilock.ts

@@ -38,6 +38,7 @@ import * as bitcoin from "./omnilock-bitcoin";
 import * as solana from "./omnilock-solana";
 import { decode as bs58Decode } from "bs58";
 import { ckbHash } from "@ckb-lumos/base/lib/utils";
+import { SupportedBtcAddressType } from "./omnilock-bitcoin";
 
 const { ScriptValue } = values;
 
@@ -66,6 +67,7 @@ export type IdentityEthereum = {
    */
   content: BytesLike;
 };
+
 export type IdentityBitcoin = {
   flag: "BITCOIN";
   /**
@@ -75,6 +77,13 @@ export type IdentityBitcoin = {
    * `Bech32(bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4)`
    */
   content: string;
+
+  /**
+   * Allows the P2PKH and P2WPKH by default.
+   * To allow the P2SH-P2WPKH address,
+   * change this option to `["P2PKH", "P2WPKH", "P2SH-P2WPKH"]`
+   */
+  allows?: SupportedBtcAddressType[];
 };
 
 export type IdentitySolana = {
@@ -110,8 +119,8 @@ const SECP256K1_SIGNATURE_PLACEHOLDER_LENGTH = 65;
 const ED25519_SIGNATURE_PLACEHOLDER_LENGTH = 96;
 
 /**
- * only support ETHEREUM and SECP256K1_BLAKE160 mode currently
- * refer to: @link https://github.com/nervosnetwork/rfcs/blob/master/rfcs/0042-omnilock/0042-omnilock.md omnilock
+ * Create an Omnilock script based on other networks' wallet
+ * @see https://github.com/nervosnetwork/rfcs/blob/master/rfcs/0042-omnilock/0042-omnilock.md
  * @param omnilockInfo
  * @param options
  * @returns
@@ -176,7 +185,10 @@ export function createOmnilockScript(
         return bytes.hexify(
           bytes.concat(
             [IdentityFlagsType.IdentityFlagsBitcoin],
-            bitcoin.decodeAddress(omnilockInfo.auth.content),
+            bitcoin.decodeAddress(
+              omnilockInfo.auth.content,
+              omnilockInfo.auth.allows
+            ),
             omnilockArgs
           )
         );

packages/common-scripts/tests/omnilock-bitcoin.test.ts

@@ -12,7 +12,11 @@ import { mockOutPoint } from "@ckb-lumos/debugger/lib/context";
 import { createOmnilockScript, OmnilockWitnessLock } from "../src/omnilock";
 import { address, AddressType, core, keyring } from "@unisat/wallet-sdk";
 import { NetworkType } from "@unisat/wallet-sdk/lib/network";
-import { Provider, signMessage } from "../src/omnilock-bitcoin";
+import {
+  Provider,
+  signMessage,
+  SupportedBtcAddressType,
+} from "../src/omnilock-bitcoin";
 import { SimpleKeyring } from "@unisat/wallet-sdk/lib/keyring";
 
 test.before(async () => {
@@ -41,15 +45,15 @@ test.serial("Omnilock#Bitcoin P2WPKH", async (t) => {
 
 test.serial("Omnilock#Bitcoin P2SH_P2WPKH", async (t) => {
   const { provider } = makeProvider(AddressType.P2SH_P2WPKH);
-  const result = await execute(provider);
+  const result = await execute(provider, ["P2SH-P2WPKH"]);
 
   t.is(result.code, 0, result.message);
 });
 
-async function execute(provider: Provider) {
+async function execute(provider: Provider, allows?: SupportedBtcAddressType[]) {
   const addr = (await provider.requestAccounts())[0];
 
-  const { txSkeleton, lock } = await setupTxSkeleton(addr);
+  const { txSkeleton, lock } = await setupTxSkeleton(addr, allows);
 
   const message = txSkeleton.get("signingEntries").get(0)!.message;
   const signature = await signMessage(message, "ecdsa", provider);
@@ -95,11 +99,14 @@ function makeProvider(addressType: AddressType): {
   };
 }
 
-async function setupTxSkeleton(addr: string) {
+async function setupTxSkeleton(
+  addr: string,
+  allows?: SupportedBtcAddressType[]
+) {
   const txSkeleton = TransactionSkeleton().asMutable();
 
   const lock = createOmnilockScript(
-    { auth: { flag: "BITCOIN", content: addr } },
+    { auth: { flag: "BITCOIN", content: addr, allows } },
     { config: managerConfig }
   );
 
@@ -117,3 +124,47 @@ async function setupTxSkeleton(addr: string) {
   common.prepareSigningEntries(txSkeleton, { config: managerConfig });
   return { txSkeleton: txSkeleton, lock };
 }
+
+// 02 indicates that the pubkey is compressed
+const pubkey =
+  "02b602ad190efb7b4f520068e3f8ecf573823d9e2557c5229231b4e14b79bbc0d8";
+
+test("Omnilock#Bitcoin P2SH", (t) => {
+  const p2shAddr = address.publicKeyToAddress(
+    pubkey,
+    AddressType.P2SH_P2WPKH,
+    NetworkType.MAINNET
+  );
+
+  t.throws(() =>
+    createOmnilockScript({ auth: { flag: "BITCOIN", content: p2shAddr } })
+  );
+
+  t.notThrows(() =>
+    createOmnilockScript({
+      auth: { flag: "BITCOIN", content: p2shAddr, allows: ["P2SH-P2WPKH"] },
+    })
+  );
+});
+
+test("Unsupported BTC address", (t) => {
+  const p2trAddr = address.publicKeyToAddress(
+    pubkey,
+    AddressType.P2TR,
+    NetworkType.MAINNET
+  );
+
+  t.throws(() =>
+    createOmnilockScript({ auth: { flag: "BITCOIN", content: p2trAddr } })
+  );
+
+  const unknownAddr = address.publicKeyToAddress(
+    pubkey,
+    AddressType.UNKNOWN,
+    NetworkType.MAINNET
+  );
+
+  t.throws(() =>
+    createOmnilockScript({ auth: { flag: "BITCOIN", content: unknownAddr } })
+  );
+});

website/docs/migrations/migrate-to-v0.24.md

@@ -0,0 +1,19 @@
+# Migrate to Lumos v0.24
+
+## Disallow the Omnilock P2SH Address by Default
+
+The default options of `createOmnilockScript` disallows the use of P2SH addresses for security reasons.
+Not all P2SH addresses are P2SH-P2WPKH addresses.
+This means that developers may unintentionally use a non-P2SH-P2WPKH address to convert to an Omnilock script,
+which can lead to the script not being lockable.
+If you still need to use a P2SH address, use the following code
+
+```diff
+createOmnilockScript({
+  auth: {
+    flag: "BITCOIN",
+    content: addr,
++   allows: ["P2WPKH", "P2PKH", "P2SH-P2WPKH"]
+  }
+})
+```