Harden input validation across Bitcoin UTXO, Aptos, Sui, EOS and Icon signers

[nikhil-gupta-tw]

This pull request introduces several input validation improvements and error handling enhancements across multiple blockchain transaction builders and signers. The main focus is on ensuring that invalid or out-of-range parameters are caught early, preventing the construction or signing of malformed transactions.

Input validation and error handling:

• Added range validation for chain_id in TransactionFactory::new_from_protobuf, returning an error if the value is outside the u8 range.
• Added a check in the Sui ProgrammableTransactionBuilder to ensure the number of recipients/amounts does not exceed the u16 argument index limit, returning an error if exceeded.
• In Bitcoin transaction signing (TransactionSigner), added validation to ensure amount, change, and fee in a transaction plan are non-negative, returning an error if invalid. [1] [2]
• In EOS Signer::buildTx, added a check to ensure asset decimals do not exceed 18, throwing an exception if the value is out of range.
• Improved input parsing in the generic signTemplate function to handle parsing failures gracefully by setting an appropriate error in the output.

Other improvements:

• Removed the noexcept qualifier from the Icon::Signer::encode method declaration and definition, aligning its signature and allowing exceptions to propagate if needed. [1] [2]
• Added a code comment clarifying the safety of casting indices to u16 in the Sui transaction builder, given the new validation.

[octane-security-app]

Summary by Octane

New Contracts

No new contracts were added.

Updated Contracts

• transaction_builder.rs: The key change is improved error handling for 'chain_id' conversion with a new validation and specific error context.
• programmable_transaction.rs: The update adds a check for a recipient/amount limit, returning an error if it exceeds u16::MAX.
• TransactionSigner.cpp: The smart contract now includes transaction plan validation to ensure non-negative values and correct UTXO sum balance.
• Signer.cpp: Implemented a validation to ensure EOS asset decimals do not exceed 18.
• Signer.cpp: Removed noexcept from the encode function, possibly allowing exceptions.

---

🔗 Commit Hash: fc4489d

[github-actions]

Binary size comparison

➡️ aarch64-apple-ios:

- 14.31 MB
+ 14.31 MB 	 +4 KB

➡️ aarch64-apple-ios-sim:

- 14.32 MB
+ 14.32 MB 	 +3 KB

➡️ aarch64-linux-android:

- 18.73 MB
+ 18.73 MB 	 +7 KB

➡️ armv7-linux-androideabi:

- 16.17 MB
+ 16.17 MB 	 +5 KB

➡️ wasm32-unknown-emscripten:

- 13.66 MB
+ 13.66 MB 	 +3 KB

[octane-security-app]

Overview

Vulnerabilities found:	5
Severity breakdown:	2 Medium, 2 Low, 1 Informational

Detailed findings

src/Bitcoin/TransactionSigner.cpp

• Incorrect plan validation in Bitcoin TransactionSigner causes DoS and segwit fee-estimation degradation. See more
• Missing revalidation of imported plan.utxos in Bitcoin signing path causes fee drain and potential DoS. See more
• Unchecked signed integer arithmetic in Bitcoin UTXO validatePlan causes DoS/correctness degradation on imported-plan signing/preimage flows. See more

src/EOS/Signer.h

• Exception introduced in EOS::Signer::buildTx crossing noexcept helpers causes process termination (DoS) in preimage/compile paths. See more

src/Zcash/TransactionBuilder.h

• Unchecked variable-length branch_id copied into 4-byte buffer in Zcash TransactionBuilder causes memory corruption or invalid signatures. See more

---

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

[sergei-boiko-trustwallet]

LGTM!

[BSCSecChef]

1. Check on the value 30 here -

   wallet-core/rust/chains/tw_aptos/src/transaction_builder.rs

   Line 82 in c908296

   transaction_expiration_time: 30,

   - probably just add in comment so callers can handle the expiration at their end.

2. Check on this too -

   wallet-core/rust/chains/tw_sui/src/transaction/programmable_transaction.rs

   Line 92 in c908296

   /// Will fail to generate if given an empty ObjVec

The comment says - "Will fail to generate if given an empty ObjVec"

but the function takes Vec

3. PR says this - "and that the sum of UTXOs matches the total output" where is this one present? Don't seem to see it in TransactionSigner.cpp - Do update the description.

4. Also, I would suggest to add in ? with unwrap calls -

   wallet-core/rust/chains/tw_sui/src/transaction/programmable_transaction.rs

   Line 76 in c908296

   let rec_arg = self.pure(recipient).unwrap();

Although, this is not going to trigger at all but still avoiding panic on FFI boundaries

5. Change is to add in throw in buildTx, but here the function uses noexcept -

   wallet-core/src/EOS/Signer.cpp

   Line 129 in c908296

   std::string Signer::buildSignedTx(const Proto::SigningInput& input, const Data& signature) noexcept {

but internally calls buildTx

same here too -

wallet-core/src/EOS/Signer.cpp

Line 124 in c908296

Data Signer::buildUnsignedTx(const Proto::SigningInput& input) noexcept {

6. Also, probably add in a comment that caller needs to validate this -

   wallet-core/src/EOS/Signer.cpp

   Line 115 in c908296

   Transaction(Data(input.reference_block_id().begin(), input.reference_block_id().end()),

reference_block_id and reference_block_time

7. Do check here, if validation is required for address, value, network_id etc -

   wallet-core/src/Icon/Signer.cpp

   Line 79 in c908296

   Proto::SigningOutput Signer::sign() const {