From bcaffc5fbd836828b547a260674c7bead3c7a396 Mon Sep 17 00:00:00 2001 From: Chrisland58 Date: Thu, 23 Apr 2026 16:52:12 +0000 Subject: [PATCH 1/2] fix: multi-signature validation edge cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Implement duplicate signer detection in check_duplicate_signers (O(n²) address comparison) - Reject signers with zero weight in validate_create_escrow and validate_escrow_parameters - Fix validate_threshold to compare against total weight instead of signer count --- contracts/teachlink/src/validation.rs | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/contracts/teachlink/src/validation.rs b/contracts/teachlink/src/validation.rs index 5afdb33a..d031fed8 100644 --- a/contracts/teachlink/src/validation.rs +++ b/contracts/teachlink/src/validation.rs @@ -106,12 +106,12 @@ impl NumberValidator { Ok(()) } - /// Validates threshold against signer count - pub fn validate_threshold(threshold: u32, signer_count: u32) -> ValidationResult<()> { + /// Validates threshold against total signer weight + pub fn validate_threshold(threshold: u32, total_weight: u32) -> ValidationResult<()> { if threshold < config::MIN_THRESHOLD { return Err(ValidationError::InvalidThreshold); } - if threshold > signer_count { + if threshold > total_weight { return Err(ValidationError::InvalidThreshold); } Ok(()) @@ -296,6 +296,9 @@ impl EscrowValidator { let mut total_weight: u32 = 0; for signer in signers.iter() { + if signer.weight == 0 { + return Err(EscrowError::InvalidSignerThreshold); + } total_weight += signer.weight; } @@ -318,8 +321,14 @@ impl EscrowValidator { /// Checks for duplicate signers in the list pub fn check_duplicate_signers(signers: &Vec) -> Result<(), EscrowError> { - // Simplified check - removed Env::current() call which doesn't exist - // This validation is now handled by the caller + let len = signers.len(); + for i in 0..len { + for j in (i + 1)..len { + if signers.get(i).unwrap().address == signers.get(j).unwrap().address { + return Err(EscrowError::DuplicateSigner); + } + } + } Ok(()) } @@ -348,6 +357,9 @@ impl EscrowValidator { // Validate threshold against total signer weight let mut total_weight: u32 = 0; for signer in params.signers.iter() { + if signer.weight == 0 { + return Err(EscrowError::InvalidSignerThreshold); + } total_weight += signer.weight; } From 449867df21e779d64e8ea4aa11d05177532a21af Mon Sep 17 00:00:00 2001 From: Chrisland58 Date: Thu, 23 Apr 2026 17:35:46 +0000 Subject: [PATCH 2/2] fix: handle multisig duplicate signatures, weight validation, and threshold boundaries - Add EscrowValidator::validate_multisig consolidating all edge case checks - Use checked_add to prevent u32 weight overflow - Enforce threshold within [1, total_weight] - Reject zero-weight and duplicate signers - Refactor validate_create_escrow and validate_escrow_parameters to delegate to validate_multisig - Add test_multisig_threshold_boundaries covering all edge cases --- contracts/teachlink/src/validation.rs | 69 +++++++++----------- contracts/teachlink/tests/test_validation.rs | 51 +++++++++++++++ 2 files changed, 83 insertions(+), 37 deletions(-) diff --git a/contracts/teachlink/src/validation.rs b/contracts/teachlink/src/validation.rs index d031fed8..c3c20852 100644 --- a/contracts/teachlink/src/validation.rs +++ b/contracts/teachlink/src/validation.rs @@ -290,21 +290,8 @@ impl EscrowValidator { // Validate amount NumberValidator::validate_amount(amount).map_err(|_| EscrowError::AmountMustBePositive)?; - // Validate signers - NumberValidator::validate_signer_count(signers.len() as usize) - .map_err(|_| EscrowError::AtLeastOneSignerRequired)?; - - let mut total_weight: u32 = 0; - for signer in signers.iter() { - if signer.weight == 0 { - return Err(EscrowError::InvalidSignerThreshold); - } - total_weight += signer.weight; - } - - if threshold < 1 || threshold > total_weight { - return Err(EscrowError::InvalidSignerThreshold); - } + // Validate signers (duplicates, weights, threshold) + Self::validate_multisig(signers, threshold)?; // Validate time constraints if let (Some(release), Some(refund)) = (release_time, refund_time) { @@ -313,9 +300,6 @@ impl EscrowValidator { } } - // Check for duplicate signers - Self::check_duplicate_signers(signers)?; - Ok(()) } @@ -332,6 +316,34 @@ impl EscrowValidator { Ok(()) } + /// Validates multi-signature configuration: no duplicates, non-zero weights, + /// no weight overflow, and threshold within [1, total_weight]. + pub fn validate_multisig( + signers: &Vec, + threshold: u32, + ) -> Result<(), EscrowError> { + NumberValidator::validate_signer_count(signers.len() as usize) + .map_err(|_| EscrowError::AtLeastOneSignerRequired)?; + + Self::check_duplicate_signers(signers)?; + + let mut total_weight: u32 = 0; + for signer in signers.iter() { + if signer.weight == 0 { + return Err(EscrowError::InvalidSignerThreshold); + } + total_weight = total_weight + .checked_add(signer.weight) + .ok_or(EscrowError::InvalidSignerThreshold)?; + } + + if threshold < 1 || threshold > total_weight { + return Err(EscrowError::InvalidSignerThreshold); + } + + Ok(()) + } + /// Validates EscrowParameters struct (refactored from individual parameters) pub fn validate_escrow_parameters( env: &Env, @@ -350,22 +362,8 @@ impl EscrowValidator { NumberValidator::validate_amount(params.amount) .map_err(|_| EscrowError::AmountMustBePositive)?; - // Validate signers - NumberValidator::validate_signer_count(params.signers.len() as usize) - .map_err(|_| EscrowError::AtLeastOneSignerRequired)?; - - // Validate threshold against total signer weight - let mut total_weight: u32 = 0; - for signer in params.signers.iter() { - if signer.weight == 0 { - return Err(EscrowError::InvalidSignerThreshold); - } - total_weight += signer.weight; - } - - if params.threshold < 1 || params.threshold > total_weight { - return Err(EscrowError::InvalidSignerThreshold); - } + // Validate signers (duplicates, weights, threshold) + Self::validate_multisig(¶ms.signers, params.threshold)?; // Validate time constraints if let (Some(release), Some(refund)) = (params.release_time, params.refund_time) { @@ -374,9 +372,6 @@ impl EscrowValidator { } } - // Check for duplicate signers - Self::check_duplicate_signers(¶ms.signers)?; - // Additional validation: depositor must be different from beneficiary if params.depositor == params.beneficiary { return Err(EscrowError::DepositorCannotBeBeneficiary); diff --git a/contracts/teachlink/tests/test_validation.rs b/contracts/teachlink/tests/test_validation.rs index 6712fcf7..8e3625a2 100644 --- a/contracts/teachlink/tests/test_validation.rs +++ b/contracts/teachlink/tests/test_validation.rs @@ -687,3 +687,54 @@ fn test_config_constants() { assert!(config::MIN_TIMEOUT_SECONDS > 0); assert!(config::MAX_TIMEOUT_SECONDS > config::MIN_TIMEOUT_SECONDS); } + +#[test] +fn test_multisig_threshold_boundaries() { + let env = Env::default(); + + let make_signer = |w: u32| EscrowSigner { + address: Address::generate(&env), + role: EscrowRole::Primary, + weight: w, + }; + + // threshold == 1 (minimum) should pass + let mut signers = Vec::new(&env); + signers.push_back(make_signer(5)); + assert!(EscrowValidator::validate_multisig(&signers, 1).is_ok()); + + // threshold == total_weight (maximum) should pass + assert!(EscrowValidator::validate_multisig(&signers, 5).is_ok()); + + // threshold == 0 should fail + assert!(EscrowValidator::validate_multisig(&signers, 0).is_err()); + + // threshold > total_weight should fail + assert!(EscrowValidator::validate_multisig(&signers, 6).is_err()); + + // zero-weight signer should fail + let mut zero_weight = Vec::new(&env); + zero_weight.push_back(make_signer(0)); + assert!(EscrowValidator::validate_multisig(&zero_weight, 1).is_err()); + + // weight overflow should fail + let mut overflow_signers = Vec::new(&env); + overflow_signers.push_back(make_signer(u32::MAX)); + overflow_signers.push_back(make_signer(1)); + assert!(EscrowValidator::validate_multisig(&overflow_signers, 1).is_err()); + + // duplicate signers should fail + let dup = Address::generate(&env); + let mut dup_signers = Vec::new(&env); + dup_signers.push_back(EscrowSigner { address: dup.clone(), role: EscrowRole::Primary, weight: 1 }); + dup_signers.push_back(EscrowSigner { address: dup.clone(), role: EscrowRole::Primary, weight: 1 }); + assert!(EscrowValidator::validate_multisig(&dup_signers, 1).is_err()); + + // multi-signer weighted threshold at exact boundary + let mut multi = Vec::new(&env); + multi.push_back(make_signer(3)); + multi.push_back(make_signer(2)); + assert!(EscrowValidator::validate_multisig(&multi, 5).is_ok()); // threshold == total + assert!(EscrowValidator::validate_multisig(&multi, 1).is_ok()); // threshold == min + assert!(EscrowValidator::validate_multisig(&multi, 6).is_err()); // threshold > total +}