Skip to content

Harden input validation in NEAR serialization and Zcash signing - #4799

Open
nikhil-gupta-tw wants to merge 7 commits into
masterfrom
fix/sec-issues-5
Open

Harden input validation in NEAR serialization and Zcash signing#4799
nikhil-gupta-tw wants to merge 7 commits into
masterfrom
fix/sec-issues-5

Conversation

@nikhil-gupta-tw

Copy link
Copy Markdown
Contributor

This pull request introduces a validation step for Zcash PCZT outputs to ensure that, if a user_address is provided, it matches the expected script_pubkey. It also adds comprehensive unit and integration tests for this behavior, and improves error handling in NEAR serialization. Additionally, it updates dependencies to include the Zcash crate for testing.

Zcash PCZT output validation and testing:

  • Added a check in OutputPczt::build to verify that if user_address is present, it matches the script_pubkey by converting the address to a script and comparing it. This prevents mismatches and potential security issues.
  • Introduced unit tests in output_pczt.rs to cover cases where the address matches, mismatches, or is invalid, ensuring robust validation logic.
  • Added integration tests in zcash_sign.rs to verify end-to-end signing behavior for PCZT transactions with matching and mismatched user_address fields.

Dependency and test infrastructure updates:

  • Added tw_zcash as a dependency in tw_tests/Cargo.toml to enable Zcash-related tests.
  • Updated imports in the Zcash test suite to support new PCZT features and address encoding/decoding.

Error handling improvement in NEAR serialization:

  • Replaced an assert with an exception in writeU128 to throw a clear error if the input is not exactly 16 bytes, improving robustness and error reporting. [1] [2]

…action confusion via undersized deposit bytes in release builds
@nikhil-gupta-tw
nikhil-gupta-tw requested review from a team and BSCSecChef as code owners June 10, 2026 12:01
@octane-security-app

Copy link
Copy Markdown

Summary by Octane

New Contracts

No new contracts were added.

Updated Contracts

  • output_pczt.rs: Added Zcash address validation, script matching, error handling, and unit tests for user addresses in OutputPczt.
  • Serialization.cpp: Enhanced error handling for a U128 number validation by replacing an assert with a thrown exception for incorrect input length.

🔗 Commit Hash: 16e3584

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown

Binary size comparison

➡️ aarch64-apple-ios:

- 14.31 MB
+ 14.31 MB 	 +5 KB

➡️ aarch64-apple-ios-sim:

- 14.31 MB
+ 14.32 MB 	 +5 KB

➡️ aarch64-linux-android:

- 18.73 MB
+ 18.74 MB 	 +8 KB

➡️ armv7-linux-androideabi:

- 16.17 MB
+ 16.18 MB 	 +6 KB

➡️ wasm32-unknown-emscripten:

- 13.66 MB
+ 13.67 MB 	 +6 KB

@sergei-boiko-trustwallet sergei-boiko-trustwallet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One change please

Comment thread rust/chains/tw_zcash/src/modules/pczt_request/output_pczt.rs Outdated

@sergei-boiko-trustwallet sergei-boiko-trustwallet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Thanks for the fix

@BSCSecChef BSCSecChef left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  1. This is never used in the function, check once -

    _public_keys: Vec<PublicKeyBytes>,

  2. Could you help to check on UI end too once. So send_amount is never used on UI end, it iterates on the output list and show each destination

  3. Probably add in validation check (length) on these -

    const auto& block_hash = input.block_hash();

…ix/sec-issues-5

# Conflicts:
#	src/NEAR/Serialization.cpp
@sonarqubecloud

Copy link
Copy Markdown

@nikhil-gupta-tw

Copy link
Copy Markdown
Contributor Author
  1. This is never used in the function, check once -
    _public_keys: Vec<PublicKeyBytes>,

Yes, it's the shared compile interface param, intentionally unused for Bitcoin.

  1. Could you help to check on UI end too once. So send_amount is never used on UI end, it iterates on the output list and show each destination

I have checked and UI doesn't use send_amount. Tagging @sergei-boiko-trustwallet as well.

  1. Probably add in validation check (length) on these -
    const auto& block_hash = input.block_hash();

Already done in #4812 .

@BSCSecChef BSCSecChef left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

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.

3 participants