From 326a559f76de6a401bf5a29bd930bbb2bbe2cfdb Mon Sep 17 00:00:00 2001 From: Kayos Date: Tue, 12 May 2026 14:42:13 -0700 Subject: [PATCH] chore: strip audit-ticket prefixes from code comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drops the ~60 ticket-prefix comments (CRIT-N, HIGH-N, MED-N, LOW-N, L-N, M-N, AUDIT-N, PLUTUS-N, "audit fix (date):", "Phase N" labels, "Adversarial-review fix:") that had accumulated in inline + doc comments over several audit cycles. Where the surrounding prose still carried useful WHY context it gets kept and tightened; where the ticket WAS the comment it gets dropped entirely. No logic, no renames, no behavior change. Audit history lives in commit messages and the audits/ tree where it belongs — eternal comments don't need to mirror it. Net 138 LOC shorter. 253 tests pass, no new clippy or fmt warnings. --- crates/aldabra-chain/src/koios.rs | 7 +- crates/aldabra-core/src/inspect.rs | 10 +- crates/aldabra-core/src/lib.rs | 1 - crates/aldabra-core/src/mint.rs | 2 +- crates/aldabra-core/src/plutus.rs | 70 ++-- crates/aldabra-core/src/sign.rs | 3 +- crates/aldabra-core/src/tx.rs | 36 +- crates/aldabra-dao/src/builder/escrow_open.rs | 39 +-- crates/aldabra-mcp/Cargo.toml | 3 +- crates/aldabra-mcp/src/bootstrap.rs | 12 +- crates/aldabra-mcp/src/main.rs | 37 +- crates/aldabra-mcp/src/tools.rs | 320 ++++++------------ 12 files changed, 201 insertions(+), 339 deletions(-) diff --git a/crates/aldabra-chain/src/koios.rs b/crates/aldabra-chain/src/koios.rs index bf09719..b209481 100644 --- a/crates/aldabra-chain/src/koios.rs +++ b/crates/aldabra-chain/src/koios.rs @@ -527,10 +527,9 @@ mod tests { ); } - // (the deserializes_tx_info_response / deserializes_empty_tx_info_response - // tests were dropped along with KoiosTxInfo when AUDIT4-1 swapped tx_status - // from /tx_info to /tx_status. Coverage is now in - // parses_koios_tx_status_shapes below.) + // tx_info-shape tests were dropped when tx_status moved from + // /tx_info to /tx_status. Coverage is in parses_koios_tx_status_shapes + // below. #[test] fn tx_status_serializes_with_tag() { diff --git a/crates/aldabra-core/src/inspect.rs b/crates/aldabra-core/src/inspect.rs index a2b1485..a0e535b 100644 --- a/crates/aldabra-core/src/inspect.rs +++ b/crates/aldabra-core/src/inspect.rs @@ -1,10 +1,10 @@ //! Decode + summarize a Conway-era tx CBOR for human review. //! -//! Used by `wallet.tx_summary` (Phase 4 / audit HIGH-2 fix). Before -//! a caller hands a pre-built CBOR to `wallet.sign_partial` or -//! `wallet.submit_signed_tx`, they should pull a summary through here -//! and review what the tx actually does — what's being spent, where -//! funds are going, what's being minted, what certs are present. +//! Backs `wallet.tx_summary`. Before handing a pre-built CBOR to +//! `wallet.sign_partial` or `wallet.submit_signed_tx`, callers should +//! summarize through here and read what the tx actually does — +//! what's being spent, where funds are going, what's being minted, +//! what certs are present. //! //! No I/O. Pure decode → typed summary. Caller serializes to JSON. diff --git a/crates/aldabra-core/src/lib.rs b/crates/aldabra-core/src/lib.rs index 6431c12..937525e 100644 --- a/crates/aldabra-core/src/lib.rs +++ b/crates/aldabra-core/src/lib.rs @@ -180,7 +180,6 @@ impl Mnemonic { // `xprv_bytes` was moved into normalize_bytes_force3rd, but // the stack slot can still hold a copy depending on calling // conventions / inlining. Defensive zeroize. - // (M-1 audit fix.) xprv_bytes.zeroize(); Ok(RootKey { xprv }) } diff --git a/crates/aldabra-core/src/mint.rs b/crates/aldabra-core/src/mint.rs index 02add92..7ead6bb 100644 --- a/crates/aldabra-core/src/mint.rs +++ b/crates/aldabra-core/src/mint.rs @@ -9,7 +9,7 @@ //! asset under that policy and sends them to a destination //! address. //! -//! ## Phase 3 scope today +//! Current scope: //! //! - **Single-sig** + **single-sig with `invalid_after`** policy //! shapes — the bread-and-butter wallet-controlled mint. diff --git a/crates/aldabra-core/src/plutus.rs b/crates/aldabra-core/src/plutus.rs index e9f7981..b0ca321 100644 --- a/crates/aldabra-core/src/plutus.rs +++ b/crates/aldabra-core/src/plutus.rs @@ -10,11 +10,9 @@ //! - a collateral input (≥ 5 ADA) from the wallet's normal UTXOs; //! consumed if the script fails on-chain. //! -//! ## Phase 4.1–4.3 scope -//! -//! Single Plutus input, single output, single collateral. Reference -//! inputs (4.2 expansion) and live ExUnits estimation (4.4) are -//! follow-ups. ExUnits today come from the caller. +//! Current scope: single Plutus input, single output, single collateral. +//! Reference inputs and live ExUnits estimation are follow-ups; ExUnits +//! today come from the caller. use bech32::FromBase32; use pallas_crypto::hash::Hash; @@ -137,8 +135,8 @@ pub fn build_signed_plutus_spend( let payout_addr = parse_address(payout_address_bech32)?; let network_id = network_id_for(network); - // PLUTUS-2 audit fix: collateral MUST be ADA-only (chain rejects - // collateral inputs that carry native assets). + // Collateral must be ADA-only — the chain rejects collateral + // inputs that carry native assets. let mut ada_only: Vec = available_utxos .iter() .filter(|u| u.assets.is_empty()) @@ -146,13 +144,12 @@ pub fn build_signed_plutus_spend( .collect(); ada_only.sort_by_key(|u| std::cmp::Reverse(u.lovelace)); - // AUDIT4-2 fix: pick the SMALLEST ADA-only UTXO that still - // qualifies for collateral (≥ 5 ADA), so the LARGEST stays - // available for funding the spend. The inverse approach (give - // collateral the biggest utxo) breaks the common case where a - // wallet has one large change utxo + a small self-send leftover, - // since funding ends up with the scrap and can't cover payout + - // fee + min_utxo. + // Pick the SMALLEST ADA-only UTXO that still qualifies for + // collateral (≥ 5 ADA), so the LARGEST stays available for + // funding the spend. Giving collateral the biggest utxo breaks + // the common case where a wallet has one large change utxo + a + // small self-send leftover — funding ends up with the scrap and + // can't cover payout + fee + min_utxo. // // Collateral is NEVER consumed on the happy path — it's only // seized if the script fails — so its size beyond the 5-ADA @@ -170,9 +167,9 @@ pub fn build_signed_plutus_spend( })? .clone(); - // PLUTUS-1 audit fix: collateral is NOT consumed on the happy - // path — it's only seized if the script fails. So we need a - // SEPARATE regular input to fund payout + fee + change. + // Collateral is NOT consumed on the happy path — it's only + // seized if the script fails — so we need a SEPARATE regular + // input to fund payout + fee + change. // // Pick the LARGEST ADA-only UTXO that's not the collateral — // funding has to cover payout + script-execution fee + change @@ -195,8 +192,8 @@ pub fn build_signed_plutus_spend( // Collateral is held off-line and only consumed on script failure. let total_in = locked.lovelace.saturating_add(funding.lovelace); - // PLUTUS-3 audit fix: Plutus tx fee = size_fee + ExUnits_fee. - // ExUnits dominates for typical scripts (~1.5 ADA at default budget). + // Plutus tx fee = size_fee + ExUnits_fee. ExUnits dominates for + // typical scripts (~1.5 ADA at default budget). let ex_fee = params.ex_units_fee(ex_units.mem, ex_units.steps); let fee_pass1 = 1_000_000u64.saturating_add(ex_fee); let need = payout_lovelace @@ -225,8 +222,8 @@ pub fn build_signed_plutus_spend( let build_with_fee = |fee: u64, change_lovelace: u64| -> Result { let mut staging = StagingTransaction::new(); - // PLUTUS-1: locked + funding as regular inputs (both consumed - // on happy path); collateral as collateral_input only. + // Locked + funding go in as regular inputs (both consumed + // on the happy path); collateral as collateral_input only. staging = staging.input(locked_input.clone()); staging = staging.input(funding_input.clone()); staging = staging.collateral_input(collateral_input.clone()); @@ -246,11 +243,11 @@ pub fn build_signed_plutus_spend( if let Some(d) = witness_datum_cbor { staging = staging.datum(d.to_vec()); } - // PLUTUS-4 audit fix: pallas-txbuilder only computes - // script_data_hash if language_view is set. Without it, the - // body's hash is None and the chain rejects with - // PPViewHashesDontMatch. PlutusV3 path requires a V3 cost - // model — caller-supplied via ProtocolParams. + // pallas-txbuilder only computes script_data_hash when + // language_view is set; without it the body hash is None + // and the chain rejects with PPViewHashesDontMatch. The + // PlutusV3 path needs a V3 cost model — caller-supplied + // via ProtocolParams. if let Some(cost_model) = params.plutus_v3_cost_model.as_deref() { if matches!(script_version, PlutusVersion::V3) { staging = @@ -379,7 +376,7 @@ mod tests { #[test] fn ex_units_fee_matches_known_values() { - // PLUTUS-3 audit fix: fee for default budget should be ~1.5 ADA. + // Fee for the default budget should be ~1.5 ADA. let p = ProtocolParams::default(); let fee = p.ex_units_fee(DEFAULT_EX_UNITS.mem, DEFAULT_EX_UNITS.steps); // 14M mem * 0.0577 + 10B steps * 7.21e-5 @@ -403,9 +400,8 @@ mod tests { lovelace: 50_000_000, }; // Only ONE wallet UTXO — collateral pick consumes it; no - // separate funding UTXO available. PLUTUS-1 fix should - // reject with a clear error rather than building a tx that - // chain rejects. + // separate funding UTXO available. Should reject with a clear + // error rather than build a tx the chain rejects. let utxos = vec![InputUtxo { tx_hash_hex: "cafebabe".repeat(8), output_index: 0, @@ -492,8 +488,8 @@ mod tests { output_index: 0, lovelace: 50_000_000, }; - // PLUTUS-1+2 audit fix: spend path needs TWO ADA-only wallet - // UTXOs — one for collateral, one for funding. + // The spend path needs TWO ADA-only wallet UTXOs — one for + // collateral, one for funding. let utxos = vec![ InputUtxo { tx_hash_hex: "cafebabe".repeat(8), @@ -541,11 +537,11 @@ mod tests { assert!(witness.redeemer.is_some(), "redeemer witness present"); } - /// AUDIT4-2 regression. A wallet with one tiny qualifying UTXO - /// alongside one huge UTXO must pick the tiny one for collateral - /// and the huge one for funding (not the inverse). The inverse - /// fails in the common wallet shape where funding then can't - /// cover payout + script-exec fee + change min_utxo. + /// A wallet with one tiny qualifying UTXO alongside one huge UTXO + /// must pick the tiny one for collateral and the huge one for + /// funding (not the inverse). The inverse fails in the common + /// wallet shape where funding then can't cover payout + script- + /// exec fee + change min_utxo. #[test] fn picks_smallest_qualifying_collateral_largest_funding() { let payment = payment_from_canonical(); diff --git a/crates/aldabra-core/src/sign.rs b/crates/aldabra-core/src/sign.rs index 03ddc5e..9dd1212 100644 --- a/crates/aldabra-core/src/sign.rs +++ b/crates/aldabra-core/src/sign.rs @@ -44,8 +44,7 @@ pub fn add_witness(payment_key: &PaymentKey, cbor_bytes: &[u8]) -> Result u64 { // ceil(mem * num / den) let mem_cost = mem @@ -341,10 +340,10 @@ fn hex_decode_32(s: &str) -> Result<[u8; 32], WalletError> { /// `BuiltTransaction::sign` can consume it. The XPrv's first 64 /// bytes are the extended secret; we reuse them directly. /// -/// **M-1 audit fix**: defensive zeroize of the stack-resident -/// extended-secret bytes after `from_bytes` consumes them. The -/// SecretKeyExtended itself zeroizes on drop (pallas-crypto handles -/// that); this just covers the local stack copy that lingers between +/// Defensive zeroize of the stack-resident extended-secret bytes +/// after `from_bytes` consumes them. The SecretKeyExtended itself +/// zeroizes on drop (pallas-crypto handles that); this just covers +/// the local stack copy that lingers between /// `extended_secret_key()` returning and `from_bytes` taking it by /// value. fn payment_key_to_private(payment: &PaymentKey) -> Result { @@ -502,10 +501,10 @@ fn build_unsigned_bytes(staging: StagingTransaction) -> Result, WalletEr /// `&[]` for `assets_to_send` to keep it ADA-only. /// /// `to_inline_datum_cbor`, when `Some`, attaches the bytes as an -/// inline datum on the recipient output. Used to lock funds at a -/// script address with a datum the validator can read (AUDIT4-3 -/// fix). Change output never gets a datum — it goes back to the -/// wallet which has no validator to satisfy. +/// inline datum on the recipient output — needed to lock funds at a +/// script address with a datum the validator can read. The change +/// output never gets a datum (it goes back to the wallet, no +/// validator to satisfy). /// /// `to_reference_script`, when `Some`, attaches the script bytes as /// a reference-script on the recipient output (Babbage/Conway era @@ -622,8 +621,8 @@ fn prepare_payment( // worth of lovelace into change in that case. let change_must_exist = !change_assets.is_empty(); - // L-1 audit fix: checked_add for the inner sum so the outer - // checked_sub is fully defensive against u64 overflow. + // checked_add on the inner sum so the outer checked_sub is fully + // defensive against u64 overflow. let outflow = lovelace .checked_add(real_fee) .ok_or_else(|| WalletError::Derivation("lovelace + fee overflow".into()))?; @@ -671,9 +670,8 @@ fn prepare_payment( // Re-shape the asset maps back into Vec for the // summary — easier for callers to display than a BTreeMap. - // L-4 audit fix: replaced .expect() with proper error - // propagation. Logic-bug paths (we built the key) become - // typed errors instead of process-level panics. + // Logic-bug paths (we built the key) propagate as typed errors + // rather than panicking the process. let send_assets_vec: Vec = target_assets .iter() .map(|(k, v)| { @@ -767,8 +765,8 @@ pub fn build_signed_payment( /// /// `to_inline_datum_cbor`, when `Some`, attaches the bytes as an /// inline datum on the recipient output — needed to lock funds at -/// a script address with a datum the validator can read (AUDIT4-3 -/// fix). `None` for normal sends to wallet addresses. +/// a script address with a datum the validator can read. `None` +/// for normal sends to wallet addresses. #[allow(clippy::too_many_arguments)] pub fn build_signed_payment_with_assets( payment_key: &PaymentKey, diff --git a/crates/aldabra-dao/src/builder/escrow_open.rs b/crates/aldabra-dao/src/builder/escrow_open.rs index f031448..596ba37 100644 --- a/crates/aldabra-dao/src/builder/escrow_open.rs +++ b/crates/aldabra-dao/src/builder/escrow_open.rs @@ -89,16 +89,12 @@ pub struct UnsignedEscrowOpen { pub fn build_unsigned_escrow_open(args: EscrowOpenArgs) -> DaoResult { // ---- preflight ---- // - // HIGH-2 fix (2026-05-09 audit): refuse `initial_contributor=None` - // entirely. Previously this opened an escrow with `deposits=[]` and - // `initial_lovelace > 0`, which the validator (pre-fix) treated as - // a refundable-by-anyone escrow because `refund_outputs_satisfy(_, [])` - // is vacuously true. Even with the validator now enforcing - // `sum(deposits) == in_value`, the empty-deposits path produces a - // permanently-stuck escrow (no Veto/Refund possible until somebody - // deposits). Cleaner v1 invariant: every escrow has at least one - // contributor at open. The "open empty, top up later" UX was never - // useful anyway — party_a can just open + deposit in a single call. + // Every escrow needs at least one contributor at open. With + // `deposits=[]` the validator's `refund_outputs_satisfy(_, [])` + // returns vacuously true (drainable on Veto/Refund), and even + // post-fix the empty-deposits path produces a permanently-stuck + // escrow until someone deposits. Cleaner invariant: party_a can + // just open + deposit in a single call. let initial_contributor = args.initial_contributor.ok_or_else(|| { DaoError::State( "initial_contributor is required — open an escrow with at least one contributor (party_a or party_b)" @@ -111,12 +107,11 @@ pub fn build_unsigned_escrow_open(args: EscrowOpenArgs) -> DaoResult DaoResult Result<()> { use std::os::unix::fs::OpenOptionsExt; @@ -55,9 +54,8 @@ fn write_owner_only(path: &Path, payload: &[u8]) -> Result<()> { } /// Read `ALDABRA_PASSPHRASE` env into a `Zeroizing` so the -/// in-process copy gets wiped when dropped. The env block itself -/// isn't zeroizable — that's a documented headless tradeoff. (M-3 -/// audit fix.) +/// in-process copy gets wiped on drop. The env block itself isn't +/// zeroizable — documented headless tradeoff. fn passphrase_from_env() -> Option> { std::env::var("ALDABRA_PASSPHRASE").ok().map(Zeroizing::new) } diff --git a/crates/aldabra-mcp/src/main.rs b/crates/aldabra-mcp/src/main.rs index f9343cd..4165211 100644 --- a/crates/aldabra-mcp/src/main.rs +++ b/crates/aldabra-mcp/src/main.rs @@ -3,20 +3,8 @@ //! Speaks MCP over stdio. Any MCP client (Claude Code, OpenClaw, etc.) //! launches this as a subprocess and gets a wallet's worth of tools. //! -//! ## Phase 1 tools (target — server wiring lands in 1.7) -//! -//! - `wallet.address` — derived CIP-1852 base address -//! - `wallet.balance` — ADA + native-asset balance via chain backend -//! - `wallet.utxos` — list UTXOs at the wallet address -//! - `wallet.network` — configured network selector -//! -//! ## Phase 2-4 tools -//! -//! See `ROADMAP.md` at the repo root. -//! -//! ## Logging -//! -//! Stderr only — stdout is the MCP transport, must stay clean. +//! Logging: stderr only — stdout is the MCP transport and must stay +//! clean. mod bootstrap; mod config; @@ -60,17 +48,12 @@ async fn run() -> Result<()> { "aldabra starting" ); - // CRIT-2 audit fix (2026-05-12): make sure the sandbox root exists - // and is daemon-only readable. Tools use canonicalize() against - // this dir to validate `*_path` args, which requires the dir to - // be a real directory on disk. Idempotent: create_dir_all is a - // no-op when the dir already exists. - // - // Adversarial-review fix (2026-05-12): chmod is `?`'d not swallowed. - // If the filesystem refuses chmod (noexec, selinux, broken mount), - // we'd otherwise silently fall back to the umask default (commonly - // 0o755) — making the security comment "daemon-only readable" a - // lie. Fail loudly instead so the operator sees + investigates. + // Make sure the sandbox root exists and is daemon-only readable. + // Tools canonicalize() against this dir to validate `*_path` + // args, which requires the dir to be a real directory on disk. + // Chmod is `?`'d not swallowed — if the filesystem refuses it + // (noexec, selinux, broken mount), fail loudly instead of + // silently falling back to umask 0o755. if !cfg.safe_reads_root.exists() { std::fs::create_dir_all(&cfg.safe_reads_root).map_err(|e| { anyhow::anyhow!( @@ -117,8 +100,8 @@ async fn run() -> Result<()> { let xprv_path = bootstrap::root_xprv_path(&cfg.data_dir); let any_key_exists = mnemonic_path.exists() || xprv_path.exists(); - // L-2 audit fix: scope `root` to a block so its XPrv drops + wipes - // as soon as we've extracted the keys we need. + // Scope `root` to a block so its XPrv drops + wipes as soon as + // we've extracted the keys we need. let (payment_key, stake_key, address) = { let root = if bootstrap_new { bootstrap::generate_and_save_root_key(&cfg.data_dir)? diff --git a/crates/aldabra-mcp/src/tools.rs b/crates/aldabra-mcp/src/tools.rs index 0969652..112a75a 100644 --- a/crates/aldabra-mcp/src/tools.rs +++ b/crates/aldabra-mcp/src/tools.rs @@ -5,23 +5,9 @@ //! tool names against `[a-zA-Z0-9_-]{1,64}` and silently drops names //! with dots, causing the daemon to run without advertising any tools. //! -//! ## Phase 1 — read path -//! -//! - `wallet_address` — bech32 base address -//! - `wallet_network` — mainnet | preview | preprod -//! - `wallet_balance` — JSON `{lovelace, assets}` -//! - `wallet_utxos` — JSON list of UTXOs -//! -//! ## Phase 2 — send path -//! -//! - `wallet_send` — build + sign + submit ADA payment, with hard -//! cap guard (`max_send_lovelace`) -//! - `wallet_tx_status` — poll a submitted tx hash -//! -//! Returns: -//! - `String` results pass through `IntoContents` directly. -//! - `Result` lets us surface chain / build errors -//! as MCP tool-call errors instead of crashing the daemon. +//! Result types: `String` passes through `IntoContents` directly; +//! `Result` surfaces chain / build errors as MCP +//! tool-call errors instead of crashing the daemon. use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -78,29 +64,17 @@ use aldabra_dao::discovery::{ }; use aldabra_dao::reader::{DaoReader, KoiosDaoReader}; -/// CRIT-2 audit fix (2026-05-12): reject any `*_path` arg whose -/// canonical form does not live under the configured sandbox root. -/// Without this guard, a prompt-injection that gets the LLM to call a -/// tool with `reference_script_path: "/var/lib/aldabra/root-xprv.age"` -/// would have the daemon happily read the encrypted key blob. Even -/// though hex decoding will fail, the previous error message included -/// the byte offset of the first non-hex character — a small but real -/// information leak on the file's binary structure. This guard + -/// content-stripped error messages close both surfaces. +/// Reject any `*_path` arg whose canonical form does not live under +/// the sandbox root. Without this, a prompt-injection could point the +/// daemon at `$ALDABRA_DATA/root-xprv.age` and leak key bytes through +/// decode-error messages. Both root and arg are canonicalized +/// (symlinks resolved, must exist); arg must `starts_with` root. /// -/// `safe_reads_root` and `user_path` are both canonicalized (resolves -/// symlinks, requires the file to exist). The user path must -/// `starts_with` the root; equality is allowed but rare in practice. -/// -/// Adversarial-review fix (2026-05-12): also reject regular files -/// whose link count > 1. `canonicalize` resolves symlinks but NOT -/// hardlinks — a hardlink IS the file (same inode, multiple directory -/// entries). Without this check, an attacker with daemon-uid write -/// access could plant a hardlink inside the sandbox pointing at the -/// inode of `$ALDABRA_DATA/root-xprv.age` and exfiltrate key bytes -/// through the read path. No legitimate use case puts hardlinks in -/// a script-CBOR sandbox; refusing them outright is conservative -/// and easy. +/// Hardlinks are refused too: `canonicalize` resolves symlinks but a +/// hardlink IS the file. If an attacker with daemon-uid write access +/// plants a hardlink to a secret inside the sandbox, the inode is +/// inside-sandbox by every filesystem-level check. Nothing legitimate +/// puts hardlinks in a script-CBOR dir, so `nlink > 1` → refuse. fn assert_inside_sandbox(safe_reads_root: &Path, user_path: &str) -> Result { let user_canon = std::fs::canonicalize(user_path) .map_err(|e| format!("resolve path: {e} (path must exist + be readable)"))?; @@ -117,12 +91,9 @@ fn assert_inside_sandbox(safe_reads_root: &Path, user_path: &str) -> Result 1 means another directory entry - // points at the same inode; we can't verify the other entry is also - // inside the sandbox without an exhaustive filesystem scan, so we - // reject conservatively. + // nlink > 1 means another directory entry points at the same inode. + // We can't verify the other entry is also inside the sandbox without + // a full filesystem scan, so reject conservatively. #[cfg(unix)] { use std::os::unix::fs::MetadataExt; @@ -139,24 +110,16 @@ fn assert_inside_sandbox(safe_reads_root: &Path, user_path: &str) -> Result~ 4500 chars get a 1-byte truncation + structural -/// rearrangement somewhere between client and stdio reader. Reading -/// from a file inside the container bypasses the JSON-RPC arg path -/// entirely. +/// The path variant exists because MCP truncates hex strings over +/// ~4500 chars somewhere between client and stdio reader. Reading +/// from a file bypasses the JSON-RPC arg path entirely. /// -/// CRIT-2 audit fix (2026-05-12): the path arg is sandboxed via -/// [`assert_inside_sandbox`] against the daemon's configured -/// `safe_reads_root` (default `$ALDABRA_DATA/scripts/`). Decode-error -/// messages no longer include the file path or byte-offset context — -/// only "could not decode as hex" — to avoid leaking structural -/// information about non-hex files (e.g. encrypted key blobs that an -/// attacker might point the tool at). +/// Path reads are sandboxed by [`assert_inside_sandbox`]. Decode +/// errors on path contents return a constant message — no byte-offset +/// or file-content leak. fn resolve_ref_script_bytes( safe_reads_root: &Path, cbor_hex: Option<&str>, @@ -180,10 +143,8 @@ fn resolve_ref_script_bytes( if cleaned.is_empty() { return Err("reference_script_path contained no hex characters".into()); } - // CRIT-2: do NOT include the hex_decode error's position- - // offset detail or the file's contents in the user-visible - // error. A failed decode is a yes/no signal; anything more - // narrows the attacker's guesses about file structure. + // Constant message — leaking byte offsets would narrow an + // attacker's guesses about file structure. hex_decode(&cleaned) .map(Some) .map_err(|_| "reference_script_path contents are not valid hex".to_string()) @@ -192,19 +153,10 @@ fn resolve_ref_script_bytes( } } -/// Resolve the Plutus minting-policy CBOR from EITHER an inline -/// hex argument OR a file path inside the container. Caller passes -/// both raw options; this fn enforces the "exactly one" rule and -/// reads the file when path is set. -/// -/// Mirrors [`resolve_ref_script_bytes`] — same workaround for the -/// MCP large-string transport bug where hex strings >~ 4500 chars -/// get a 1-byte truncation between client and stdio reader, -/// surfacing as "odd length" hex decode errors and blocking debug- -/// build minting policies. Reading from a file inside the container -/// bypasses the JSON-RPC arg path entirely. -/// -/// CRIT-2 audit fix (2026-05-12): see [`resolve_ref_script_bytes`]. +/// Resolve a Plutus minting-policy CBOR from either inline hex or +/// a file path. Exactly one must be set. Mirrors +/// [`resolve_ref_script_bytes`] — same transport workaround, same +/// sandbox + constant-message contract. fn resolve_policy_cbor_bytes( safe_reads_root: &Path, cbor_hex: Option<&str>, @@ -325,10 +277,9 @@ struct WalletInner { /// quotas. None = public tier. Sourced from env only — never from /// disk; never logged. koios_bearer: Option, - /// CRIT-2 audit fix (2026-05-12): sandbox root for `*_path` tool - /// args. Files outside this directory (canonical) are refused to - /// keep prompt-injection from steering reads at the encrypted - /// key material under `$ALDABRA_DATA/`. + /// Sandbox root for `*_path` tool args. Files outside this dir + /// (canonical) are refused, keeping prompt-injection from steering + /// reads at the encrypted key material under `$ALDABRA_DATA/`. safe_reads_root: PathBuf, } @@ -386,10 +337,9 @@ impl WalletService { } /// Reject if `lovelace` exceeds the wallet's hard cap unless - /// `force=true`. Used by every tool that moves lovelace to a - /// non-wallet destination — wallet_send, wallet_mint, - /// wallet_mint_cip68_nft, wallet_script_spend. - /// (HIGH-1 audit fix: previously only wallet_send had this guard.) + /// `force=true`. Every tool that moves lovelace to a non-wallet + /// destination calls this — `wallet_send`, `wallet_mint`, + /// `wallet_mint_cip68_nft`, `wallet_script_spend`. fn enforce_value_cap(&self, lovelace: u64, force: bool) -> Result<(), String> { if lovelace > self.inner.max_send_lovelace && !force { return Err(format!( @@ -400,23 +350,16 @@ impl WalletService { Ok(()) } - /// CRIT-1 audit fix (2026-05-12): scan a Conway-era tx CBOR and - /// enforce the wallet's `max_send_lovelace` cap against the sum - /// of lovelace going to any address that isn't this wallet's own. - /// This is the chokepoint that defends `wallet_sign_partial` and - /// `wallet_submit_signed_tx` against an unsigned-tx → - /// sign-partial → submit drain via prompt injection. Each of - /// those tools individually had `lovelace > 0` validation but no - /// blast-radius limit. + /// Chokepoint for `wallet_sign_partial` and `wallet_submit_signed_tx`: + /// sum lovelace going to any non-self address and apply + /// `enforce_value_cap`. Without this, a prompt-injection can walk + /// the unsigned → sign → submit chain and drain past the cap — + /// each step alone only checks `lovelace > 0`. /// - /// "Non-self" is computed by comparing the wallet's bech32 address - /// (decoded to raw bytes via pallas-addresses, then hexed) against - /// each output's `address_hex` in the tx body. Outputs whose - /// address matches are treated as change/self-send and excluded. - /// This is intentionally conservative on the same wallet's other - /// accounts/indexes — they'll be flagged as non-self until/unless - /// the wallet learns additional addresses to consider its own. - /// Force `=true` overrides; mirror the `enforce_value_cap` contract. + /// "Non-self" matches the wallet's primary bech32 address only. + /// Other accounts/indexes of the same wallet are conservatively + /// counted as non-self. `force=true` overrides, same contract as + /// `enforce_value_cap`. fn enforce_cap_on_cbor(&self, cbor_bytes: &[u8], force: bool) -> Result<(), String> { let total = sum_non_self_lovelace(cbor_bytes, &self.inner.address)?; self.enforce_value_cap(total, force) @@ -428,13 +371,10 @@ impl WalletService { /// function so the cap-check logic is unit-testable without standing /// up a full WalletService. /// -/// Adversarial-review fix (2026-05-12): `try_fold` + `checked_add` -/// instead of `.sum::()`. The naive `.sum()` wraps on overflow -/// in release builds — a prompt-injection could craft a CBOR with -/// outputs summing past `u64::MAX`, wrap to a small value, and -/// silently pass the cap check. Real-world unreachable (Cardano max -/// supply is ~45B ADA = 4.5e16 lovelace, ~400x below `u64::MAX`) but -/// trivial to harden. +/// Uses `try_fold` + `checked_add` rather than `.sum::()` so a +/// crafted CBOR that overflows can't silently wrap to a small value +/// in release builds. Cardano max supply is ~400x below `u64::MAX` +/// so the overflow path is unreachable in practice, but free to harden. fn sum_non_self_lovelace(cbor_bytes: &[u8], wallet_bech32: &str) -> Result { use pallas_addresses::Address; let wallet_addr = @@ -587,10 +527,9 @@ pub struct UnsignedSendArgs { /// [`SendArgs::reference_script_kind`]. #[serde(default)] pub reference_script_kind: Option, - /// CRIT-1 audit fix (2026-05-12): bypass the configured - /// `max_send_lovelace` hard cap. Required to build an unsigned - /// tx that would otherwise drain the wallet past the cap once - /// signed + submitted. Mirrors `wallet_send.force`. + /// Bypass the `max_send_lovelace` cap. Required when an unsigned + /// tx would otherwise drain the wallet past the cap once signed + /// and submitted. Mirrors `wallet_send.force`. #[serde(default)] pub force: bool, } @@ -601,12 +540,10 @@ pub struct SubmitSignedArgs { /// cold-signer that consumed the unsigned CBOR returned by /// `wallet_send_unsigned`. pub signed_cbor_hex: String, - /// CRIT-1 audit fix (2026-05-12): bypass the configured - /// `max_send_lovelace` hard cap. The submit path scans the tx - /// CBOR and sums lovelace going to any non-self address; pass - /// `force=true` to push a tx that exceeds that cap (cold-signed - /// multi-sig flows where a treasury moves real value - /// intentionally). + /// Bypass the `max_send_lovelace` cap. The submit path scans the + /// tx CBOR and sums lovelace going to any non-self address; pass + /// `force=true` for cold-signed multi-sig flows where a treasury + /// moves real value intentionally. #[serde(default)] pub force: bool, } @@ -679,10 +616,9 @@ pub struct PlutusMintUnsignedArgs { pub ex_units_mem: Option, #[serde(default)] pub ex_units_steps: Option, - /// CRIT-1 audit fix (2026-05-12): bypass the configured - /// `max_send_lovelace` hard cap on `dest_lovelace`. Required for - /// large script-bootstrap mints (governor / stakes / proposal - /// outputs that carry sizeable min-utxo + datum overhead). + /// Bypass the `max_send_lovelace` cap on `dest_lovelace`. Needed + /// for large script-bootstrap mints (governor / stakes / proposal + /// outputs that carry sizable min-utxo + datum overhead). #[serde(default)] pub force: bool, } @@ -721,8 +657,7 @@ pub struct MintUnsignedArgs { /// optional metadata for downstream signers. #[serde(default)] pub disclosed_signer_pkh_hex: Option, - /// CRIT-1 audit fix (2026-05-12): bypass the configured - /// `max_send_lovelace` hard cap on `dest_lovelace`. Mirrors + /// Bypass the `max_send_lovelace` cap on `dest_lovelace`. Mirrors /// `wallet_mint.force`. #[serde(default)] pub force: bool, @@ -845,12 +780,10 @@ pub struct SignPartialArgs { /// partially signed by another party. The wallet's payment key /// gets appended as an additional VKeyWitness. pub cbor_hex: String, - /// CRIT-1 audit fix (2026-05-12): bypass the configured - /// `max_send_lovelace` hard cap. Before signing, the tool sums - /// lovelace across every non-self output in the CBOR (anything - /// not addressed to this wallet) and rejects if the total - /// exceeds the cap. Pass `force=true` for cold-signed multi-sig - /// flows where a treasury moves real value intentionally. + /// Bypass the `max_send_lovelace` cap. Before signing, the tool + /// sums lovelace across every non-self output in the CBOR; pass + /// `force=true` for cold-signed multi-sig flows where a treasury + /// moves real value intentionally. #[serde(default)] pub force: bool, } @@ -1016,12 +949,11 @@ impl WalletService { if lovelace == 0 { return Err("lovelace must be > 0".into()); } - // AUDIT4-G2 fix: catch sub-min-utxo ada-only sends client-side - // before the chain rejects them (saves a koios round-trip + the - // user's mental model of "tx submitted" → "tx failed minutes later"). - // Asset-bearing sends have a dynamic min driven by asset count + - // name lengths — let those reach chain so the real number is in - // the error. + // Catch sub-min-utxo ada-only sends client-side so the user + // gets a clean error instead of "tx submitted" → "tx failed + // minutes later". Asset-bearing sends have a dynamic min + // (asset count + name length) — let those reach chain so + // the real number is in the rejection. let default_min = ProtocolParams::default().min_utxo_lovelace; if assets.is_empty() && lovelace < default_min { return Err(format!( @@ -1142,10 +1074,8 @@ impl WalletService { if lovelace == 0 { return Err("lovelace must be > 0".into()); } - // CRIT-1 audit fix (2026-05-12): apply the same cap as wallet_send - // so the unsigned-build chain isn't a quiet bypass of the daemon's - // value-safety policy. Defense-in-depth — the sign / submit - // tools also re-check via enforce_cap_on_cbor. + // Same cap as wallet_send so the unsigned-build chain isn't a + // quiet bypass. Sign + submit re-check via enforce_cap_on_cbor. self.enforce_value_cap(lovelace, force)?; let utxos = self @@ -1225,11 +1155,9 @@ impl WalletService { }: SubmitSignedArgs, ) -> Result { let bytes = hex_decode(&signed_cbor_hex).map_err(|e| format!("decode: {e}"))?; - // CRIT-1 audit fix (2026-05-12): scan the tx for outputs going - // to non-self addresses and enforce the cap on the sum. Closes - // the prompt-injection drain path where an attacker convinces - // the LLM to hand-craft + sign + submit a tx that bypasses the - // cap on the individual wallet_send tool. + // Sum non-self outputs and enforce the cap. Closes the prompt- + // injection drain path through hand-crafted + signed CBOR that + // bypasses the per-tool wallet_send cap. self.enforce_cap_on_cbor(&bytes, force)?; self.inner .chain @@ -1293,8 +1221,6 @@ impl WalletService { "dest_lovelace {dest_lovelace} below 1 ADA min — token-bearing UTXO will be rejected" )); } - // HIGH-1 audit fix: enforce hard cap on lovelace going to a - // potentially-non-wallet destination. self.enforce_value_cap(dest_lovelace, force)?; let utxos = self @@ -1369,9 +1295,7 @@ impl WalletService { if user_lovelace < 1_000_000 || ref_lovelace < 1_000_000 { return Err("user_lovelace and ref_lovelace must each be ≥ 1 ADA".into()); } - // HIGH-1: cap on the total lovelace that leaves the wallet - // toward non-self destinations. Sum the two outputs; if either - // overflows, also reject. + // Cap the total lovelace leaving the wallet (both outputs). let total = user_lovelace .checked_add(ref_lovelace) .ok_or("user_lovelace + ref_lovelace overflow")?; @@ -1457,8 +1381,6 @@ impl WalletService { force, }: ScriptSpendArgs, ) -> Result { - // HIGH-1: cap on payout_lovelace (the funds going to the - // potentially-non-wallet payout address). self.enforce_value_cap(payout_lovelace, force)?; let version = match plutus_version.to_ascii_lowercase().as_str() { "v1" => PlutusVersion::V1, @@ -1511,10 +1433,9 @@ impl WalletService { lovelace: locked_lovelace, }; - // Plutus paths require the V3 cost model so the chain can - // verify script_data_hash. Hardcoded preprod value from - // koios epoch_params; future improvement is pulling fresh - // from the chain on each call. (PLUTUS-4 audit fix.) + // Plutus paths need the V3 cost model so the chain can verify + // script_data_hash. Hardcoded preprod value from koios + // epoch_params; eventually fetch fresh per call. let params = ProtocolParams { plutus_v3_cost_model: Some(PLUTUS_V3_COST_MODEL_PREPROD.to_vec()), ..ProtocolParams::default() @@ -1839,8 +1760,6 @@ impl WalletService { "dest_lovelace {dest_lovelace} below 1 ADA min for asset-bearing UTXO" )); } - // CRIT-1 audit fix (2026-05-12): cap the unsigned-mint output's - // dest_lovelace, mirroring wallet_mint. self.enforce_value_cap(dest_lovelace, force)?; // Resolve PolicySpec — caller-supplied JSON or wallet default. @@ -1928,8 +1847,6 @@ impl WalletService { "dest_lovelace {dest_lovelace} below 1 ADA min for asset-bearing UTXO" )); } - // CRIT-1 audit fix (2026-05-12): cap the Plutus-mint - // dest_lovelace, mirroring wallet_mint. self.enforce_value_cap(dest_lovelace, force)?; let policy_cbor = resolve_policy_cbor_bytes( @@ -2073,11 +1990,9 @@ impl WalletService { #[tool(aggr)] SignPartialArgs { cbor_hex, force }: SignPartialArgs, ) -> Result { let bytes = hex_decode(&cbor_hex).map_err(|e| format!("decode: {e}"))?; - // CRIT-1 audit fix (2026-05-12): scan the unsigned/partial tx - // for outputs going to non-self addresses and enforce the cap - // on the sum BEFORE appending a witness. Closes the prompt- - // injection drain path through the unsigned-build → sign → - // submit chain (each step individually had no cap-awareness). + // Sum non-self outputs and enforce the cap BEFORE appending a + // witness — closes the prompt-injection drain through the + // unsigned-build → sign → submit chain. self.enforce_cap_on_cbor(&bytes, force)?; let updated = add_witness(&self.inner.payment_key, &bytes).map_err(|e| format!("sign: {e}"))?; @@ -4110,13 +4025,12 @@ impl WalletService { let wallet_utxos = pull_wallet_utxos(&self.inner.chain, &self.inner.address).await?; let (tip_slot, _tip_ms) = fetch_tip_slot_ms(self).await?; - // MED-2/3 fix (2026-05-09 audit): derive `validity_lower_ms` - // from the slot via the Shelley constants, NOT from Koios's - // `block_time*1000`. The chain reconstructs `lower` from - // `valid_from_slot(slot)` via slot↔ms math; using Koios's + // Derive `validity_lower_ms` from the slot via Shelley + // constants, NOT from Koios's `block_time*1000`. The chain + // reconstructs `lower` from the slot via slot↔ms math; Koios's // block_time can drift up to ~1s, which at the strict-`>` - // validator boundary makes the off-chain preflight pass while - // the chain rejects → fees + collateral burned. + // validator boundary lets the off-chain preflight pass while + // the chain rejects — burning fees + collateral. let validity_lower_slot = tip_slot; let validity_lower_ms = slot_to_posix_ms(cfg.network, validity_lower_slot)?; @@ -4184,7 +4098,7 @@ impl WalletService { let cfg = self.dao_cfg_for_escrow()?; let wallet_utxos = pull_wallet_utxos(&self.inner.chain, &self.inner.address).await?; let (tip_slot, _tip_ms) = fetch_tip_slot_ms(self).await?; - // MED-2/3 fix: same slot-derived ms as Settle (see escrow_settle_unsigned). + // Same slot-derived ms as Settle (see escrow_settle_unsigned). let validity_lower_slot = tip_slot; let validity_lower_ms = slot_to_posix_ms(cfg.network, validity_lower_slot)?; let validity_upper_slot = tip_slot + validity_window_seconds.unwrap_or(1800); @@ -4493,16 +4407,15 @@ pub struct EscrowOpenUnsignedArgs { pub open_deadline_ms: i64, /// Veto-window length after Agree, in ms. pub lock_period_ms: i64, - /// Optional pkh of the initial contributor (must equal party_a or party_b). - /// **HIGH-2 fix 2026-05-09:** The builder now REJECTS `None` here — every - /// escrow must have at least one initial contributor at open time. - /// Sending `null` returns an error. Field stays `Option` for - /// schema-stability. + /// pkh of the initial contributor (must equal party_a or party_b). + /// Required — every escrow needs at least one contributor at open + /// time. `None` returns an error. The field stays `Option` + /// for schema stability. #[serde(default)] pub initial_contributor_pkh_hex: Option, - /// Lovelace to lock at the escrow output. **MED-5 fix:** must clear - /// 2_000_000 (≥2 ADA) — Conway-era inline-datum + script-address - /// outputs need ~1.4-1.7 ADA min-utxo, with headroom. + /// Lovelace to lock at the escrow output. Must clear 2_000_000 + /// (≥2 ADA) — Conway-era inline-datum + script-address outputs + /// need ~1.4-1.7 ADA min-utxo, with headroom. pub initial_lovelace: u64, } @@ -4830,8 +4743,7 @@ mod sandbox_tests { use std::fs; use std::io::Write; - /// CRIT-2 audit fix (2026-05-12): files inside the sandbox root resolve - /// cleanly via canonicalization. + /// Files inside the sandbox root resolve cleanly via canonicalization. #[test] fn path_inside_sandbox_is_accepted() { let root = tempfile::tempdir().expect("tempdir"); @@ -4880,12 +4792,11 @@ mod sandbox_tests { ); } - /// Adversarial-review fix: hardlinked files inside the sandbox - /// must be refused. `canonicalize` resolves symlinks but NOT - /// hardlinks — a hardlink IS the file (same inode, different - /// directory entry). Without this, an attacker with daemon-uid - /// write access could `link()` the encrypted key blob's inode - /// into the sandbox and exfiltrate the bytes. + /// Hardlinked files inside the sandbox must be refused. + /// `canonicalize` resolves symlinks but not hardlinks (a hardlink + /// IS the file — same inode), so without this guard an attacker + /// with daemon-uid write access could `link()` the encrypted key + /// blob's inode into the sandbox and exfiltrate the bytes. #[cfg(unix)] #[test] fn hardlinked_file_inside_sandbox_is_rejected() { @@ -4996,9 +4907,8 @@ mod cap_tests { (cbor, wallet_addr) } - /// CRIT-1: a tx whose non-self output sits below the cap returns 0 - /// total when the destination IS self (change-only); above cap the - /// sum reflects the actual outbound lovelace. + /// A tx whose only outputs are self-sends contributes 0 to the + /// non-self total — self-moves don't count against the cap. #[test] fn sum_non_self_excludes_self_outputs() { // Send to the wallet's OWN address — every output is "self"; @@ -5016,9 +4926,8 @@ mod cap_tests { ); } - /// CRIT-1: a tx sending lovelace to an UNRELATED address has its - /// destination output reflected in the non-self sum, so the - /// enforce_value_cap chokepoint can refuse to sign/submit it. + /// Outbound lovelace to an unrelated address counts toward the + /// non-self sum so the cap chokepoint can refuse to sign/submit. #[test] fn sum_non_self_counts_outbound_outputs() { // Send to a different derivation (account=0, index=1) of the @@ -5038,22 +4947,11 @@ mod cap_tests { ); } - /// Adversarial-review fix: `.sum::()` wraps on overflow in - /// release builds. The cap check must NEVER silently roll over a - /// u64 — a prompt-injection could craft CBOR with outputs summing - /// past u64::MAX, wrap to a small value, pass the check. Now uses - /// `checked_add` and returns Err on overflow. We test this via the - /// free function with hand-constructed `OutputSummary` mock data - /// rather than building a real CBOR (Cardano min-utxo + max-supply - /// constraints make a real overflow tx impossible to build). - /// - /// This test exercises `OutputSummary` from `aldabra-core::inspect` - /// to confirm the type's `lovelace: u64` field is what we expect, - /// and that the cap check refuses to silently roll over. A future - /// refactor could expose a `compute_total` helper taking a slice - /// of OutputSummary so we can test overflow in isolation; for now - /// the test relies on documenting the invariant via a regression - /// case in the free function below. + /// The cap check must never silently roll over a u64 — + /// `.sum::()` wraps on overflow in release builds. We use + /// `checked_add` and return Err instead. Tested against + /// synthetic `OutputSummary` values because Cardano min-utxo + + /// max-supply make a real overflow CBOR impossible to build. #[test] fn sum_non_self_handles_overflow_safely() { // Build two synthetic OutputSummary mocks via the public type.