From 48c54b9d9418bc5a6bf366eae61e205872c8602e Mon Sep 17 00:00:00 2001 From: Shahan Khatchadourian Date: Wed, 25 Mar 2026 14:22:37 -0400 Subject: [PATCH 1/2] fix: guard against panics on malformed transaction data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two panic sites in the legacy transaction path: 1. instructions.rs: unchecked indexing into account_keys with program_id_index and account indices from compiled instructions. Malformed transactions with out-of-bounds indices cause index-out-of-bounds panics. 2. accounts/decode.rs: arithmetic underflow when message header values (num_readonly_signed_accounts, num_readonly_unsigned_accounts) exceed the actual account keys array length. Fix: apply the same defensive patterns already used in the v0 transaction path — filter_map with bounds checks for instruction indices, saturating_sub for header arithmetic, and an explicit error for empty account keys. Verified with cargo-fuzz: ~930,000 runs across both fuzz targets (fuzz_transaction_string, fuzz_versioned_transaction) with zero crashes. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../src/core/accounts/decode.rs | 34 +++++++++------- .../src/core/instructions.rs | 39 +++++++++++++------ 2 files changed, 48 insertions(+), 25 deletions(-) diff --git a/src/chain_parsers/visualsign-solana/src/core/accounts/decode.rs b/src/chain_parsers/visualsign-solana/src/core/accounts/decode.rs index 2ffeaa54..e7633883 100644 --- a/src/chain_parsers/visualsign-solana/src/core/accounts/decode.rs +++ b/src/chain_parsers/visualsign-solana/src/core/accounts/decode.rs @@ -24,16 +24,19 @@ pub fn decode_accounts(message: &Message) -> Result, Visu let is_signer = i < message.header.num_required_signatures as usize; let is_writable = if i < message.header.num_required_signatures as usize { // For signers: readonly ones come at the end of the signer range - let readonly_signer_start = message.header.num_required_signatures as usize - - message.header.num_readonly_signed_accounts as usize; + let readonly_signer_start = (message.header.num_required_signatures as usize) + .saturating_sub(message.header.num_readonly_signed_accounts as usize); i < readonly_signer_start } else { // For non-signers: readonly ones come at the end of the non-signer range - let non_signer_index = i - message.header.num_required_signatures as usize; - let total_non_signers = - message.account_keys.len() - message.header.num_required_signatures as usize; - let writable_non_signers = - total_non_signers - message.header.num_readonly_unsigned_accounts as usize; + let non_signer_index = + i.saturating_sub(message.header.num_required_signatures as usize); + let total_non_signers = message + .account_keys + .len() + .saturating_sub(message.header.num_required_signatures as usize); + let writable_non_signers = total_non_signers + .saturating_sub(message.header.num_readonly_unsigned_accounts as usize); non_signer_index < writable_non_signers }; @@ -94,16 +97,19 @@ pub fn decode_v0_accounts( let is_signer = i < v0_message.header.num_required_signatures as usize; let is_writable = if i < v0_message.header.num_required_signatures as usize { // For signers: readonly ones come at the end of the signer range - let readonly_signer_start = v0_message.header.num_required_signatures as usize - - v0_message.header.num_readonly_signed_accounts as usize; + let readonly_signer_start = (v0_message.header.num_required_signatures as usize) + .saturating_sub(v0_message.header.num_readonly_signed_accounts as usize); i < readonly_signer_start } else { // For non-signers: readonly ones come at the end of the non-signer range - let non_signer_index = i - v0_message.header.num_required_signatures as usize; - let total_non_signers = v0_message.account_keys.len() - - v0_message.header.num_required_signatures as usize; - let writable_non_signers = - total_non_signers - v0_message.header.num_readonly_unsigned_accounts as usize; + let non_signer_index = + i.saturating_sub(v0_message.header.num_required_signatures as usize); + let total_non_signers = v0_message + .account_keys + .len() + .saturating_sub(v0_message.header.num_required_signatures as usize); + let writable_non_signers = total_non_signers + .saturating_sub(v0_message.header.num_readonly_unsigned_accounts as usize); non_signer_index < writable_non_signers }; diff --git a/src/chain_parsers/visualsign-solana/src/core/instructions.rs b/src/chain_parsers/visualsign-solana/src/core/instructions.rs index 19f0541d..55bd1c1d 100644 --- a/src/chain_parsers/visualsign-solana/src/core/instructions.rs +++ b/src/chain_parsers/visualsign-solana/src/core/instructions.rs @@ -25,23 +25,40 @@ pub fn decode_instructions( let message = &transaction.message; let account_keys = &message.account_keys; - // Convert compiled instructions to full instructions + if account_keys.is_empty() { + return Err(VisualSignError::ParseError( + TransactionParseError::DecodeError("Transaction has no account keys".to_string()), + )); + } + + // Convert compiled instructions to full instructions, skipping any with + // out-of-bounds account indices (same approach as v0 transaction handling). let instructions: Vec = message .instructions .iter() - .map(|ci| Instruction { - program_id: account_keys[ci.program_id_index as usize], - accounts: ci + .filter_map(|ci| { + if (ci.program_id_index as usize) >= account_keys.len() { + return None; + } + let accounts: Vec = ci .accounts .iter() - .map(|&i| { - solana_sdk::instruction::AccountMeta::new_readonly( - account_keys[i as usize], - false, - ) + .filter_map(|&i| { + if (i as usize) < account_keys.len() { + Some(solana_sdk::instruction::AccountMeta::new_readonly( + account_keys[i as usize], + false, + )) + } else { + None + } }) - .collect(), - data: ci.data.clone(), + .collect(); + Some(Instruction { + program_id: account_keys[ci.program_id_index as usize], + accounts, + data: ci.data.clone(), + }) }) .collect(); From e1286e9f196d2d57eac2b0b72a17d2586d8312d0 Mon Sep 17 00:00:00 2001 From: Shahan Khatchadourian Date: Wed, 25 Mar 2026 16:18:59 -0400 Subject: [PATCH 2/2] Address Copilot review: improve comments, error message, add tests - Clarify comment to distinguish skipped instructions (OOB program_id) from omitted accounts (OOB account index) - Use "Legacy transaction" in error message for consistency with v0 path - Add unit tests for decode_accounts and decode_v0_accounts with inconsistent header values to lock in saturating_sub behavior Co-Authored-By: Claude Opus 4.6 (1M context) --- .../src/core/accounts/decode.rs | 41 +++++++++++++++++++ .../src/core/instructions.rs | 9 ++-- 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/src/chain_parsers/visualsign-solana/src/core/accounts/decode.rs b/src/chain_parsers/visualsign-solana/src/core/accounts/decode.rs index e7633883..8bc6918f 100644 --- a/src/chain_parsers/visualsign-solana/src/core/accounts/decode.rs +++ b/src/chain_parsers/visualsign-solana/src/core/accounts/decode.rs @@ -809,4 +809,45 @@ mod tests { _ => panic!("Expected PreviewLayout field"), } } + + /// Malformed legacy message: header counts exceed account keys length. + /// Must not panic (saturating_sub prevents underflow). + #[test] + fn test_decode_accounts_inconsistent_header_no_panic() { + let account1 = Pubkey::new_unique(); + + // num_required_signatures (5) > account_keys.len() (1), + // num_readonly_signed_accounts (3) > num_required_signatures would be too, + // num_readonly_unsigned_accounts (2) > total non-signers (0). + let message = create_test_message(5, 3, 2, vec![account1]); + + // Must not panic — the saturating_sub ensures graceful degradation. + let accounts = decode_accounts(&message).unwrap(); + assert_eq!(accounts.len(), 1); + } + + /// Malformed V0 message: header counts exceed account keys length. + /// Must not panic (saturating_sub prevents underflow). + #[test] + fn test_decode_v0_accounts_inconsistent_header_no_panic() { + use solana_sdk::message::{MessageHeader, v0::Message as V0Message}; + + let account1 = Pubkey::new_unique(); + + let v0_message = V0Message { + header: MessageHeader { + num_required_signatures: 10, + num_readonly_signed_accounts: 8, + num_readonly_unsigned_accounts: 5, + }, + account_keys: vec![account1], + recent_blockhash: Hash::new_unique(), + instructions: vec![], + address_table_lookups: vec![], + }; + + // Must not panic — the saturating_sub ensures graceful degradation. + let accounts = decode_v0_accounts(&v0_message).unwrap(); + assert_eq!(accounts.len(), 1); + } } diff --git a/src/chain_parsers/visualsign-solana/src/core/instructions.rs b/src/chain_parsers/visualsign-solana/src/core/instructions.rs index 55bd1c1d..97849193 100644 --- a/src/chain_parsers/visualsign-solana/src/core/instructions.rs +++ b/src/chain_parsers/visualsign-solana/src/core/instructions.rs @@ -27,12 +27,15 @@ pub fn decode_instructions( if account_keys.is_empty() { return Err(VisualSignError::ParseError( - TransactionParseError::DecodeError("Transaction has no account keys".to_string()), + TransactionParseError::DecodeError( + "Legacy transaction has no account keys".to_string(), + ), )); } - // Convert compiled instructions to full instructions, skipping any with - // out-of-bounds account indices (same approach as v0 transaction handling). + // Convert compiled instructions to full instructions. Instructions with an + // out-of-bounds program_id_index are skipped entirely, while individual + // out-of-bounds account indices are silently omitted (same approach as v0 transaction handling). let instructions: Vec = message .instructions .iter()