Skip to content

fix(overflow): Fix integer overflows in BinaryEncoding and Pactus#4592

Merged
satoshiotomakan merged 2 commits intomasterfrom
fix/binary-coding
Dec 16, 2025
Merged

fix(overflow): Fix integer overflows in BinaryEncoding and Pactus#4592
satoshiotomakan merged 2 commits intomasterfrom
fix/binary-coding

Conversation

@satoshiotomakan
Copy link
Copy Markdown
Contributor

This pull request addresses error handling and input validation in serialization and deserialization logic for both Rust and C++ components. The main focus is on preventing invalid or unsafe operations when dealing with potentially problematic input data.

Rust amount encoding improvements:

  • Improved error handling in the Amount struct's encode method by explicitly checking for conversion errors when casting from i64 to usize, and returning an appropriate error if the value is negative or out of range.

C++ string decoding validation:

  • Enhanced bounds checking in the decodeString function to prevent buffer overflows by ensuring that the input data is large enough before attempting to read the requested number of bytes.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request addresses integer overflow vulnerabilities in serialization/deserialization code for both C++ and Rust components. The changes focus on preventing unsafe operations when handling potentially malicious or invalid input data.

  • Adds overflow protection in C++ decodeString function to prevent buffer overflow when indexInOut + len would overflow
  • Implements error handling in Rust Amount::encode to reject negative values during i64 to usize conversion

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/BinaryCoding.cpp Adds overflow check to prevent integer overflow when calculating buffer bounds in decodeString
rust/chains/tw_pactus/src/types/amount.rs Adds explicit error handling for negative amounts in encode method using try_into()
Comments suppressed due to low confidence (1)

rust/chains/tw_pactus/src/types/amount.rs:21

  • The encoded_size method still uses an unsafe cast from i64 to usize on line 20, which can cause integer overflow for negative values. This is inconsistent with the overflow protection added to the encode method. Consider using try_into() here as well and handling negative values appropriately. Since encoded_size returns usize, you could either return 0 for negative values, or use saturating_abs() to get the absolute value before casting.
    fn encoded_size(&self) -> usize {
        VarInt::from(self.0 as usize).encoded_size()
    }

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

@github-actions
Copy link
Copy Markdown

Binary size comparison

➡️ aarch64-apple-ios: 14.31 MB

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

➡️ aarch64-linux-android: 18.87 MB

➡️ armv7-linux-androideabi: 15.80 MB

➡️ wasm32-unknown-emscripten: 13.46 MB

@satoshiotomakan satoshiotomakan merged commit 7e75dd8 into master Dec 16, 2025
21 checks passed
@satoshiotomakan satoshiotomakan deleted the fix/binary-coding branch December 16, 2025 15:16
sergei-boiko-trustwallet added a commit that referenced this pull request Jan 14, 2026
* fiux(ci): Use `iPhone 16` in ios-release (#4493)

* Throw exception rather than SIGSEGV (#4443)

* Throw exception rather than SIGSEGV

* Fix test cases

* Add NULL checker for exceptionClass

* feat(Plasma): Add Plasma Mainnet (#4499)

* feat(plasma): Add Plasma Mainnet

* feat(plasma): Add mobile tests

* [Solana]: Adds ability to transfer tokens to the feepayer (#4503)

* Initial setup

* Minor updates

* Restructure

* fix(barz): Replace u32 parameters with i32 (#4504)

* Disallow to use unsigned integer parameters within class methods

* fix(data): Allocate empty array on `tw_data_create_with_bytes` if data is NULL (#4508)

* fix(data): Allocate empty array on `tw_data_create_with_bytes` if data is NULL

* fix(data): Add TWData test

* feat(Biz): Add helper functions to support `BizPasskeySessionAccount` (#4516)

* feat(biz-passkey): Add `TWBarzEncodeRegisterSessionCall` FFI

* feat(biz-passkey): Add `TWBarzEncodeRemoveSessionCall` FFI

* feat(biz-passkey): Add `TWBarzEncodePasskeySessionNonce` FFI

* feat(biz-passkey): Add `TWBarzEncodeExecuteWithPasskeySessionCall` FFI

* refactor(barz): Move some functions to TWBiz and TWEip7702 modules

* feat(biz): Add WebAuthn

* feat(biz): Add `TWWebAuthnGetMessageHash` and `TWWebAuthnGetFormattedSignature`

* feat(biz): Fix C++ and Mobile

* Rename `TWWebAuthn` module to `TWWebAuthnSolidity`

* feat(biz): Adjust `executeWithPasskeySession` arguments

* feat(biz): Fix fmt

* feat(biz): Add Biz Android tests

* feat(biz): Add final Biz Android test

* feat(biz): Fix lints

* feat(biz): Minor change

* feat(biz): fmt

* feat(solana): Add `SolanaTransaction.insertTransferInstruction()` (#4523)

* feat(solana): Add `transfer_to_fee_payer` instruction

* feat(solana): Add `tw_solana_transaction_insert_transfer_instruction` FFI

* feat(solana): Add a note comment

* feat(solana): Add one more comment

* feat(zcash): Add support for TEX address (#4527)

* feat(zcash): Add `TexAddress` in Rust

* feat(zcash): Add successfully broadcasted transaction test

* feat(zcash): Add successfully broadcasted transaction C++ test

* feat(zcash): Fix C++ tests

* feat(zcash): Reuse code in `TexAddress::isValid`

* feat(zcash): Fix `TexAddress:isValid`

* [Solana]: Renames fee recipient token account for clarity (#4528)

* [Solana]: Renames fee recipient token account for clarity

* Fix typo

* Feature: Add trade and secured fields to THORChain Asset (#4500)

* my tracked commit

* MsgInstantiateContract

* revert formatting

* test

* Tests fixes and broadcast

* cargo fmt

* remove unused

* feat: add trade and secured fields to Thorchain Asset

- Add trade and secured boolean fields to Asset proto message
- Update ThorchainAsset struct in Rust with new fields
- Update asset initialization in tx_builder.rs
- Matches Thorchain blockchain Asset struct definition

---------

Co-authored-by: gupnik <mail.guptanikhil@gmail.com>

* feat(kusama-asset-hub): Add ability to force to use `ChargeAssetTxPayment` (#4541)

* feat(kusama-asset-hub): Add ability to force to use `ChargeAssetTxPayment` for native tip

* feat(kusama-asset-hub): Add a unit test

* feat(monad): Add Monad testnet (#4543)

* feat(monad): Update Monad to mainnet (#4550)

* TODO update explorer

* feat(monad): Update block explorer (#4553)

* feat(monad): Update explorer

* feat(monad): Update explorer schema

* Fix incorrect value of NO_PAD constant (#4487)

Co-authored-by: Sergei Boiko <127754187+satoshiotomakan@users.noreply.github.com>

* fix(public-key): Fix `PublicKey::verify` by adding signature length validation (#4565)

* fix(public-key): Fix `PublicKey::verify` by adding signature length validation

* fix(public-key): Fix PR comments

* Fix Reveal Operations for Seoul Protocol (#4557)

* Since the Seoul protocol, reveal operations require the presence of a boolean for whether or not the proof field is present. This is required when manually forging bytes. Added.

* Fix TezosTests.swift

* Fix TezosTests.swift

* Update tests

* Update tests

* Fix TezosCompiler.CompileWithSignatures

* Fix TezosCompiler.CompileWithSignatures

* fix(tezos): Fix Tezos compile test

* fix(tezos): Fix Tezos signing test

* fix(tezos): Add mainnet test transaction

---------

Co-authored-by: gupnik <nikhil.g@trustwallet.com>
Co-authored-by: Sergei Boiko <satoshiotomakan8@gmail.com>

* fix(public-key): Check zilliqa schnorr signature size (#4571)

* fix(public-key): Check zilliqa schnorr signature size

* Replace assert macros with proper checks

* fix(public-key): Address a PR comment

* fix(public-key): Address a PR comments

* fix(public-key): Fix a bug in PublicKey constructor

* feat(pczt): Add implementation of PCZT signing (#4576)

* feat(pczt): Add implementation of PCZT signing

* feat(pczt): Add a successfully broadcasted tx test

* feat(pczt): Address PR comments

* fix(webauthn): Fix buffer out of bounds (#4577)

* fix(webauthn): Fix buffer out of bounds

* fix(webauthn): Fix `TWWebAuthnGetRSValues` implicit Data conversion

* fix(webauthn): Extra checks for algorithm, and public key parts

* fix(webauthn): Test common buffer overflows

* fix(webauthn): Check `crv` and `kty` parameters

* fix(private-key): Add PrivateKey Data move constructor

* chore(noexcept): Remove wrong noexcept

* fix(private-key): Zeroize memory on move `operator=`

* fix(private-key): Avoid cleaning private key if self-assign

* fix(aes): Add iv size validation (#4580)

* fix(aes): Add iv size validation

* fix(aes): Add unit tests

* Update Dockerfile to install cbindgen with --locked and provide explicit links to clang binaries (#4568)

* install cbindgen with --locked

* add explicit links to clang/clang++ since CMake will not use the ENV

---------

Co-authored-by: Sergei Boiko <127754187+satoshiotomakan@users.noreply.github.com>

* feat(stable-account): Add `TWDerivationSmartChainStableAccount` derivation type (#4585)

* fix(overflow): Fix integer overflows in `BinaryEncoding` and `Pactus` (#4592)

* fix(binary-coding): Avoid unsigned integer overflow

* fix(pactus): Avoid unsigned integer overflow

* feat(tron): Add `raw_data_hex` parameter in JSON transaction representation (#4593)

* feat(tron): Add `raw_data_hex` parameter in JSON transaction representation

* feat(tron): Fix iOS test

* feat(biz): Add `Biz.signExecuteWithSignatureCall` and `BizPasskeySession.signExecuteWithSignatureCall` (#4594)

* feat(biz): Add `Biz.SignExecuteWithSignatureCall`

* Add new `BizPasskeySession` module

* feat(biz): Add `Biz.SignExecuteWithSignatureCall`

* Finish `BizPasskeySession` module separation

* feat(biz): Fix android tests

* feat(biz): Address PR review comments

* chore(codeowners): Add onchain-corex-enabling team to CODEOWNERS (#4600)

* fix(ethereum-abi): Fix Ethereum ABI type parsing recursion + chore actions and toolchain (#4597)

* fix(ethereum-abi): Fix Ethereum ABI type parsing recursion

* fix(ethereum-abi): Freeze community github actions

* chore(rust): Bump rust toolchain version to nightly-2025-12-11

* chore(wasm): Try to update emsdk to 4.0.22

* chore(rust): Decrease code coverage to 94.1 due to a recent toolchain update

* chore(rust): Freeze `actions` hashes

---------

Co-authored-by: Sergei Boiko <satoshiotomakan8@gmail.com>
Co-authored-by: Sergei <>

* chore(actions): Replace unverified actions with GH scripts (#4604)

* chore(actions): Replace unverified actions with GH scripts

* chore(actions): Update rust toolchain in Dockerfile

* fix(bitcoin-v2): Fail if `max_amount_output` provided with `outputs` or `change_output` at a time (#4607)

* fix(bitcoin-v2): Fail if `max_amount_output` provided with `outputs` or `change_output` at a time

* fix(bitcoin-v2): PR notes

* fix(cbor): Fix Cbor Map with odd elements number decoding (#4610)

* fix(cbor): Fix Cbor Map with odd elements number decoding

* fix(cbor): Change test name

* chore(sync): Fix merge conflicts

Breaking changes:
* The following FFIs take i32 instead of u32 due to Kotlin bindings limitations: `tw_hd_node_create_with_seed`, `tw_hd_node_create_with_extended_private_key`, `tw_hd_node_derive_from_path`, `tw_hd_node_extended_private_key`, `tw_hd_node_extended_public_key`, `tw_hd_node_public_create_with_extended_public_key`, `tw_hd_node_public_derive_from_path`, `tw_mnemonic_generate`, `tw_mnemonic_get_word`, `tw_pbkdf2_hmac_sha512`, `tw_ecdsa_pubkey_hash`
* `crypto_scrypt` FFI is no longer marked as `tw_ffi` to avoid changing u32 and usize argument types to i32

* chore(sync): Increase test coverage

* Add Bitcoin PSBT compile test
* Add `PrivateKey.CopyAssignmentOperator` and `PrivateKey.MoveAssignmentOperator` tests
* Copy Rust TWMnemonic tests to C++
* Copy Rust Biz tests to C++
* Copy Rust BizPasskeySession tests to C++

* chore(sync): Push missing PSBT implementation + remove unused code

* chore(sync): Fix Biz android tests

---------

Co-authored-by: Sergei Boiko <127754187+satoshiotomakan@users.noreply.github.com>
Co-authored-by: 10gic <github10gic@proton.me>
Co-authored-by: gupnik <nikhil.g@trustwallet.com>
Co-authored-by: Enrique Souza <enriquesouza@live.com>
Co-authored-by: gupnik <mail.guptanikhil@gmail.com>
Co-authored-by: Corey Soreff <csoreff@gmail.com>
Co-authored-by: Sergei Boiko <satoshiotomakan8@gmail.com>
Co-authored-by: Joey Yandle <xoloki@gmail.com>
perpetua-engineering pushed a commit to perpetua-engineering/wallet-core that referenced this pull request Jan 20, 2026
…trustwallet#4592)

* fix(binary-coding): Avoid unsigned integer overflow

* fix(pactus): Avoid unsigned integer overflow
perpetua-engineering pushed a commit to perpetua-engineering/wallet-core that referenced this pull request Feb 25, 2026
…trustwallet#4592)

* fix(binary-coding): Avoid unsigned integer overflow

* fix(pactus): Avoid unsigned integer overflow
perpetua-engineering pushed a commit to perpetua-engineering/wallet-core that referenced this pull request Mar 2, 2026
…trustwallet#4592)

* fix(binary-coding): Avoid unsigned integer overflow

* fix(pactus): Avoid unsigned integer overflow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants