fix: address remaining SDK findings from the 0.14 audit

packages/cashscript/jest/package.json

@@ -1,5 +1,5 @@
 {
   "type": "module",
-  "types": "../dist/test/TestExtensions.d.ts",
-  "main": "../dist/test/TestExtensions.js"
+  "types": "../dist/test/JestExtensions.d.ts",
+  "main": "../dist/test/JestExtensions.js"
 }

packages/cashscript/src/Argument.ts

@@ -1,4 +1,4 @@
-import { hexToBin } from '@bitauth/libauth';
+import { hexToBin, isHex } from '@bitauth/libauth';
 import {
   AbiFunction,
   Artifact,
@@ -27,14 +27,14 @@ export type EncodeFunction = (arg: FunctionArgument, typeStr: string) => Encoded
  * - `bool`, `int`, `string` → encoded via their CashScript primitive encoders.
  * - `sig` passed as a `SignatureTemplate` → returned as-is so the transaction builder can
  *   produce the signature at build time.
- * - `sig` / `datasig` / `bytes[N]` passed as a `Uint8Array` or hex string → byte-checked and
+ * - `sig` / `datasig` / `pubkey` / `bytes[N]` passed as a `Uint8Array` or hex string → byte-checked and
  *   returned as bytes.
  *
  * @param argument - The runtime argument value provided by the caller.
  * @param typeStr - The CashScript type string from the contract ABI (e.g. `int`, `bytes20`, `sig`).
  * @returns The encoded argument, ready for inclusion in an unlocking script.
  * @throws A `TypeError` when the JS type does not match the expected CashScript type, or when a
- *   bounded bytes type receives a value of the wrong length.
+ *   bounded bytes type receives a value of the wrong length. An `Error` when a hex string is not valid hex.
  */
 export function encodeFunctionArgument(argument: FunctionArgument, typeStr: string): EncodedFunctionArgument {
   let type = parseType(typeStr);
@@ -64,11 +64,13 @@ export function encodeFunctionArgument(argument: FunctionArgument, typeStr: stri
 
   // Convert hex string to Uint8Array
   if (typeof argument === 'string') {
-    if (argument.startsWith('0x')) {
-      argument = argument.slice(2);
+    const hex = argument.startsWith('0x') ? argument.slice(2) : argument;
+
+    if (!isHex(hex)) {
+      throw Error(`Value for type ${type} should be a valid hex string with an even number of digits, found '${argument}'`);
     }
 
-    argument = hexToBin(argument);
+    argument = hexToBin(hex);
   }
 
   if (!(argument instanceof Uint8Array)) {
@@ -83,6 +85,14 @@ export function encodeFunctionArgument(argument: FunctionArgument, typeStr: stri
     type = new BytesType(argument.byteLength);
   }
 
+  // Redefine PUBKEY as a bytes33 (compressed) or bytes65 (uncompressed)
+  if (type === PrimitiveType.PUBKEY) {
+    if (![33, 65].includes(argument.byteLength)) {
+      throw new TypeError(`bytes${argument.byteLength}`, type);
+    }
+    type = new BytesType(argument.byteLength);
+  }
+
   // Redefine DATASIG as a bytes64 (Schnorr) or bytes70, bytes71, bytes72 (ECDSA) or bytes0 (for NULLFAIL)
   if (type === PrimitiveType.DATASIG) {
     if (![0, 64, 70, 71, 72].includes(argument.byteLength)) {

packages/cashscript/src/Errors.ts

@@ -3,6 +3,7 @@ import {
   CallStackEntry,
   ResolvedFrame,
   getLocationDataForFrame,
+  getRequireLocationDataForFrame,
   resolveInlineAttribution,
   rootFrame,
 } from './debug-frame.js';
@@ -189,8 +190,9 @@ export class FailedRequireError extends FailedTransactionError {
     const inline = resolveInlineAttribution(artifact, resolvedFrame, requireStatement, 'requires');
     const attributedFrame = inline?.frame ?? resolvedFrame;
     const attributedIp = inline?.entry.ip ?? failingInstructionPointer;
+    const requireLine = inline?.entry.line ?? requireStatement.line;
 
-    const { statement, lineNumber } = getLocationDataForFrame(attributedFrame, attributedIp);
+    const { statement, lineNumber } = getRequireLocationDataForFrame(attributedFrame, attributedIp, requireLine);
     const context = formatFrameContext(attributedFrame, artifact.contractName, lineNumber);
 
     const baseMessage = `${attributedFrame.sourceName}:${lineNumber} Require statement failed at input ${inputIndex} ${context}`;

packages/cashscript/src/TransactionBuilder.ts

@@ -319,8 +319,13 @@ export class TransactionBuilder {
    *
    * @param locktime - The absolute locktime to use (block height or UNIX timestamp).
    * @returns This builder for chaining.
+   * @throws If the locktime is not an unsigned 32-bit integer.
    */
   setLocktime(locktime: number): this {
+    if (!Number.isInteger(locktime) || locktime < 0 || locktime > 0xffffffff) {
+      throw new Error(`Locktime ${locktime} should be an integer between 0 and 4294967295`);
+    }
+
     this.locktime = locktime;
     return this;
   }
@@ -508,9 +513,9 @@ export class TransactionBuilder {
       this.debug();
     }
 
+    let txid: string;
     try {
-      const txid = await this.provider.sendRawTransaction(tx);
-      return raw ? await this.getTxDetails(txid, raw) : await this.getTxDetails(txid);
+      txid = await this.provider.sendRawTransaction(tx);
     } catch (e: any) {
       const reason = e.error ?? e.message;
 
@@ -524,6 +529,9 @@ export class TransactionBuilder {
 
       throw new FailedTransactionError(reason, getBitauthUriWithFallback());
     }
+
+    // The transaction was broadcast successfully, so failing to retrieve it afterwards is not a failed transaction
+    return raw ? this.getTxDetails(txid, raw) : this.getTxDetails(txid);
   }
 
   private async getTxDetails(txid: string): Promise<TransactionDetails>;
@@ -543,8 +551,7 @@ export class TransactionBuilder {
       }
     }
 
-    // Should not happen
-    throw new Error('Could not retrieve transaction details for over 10 minutes');
+    throw new Error(`Transaction ${txid} was broadcast, but its details could not be retrieved for over 10 minutes`);
   }
 
   /**
@@ -578,19 +585,20 @@ export class TransactionBuilder {
     const transactionSize = this.getEncodedTransactionSize(transaction);
 
     const fee = totalInputAmount - totalOutputAmount;
-    const feePerByte = Number((Number(fee) / transactionSize).toFixed(2));
+    const feePerByte = Number(fee) / transactionSize;
 
     if (this.options.maximumFeeSatoshis && fee > this.options.maximumFeeSatoshis) {
       throw new TransactionFeeTooHighError(fee, this.options.maximumFeeSatoshis);
     }
 
+    // The limits are checked against the exact fee per byte, which is only rounded (away from the limit) for display
     if (this.options.maximumFeeSatsPerByte && feePerByte > this.options.maximumFeeSatsPerByte) {
-      throw new TransactionFeePerByteTooHighError(feePerByte, this.options.maximumFeeSatsPerByte);
+      throw new TransactionFeePerByteTooHighError(Math.ceil(feePerByte * 100) / 100, this.options.maximumFeeSatsPerByte);
     }
 
     const STANDARD_MIN_FEE_PER_BYTE = 1.0;
     if (feePerByte < STANDARD_MIN_FEE_PER_BYTE) {
-      throw new TransactionFeePerByteTooLowError(feePerByte, STANDARD_MIN_FEE_PER_BYTE);
+      throw new TransactionFeePerByteTooLowError(Math.floor(feePerByte * 100) / 100, STANDARD_MIN_FEE_PER_BYTE);
     }
   }
 

packages/cashscript/src/debug-frame.ts

@@ -234,6 +234,21 @@ const toCallStackEntry = (frame: ResolvedFrame, instructionPointer: number): Cal
   };
 };
 
+// A require statement's own opcodes can be optimised away (e.g. a final `require(ok)` where `ok` is already on top of
+// the stack), in which case the failing opcode belongs to an earlier statement. We then use the require's own line.
+export const getRequireLocationDataForFrame = (
+  frame: ResolvedFrame,
+  instructionPointer: number,
+  requireLine: number,
+): { lineNumber: number, statement: string } => {
+  const locationData = getLocationDataForFrame(frame, instructionPointer);
+  const statementEndLine = locationData.lineNumber + locationData.statement.split('\n').length - 1;
+  if (statementEndLine >= requireLine) return locationData;
+
+  const statement = frame.source.split('\n')[requireLine - 1].trim().replace(/;$/, '');
+  return { lineNumber: requireLine, statement };
+};
+
 export const getLocationDataForFrame = (
   frame: ResolvedFrame,
   instructionPointer: number,

packages/cashscript/src/network/ElectrumNetworkProvider.ts

@@ -178,15 +178,15 @@ export default class ElectrumNetworkProvider implements NetworkProvider {
     try {
       result = await this.electrum.request(name, ...parameters);
     } finally {
+      this.concurrentRequests -= 1;
+
       // Always disconnect the electrum client, also if the request fails
       // as long as no other concurrent requests are running
       if (this.shouldDisconnect()) {
         await this.electrum.disconnect();
       }
     }
 
-    this.concurrentRequests -= 1;
-
     if (result instanceof Error) throw result;
 
     return result;
@@ -200,7 +200,7 @@ export default class ElectrumNetworkProvider implements NetworkProvider {
 
   private shouldDisconnect(): boolean {
     if (this.manualConnectionManagement) return false;
-    if (this.concurrentRequests !== 1) return false;
+    if (this.concurrentRequests !== 0) return false;
     return true;
   }
 }

packages/cashscript/src/network/MockNetworkProvider.ts

@@ -4,6 +4,7 @@ import { SpendableUtxo, Utxo, Network, VmTarget } from '../interfaces.js';
 import NetworkProvider from './NetworkProvider.js';
 import { addressToLockScript, cashScriptOutputToLibauthOutput, libauthTokenDetailsToCashScriptTokenDetails } from '../utils.js';
 import { createVirtualMachine, DEFAULT_VM_TARGET } from '../libauth-template/utils.js';
+import { NetworkProviderAbsoluteTimelockError } from './errors.js';
 
 /**
  * Options accepted by the `MockNetworkProvider` constructor.
@@ -18,8 +19,9 @@ export interface MockNetworkProviderOptions {
   /**
    * When `true` (default), broadcasting a transaction via `sendRawTransaction` evaluates it
    * against the BCH VM using the *actual* locking bytecode of the spent UTXOs (like a real node
-   * would), rejecting invalid transactions. Requires `updateUtxoSet` to be enabled, since spent
-   * UTXOs are only looked up when the UTXO set is tracked.
+   * would), rejecting invalid transactions. Transactions with a block height locktime above the mock
+   * block height are rejected as non-final. Time-based locktimes and relative timelocks (sequence
+   * numbers) are not checked, since the mock network has no block times or UTXO confirmation heights.
    */
   validateTransactions?: boolean;
   /** The BCH VM target used for local debugging and transaction validation. Defaults to the current stable VM. */
@@ -57,7 +59,7 @@ export default class MockNetworkProvider implements NetworkProvider {
   }
 
   async getUtxosForLockingBytecode(lockingBytecode: Uint8Array | string): Promise<SpendableUtxo[]> {
-    const lockingBytecodeHex = typeof lockingBytecode === 'string' ? lockingBytecode : binToHex(lockingBytecode);
+    const lockingBytecodeHex = typeof lockingBytecode === 'string' ? lockingBytecode.toLowerCase() : binToHex(lockingBytecode);
     return this.utxoSet.filter(([key]) => key === lockingBytecodeHex).map(([, utxo]) => utxo);
   }
 
@@ -88,8 +90,8 @@ export default class MockNetworkProvider implements NetworkProvider {
       return txid;
     }
 
-    // If updateUtxoSet is false, we don't track spent UTXOs, so we cannot validate the transaction either
-    if (!this.options.updateUtxoSet) {
+    // Without validation or UTXO set updates, the spent UTXOs are not needed (so they don't need to exist either)
+    if (!this.options.validateTransactions && !this.options.updateUtxoSet) {
       this.transactionMap[txid] = txHex;
       return txid;
     }
@@ -102,6 +104,10 @@ export default class MockNetworkProvider implements NetworkProvider {
     }
 
     this.transactionMap[txid] = txHex;
+
+    // If updateUtxoSet is false, the UTXO set stays the same
+    if (!this.options.updateUtxoSet) return txid;
+
     this.utxoSet = this.utxoSet.filter((entry) => !spentUtxoEntries.includes(entry));
 
     decodedTransaction.outputs.forEach((output, vout) => {
@@ -120,9 +126,9 @@ export default class MockNetworkProvider implements NetworkProvider {
     const remainingUtxoEntries = [...this.utxoSet];
 
     return transaction.inputs.map((input) => {
-      const utxoIndex = remainingUtxoEntries.findIndex(
-        ([, utxo]) => utxo.txid === binToHex(input.outpointTransactionHash) && utxo.vout === input.outpointIndex,
-      );
+      const utxoIndex = remainingUtxoEntries.findIndex(([, utxo]) => (
+        utxo.txid.toLowerCase() === binToHex(input.outpointTransactionHash) && utxo.vout === input.outpointIndex
+      ));
 
       // TODO: we should check what error a BCHN node throws, so we can throw the same error here
       if (utxoIndex === -1) {
@@ -135,6 +141,8 @@ export default class MockNetworkProvider implements NetworkProvider {
 
   // Evaluates the transaction against the BCH VM using the spent UTXOs (like a real node would)
   private validateTransaction(transaction: LibauthTransaction, spentUtxoEntries: Array<[string, SpendableUtxo]>): void {
+    this.validateLocktime(transaction);
+
     const sourceOutputs = spentUtxoEntries.map(([lockingBytecode, utxo]) => cashScriptOutputToLibauthOutput({
       to: hexToBin(lockingBytecode),
       amount: utxo.satoshis,
@@ -149,6 +157,20 @@ export default class MockNetworkProvider implements NetworkProvider {
     }
   }
 
+  // A real node only accepts transactions that are final in the next block: a block height locktime must not be above
+  // the current block height, unless all inputs have a final sequence number (which disables the locktime)
+  private validateLocktime(transaction: LibauthTransaction): void {
+    const LOCKTIME_THRESHOLD = 500_000_000;
+    const SEQUENCE_FINAL = 0xffffffff;
+
+    if (transaction.locktime >= LOCKTIME_THRESHOLD || transaction.locktime <= this.blockHeight) return;
+    if (transaction.inputs.every((input) => input.sequenceNumber === SEQUENCE_FINAL)) return;
+
+    throw new NetworkProviderAbsoluteTimelockError(
+      `non-final: locktime ${transaction.locktime} is above the current block height ${this.blockHeight}`,
+    );
+  }
+
   // Note: the user can technically add the same UTXO multiple times (txid + vout), to the same or different addresses
   // but we don't check for this in the sendRawTransaction method. We might want to prevent duplicates from being added
   // in the first place.
@@ -162,7 +184,7 @@ export default class MockNetworkProvider implements NetworkProvider {
    */
   addUtxo(addressOrLockingBytecode: string, utxo: Utxo): SpendableUtxo {
     const lockingBytecode = isHex(addressOrLockingBytecode) ?
-      addressOrLockingBytecode : binToHex(addressToLockScript(addressOrLockingBytecode));
+      addressOrLockingBytecode.toLowerCase() : binToHex(addressToLockScript(addressOrLockingBytecode));
 
     const annotatedUtxo = { ...utxo, lockingBytecode };
     this.utxoSet.push([lockingBytecode, annotatedUtxo]);

packages/cashscript/src/test/JestExtensions.ts

@@ -0,0 +1,12 @@
+import './TestExtensions.js';
+
+declare global {
+  namespace jest {
+    // eslint-disable-next-line
+    interface Matchers<R> {
+      toLog(value?: RegExp | string): Promise<void>;
+      toFailRequireWith(value: RegExp | string): Promise<void>;
+      toFailRequire(): Promise<void>;
+    }
+  }
+}

packages/cashscript/src/test/TestExtensions.ts

@@ -1,15 +1,7 @@
 import { DebugResults } from '../debugging.js';
 
-declare global {
-  namespace jest {
-    // eslint-disable-next-line
-    interface Matchers<R> {
-      toLog(value?: RegExp | string): Promise<void>;
-      toFailRequireWith(value: RegExp | string): Promise<void>;
-      toFailRequire(): Promise<void>;
-    }
-  }
-}
+// Registers the custom matchers at runtime. Their types are declared separately for Vitest and Jest, in
+// VitestExtensions.ts and JestExtensions.ts (which are exported as cashscript/vitest and cashscript/jest).
 
 interface Debuggable {
   debug(): DebugResults;
@@ -18,7 +10,7 @@ interface Debuggable {
 type TestFramework = typeof vi;
 const testFramework: TestFramework = (globalThis as any).vi ?? (globalThis as any).jest;
 
-// Extend Vitest with the custom matchers, this file needs to be imported in the vitest.setup.ts file or the test file
+// Extend Vitest or Jest with the custom matchers
 expect.extend({
   toLog(
     transaction: Debuggable,

packages/cashscript/src/test/VitestExtensions.ts

@@ -0,0 +1,11 @@
+import './TestExtensions.js';
+
+// The type parameters have to match the ones of Vitest's own Matchers interface for the declarations to merge
+declare module 'vitest' {
+  // eslint-disable-next-line @typescript-eslint/no-unused-vars
+  interface Matchers<R extends void | Promise<void> = void | Promise<void>, T = unknown> {
+    toLog(value?: RegExp | string): R;
+    toFailRequireWith(value: RegExp | string): R;
+    toFailRequire(): R;
+  }
+}

packages/cashscript/test/Contract.test.ts

@@ -11,7 +11,7 @@ import {
 } from '../src/index.js';
 import {
   aliceAddress,
-  alicePkh, alicePriv, alicePub, bobPriv,
+  alicePkh, alicePriv, alicePub, bobPriv, bobPub,
 } from './fixture/vars.js';
 import { addUtxo } from './test-util.js';
 import { generateLibauthSourceOutputs } from '../src/utils.js';
@@ -39,6 +39,14 @@ describe('Contract', () => {
       expect(() => new Contract(p2pkhArtifact, [placeholder(21)], { provider })).toThrow();
     });
 
+    it('should fail with malformed hex string constructor args', () => {
+      const provider = new MockNetworkProvider();
+
+      // An odd number of digits or non-hex characters would otherwise silently produce different bytes
+      expect(() => new Contract(p2pkhArtifact, [`0x${'ab'.repeat(19)}a`], { provider })).toThrow(/valid hex string/);
+      expect(() => new Contract(p2pkhArtifact, ['zz'.repeat(20)], { provider })).toThrow(/valid hex string/);
+    });
+
     it('should fail with artifact compiled with unsupported compiler version', async () => {
       const provider = new ElectrumNetworkProvider(Network.CHIPNET);
       const constructorArgs = [placeholder(20), placeholder(20), 1000000n];
@@ -168,11 +176,13 @@ describe('Contract', () => {
       expect(() => instance.unlock.spend(alicePub, new SignatureTemplate(alicePriv), 0n)).toThrow();
       expect(() => bbInstance.unlock.spend(hexToBin('e803'), 1000n)).toThrow();
       expect(() => bbInstance.unlock.spend(hexToBin('e803000000'), 1000n)).toThrow();
+      expect(() => instance.unlock.spend(alicePkh, placeholder(65)))
+        .toThrow("Found type 'bytes20' where type 'pubkey' was expected");
     });
 
     it('can call spend with incorrect arguments', () => {
       expect(() => instance.unlock.spend(alicePub, new SignatureTemplate(bobPriv))).not.toThrow();
-      expect(() => instance.unlock.spend(alicePkh, placeholder(65))).not.toThrow();
+      expect(() => instance.unlock.spend(bobPub, placeholder(65))).not.toThrow();
       expect(() => bbInstance.unlock.spend(hexToBin('e8031234'), 1000n)).not.toThrow();
     });