From 71eb2bc7d31782bd23df65b55844180e60f41e09 Mon Sep 17 00:00:00 2001 From: SarahDoma Date: Sun, 18 Oct 2026 12:23:09 +0100 Subject: [PATCH] Add contract validation enhancements for robustness ## Summary Implements four critical contract validation improvements: ### #785 - StreamCancelled event includes remaining deposit - Added remaining_deposit parameter to cancelled() and force_cancelled() events - Provides complete financial breakdown in event logs - Updated lib.rs calls to pass refund_to_sender as remaining_deposit ### #786 - SEP-0041 transfer return value validation - Validates return values from all tk.transfer() calls in token-vault - Maps transfer errors to appropriate contract errors - Catches transfer failures before state mutations - Affects 5 transfer points in lib.rs ### #787 - Duplicate stream ID validation in batch cancel - Added DuplicateStreamId error code (33) to factory errors - Validates non-duplicate stream addresses before batch processing - Uses Map-backed set for O(log n) duplicate detection - Rejects batch early if duplicates detected ### #788 - WASM hash registry verification - Validates WASM hash exists in Soroban ledger before deployment - Uses env.ledger().get_wasm() to verify registry - Prevents deployment failures from stale/invalid hashes - Early validation in deploy_stream() Closes #785 Closes #786 Closes #787 Closes #788 --- contracts/factory/src/deploy.rs | 6 ++++++ contracts/factory/src/errors.rs | 2 ++ contracts/factory/src/lib.rs | 19 +++++++++---------- contracts/stream/src/events.rs | 22 ++++++++++++++++++---- contracts/stream/src/lib.rs | 4 ++-- contracts/token-vault/src/lib.rs | 15 ++++++++++----- 6 files changed, 47 insertions(+), 21 deletions(-) diff --git a/contracts/factory/src/deploy.rs b/contracts/factory/src/deploy.rs index 27b817a9..b5e9ec5d 100644 --- a/contracts/factory/src/deploy.rs +++ b/contracts/factory/src/deploy.rs @@ -32,6 +32,12 @@ pub fn deploy_stream( stream_id: u64, init_args: Vec, ) -> Result { + // Validate WASM hash registry prior to instantiation (Issue #788). + // Verify that the WASM hash is registered and available before attempting deployment. + // This prevents deploying with a stale or invalid hash that would fail mid-transaction. + let _ = env.ledger().get_wasm(wasm_hash) + .map_err(|_| Error::InvalidWasmHash)?; + // Derive a deterministic salt from the stream ID so each stream gets a // unique, reproducible contract address. let salt = compute_salt(env, stream_id); diff --git a/contracts/factory/src/errors.rs b/contracts/factory/src/errors.rs index 7cac36db..85d0fdc8 100644 --- a/contracts/factory/src/errors.rs +++ b/contracts/factory/src/errors.rs @@ -63,4 +63,6 @@ pub enum Error { InvalidGovernor = 31, /// The salt used for deploying the stream contract has already been used. SaltAlreadyUsed = 32, + /// `cancel_batch_streams` was called with duplicate stream addresses (Issue #787). + DuplicateStreamId = 33, } diff --git a/contracts/factory/src/lib.rs b/contracts/factory/src/lib.rs index c095a80a..bfee6973 100644 --- a/contracts/factory/src/lib.rs +++ b/contracts/factory/src/lib.rs @@ -604,21 +604,20 @@ impl DripFactory { return Err(Error::BatchTooLarge); } - // Deduplicate addresses to prevent attempting multiple cancels on the same stream. - // Issue #416, #627: If a duplicated address is passed, the first cancel succeeds - // and sets FLAG_CANCELLED; the second cancel on the now-cancelled stream would - // return Error::StreamCancelled, which the non-try_ variant turns into a panic. - // Deduplicating the list ensures each unique stream is cancelled exactly once. - // Uses a Map-backed set (O(log n) lookup/insert) rather than an O(n²) nested loop scan. + // Validate that no duplicate stream IDs are present in the batch (Issue #787). + // Check for duplicates using a Map-backed set (O(log n) lookup/insert). let mut seen = Map::new(&env); - let mut unique_addresses: Vec
= Vec::new(&env); for stream_addr in stream_addresses.iter() { - if !seen.contains_key(stream_addr.clone()) { - seen.set(stream_addr.clone(), ()); - unique_addresses.push_back(stream_addr); + if seen.contains_key(stream_addr.clone()) { + return Err(Error::DuplicateStreamId); } + seen.set(stream_addr.clone(), ()); } + // Process cancellations for all unique stream addresses. + // Issue #416, #627: The validation above ensures each address appears exactly once. + let unique_addresses = stream_addresses; + for stream_addr in unique_addresses.iter() { let stream_client = drip_stream::DripStreamClient::new(&env, &stream_addr); stream_client.cancel(&sender); diff --git a/contracts/stream/src/events.rs b/contracts/stream/src/events.rs index e2a7252f..5f0e72e9 100644 --- a/contracts/stream/src/events.rs +++ b/contracts/stream/src/events.rs @@ -84,14 +84,21 @@ pub fn withdrawn( ); } -pub fn cancelled(env: &Env, sender: &Address, refund_amount: i128, withdrawn_so_far: i128) { +pub fn cancelled( + env: &Env, + sender: &Address, + refund_amount: i128, + withdrawn_so_far: i128, + remaining_deposit: i128, +) { assert_non_negative_amount(env, refund_amount); assert_non_negative_amount(env, withdrawn_so_far); + assert_non_negative_amount(env, remaining_deposit); let sequence = next_sequence(env); env.events().publish( (symbol_short!("cancelled"), sender.clone(), sequence), - (refund_amount, withdrawn_so_far), + (refund_amount, withdrawn_so_far, remaining_deposit), ); } @@ -99,14 +106,21 @@ pub fn cancelled(env: &Env, sender: &Address, refund_amount: i128, withdrawn_so_ /// the stream paused past the threshold. Distinct from `cancelled` (the /// sender/operator-initiated path via `cancel`) so consumers of the event log /// can tell the two apart without correlating who signed the transaction. -pub fn force_cancelled(env: &Env, sender: &Address, refund_amount: i128, withdrawn_so_far: i128) { +pub fn force_cancelled( + env: &Env, + sender: &Address, + refund_amount: i128, + withdrawn_so_far: i128, + remaining_deposit: i128, +) { assert_non_negative_amount(env, refund_amount); assert_non_negative_amount(env, withdrawn_so_far); + assert_non_negative_amount(env, remaining_deposit); let sequence = next_sequence(env); env.events().publish( (symbol_short!("force_cxl"), sender.clone(), sequence), - (refund_amount, withdrawn_so_far), + (refund_amount, withdrawn_so_far, remaining_deposit), ); } diff --git a/contracts/stream/src/lib.rs b/contracts/stream/src/lib.rs index c2642aa2..2e2d685b 100644 --- a/contracts/stream/src/lib.rs +++ b/contracts/stream/src/lib.rs @@ -432,7 +432,7 @@ impl DripStream { tk.transfer(&contract_addr, &info.sender, &refund_to_sender); } - events::cancelled(env, &info.sender, refund_to_sender, total_withdrawn); + events::cancelled(env, &info.sender, refund_to_sender, total_withdrawn, refund_to_sender); Ok(()) } @@ -1084,7 +1084,7 @@ impl DripStream { tk.transfer(&contract_addr, &info.sender, &refund_to_sender); } - events::force_cancelled(env, &info.sender, refund_to_sender, total_withdrawn); + events::force_cancelled(env, &info.sender, refund_to_sender, total_withdrawn, refund_to_sender); Ok(()) } diff --git a/contracts/token-vault/src/lib.rs b/contracts/token-vault/src/lib.rs index 5d86c104..c1d4a060 100644 --- a/contracts/token-vault/src/lib.rs +++ b/contracts/token-vault/src/lib.rs @@ -175,7 +175,8 @@ fn withdraw_payouts( expected_balance = expected_balance .checked_sub(amount) .ok_or(Error::ArithmeticOverflow)?; - tk.transfer(&env.current_contract_address(), &to, &amount); + tk.transfer(&env.current_contract_address(), &to, &amount) + .map_err(|_| Error::WithdrawTransferFailed)?; let new_balance = vault_balance(env)?; if new_balance != expected_balance { return Err(Error::WithdrawTransferFailed); @@ -252,7 +253,8 @@ impl TokenVault { } let tk = token_client(&env)?; - tk.transfer(&from, &env.current_contract_address(), &amount); + tk.transfer(&from, &env.current_contract_address(), &amount) + .map_err(|_| Error::DepositTransferFailed)?; let new_balance = tk.balance(&env.current_contract_address()); if new_balance > max { return Err(Error::LimitExceeded); @@ -330,7 +332,8 @@ impl TokenVault { let contract_addr = env.current_contract_address(); for deposit in deposits.iter() { - tk.transfer(&deposit.from, &contract_addr, &deposit.amount); + tk.transfer(&deposit.from, &contract_addr, &deposit.amount) + .map_err(|_| Error::DepositTransferFailed)?; let current_balance = tk.balance(&contract_addr); events::deposited(&env, &deposit.from, deposit.amount, current_balance); } @@ -729,7 +732,8 @@ impl TokenVault { } let tk = token::Client::new(&env, &token); - tk.transfer(&from, &env.current_contract_address(), &amount); + tk.transfer(&from, &env.current_contract_address(), &amount) + .map_err(|_| Error::DepositTransferFailed)?; let current_acc_balance = get_token_account_balance(&env, &token, &from); let new_acc_balance = current_acc_balance @@ -776,7 +780,8 @@ impl TokenVault { return Err(Error::LimitExceeded); } - tk.transfer(&env.current_contract_address(), &to, &amount); + tk.transfer(&env.current_contract_address(), &to, &amount) + .map_err(|_| Error::WithdrawTransferFailed)?; if current_acc_balance >= amount { set_token_account_balance(&env, &token, &caller, current_acc_balance - amount);