trustwallet/wallet-core

uint32_t elemLen = nextElem.getTotalLen();
...
uint32_t dataLen = skipClone(typeDesc.byteCount).getTotalLen();

getTotalLen and getCompoundLength recurse once per nesting level, and isValid and getCompoundElements follow the same pattern. A few kilobytes of input such as 0x81 0x81 0x81 … or 0xC0 0xC0 … makes one stack frame per byte. That input is reached through Cardano address parsing after base58 decoding, and through Signer::plan validation of auxiliary data. A stack overflow cannot be caught by the surrounding catch, so it crashes the host app, especially on small-stack worker threads.

Add a nesting-depth limit (for example 64) to every recursive walker, or make the length walk iterative. Reject input that exceeds it.

for elem_idx in 0..len {
    let res = decode_param(kind, tail, new_offset)...;
    tokens.push(res.token);

len comes from the calldata (up to 2^32−1) and is not checked against the remaining data. For element types that consume no bytes (an empty tuple, tuple[]), a 32-byte input pushes billions of tokens. For bytes[] or string[], every element offset can point at the same large blob, which is copied once per element. A 256 KB calldata then allocates about a gigabyte. This is reachable from the public ABI decode functions, and with panic = "abort" the result is a host-process abort or OOM kill.

Before the loop, require len × minimum element size ≤ remaining bytes. Reject zero-sized element types, and keep a total decoded-bytes and token budget per call, as the encoder already does with its 4096 cap.

let len = *VarInt::decode(r)?;
let mut buf = vec![0; len as usize];
r.read_exact(&mut buf)?;

About 15 bytes of transaction hex with a memo length of 2^40 make the library try a terabyte allocation. This is reachable from the transaction-hash utility. Under panic = "abort" the host app is killed.

Read through r.take(len) and verify the byte count, or cap len at the protocol maximum before allocating.

wget -O "$LCOV_DEB" http://mirrors.kernel.org/ubuntu/pool/universe/l/lcov/lcov_1.15-1_all.deb
sudo apt-get install "$LCOV_DEB"

A local .deb installed this way has no signature verification. Anyone positioned on the network path, or a compromised mirror, can substitute the package. Its maintainer scripts then run as root on developer machines and on every Linux CI runner that builds the wallet library.

Use HTTPS and verify a pinned SHA-256 before installing, or use the distribution package or a checksummed source archive.

if (json[CodingKeys::derivationPath].is_object()) {
...
coin = TWCoinType(uint32_t(derivationPath.indices[1].value));

json is const, so operator[] on a missing key is undefined behaviour. An account with neither coin nor a two-level path indexes an empty vector. A malformed backup file can crash the host app during import, and a crash cannot be caught by the import's catch (...).

Check contains() before every read, and reject accounts without a coin or a path of sufficient depth.

if ((r > std::numeric_limits<uint32_t>::max() / 128 / p) ||
    (n > std::numeric_limits<uint32_t>::max() / 128 / r)) {

validate() runs on JSON from the imported keystore file. With "p": 0 (or "r": 0) it divides by zero. On x86 hosts (desktop, server, simulators) this raises SIGFPE, and catch (...) cannot stop it. On every platform it is undefined behaviour. A single malicious "backup file" crashes the importing app.

Reject p == 0 and r == 0 before any division, and set sane bounds on n, dklen and the PBKDF2 iteration count.

let k = Scalar::from_hash(h);
let s = k * self.key + r;
Ok(Signature { R, s })

Signing is assembled in-house from curve primitives, a port of an old ed25519-dalek release with a pluggable hasher. The signing method also takes the public key as a separate argument. Hand-maintained signature code is the most expensive place to carry a subtle bug, and a past advisory already concerned this exact function. No defect was demonstrated.

Use a maintained signature crate (for example ed25519-dalek 2.x) for the standard SHA-512 variant. Keep custom code only for the non-standard hashers, isolated in one small module. Add the RFC 8032 and Wycheproof vectors to CI, and make sure the variant that takes a caller-chosen public key cannot be reached from outside the crate.

/// TODO make sure if this is the right way to create an extended secret key (extsk).
pub(crate) fn with_extended_secret(secret: H256, extension: H256) -> Self {
    let key = Scalar::from_bytes_mod_order(secret.take());

The code's own comment says the reduction changes the secret, and that it is not certain this is correct. Key-material derivation that is knowingly unverified is a latent risk.

Resolve the TODO. Use a vetted BIP32-Ed25519 implementation, or add reference test vectors that prove the signing scalar and public key match the reference.

fn diffie_hellman(private: &SigningKey, public: &VerifyingKey) -> AffinePoint {
    let public_point = ProjectivePoint::from(*public.as_affine());
    (public_point * secret_scalar).to_affine()

The key agreement does the scalar multiplication by hand and hashes the compressed point with its tag byte, instead of using the library's shared-secret API. This is hard to audit and easy to mismatch with peers.

Build on k256::ecdh::diffie_hellman. If the legacy output must be kept, layer it on top and label it as a compatibility-only KDF, with test vectors.

let genesis_hash = input.genesis_hash.as_ref().try_into().unwrap_or_default();
let current_hash = input.block_hash.as_ref().try_into().unwrap_or_default();

A genesis or block hash of the wrong length becomes an all-zero hash, and the transaction is signed over it. The caller gets a valid-looking signature that the network rejects, or one bound to the wrong chain context, with no error.

Return an invalid-value error when the length is not 32. Allow an empty block hash only where the era makes it legitimate.

let genesis_hash = input.genesis_hash.as_ref().try_into().unwrap_or_default();
let current_hash = input.block_hash.as_ref().try_into().unwrap_or_default();

This is the same defect as finding 4, in the Polymesh entry point.

As in finding 4.

blake2_b(&self.sign_bytes().unwrap_or_default(), 32).unwrap_or_default()

If encoding fails, the id becomes the hash of an empty buffer. That is the same plausible-looking constant for every failed transaction.

Return a Result from id() and propagate the error.

// Return an empty signature if an error occurs.
let sig = private.0.sign(message_to_sign, curve).unwrap_or_default();
CByteArray::from(sig)

A host that does not check the length can treat a failed signature as a success.

Return the existing status-carrying result type, or a null array, and document that callers must check it.

const byte paddingSize = result[result.size() - 1];
if (paddingSize <= result.size()) {
    const size_t unpaddedSize = result.size() - paddingSize;

A final padding byte of 0, or any value above 16, is accepted. The function then silently returns the wrong plaintext (untrimmed, or truncated by up to 255 bytes) instead of failing. The padding bytes themselves are never checked. Callers of TWAESDecryptCBC cannot tell corrupt or wrong-key ciphertext from good data.

Require 1 <= paddingSize <= 16 and check that every padding byte equals paddingSize, otherwise throw. Prefer an authenticated mode (AES-GCM, or encrypt-then-MAC) for new uses.

} catch (...) {
    return nullptr;
}

At the C boundary, a wrong password, a corrupt file, an unsupported cipher and an unsupported KDF all become nullptr. The Swift wrapper turns every nil into Error.invalidPassword. A user with a damaged or unsupported keystore is told to retype a password that is in fact correct, and may give up on the funds.

Expose the failure cause, as an error code or a result type carrying the existing DecryptionError values. Map each cause to a distinct, understandable error in the Swift, Kotlin and TypeScript wrappers.

if (json.count(CodingKeys::iterations) != 0) {
    ...
    iterations = json[CodingKeys::iterations];

A keystore with a missing or misspelt c field is decrypted with 262144 iterations. It then fails as "invalid password" instead of being reported as malformed. Similar silent defaults exist for a missing scrypt salt and for a missing coin (the Ethereum fallback).

Treat a missing iteration count as a malformed file, or as an explicitly flagged legacy case.

android.enableJetifier=true

Jetifier slows every build. It is only needed because of the pre-AndroidX test dependency android.arch.core:core-testing:1.1.1 (android/app/build.gradle:48).

Switch to androidx.arch.core:core-testing and set android.enableJetifier=false.

ext.kotlin_version = '1.3.50'
classpath 'com.android.tools.build:gradle:7.2.1'

Kotlin Gradle plugin 1.3.50 predates Gradle 7, which the sample uses (7.3.3). Toolchain mismatches at that boundary show up as confusing build errors for anyone copying the sample.

Move the sample to the same AGP, Gradle and Kotlin set as android/ (AGP 8.8.0, Gradle 8.10.2, Kotlin 2.1.0).

derivedKey.resize(scryptParams->defaultDesiredKeyLength);
scrypt(..., derivedKey.data(), scryptParams->defaultDesiredKeyLength);

The file's dklen is parsed, written back, and used when encrypting, but decryption always uses the default 32. The MAC key comes from the end of the derived key, so any keystore with dklen other than 32 is rejected as "invalid password" even with the right password.

Use the parsed dklen in decrypt(), and fall back to the default only when the field is absent. Validate that dklen is at least the cipher key length.

if (json.count(CodingKeys::SK::type) != 0 &&
    json[CodingKeys::SK::type].get<std::string>() == TypeString::mnemonic) {
    type = StoredKeyType::mnemonicPhrase;
} else {
    type = StoredKeyType::privateKey;

The encrypted payload holds either a mnemonic or a raw key, and only type says which. Any unknown or misspelt value is coerced to "private key".

Accept exactly mnemonic and private-key, treat a missing value as legacy, and reject anything else.

const auto data = key->impl.payload.decrypt(passwordData);
return TWDataCreateWithBytes(data.data(), data.size());

TWStoredKeyDecryptPrivateKey on a mnemonic wallet returns the mnemonic's ASCII bytes as if they were a key. TWStoredKeyDecryptMnemonic on a private-key wallet returns raw bytes as a string. The Swift layer currently uses the first as a generic password check, so this is a contract ambiguity rather than a demonstrated bug.

Add kind-checked variants (or document that these functions are kind-agnostic), and keep a separate, explicit password-verification call.

auto data = payload.decrypt(password);
const auto dataHex = TW::hex(data);

On a mnemonic wallet without an encoded payload, this returns the hex of the mnemonic phrase as an "encoded private key".

Throw unless the wallet is a private-key wallet.

hdnode_private_ckd(&node, path.change());
hdnode_private_ckd(&node, path.address());
return PrivateKey(Data(node.private_key, node.private_key + 32), curve);

deserialize() accepts public versions and leaves private_key zeroed. getPrivateKeyFromExtended then derives from a zero parent, ignores the derivation's return value, and returns a "private key". Anyone holding the xpub can compute it. Funds sent to its address are not safe.

Fail unless the version is a private version, and check every hdnode_private_ckd return value.

if (getAccount(coin, address).has_value()) {
    // address already present
    return;

The "already present" check uses only coin and address. Accounts loaded from legacy files (empty public key and xpub, default path) are never enriched by a later, more complete addAccount.

Update missing or different fields on the existing account, or report that it already exists.

Account accountLval = account.value();
return fillAddressIfMissing(accountLval, wallet);

The stored account keeps the empty values, so they are re-derived on every call, and the in-memory and persisted states diverge.

Write the filled values back to the stored account through one setter.

for (auto& accountJSON : json[CodingKeys::SK::activeAccounts]) {
    accounts.emplace_back(accountJSON);
}

addAccount keeps (coin, address) unique. The public loadJson appends without de-duplicating and without clearing existing accounts.

Route loading through the same guard and check the invariants after loading.

uint32_t len = getCompoundLength(countMultiplier);
...
if (!nextElem.isValid()) { return false; }
idx += nextElem.getTotalLen();

At every level the subtree is walked again, and WebAuthn.cpp calls dumpToString() on every key of an attacker-supplied attestation object. Moderately sized inputs cost far more CPU than their size suggests.

Validate in one pass that returns the consumed length under a shared step budget. Compare map keys by type and value instead of rendering them, and cap the number of entries.

uint32_t count = typeDesc.isIndefiniteValue ? 0 : (uint32_t)(typeDesc.value * countMultiplier);

For a declared count of 2^32, the length walker sees an empty array while the validator expects 2^32 elements. The two parts of the decoder disagree on what the structure contains.

Reject any declared count larger than the remaining bytes, and compute the count in one place.

if (string.empty()) { return {}; }
...
Rust::CByteArrayResultWrapper res = Rust::decode_base58(string.c_str(), alphabet);

Address validation decodes the whole string before checking its size, and base58 decoding is quadratic. A pasted or deep-linked multi-megabyte "address" blocks the calling thread for a long time.

Reject strings longer than the largest legitimate payload (for example 128 characters, or about 512 for Cardano legacy addresses) before decoding.

jsonTx["fee"] = fee;
jsonTx["amount"] = amount;

Values above 2^53 are possible and lose precision when a JavaScript host round-trips the JSON. The node then rejects the signature.

If the node API requires numbers, document that a BigInt-safe parser is needed. Otherwise also expose the amounts as decimal strings.

curl -fSsOL https://github.com/google/googletest/releases/download/v$GTEST_VERSION/googletest-$GTEST_VERSION.tar.gz

googletest, libcheck, nlohmann/json and protobuf are fetched over HTTPS but never hash-checked. The cache check also trusts any file already on disk.

Store a SHA-256 next to each version in tools/dependencies-version and verify it before unpacking, including for cached files.

curl -sSfL "https://binaries.sonarsource.com/Distribution/sonar-scanner-cli/${TARGET}" --output "${TARGET}"
unzip -q "${TARGET}"

The scanner runs with SONAR_TOKEN and GITHUB_TOKEN in the environment.

Pin and verify the archive's SHA-256, as the same workflow already does for another jar.

git clone https://github.com/emscripten-core/emsdk.git

Check out a fixed emsdk commit and verify the boost zip's SHA-256.

s.source = {
    http: '${download_url}'
  }

Add sha256: with the tarball's hash, as the Swift package release script already does.

RUN curl -sSL --proto '=https' --proto-redir '=https' \
	https://download.swift.org/swift-5.8-release/ubuntu2204/swift-5.8-RELEASE/swift-5.8-RELEASE-ubuntu22.04.tar.gz \

Verify a pinned checksum, as the same Dockerfile already does for rustup-init. Replace the deprecated apt-key add.

./gradlew :wallet-core-kotlin:publishAllPublicationsToGitHubPackagesRepository -Pversion="$version" || echo "Warning: GitHub Packages publishing failed"
...
echo "Kotlin build uploaded"

A failed publish is turned into a warning, the script exits 0 and prints that the build was uploaded. A release can ship with no artifact and nobody notices.

Let the publish failure stop the script, then confirm that the artifact exists at the registry.

fs::create_dir(coin_tests_dir)?;
if tw_coin_type_tests_path.exists() {
    println!("[SKIP] {tw_coin_type_tests_path:?} already exists");

create_dir fails when the directory exists, so the skip branch never runs. Scaffolding aborts part-way, after Coin.cpp and the enums have already been edited.

Use create_dir_all, as the sibling generators do.

EntryGenerator::generate(coin)?;
TWCoinTypeGenerator::generate_coin_type_variant(coin)?;
BlockchainDispatcherGenerator::generate_new_blockchain_type_dispatching(coin)?;

The first step skips an existing chain, but the next steps insert enum values, includes, entries and case labels unconditionally. Re-running for an existing chain breaks the build with duplicates. The Rust workspace member insertion has the same problem.

Check whether each line is already present before inserting it, as the older Ruby editor does.

FileUtils.mkdir_p folder
path = File.join(folder, fileName)
File.write(path, result)

Running the generator for an existing coin (its own usage text suggests ethereum) replaces hand-written Kotlin and Swift tests with skeletons. Uncommitted work is lost.

Skip or abort when the file exists unless an explicit overwrite flag is given, as coin_test_gen.rb does.

let file_to_write = fs::File::create(write_to_path)?;

Safety depends on each of roughly 20 callers checking first, and several check only the parent directory.

Open with create_new(true) and make overwriting an explicit option.

key: ${{ runner.os }}-${{ runner.arch }}-internal-${{ hashFiles('tools/install-dependencies') }}-${{ hashFiles('tools/dependencies-version') }}

Three workflows include the system-dependency script in the cache key and five do not. A toolchain change can therefore leave stale caches in five pipelines.

Move the cache, install and codegen steps into one composite action or reusable workflow.

echo "Publishing to Maven Local..."

When fixing finding 34, move the shared publish steps into tools/library so the fix is applied once.

TWString(CString::new(s).expect("CString::new(String) should never fail"))

The function-signature helper builds its output from a name parsed out of caller-supplied ABI JSON. A name containing \u0000 makes CString::new fail. Because the workspace builds with panic = "abort", the whole app terminates instead of receiving null.

Make the conversion fallible (return null, or strip or reject interior NULs), and reject NULs in ABI names.

const auto resultSize = data.size() + padding;
for (idx = 0; idx < resultSize - blockSize; idx += blockSize) {
    aes_cbc_encrypt(data.data() + idx, result.data() + idx, blockSize, iv.data(), &ctx);

With empty data and zero padding, resultSize is 0 and resultSize - blockSize underflows to a huge value. The loop then reads and writes past empty buffers, causing a crash or memory corruption. TWAESEncryptCBC passes caller input straight through.

Return early for empty input, and write the bound without unsigned subtraction (idx + blockSize < resultSize).

auto stream = std::ofstream(path);
...
stream << jsonData;

The public store() truncates the existing wallet file before writing. A crash, kill or full disk mid-write leaves an empty or truncated keystore, which may be the only copy of the encrypted seed.

Always write to a temporary file in the same directory, fsync it, then rename it over the target. storeWithTemporaryFile already renames but does not fsync.

cachedJVM->AttachCurrentThread(&env, nullptr);
...
env->CallVoidMethod(random, nextBytes, array);

None of the JNI calls is checked, and no exception check follows nextBytes. If any step fails, the zero-initialised array is copied out as "random" bytes for keys, salts or IVs. Threads attached here are never detached.

Check each result and ExceptionCheck(), and call std::terminate() on failure, as the /dev/urandom branch already does. Detach threads attached here.

getFilename(id): string {
  return this.directory + id + ".json";
}

importWallet() stores a wallet under wallet.id taken from the imported JSON. An id such as ../../x reads, writes or deletes .json files outside the keystore directory.

Validate ids against a strict pattern (UUID or [A-Za-z0-9._-]+), or resolve the path and confirm that it stays inside the directory.

return this.getWalletIds().then((ids) => {
  if (ids.indexOf(id) === -1) { ids.push(id); }
  return this.storage.set({ [id]: wallet, [this.walletIdsKey]: ids });

Two overlapping set() or delete() calls each read the old id list, and the last writer wins. A stored wallet can disappear from loadAll().

Serialise storage mutations through a promise queue, or derive the list from the stored keys.

hdnode_public_ckd(&node, path.change());
hdnode_public_ckd(&node, path.address());
hdnode_fill_public_key(&node);

On failure, for example with a hardened index or a private extended key whose public key is empty, the node is left unchanged. The account-level key is then returned as the derived child. The caller shows or uses the wrong receive address without any error.

Return std::nullopt when either derivation call returns 0.

// scan-and-flag posts the reminder → that triggers an issue_comment event
// → this job runs, finds the reminder, and fails until a sign-off exists.
if (!hasReminder) { core.info('No BC-risk reminder posted; nothing to verify.');

The reminder is posted with the default workflow token, and comments made with that token do not trigger new workflow runs. The cascade the gate relies on never happens. On PR open the verify job runs alongside the scan, finds no reminder, and reports success. A PR touching keystore, proto or registry paths can merge without sign-off if it is not pushed again.

Make the verify job depend on the scan job (needs:), or compute the flagged paths inside it, and fail whenever flags exist without a sign-off.

const signoff = comments.find(c => {
  const tokenMatch = c.body.match(tokenRegex);
  if (!tokenMatch) return false;

Any account that can comment, including the PR author, can satisfy the gate with the token and 60 characters of text.

Require author_association of OWNER, MEMBER or COLLABORATOR, preferably not the PR author, or use CODEOWNERS review on these paths.

pushd codegen
codegen/bin/newcoin-mobile-tests $1
popd # codegen

After pushd codegen the path resolves to codegen/codegen/bin/…, which does not exist (searched; only codegen/bin/newcoin-mobile-tests exists). The mobile test skeletons are never generated. Because the script has no set -e or argument check, the failure is easy to miss.

Call bin/newcoin-mobile-tests "$1", add set -euo pipefail, and require an argument.

delete(id: string, password: string): Promise<void> {
  return fs.unlink(this.getFilename(id));
}

The API implies an authentication check, as the Swift keystore performs, but any caller can delete a wallet.

Decrypt the stored key with the password before deleting, or remove the parameter so the contract is honest.