security: fix unauthorized signer vulnerability in TransactionValidator

[claw-autonomous]

Problem

Kora Relayer previously failed to correctly identify and restrict the fee_payer as a signer in instructions. Specifically:

1. IxUtils::uncompile_instructions hardcoded is_signer: false, losing critical permission metadata from the transaction message.
2. TransactionValidator only validated program_id for allowed programs, but did not check if the fee_payer was used as an unauthorized signer for those programs.

This allowed an attacker to craft a transaction where the fee_payer (relayer) was used as a signer for an arbitrary allowed program (e.g., a swap or escrow), potentially draining relayer funds.

Solution

1. Fixed uncompile_instructions to correctly restore is_signer and is_writable flags based on the transaction message header.
2. Added a strict check in TransactionValidator::validate_fee_payer_usage that rejects any instruction where the fee_payer is a signer, unless it is a recognized and validated program (System, SPL Token, ATA, Compute Budget).

Verification

A PoC test case was created that successfully bypassed validation before the fix and is now correctly rejected. All 300+ existing tests pass.

---

[greptile-apps]

Greptile Summary

This PR patches two related security vulnerabilities in the Kora relayer: (1) IxUtils::uncompile_instructions previously hardcoded is_signer: false on every account, discarding the transaction message header's permission metadata, and (2) TransactionValidator did not check whether the fee_payer was used as a signer for arbitrary allowed programs, enabling an attacker to drain the relayer by crafting a transaction where the fee_payer signs an instruction for a swap or escrow program. The fix correctly reconstructs is_signer/is_writable from the message header for outer instructions and adds a strict program-whitelist guard in validate_fee_payer_usage.

Key observations:

• The is_signer/is_writable reconstruction in fetch_inner_instructions reuses the outer transaction's message header for simulated CPI (inner) instructions. Since the fee_payer is always at index 0 (< num_required_signatures), it will be marked is_signer=true in every inner instruction that references its account, even when the CPI caller did not pass it as a signer. This can cause validate_fee_payer_usage to reject valid transactions where an allowed-but-not-hardcoded-safe program's inner instruction merely reads/writes the fee_payer's account without actually authorizing it.
• The ComputeBudget program ID in validate_fee_payer_usage is compared via to_string() against a hardcoded string literal, inconsistent with the typed constants used for the adjacent programs; solana_compute_budget_interface::id() is already imported elsewhere in the crate.
• from_kora_built_transaction in versioned_transaction.rs declares resolved as mut but never mutates it, producing a compiler warning.
• validate_fee_payer_usage is ordered after the async validate_transfer_amounts RPC call; moving it earlier would short-circuit expensive network I/O for clearly invalid transactions.
• The updated unit test for uncompile_instructions passes 0, 0, 0 as header parameters but never asserts on is_signer or is_writable, leaving the new permission-restoration logic without meaningful test coverage.

Confidence Score: 3/5

• The core security fix is directionally correct but the inner-instruction is_signer approximation can produce false positives in edge cases.
• The outer-instruction path (the primary attack vector) is correctly patched and the program-whitelist guard in validate_fee_payer_usage addresses the reported vulnerability. However, reusing the outer transaction header to reconstruct is_signer for inner (CPI) instructions is semantically incorrect and can cause the new check to over-reject valid transactions. The hardcoded ComputeBudget string comparison, unused mut, suboptimal validation ordering, and absent unit-test assertions for the new logic collectively lower confidence in the correctness of the implementation.
• crates/lib/src/transaction/versioned_transaction.rs — specifically the fetch_inner_instructions call to uncompile_instructions and the unused mut in from_kora_built_transaction.

Sequence Diagram

sequenceDiagram
    participant Client
    participant RPC as Kora RPC
    participant VTR as VersionedTransactionResolved
    participant IxUtils
    participant Validator as TransactionValidator

    Client->>RPC: Submit transaction
    RPC->>VTR: from_transaction(tx, rpc_client, sig_verify)
    VTR->>VTR: Extract header (num_required_signatures,<br/>num_readonly_signed_accounts,<br/>num_readonly_unsigned_accounts)
    VTR->>IxUtils: uncompile_instructions(outer_ixs, keys, header…)
    Note over IxUtils: is_signer = index < num_required_signatures<br/>is_writable derived from header ranges
    IxUtils-->>VTR: Vec<Instruction> (with correct is_signer/is_writable)
    VTR->>VTR: fetch_inner_instructions (simulation)
    VTR->>IxUtils: uncompile_instructions(inner_ixs, keys, same header…)
    Note over IxUtils: ⚠️ CPI signer/writable flags<br/>approximated from outer header
    IxUtils-->>VTR: Vec<Instruction> (inner, approximated flags)
    VTR->>VTR: all_instructions = outer + inner
    VTR-->>RPC: VersionedTransactionResolved

    RPC->>Validator: validate_transaction(resolved)
    Validator->>Validator: validate_programs() — all programs in allowed list?
    Validator->>Validator: validate_transfer_amounts() — async RPC
    Validator->>Validator: validate_disallowed_accounts()
    Validator->>Validator: validate_fee_payer_usage()
    Note over Validator: For each instruction in all_instructions:<br/>If fee_payer is_signer AND program not in<br/>[System, SPL Token, Token2022, ATA, ComputeBudget]<br/>→ reject
    Validator-->>RPC: Ok or InvalidTransaction error
    RPC-->>Client: Response

Loading

Comments Outside Diff (2)

1. crates/lib/src/validator/transaction_validator.rs, line 114-121 (link)

   validate_fee_payer_usage is a pure, CPU-only check but is placed after validate_transfer_amounts, which makes async RPC calls (account fetches, price lookups). For fail-fast behavior and to avoid unnecessary network I/O on obviously invalid transactions, the inexpensive signer check should run earlier in the sequence.

2. crates/lib/src/transaction/instruction_util.rs, line 2544-2566 (link)

   The existing test was updated to pass 0, 0, 0 as header parameters but asserts only on pubkey and data values — it never checks is_signer or is_writable. The core change in this PR (correctly restoring permission metadata) has no test coverage. Consider adding assertions that verify:

   • Accounts with index < num_required_signatures get is_signer = true
   • Accounts in the readonly-signed range (>= num_required_signatures - num_readonly_signed_accounts) get is_writable = false
   • Accounts in the readonly-unsigned range get is_writable = false
   • The fee_payer (index 0) is correctly identified as a signer when num_required_signatures >= 1

_{Last reviewed commit: 29581b1}

[greptile-apps]

The ComputeBudget program ID is compared using a string literal rather than a typed constant. All other programs in this block use their typed interface constants. solana_compute_budget_interface is already used elsewhere in the codebase (e.g., crates/lib/src/admin/token_util.rs), so its program ID constant is available.

String-based comparison is slower, more brittle (no compile-time verification), and inconsistent with the adjacent lines.

Suggested change

	let is_allowed_program =
	instruction.program_id == solana_system_interface::program::ID ||
	instruction.program_id == spl_token_interface::id() ||
	instruction.program_id == spl_token_2022_interface::id() ||
	instruction.program_id == spl_associated_token_account_interface::program::id() ||
	instruction.program_id.to_string() == "ComputeBudget111111111111111111111111111111";
	let is_allowed_program =
	instruction.program_id == solana_system_interface::program::ID ||
	instruction.program_id == spl_token_interface::id() ||
	instruction.program_id == spl_token_2022_interface::id() ||
	instruction.program_id == spl_associated_token_account_interface::program::id() ||
	instruction.program_id == solana_compute_budget_interface::id();

[greptile-apps]

resolved is declared as mut but never mutated after construction — the struct is built in-place and immediately returned. This will emit a compiler warning.

Suggested change

	let mut resolved = VersionedTransactionResolved {
	transaction: transaction.clone(),
	all_account_keys: transaction.message.static_account_keys().to_vec(),
	all_instructions: IxUtils::uncompile_instructions(
	transaction.message.instructions(),
	transaction.message.static_account_keys(),
	num_required_signatures,
	num_readonly_signed_accounts,
	num_readonly_unsigned_accounts,
	)?,
	parsed_system_instructions: None,
	parsed_spl_instructions: None,
	})
	};
	Ok(resolved)
	let resolved = VersionedTransactionResolved {
	transaction: transaction.clone(),
	all_account_keys: transaction.message.static_account_keys().to_vec(),
	all_instructions: IxUtils::uncompile_instructions(
	transaction.message.instructions(),
	transaction.message.static_account_keys(),
	num_required_signatures,
	num_readonly_signed_accounts,
	num_readonly_unsigned_accounts,
	)?,
	parsed_system_instructions: None,
	parsed_spl_instructions: None,
	};
	Ok(resolved)

[greptile-apps]

uncompile_instructions now derives is_signer and is_writable from the outer transaction's message header. For outer instructions this is correct — the header precisely defines those flags. However, for inner instructions returned by simulation (CPIs), the same header fields do not represent the actual CPI call semantics:

is_signer: In a CPI, the calling program decides which accounts it passes as signers. A PDA signed via invoke_signed has an index ≥ num_required_signatures and will therefore have is_signer = false even though it was effectively a signer. Conversely, the fee_payer (index 0, always < num_required_signatures) will be reconstructed as is_signer = true in every inner instruction that lists it as an account — even if the calling program never passed it as a signer in the CPI. Combined with the new validate_fee_payer_usage check that runs on all_instructions (which includes these inner instructions), this can produce false positives: a legitimate inner instruction from an allowed-but-not-hardcoded-safe program that merely references the fee_payer's account as a read/writable operand (not an authority) will incorrectly trigger the rejection.

is_writable: Similarly, programs can grant write access to accounts during a CPI that the outer header marks as read-only, so reconstructed is_writable values will be wrong for those accounts.

Consider tracking is_signer for inner instructions differently (e.g., treating them as read-only reconstructions, or skipping is_signer restoration for simulated inner instructions), or at least documenting the approximation and ensuring downstream consumers aren't security-sensitive with respect to inner instruction signer metadata.