Enhance encryption handling with error reporting and validation

[sergei-boiko-trustwallet]

Summary

• TWResultVoid — new structured error type. A lightweight Rust-backed result type
  (TWResultVoidCreateOk / TWResultVoidCreateError) is exposed through the C ABI and
  regenerated into all binding layers (Swift, Kotlin/KMP, JNI C, WASM). It carries an
  optional error string so callers can distinguish failure reasons without parsing exception
  messages.

• TWStoredKeyFixEncryption return type: bool → TWResultVoid*. Callers can now get
  the actual error message (e.g. "Invalid password: MAC verification failed") instead of a
  boolean false. Breaking C ABI change — existing compiled binaries that read the return
  as bool will always see true (non-null pointer), which is a silent no-op on failure.
  Documented in docs/bc-footguns.md.

• New API: TWStoredKeyValidateJson and TWStoredKeyValidateFile. Structural validation
  of a keystore JSON blob or file path without decrypting. Confirms the crypto parameters
  (KDF, cipher, IV) are well-formed without requiring the password.

• KeyStore.invalidKeys (Swift). KeyStore now surfaces files that failed to load as
  [InvalidKey] instead of silently discarding them. Each entry carries the file URL, a
  human-readable error, whether it looks like a third-party keystore (top-level address
  field), and the first account address if present in the JSON.

• DecryptionError and AESValidationError enums removed. All error paths now throw
  std::invalid_argument or std::runtime_error with descriptive messages, making error
  text visible at the C ABI boundary.

• Bug fix: @Test annotation was missing on testFixScryptWithEmptySalt in the Android
  instrumented tests — that test was silently never running.

Test plan

• C++: ./build/tests/tests --gtest_filter="*StoredKey*" and --gtest_filter="*TWResultVoid*"
• Swift (tools/ios-test): testValidateFile, testValidateJson, testLoadAllInvalidKeys,
  testLoadOneInvalidKeyAlongsideValidWallet, testLoadInvalidKeysEmptyWhenAllWalletsAreValid,
  testFixScryptWithEmptySalt
• Android (./gradlew connectedAndroidTest): testValidateFile, testValidateJson,
  testFixScryptWithEmptySalt
• WASM (cd wasm && npm run build-and-test): test validateJson, test fix scrypt with empty salt

[octane-security-app]

Summary by Octane

New Contracts

• tw_result.rs: The contract provides error handling with success/error states, allowing creation, querying, and deletion of TWResultVoid outcomes.

Updated Contracts

• mod.rs: Added a new module tw_result to the smart contract's features.
• TestKeyStore.kt: Added error handling for incorrect passwords during encryption fix; improved verification by adding a unit test for empty salt scenarios.
• AESParameters.cpp: The error handling in AES validation now provides detailed messages and returns a TW::Result<void> type instead of an optional error.
• EncryptionParameters.cpp: Refined error handling by replacing specific error types with standard exceptions and streamlined validation checks.
• StoredKey.cpp: Improved error handling for missing 'crypto' field in stored key JSON.

---

🔗 Commit Hash: ad8c14a

[github-actions]

⚠️ Backward-compatibility check needed

This PR touches persistence-sensitive files: include/TrustWalletCore/TWStoredKey.h, src/Keystore/AESParameters.cpp, src/Keystore/AESParameters.h, src/Keystore/EncryptionParameters.cpp, src/Keystore/EncryptionParameters.h, src/Keystore/StoredKey.cpp, src/Keystore/StoredKey.h, tests/common/Keystore/StoredKeyTests.cpp.

Post a comment with one of:

• [bc-check: Pass] — audit run, Verdict: SAFE. Must include audit output.
• [bc-check: Mitigated] — audit found RISK or BLOCKER; you fixed it in code. Must include the post-fix audit output.
• [bc-check: Risk-Accepted] — audit found RISK or BLOCKER; investigation confirms blast radius is effectively zero (no user data in the wild can trigger it). Must include audit output + explicit evidence (who confirmed it, what data or reasoning).
• [bc-check: N/A] — scanner fired on a file with zero BC relevance (comment edit, test fixture, renamed variable). Invalid if the audit found a real risk.

Each token must be accompanied by >=60 chars of reasoning. The reasoning must be fresh — posted or edited at or after the HEAD commit.

Why audit evidence is required for Pass / Mitigated: humans don't reliably ask the right BC question on every PR. AI being in the loop is the whole point of this gate. The bot does not judge whether your reasoning is correct — reviewers do, like any other code review.

Things worth thinking about: could a previous version have written data this PR's new check would now reject? Was there a partial migration ("regenerate on next user action") that may not have completed for all users? Does this format live in iCloud / Google Drive backup, exported files, or sync payloads? See docs/bc-footguns.md for known cases.

Changed files (8)

• include/TrustWalletCore/TWStoredKey.h
• src/Keystore/AESParameters.cpp
• src/Keystore/AESParameters.h
• src/Keystore/EncryptionParameters.cpp
• src/Keystore/EncryptionParameters.h
• src/Keystore/StoredKey.cpp
• src/Keystore/StoredKey.h
• tests/common/Keystore/StoredKeyTests.cpp

Copy this audit prompt into Claude Code on this branch (or run the /bc-check skill) for a structured analysis you can paste into your sign-off

You are auditing a Trust Wallet Core PR against origin/master
for **backward-compatibility risk against persisted user data and wire formats**.
Find cases where this PR will reject, mis-parse, or mishandle inputs that older
versions of our software already wrote to disk, to backups, or onto the network.

**Trust Wallet Core domain context:**
- src/Keystore/ — JSON keystore files (user encrypted keys/mnemonics).
  These live in iCloud, Google Drive, and manual exports.
  A parse failure = wallet inaccessible = user cannot access funds.
- src/proto/*.proto — Protobuf SigningInput/SigningOutput wire formats.
  Field numbers are permanent; reuse or removal silently corrupts binary data.
- include/TrustWalletCore/TW*.h — Public C ABI.
  Removing/renaming TW* functions or changing enum values breaks compiled bindings.
- registry.json — coin metadata (SLIP44, derivation path, curve, address encoding).
  Changing any of these re-derives different addresses for all existing wallets.

Do NOT trust the PR description's framing.
'Just a security fix' / 'stricter validation' is exactly the framing that hides this class of bug.
Read docs/bc-footguns.md first.

Walk these 5 steps. Cite file:line and commit SHA everywhere:

1. Classify: tightening validation? Parsing change? Exception type change?
   Removing 'if missing use default'? Moving validation into a constructor?
2. Historical baseline. For each tightened rule:
   - Could a prior version have PRODUCED data that violates the new rule? (cite SHA)
   - Was there a partial migration that may not have completed for all users?
   - Does the format ever leave the device (backup, export, sync)?
3. Concrete failure scenarios: old version -> action -> where stored -> code path
   (file:line) -> user symptom -> blast radius. No hand-waving.
4. Red-flag checklist (yes/no + file:line evidence):
   - New throw on a read/load/decode/import path?
   - Removed an 'if missing use default' branch?
   - New length/range/enum check on a >1-year-old field?
   - Constructor changed from lenient parse to parse+validate?
   - Changed which exception type a public API throws?
   - Format ever in backup/export/sync?
   - Prior PR shipped a 'regenerate on next user action' partial migration? (cite SHA)
   - Proto: field number reused or removed? Enum value renumbered/removed?
   - Keystore: JSON key renamed/removed/made required without default fallback?
   - Registry: slip44, curve, or address-encoding field changed?
5. Mitigations: accept legacy at read + normalize on write (preferred);
   gate strict check behind 'newly created' flag; one-time migration with clear UX;
   or apply tightening to write paths only.

Output a markdown report: Verdict (SAFE/RISK/BLOCKER) at top, then steps 1-5,
then a 'Suggested PR comment' block ([bc-check: Pass|Mitigated|N/A] + reasoning),
then a 'Suggested addition to docs/bc-footguns.md' block (or 'none').

Merge is blocked by verify-bc-check-comment until a tagged comment is posted.

[Copilot]

Pull request overview

This PR modernizes encryption-related error handling in Wallet Core by introducing a standardized FFI “void result” type (TWResultVoid) and refactoring fixEncryption (and related keystore code) to return structured success/error outcomes with descriptive messages across language bindings.

Changes:

• Introduces TWResultVoid in the Rust FFI layer (plus interface tests) and integrates it into the public C API surface.
• Refactors TWStoredKeyFixEncryption to return TWResultVoid* and propagates exception-based error messages instead of boolean/enums.
• Updates multi-platform tests (C++/Rust/Swift/Android/Wasm) to validate success/error states and error strings.

Reviewed changes

Copilot reviewed 20 out of 22 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
wasm/tests/KeyStore+fs.test.ts	Updates Wasm keystore test to use TWResultVoid-style result object and delete it.
wasm/tests/KeyStore+extension.test.ts	Same as above for extension storage variant.
wasm/tests/KeyStore.test.ts	Uses result object from fixEncryption() and releases it; keeps behavior assertions.
tests/interface/TWStoredKeyTests.cpp	Updates TWStoredKeyFixEncryption tests to assert success/error and verify error message.
tests/interface/TWResultVoidTests.cpp	Adds C++ interface tests validating TWResultVoid behavior.
tests/common/Keystore/StoredKeyTests.cpp	Updates exception expectations after removal of custom decryption enums.
swift/Tests/Keystore/KeyStoreTests.swift	Updates Swift tests to assert fixEncryption error message on wrong password.
src/Keystore/StoredKey.h	Updates API docs to reflect exception types thrown.
src/Keystore/StoredKey.cpp	Replaces custom decryption error with std::invalid_argument and message for invalid JSON format.
src/Keystore/EncryptionParameters.h	Removes DecryptionError enum from keystore parameters.
src/Keystore/EncryptionParameters.cpp	Switches validation to TW::Result<void> and refactors decrypt errors to standard exceptions/messages.
src/Keystore/AESParameters.h	Changes AES parameter validation to return TW::Result<void> with message.
src/Keystore/AESParameters.cpp	Implements new validation result with detailed IV-size error.
src/interface/TWStoredKey.cpp	Changes TWStoredKeyFixEncryption to return TWResultVoid* and map exceptions to error messages.
rust/tw_memory/tests/tw_result_ffi_tests.rs	Adds Rust-side FFI tests covering success/error/null-pointer behavior and deletion.
rust/tw_memory/src/ffi/tw_result.rs	Adds Rust FFI implementation of TWResultVoid and its exported functions.
rust/tw_memory/src/ffi/mod.rs	Exposes the new tw_result module.
rust/tw_memory/Cargo.toml	Adds tw_macros dependency for FFI metadata generation.
rust/Cargo.lock	Locks in the new tw_macros dependency for tw_memory.
include/TrustWalletCore/TWStoredKey.h	Updates public header: TWStoredKeyFixEncryption now returns TWResultVoid* with updated docs.
android/app/src/androidTest/java/com/trustwallet/core/app/utils/TestKeyStore.kt	Updates Android instrumentation test to assert fixEncryption error message on wrong password.
.gitignore	Ignores generated include/TrustWalletCore/TWResultVoid.h.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

Binary size comparison

➡️ aarch64-apple-ios: 14.31 MB

➡️ aarch64-apple-ios-sim: 14.32 MB

➡️ aarch64-linux-android:

- 18.73 MB
+ 18.73 MB 	 +3 KB

➡️ armv7-linux-androideabi:

- 16.17 MB
+ 16.17 MB 	 +3 KB

➡️ wasm32-unknown-emscripten:

- 13.66 MB
+ 13.66 MB 	 -1 KB

[octane-security-app]

Overview

Vulnerabilities found:	4
Severity breakdown:	3 Low, 1 Informational

Warnings found:	4

Detailed findings

rust/tw_memory/src/ffi/tw_result.rs

• Lossy UTF-8 handling in TWResultVoidCreateError causes empty error messages. See more

src/Keystore/AESParameters.h

• Allocating error formatting in noexcept validator in Keystore AESParameters::validate() causes crash-only DoS on malformed keystore under memory pressure. See more

src/Keystore/EncryptionParameters.cpp

• Missing derived-key length check in Keystore re-encryption (scrypt) causes crash/DoS. See more
• Unbounded ciphertext import in keystore decrypt/repair causes memory/CPU DoS. See more

Warnings

codegen-v2/src/codegen/cpp/code_gen.rs

• Unchecked _Nonnull pointer dereferences in C++ wrappers for TWResultVoid API cause process crash on null inputs. See more

rust/tw_memory/src/ffi/tw_result.rs

• Missing size caps on TWResultVoid FFI error strings in tw_result.rs causes helper-level DoS via memory exhaustion. See more

src/Keystore/StoredKey.cpp

• No-op success in TWStoredKeyFixEncryption for non-fix cases causes password-verification bypass footgun in keystore migration. See more
• Exception type change and in-place mutation order in StoredKey::loadJson causes persisted keystore metadata corruption/DoS. See more

---

🔗 Commit Hash: ad8c14a
🛡️ Octane Dashboard: All vulnerabilities

[sergei-boiko-trustwallet]

[bc-check: Risk-Accepted]

BC-risk audit — fix/ripple-x-address-tag → master @ 175af63 — 2026-06-03

Verdict: RISK (investigated; blast radius is effectively zero for user keystore data)

Finding: TWStoredKeyFixEncryption C ABI return type changed from bool to struct TWResultVoid* (include/TrustWalletCore/TWStoredKey.h:515). Existing compiled binaries against the prior ABI would interpret a non-null TWResultVoid* pointer as true, meaning a failed migration (wrong password)
could appear as success. The key is not mutated on failure, so no keystore data is corrupted or made inaccessible — the worst outcome is that the scrypt-salt strengthening migration silently no-ops.

All keystore JSON fields are unchanged: "crypto", "salt", "dklen", "n"/"p"/"r", "iv", "kdfparams", "cipherparams", "activeAccounts" — identical field names and fallback behavior (ScryptParameters.cpp:130 missing-salt → empty-Data fallback preserved). Every valid keystore that
loaded before this PR will continue to load after it.

DecryptionError enum removal (EncryptionParameters.h) is internal-only; all public TW* functions caught (...) before and continue to. SDK consumers are unaffected.

Recommended action: Bump version minor to signal ABI change; note in release notes that fixEncryption now returns ResultVoid instead of bool.

[Copilot]

Pull request overview

Copilot reviewed 25 out of 27 changed files in this pull request and generated 2 comments.

[sergei-boiko-trustwallet]

[bc-check: Risk-Accepted]

BC-risk audit — fix/ripple-x-address-tag → master @ 175af63 — 2026-06-03

Verdict: RISK (investigated; blast radius is effectively zero for user keystore data)

Finding: TWStoredKeyFixEncryption C ABI return type changed from bool to struct TWResultVoid* (include/TrustWalletCore/TWStoredKey.h:515). Existing compiled binaries against the prior ABI would interpret a non-null TWResultVoid* pointer as true, meaning a failed migration (wrong password)
could appear as success. The key is not mutated on failure, so no keystore data is corrupted or made inaccessible — the worst outcome is that the scrypt-salt strengthening migration silently no-ops.

All keystore JSON fields are unchanged: "crypto", "salt", "dklen", "n"/"p"/"r", "iv", "kdfparams", "cipherparams", "activeAccounts" — identical field names and fallback behavior (ScryptParameters.cpp:130 missing-salt → empty-Data fallback preserved). Every valid keystore that
loaded before this PR will continue to load after it.

DecryptionError enum removal (EncryptionParameters.h) is internal-only; all public TW* functions caught (...) before and continue to. SDK consumers are unaffected.

Recommended action: Bump version minor to signal ABI change; note in release notes that fixEncryption now returns ResultVoid instead of bool.

[BSCSecChef]

LGTM