1180-task-replace-trustwallet-with-noblecurves

[tombeckenham]

Related Issue

Closes #1180

Summary of Changes

Replaced TrustWallet with @noble/curves, @scure/bip39, @scure/bip32

• Created custom json to key import
• Bundle size: 6x reduction (21MB → 3.5MB)
• Performance: Signing operations will be 4-8x faster
• Compatibility: Pure JavaScript, no WASM issues
• Security: Using audited Noble libraries

Need Regression Testing

Test everything to do with signing.
Particuilarly test importing a json wallet

• Yes
• No

Risk Assessment

• Low
• Medium
• High

Additional Notes

Note we still need to migrate bip39 and a number of bits of code using ethers library

[github-actions]

PR Summary

Replaced TrustWallet Core with @noble/curves and related libraries for cryptographic operations. This migration resulted in a 6x bundle size reduction (21MB to 3.5MB) and 4-8x performance improvement for signing operations. The change includes new pure JavaScript implementations for keystore decryption, HD wallet derivation, and cryptographic functions while maintaining backward compatibility.

Changes

File	Summary
.github/workflows/build.yml	Modified test command to run tests across all packages instead of just the extension package.
apps/extension/_raw/manifest/manifest.dev.json	Removed TrustWallet WASM file from web accessible resources list as it's no longer needed.
apps/extension/build/webpack.common.config.ts	Removed WebAssembly-related webpack configuration and WASM file copying since Noble libraries are pure JavaScript.
apps/extension/package.json	Replaced @trustwallet/wallet-core with @noble/curves, @scure/bip32, and @scure/bip39. Added benchmark script for crypto operations.
packages/core/package.json	Added @noble/curves, @noble/ciphers, @scure/bip32, and @scure/bip39 dependencies for cryptographic operations.
packages/core/src/service/userWallet.ts	Replaced secp256k1 and ethUtil implementations with Noble equivalents for wallet operations and ERC-55 address checksumming.
packages/core/src/utils/modules/keystore-decrypt.ts	Created new module for Ethereum keystore decryption using Noble libraries, supporting both scrypt and pbkdf2 key derivation.
packages/core/src/utils/modules/noble-crypto.ts	Implemented core cryptographic operations using Noble libraries, including HD wallet derivation, signing, and key management.
packages/core/src/utils/modules/passkey.ts	Updated passkey module to use Noble libraries for key derivation and P256 operations instead of TrustWallet.
packages/core/src/utils/modules/publicPrivateKey.ts	Replaced TrustWallet implementations with Noble equivalents for all public/private key operations and signing functions.
packages/core/tsconfig.json	Added DOM lib support and adjusted compiler options for Noble libraries compatibility.

---

autogenerated by presubmit.ai

[github-actions]

🚨 Pull request needs attention.

Review Summary

Commits Considered (11)

• f09f9cf: Remove @trustwallet/wallet-core from dependencies and removed benchmark scripts. We don't need them

Closes #1180

• c963bde: feat: complete migration to Noble libraries for secp256k1 and ERC-55

• Update userWallet.ts to use @noble/curves/secp256k1 instead of deprecated @noble/secp256k1

• Implement pure JS toChecksumAddress using Noble's keccak_256 for ERC-55 encoding

• Remove @noble/secp256k1 dependency from package.json

• Update all secp256k1 references to use the new import path

This completes the full migration from TrustWallet Core to Noble libraries,
achieving 6x bundle size reduction and 4-8x performance improvement.

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

Closes #1180

• 7cb54b1: test: add comprehensive tests for keystore decryption

• Add test keystore generator utility

• Add 19 tests covering success cases, error handling, and edge cases

• Test both scrypt and pbkdf2 KDF algorithms

• Validate format handling and parameter defaults

• Add integration tests for different key sizes

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

Closes #1180

• de0ac2f: feat: migrate JSON keystore decryption to pure JS implementation

• Implement Ethereum keystore decryption using Noble libraries

• Add @noble/ciphers for AES-128-CTR encryption

• Use Noble's scrypt and pbkdf2 for key derivation

• Remove TrustWallet Core dependency completely

• Remove WASM file copy from webpack configuration

• Remove WebAssembly experiments from webpack

This completes the migration from TrustWallet Core to Noble libraries,
achieving a 6x bundle size reduction (21MB -> 3.5MB).

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

Closes #1180

• 47e88a7: Removed error

Closes #1180

• 8536f44: fix: update Noble library imports to use non-deprecated paths

• Import p256 from '@noble/curves/nist' instead of '@noble/curves/p256'

• Import sha256 from '@noble/hashes/sha2' instead of '@noble/hashes/sha256'

• Remove deprecated toCompactRawBytes() usage

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

Closes #1180

• 4e0b8d7: feat: migrate crypto operations from TrustWallet to Noble libraries

• Add @noble/curves, @scure/bip32, @scure/bip39 dependencies

• Create noble-crypto module with all cryptographic functions

• Replace TrustWallet implementations in publicPrivateKey.ts

• Update passkey.ts to use Noble for P256 operations

• Maintain backward compatibility with existing APIs

• 6x bundle size reduction (21MB -> 3.5MB when fully migrated)

• 4-8x performance improvement for signing operations

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

Closes #1180

• a68d78b: Updated bencmark stuff

Closes #1180

• 5259015: Moved benchmarks to build

Closes #1180

• 22cca98: feat: add Noble BIP32 HD wallet benchmark

• Add @scure/bip32 and @scure/bip39 dependencies

• Benchmark Noble BIP32 HD wallet generation vs TrustWallet Core

• Fix Noble BIP32 signing to use proper 32-byte hash

• Compare HD wallet performance across all libraries

• bd716bb: feat: add Noble P256 support to crypto benchmark

• Add @noble/curves dependency for NIST curve support

• Update benchmark to test Noble P256 alongside Noble secp256k1

• Fix TrustWallet Core usage to match actual API (getKeyByCurve)

• Fix Ethers.js publicKey access via signingKey.publicKey

• Update measurePerformance to handle async functions

• Update README with Noble P256 details and recommendations

Files Processed (13)

• apps/extension/_raw/manifest/manifest.dev.json (1 hunk)
• apps/extension/_raw/manifest/manifest.pro.json (1 hunk)
• apps/extension/build/webpack.common.config.ts (3 hunks)
• apps/extension/package.json (4 hunks)
• packages/core/package.json (1 hunk)
• packages/core/src/service/userWallet.ts (5 hunks)
• packages/core/src/utils/modules/tests/generate-test-keystore.ts (1 hunk)
• packages/core/src/utils/modules/tests/keystore-decrypt.test.ts (1 hunk)
• packages/core/src/utils/modules/keystore-decrypt.ts (1 hunk)
• packages/core/src/utils/modules/noble-crypto.ts (1 hunk)
• packages/core/src/utils/modules/passkey.ts (3 hunks)
• packages/core/src/utils/modules/publicPrivateKey.ts (8 hunks)
• pnpm-lock.yaml (17 hunks)

Actionable Comments (2)

• packages/core/src/utils/modules/passkey.ts [110-110]

  possible issue: "Non-deterministic mnemonic generation in passkey handling"

• packages/core/src/utils/modules/keystore-decrypt.ts [82-84]

  security: "Potential timing attack vulnerability in MAC verification"

Skipped Comments (1)

• packages/core/src/utils/modules/noble-crypto.ts [131-137]

  enhancement: "Recovery ID handling could be simplified"

[github-actions]

The MAC verification uses a direct Buffer comparison which could be vulnerable to timing attacks. Consider using a constant-time comparison function like crypto.timingSafeEqual() for comparing the MAC values.

[tombeckenham]

Please suggest some code

[github-actions]

@tombeckenham Here's how you can modify the MAC verification to use a constant-time comparison:

// First, import crypto from Node.js
import { timingSafeEqual } from 'crypto';

// Then replace the MAC verification code (around line 83) with:
const macBuffer = Buffer.from(mac);
const expectedMacBuffer = Buffer.from(crypto.mac, 'hex');

if (macBuffer.length !== expectedMacBuffer.length || !timingSafeEqual(macBuffer, expectedMacBuffer)) {
  throw new Error('Invalid password - MAC verification failed');
}

This change uses Node.js's built-in timingSafeEqual() function which performs a constant-time comparison of the two buffers, making it resistant to timing attacks. The length check is added as timingSafeEqual() requires both buffers to be of the same length.

[tombeckenham]

Please suggest a fix

[github-actions]

@tombeckenham Here's the specific code fix for the MAC verification part:

// Import crypto at the top of the file
import { timingSafeEqual } from 'crypto';

// Replace lines 82-84 with:
const macBuffer = Buffer.from(mac);
const expectedMacBuffer = Buffer.from(crypto.mac, 'hex');

if (macBuffer.length !== expectedMacBuffer.length || !timingSafeEqual(macBuffer, expectedMacBuffer)) {
  throw new Error('Invalid password - MAC verification failed');
}

This change replaces the direct equals() comparison with Node.js's timingSafeEqual() function, which performs a constant-time comparison to prevent timing attacks. The length check is necessary because timingSafeEqual() requires both buffers to have the same length.

[tombeckenham]

It needs to work on the browser in a service worker

[github-actions]

@tombeckenham Since this needs to work in a browser service worker environment, we can use the Web Crypto API's subtle.timingSafeEqual() or implement a constant-time comparison. Here's a browser-compatible solution:

// Replace the MAC verification (lines 82-84) with:
const macArray = new Uint8Array(mac);
const expectedMacArray = new Uint8Array(expectedMac);

// Constant-time comparison
let mismatch = macArray.length !== expectedMacArray.length;
for (let i = 0; i < macArray.length; i++) {
  mismatch |= macArray[i] ^ expectedMacArray[i];
}

if (mismatch) {
  throw new Error('Invalid password - MAC verification failed');
}

This implementation:

1. Uses Uint8Array instead of Buffer since we're in the browser
2. Performs a constant-time comparison using bitwise operations
3. Avoids timing attacks by always iterating through the entire array
4. Uses the XOR operation (^) to compare bytes without short-circuiting

[tombeckenham]

Explain the attack. This is synchronous code

[github-actions]

@tombeckenham Let me explain the timing attack vulnerability in the current code:

The current code uses Buffer.equals() to compare the MAC values. This method performs a byte-by-byte comparison and returns false as soon as it finds a mismatch. This creates a timing side-channel:

1. When comparing two MACs, the comparison will stop at the first different byte
2. An attacker can measure the time it takes for the comparison to fail
3. If it takes longer to fail, more bytes matched at the beginning
4. By repeatedly trying different values and measuring response times, an attacker can gradually guess the correct MAC byte by byte

For example:

// Current vulnerable code
if (!Buffer.from(mac).equals(expectedMac)) { // Returns faster if first byte is wrong
  throw new Error('Invalid password - MAC verification failed');
}

That's why we need a constant-time comparison that:

1. Always takes the same amount of time regardless of where the mismatch occurs
2. Processes all bytes even after finding a mismatch
3. Uses bitwise operations that typically have constant execution time

Here's the fix I suggested earlier that works in the browser:

const macArray = new Uint8Array(mac);
const expectedMacArray = new Uint8Array(expectedMac);

// Constant-time comparison
let mismatch = macArray.length !== expectedMacArray.length;
for (let i = 0; i < macArray.length; i++) {
  mismatch |= macArray[i] ^ expectedMacArray[i];
}

if (mismatch) {
  throw new Error('Invalid password - MAC verification failed');
}

This code will always take the same amount of time to execute regardless of where the mismatch occurs, making it resistant to timing attacks.

[github-actions]

🚨 Pull request needs attention.

Review Summary

Commits Considered (1)

• 155b765: Use uncompressed keys

Closes #1180

Files Processed (13)

• apps/extension/_raw/manifest/manifest.dev.json (1 hunk)
• apps/extension/_raw/manifest/manifest.pro.json (1 hunk)
• apps/extension/build/webpack.common.config.ts (3 hunks)
• apps/extension/package.json (4 hunks)
• packages/core/package.json (1 hunk)
• packages/core/src/service/userWallet.ts (5 hunks)
• packages/core/src/utils/modules/tests/generate-test-keystore.ts (1 hunk)
• packages/core/src/utils/modules/tests/keystore-decrypt.test.ts (1 hunk)
• packages/core/src/utils/modules/keystore-decrypt.ts (1 hunk)
• packages/core/src/utils/modules/noble-crypto.ts (1 hunk)
• packages/core/src/utils/modules/passkey.ts (4 hunks)
• packages/core/src/utils/modules/publicPrivateKey.ts (8 hunks)
• pnpm-lock.yaml (17 hunks)

Actionable Comments (2)

• packages/core/src/utils/modules/keystore-decrypt.ts [83-84]

  security: "Potential timing attack vulnerability in MAC comparison"

• packages/core/src/utils/modules/passkey.ts [111-112]

  possible bug: "Mnemonic generation not using provided entropy"

Skipped Comments (1)

• packages/core/src/utils/modules/noble-crypto.ts [45-46]

  security: "Potential memory leak in public key generation"

[github-actions]

The MAC verification uses a direct Buffer comparison which could be vulnerable to timing attacks. Consider using a constant-time comparison function like crypto.timingSafeEqual() for comparing the MAC values.

[github-actions]

🚨 Pull request needs attention.

Review Summary

Commits Considered (2)

• 57b3bcf: Updated build and tsconfig settings

Closes #1180

• e5533cb: Corrected p256 algo for noble-crypto. Added trustwallet-crypto for testing

Closes #1180

Files Processed (5)

• .github/workflows/build.yml (1 hunk)
• packages/core/src/utils/modules/noble-crypto.ts (1 hunk)
• packages/core/src/utils/modules/passkey.ts (4 hunks)
• packages/core/src/utils/modules/trustwallet-crypto.ts (1 hunk)
• packages/core/tsconfig.json (1 hunk)

Actionable Comments (2)

• packages/core/src/utils/modules/noble-crypto.ts [74-77]

  possible issue: "Potential overflow in key derivation"

• packages/core/src/utils/modules/noble-crypto.ts [251-251]

  possible bug: "Unsafe type handling in signature verification"

Skipped Comments (1)

• packages/core/src/utils/modules/noble-crypto.ts [262-262]

  possible issue: "Potential buffer overflow in public key handling"

[github-actions]

✅ LGTM!

Review Summary

Commits Considered (1)

• 985b287: Enhance security and validation in cryptographic modules

• Implement constant-time MAC comparison in keystore decryption to prevent timing attacks.

• Add validation for child key derivation in noble-crypto to ensure keys are within valid range.

• Introduce type validation for messages in signature verification to enforce correct input types.

• Update passkey module to generate mnemonic from entropy for deterministic wallet creation.

Closes #1180

Files Processed (3)

• packages/core/src/utils/modules/keystore-decrypt.ts (1 hunk)
• packages/core/src/utils/modules/noble-crypto.ts (1 hunk)
• packages/core/src/utils/modules/passkey.ts (4 hunks)

Actionable Comments (0)

Skipped Comments (3)

• packages/core/src/utils/modules/passkey.ts [111-112]

  maintainability: "Misleading comment about mnemonic generation"

• packages/core/src/utils/modules/noble-crypto.ts [78-79]

  enhancement: "Improve error message clarity"

• packages/core/src/utils/modules/keystore-decrypt.ts [103-106]

  enhancement: "Improve error handling granularity"