diff --git a/contracts/parameters-contract/src/errors.rs b/contracts/parameters-contract/src/errors.rs index 0159c8c..8e34f1f 100644 --- a/contracts/parameters-contract/src/errors.rs +++ b/contracts/parameters-contract/src/errors.rs @@ -8,4 +8,5 @@ pub enum ParametersError { NotAdmin = 2, InvalidParameters = 3, NotInitialized = 4, + ReentrancyDetected = 5, } diff --git a/contracts/parameters-contract/src/lib.rs b/contracts/parameters-contract/src/lib.rs index 6f16ce0..fa687c7 100644 --- a/contracts/parameters-contract/src/lib.rs +++ b/contracts/parameters-contract/src/lib.rs @@ -37,7 +37,9 @@ impl ParametersContract { pub fn upgrade(env: Env, new_wasm_hash: soroban_sdk::BytesN<32>) { let admin = storage::get_admin(&env).unwrap_or_else(|err| panic_with_error!(&env, err)); admin.require_auth(); + Self::enter_non_reentrant(&env); env.deployer().update_current_contract_wasm(new_wasm_hash); + Self::exit_non_reentrant(&env); } pub fn get_admin(env: Env) -> Result { storage::get_admin(&env) @@ -48,8 +50,10 @@ impl ParametersContract { old_admin.require_auth(); access::require_admin(&env, &old_admin); + Self::enter_non_reentrant(&env); storage::set_admin(&env, &new_admin); events::emit_admin_updated(&env, &old_admin, &new_admin); + Self::exit_non_reentrant(&env); } pub fn get_parameters(env: Env) -> Result { @@ -61,8 +65,10 @@ impl ParametersContract { access::require_admin(&env, &admin); Self::validate_parameters(&env, ¶ms); + Self::enter_non_reentrant(&env); storage::set_parameters(&env, ¶ms); events::emit_parameters_updated(&env, &admin, ¶ms); + Self::exit_non_reentrant(&env); } fn validate_parameters(env: &Env, params: &ProtocolParameters) { @@ -73,6 +79,19 @@ impl ParametersContract { panic_with_error!(env, ParametersError::InvalidParameters); } } + + fn enter_non_reentrant(env: &Env) { + if storage::is_reentrancy_locked(env) + .unwrap_or_else(|err| panic_with_error!(env, err)) + { + panic_with_error!(env, ParametersError::ReentrancyDetected); + } + storage::set_reentrancy_locked(env, true); + } + + fn exit_non_reentrant(env: &Env) { + storage::set_reentrancy_locked(env, false); + } } #[cfg(test)] diff --git a/contracts/parameters-contract/src/storage.rs b/contracts/parameters-contract/src/storage.rs index ad54818..c1a408c 100644 --- a/contracts/parameters-contract/src/storage.rs +++ b/contracts/parameters-contract/src/storage.rs @@ -5,6 +5,7 @@ use crate::types::ProtocolParameters; pub const ADMIN_KEY: Symbol = symbol_short!("ADMIN"); pub const PARAMS_KEY: Symbol = symbol_short!("PARAMS"); +pub const REENTRANCY_LOCK: Symbol = symbol_short!("LOCKED"); pub fn has_admin(env: &Env) -> bool { env.storage().instance().has(&ADMIN_KEY) @@ -31,3 +32,15 @@ pub fn get_parameters(env: &Env) -> Result pub fn set_parameters(env: &Env, params: &ProtocolParameters) { env.storage().instance().set(&PARAMS_KEY, params); } + +pub fn is_reentrancy_locked(env: &Env) -> Result { + Ok(env + .storage() + .instance() + .get(&REENTRANCY_LOCK) + .unwrap_or(false)) +} + +pub fn set_reentrancy_locked(env: &Env, locked: bool) { + env.storage().instance().set(&REENTRANCY_LOCK, &locked); +} diff --git a/contracts/parameters-contract/src/tests.rs b/contracts/parameters-contract/src/tests.rs index c31f68e..6580f47 100644 --- a/contracts/parameters-contract/src/tests.rs +++ b/contracts/parameters-contract/src/tests.rs @@ -2,7 +2,11 @@ use crate::{ default_parameters, ParametersContract, ParametersContractClient, ParametersError, ProtocolParameters, }; -use soroban_sdk::{testutils::Address as _, Address, Env}; +use soroban_sdk::{ + contract, contractimpl, + testutils::Address as _, + Address, Env, Symbol, +}; fn setup() -> (Env, ParametersContractClient<'static>, Address) { let env = Env::default(); diff --git a/contracts/reputation-contract/src/errors.rs b/contracts/reputation-contract/src/errors.rs index fe8bc42..9685212 100644 --- a/contracts/reputation-contract/src/errors.rs +++ b/contracts/reputation-contract/src/errors.rs @@ -11,4 +11,5 @@ pub enum ReputationError { Overflow = 4, Underflow = 5, NotInitialized = 6, + ReentrancyDetected = 7, } diff --git a/contracts/reputation-contract/src/lib.rs b/contracts/reputation-contract/src/lib.rs index 00575c1..112e97d 100644 --- a/contracts/reputation-contract/src/lib.rs +++ b/contracts/reputation-contract/src/lib.rs @@ -35,6 +35,8 @@ impl ReputationContract { updater.require_auth(); access::require_updater(&env, &updater); + Self::enter_non_reentrant(&env); + let old_score = storage::read_score(&env, &user) .unwrap_or_else(|err| soroban_sdk::panic_with_error!(&env, err)); let new_score = old_score @@ -50,6 +52,8 @@ impl ReputationContract { let reason = symbol_short!("increase"); events::emit_score_changed(&env, &user, old_score, new_score, &reason); + + Self::exit_non_reentrant(&env); } /// Decrease a user's reputation score by a given amount @@ -58,6 +62,8 @@ impl ReputationContract { updater.require_auth(); access::require_updater(&env, &updater); + Self::enter_non_reentrant(&env); + let old_score = storage::read_score(&env, &user) .unwrap_or_else(|err| soroban_sdk::panic_with_error!(&env, err)); let new_score = match old_score.checked_sub(amount) { @@ -69,6 +75,8 @@ impl ReputationContract { let reason = symbol_short!("decrease"); events::emit_score_changed(&env, &user, old_score, new_score, &reason); + + Self::exit_non_reentrant(&env); } /// Set a user's reputation score to a specific value @@ -81,12 +89,16 @@ impl ReputationContract { soroban_sdk::panic_with_error!(&env, ReputationError::OutOfBounds); } + Self::enter_non_reentrant(&env); + let old_score = storage::read_score(&env, &user) .unwrap_or_else(|err| soroban_sdk::panic_with_error!(&env, err)); storage::write_score(&env, &user, new_score); let reason = symbol_short!("set"); events::emit_score_changed(&env, &user, old_score, new_score, &reason); + + Self::exit_non_reentrant(&env); } /// Add a mentor vouching boost to a user's reputation score. @@ -95,6 +107,8 @@ impl ReputationContract { updater.require_auth(); access::require_updater(&env, &updater); + Self::enter_non_reentrant(&env); + let old_score = storage::read_score(&env, &user) .unwrap_or_else(|err| soroban_sdk::panic_with_error!(&env, err)); let new_score = old_score @@ -110,6 +124,8 @@ impl ReputationContract { let reason = symbol_short!("boost"); events::emit_score_changed(&env, &user, old_score, new_score, &reason); + + Self::exit_non_reentrant(&env); } /// Remove a mentor vouching boost from a user's reputation score. @@ -118,6 +134,8 @@ impl ReputationContract { updater.require_auth(); access::require_updater(&env, &updater); + Self::enter_non_reentrant(&env); + let old_score = storage::read_score(&env, &user) .unwrap_or_else(|err| soroban_sdk::panic_with_error!(&env, err)); let new_score = match old_score.checked_sub(amount) { @@ -129,6 +147,8 @@ impl ReputationContract { let reason = symbol_short!("unboost"); events::emit_score_changed(&env, &user, old_score, new_score, &reason); + + Self::exit_non_reentrant(&env); } /// Set or remove an address as an authorized updater @@ -137,8 +157,12 @@ impl ReputationContract { admin.require_auth(); access::require_admin(&env, &admin); + Self::enter_non_reentrant(&env); + storage::set_updater(&env, &updater, allowed); events::emit_updater_changed(&env, &updater, allowed); + + Self::exit_non_reentrant(&env); } /// Check if an address is an authorized updater @@ -156,8 +180,13 @@ impl ReputationContract { // Admin exists, require current admin authorization old_admin.require_auth(); access::require_admin(&env, &old_admin); + + Self::enter_non_reentrant(&env); + storage::set_admin(&env, &new_admin); events::emit_admin_changed(&env, &old_admin, &new_admin); + + Self::exit_non_reentrant(&env); } else { // No admin exists, allow setting (initialization) storage::set_admin(&env, &new_admin); @@ -171,11 +200,27 @@ impl ReputationContract { let admin = storage::get_admin(&env) .unwrap_or_else(|err| soroban_sdk::panic_with_error!(&env, err)); admin.require_auth(); + + Self::enter_non_reentrant(&env); env.deployer().update_current_contract_wasm(new_wasm_hash); + Self::exit_non_reentrant(&env); } pub fn get_admin(env: Env) -> Result { storage::get_admin(&env) } + + fn enter_non_reentrant(env: &Env) { + if storage::is_reentrancy_locked(env) + .unwrap_or_else(|err| soroban_sdk::panic_with_error!(env, err)) + { + soroban_sdk::panic_with_error!(env, ReputationError::ReentrancyDetected); + } + storage::set_reentrancy_locked(env, true); + } + + fn exit_non_reentrant(env: &Env) { + storage::set_reentrancy_locked(env, false); + } } #[cfg(test)] diff --git a/contracts/reputation-contract/src/storage.rs b/contracts/reputation-contract/src/storage.rs index 47824f4..3fa299c 100644 --- a/contracts/reputation-contract/src/storage.rs +++ b/contracts/reputation-contract/src/storage.rs @@ -6,6 +6,7 @@ use crate::errors::ReputationError; pub const ADMIN_KEY: Symbol = symbol_short!("ADMIN"); pub const UPDATERS_MAP: Symbol = symbol_short!("UPDATERS"); pub const SCORES_MAP: Symbol = symbol_short!("SCORES"); +pub const REENTRANCY_LOCK: Symbol = symbol_short!("LOCKED"); /// Get the admin address from storage pub fn get_admin(env: &Env) -> Result { @@ -70,3 +71,15 @@ pub fn set_updater(env: &Env, updater: &Address, allowed: bool) { env.storage().instance().set(&UPDATERS_MAP, &updaters); } + +pub fn is_reentrancy_locked(env: &Env) -> Result { + Ok(env + .storage() + .instance() + .get(&REENTRANCY_LOCK) + .unwrap_or(false)) +} + +pub fn set_reentrancy_locked(env: &Env, locked: bool) { + env.storage().instance().set(&REENTRANCY_LOCK, &locked); +} diff --git a/contracts/reputation-contract/src/tests.rs b/contracts/reputation-contract/src/tests.rs index cc8b5ad..1971a39 100644 --- a/contracts/reputation-contract/src/tests.rs +++ b/contracts/reputation-contract/src/tests.rs @@ -1030,6 +1030,258 @@ fn it_allows_zero_decrease_score() { assert_eq!(client.get_score(&user), 50); } +// ============================================================================ +// Reentrancy Guard Tests +// ============================================================================ + +#[test] +#[should_panic(expected = "Error(Contract, #7)")] +fn test_reentrancy_guard_rejects_reentrant_increase_score() { + let env = Env::default(); + env.mock_all_auths(); + + let contract_id = env.register(ReputationContract, ()); + let client = ReputationContractClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + client.set_admin(&admin); + + let updater = Address::generate(&env); + client.set_updater(&admin, &updater, &true); + + let user = Address::generate(&env); + client.set_score(&updater, &user, &50); + + // Lock the contract to simulate a reentrant call + env.as_contract(&contract_id, || { + env.storage() + .instance() + .set(&crate::storage::REENTRANCY_LOCK, &true); + }); + + client.increase_score(&updater, &user, &10); +} + +#[test] +#[should_panic(expected = "Error(Contract, #7)")] +fn test_reentrancy_guard_rejects_reentrant_decrease_score() { + let env = Env::default(); + env.mock_all_auths(); + + let contract_id = env.register(ReputationContract, ()); + let client = ReputationContractClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + client.set_admin(&admin); + + let updater = Address::generate(&env); + client.set_updater(&admin, &updater, &true); + + let user = Address::generate(&env); + client.set_score(&updater, &user, &50); + + env.as_contract(&contract_id, || { + env.storage() + .instance() + .set(&crate::storage::REENTRANCY_LOCK, &true); + }); + + client.decrease_score(&updater, &user, &10); +} + +#[test] +#[should_panic(expected = "Error(Contract, #7)")] +fn test_reentrancy_guard_rejects_reentrant_set_score() { + let env = Env::default(); + env.mock_all_auths(); + + let contract_id = env.register(ReputationContract, ()); + let client = ReputationContractClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + client.set_admin(&admin); + + let updater = Address::generate(&env); + client.set_updater(&admin, &updater, &true); + + let user = Address::generate(&env); + + env.as_contract(&contract_id, || { + env.storage() + .instance() + .set(&crate::storage::REENTRANCY_LOCK, &true); + }); + + client.set_score(&updater, &user, &50); +} + +#[test] +#[should_panic(expected = "Error(Contract, #7)")] +fn test_reentrancy_guard_rejects_reentrant_add_boost() { + let env = Env::default(); + env.mock_all_auths(); + + let contract_id = env.register(ReputationContract, ()); + let client = ReputationContractClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + client.set_admin(&admin); + + let updater = Address::generate(&env); + client.set_updater(&admin, &updater, &true); + + let user = Address::generate(&env); + client.set_score(&updater, &user, &40); + + env.as_contract(&contract_id, || { + env.storage() + .instance() + .set(&crate::storage::REENTRANCY_LOCK, &true); + }); + + client.add_boost(&updater, &user, &10); +} + +#[test] +#[should_panic(expected = "Error(Contract, #7)")] +fn test_reentrancy_guard_rejects_reentrant_remove_boost() { + let env = Env::default(); + env.mock_all_auths(); + + let contract_id = env.register(ReputationContract, ()); + let client = ReputationContractClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + client.set_admin(&admin); + + let updater = Address::generate(&env); + client.set_updater(&admin, &updater, &true); + + let user = Address::generate(&env); + client.set_score(&updater, &user, &50); + + env.as_contract(&contract_id, || { + env.storage() + .instance() + .set(&crate::storage::REENTRANCY_LOCK, &true); + }); + + client.remove_boost(&updater, &user, &10); +} + +#[test] +#[should_panic(expected = "Error(Contract, #7)")] +fn test_reentrancy_guard_rejects_reentrant_set_updater() { + let env = Env::default(); + env.mock_all_auths(); + + let contract_id = env.register(ReputationContract, ()); + let client = ReputationContractClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + client.set_admin(&admin); + + let updater = Address::generate(&env); + + env.as_contract(&contract_id, || { + env.storage() + .instance() + .set(&crate::storage::REENTRANCY_LOCK, &true); + }); + + client.set_updater(&admin, &updater, &true); +} + +#[test] +#[should_panic(expected = "Error(Contract, #7)")] +fn test_reentrancy_guard_rejects_reentrant_set_admin() { + let env = Env::default(); + env.mock_all_auths(); + + let contract_id = env.register(ReputationContract, ()); + let client = ReputationContractClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + client.set_admin(&admin); + + let new_admin = Address::generate(&env); + + env.as_contract(&contract_id, || { + env.storage() + .instance() + .set(&crate::storage::REENTRANCY_LOCK, &true); + }); + + client.set_admin(&new_admin); +} + +/// Test: Mutating functions succeed when NOT reentrant +#[test] +fn test_reentrancy_guard_allows_normal_operations() { + let env = Env::default(); + env.mock_all_auths(); + + let contract_id = env.register(ReputationContract, ()); + let client = ReputationContractClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + client.set_admin(&admin); + + let updater = Address::generate(&env); + client.set_updater(&admin, &updater, &true); + + let user = Address::generate(&env); + + // Normal operations should succeed + client.set_score(&updater, &user, &50); + assert_eq!(client.get_score(&user), 50); + + client.increase_score(&updater, &user, &10); + assert_eq!(client.get_score(&user), 60); + + client.decrease_score(&updater, &user, &5); + assert_eq!(client.get_score(&user), 55); + + client.add_boost(&updater, &user, &10); + assert_eq!(client.get_score(&user), 65); + + client.remove_boost(&updater, &user, &5); + assert_eq!(client.get_score(&user), 60); + + let new_admin = Address::generate(&env); + client.set_admin(&new_admin); + assert_eq!(client.get_admin(), new_admin); + + let new_updater = Address::generate(&env); + client.set_updater(&new_admin, &new_updater, &true); + assert!(client.is_updater(&new_updater)); +} + +/// Test: Guard is properly released after function completes +#[test] +fn test_reentrancy_guard_is_released_after_call() { + let env = Env::default(); + env.mock_all_auths(); + + let contract_id = env.register(ReputationContract, ()); + let client = ReputationContractClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + client.set_admin(&admin); + + let updater = Address::generate(&env); + client.set_updater(&admin, &updater, &true); + + let user = Address::generate(&env); + + // First call should succeed + client.set_score(&updater, &user, &50); + + // Lock should be released, second call should also succeed + client.increase_score(&updater, &user, &10); + assert_eq!(client.get_score(&user), 60); +} + /// Test: Allows setting score to current value #[test] fn it_allows_setting_score_to_current_value() { diff --git a/contracts/vendor-registry-contract/src/errors.rs b/contracts/vendor-registry-contract/src/errors.rs index cebbe84..330ad19 100644 --- a/contracts/vendor-registry-contract/src/errors.rs +++ b/contracts/vendor-registry-contract/src/errors.rs @@ -11,4 +11,5 @@ pub enum Error { InvalidName = 5, Unauthorized = 6, Overflow = 7, + ReentrancyDetected = 8, } diff --git a/contracts/vendor-registry-contract/src/lib.rs b/contracts/vendor-registry-contract/src/lib.rs index f22d5ed..7e5056c 100644 --- a/contracts/vendor-registry-contract/src/lib.rs +++ b/contracts/vendor-registry-contract/src/lib.rs @@ -32,6 +32,18 @@ impl VendorRegistryContract { Ok(()) } + fn check_non_reentrant(env: &Env) -> Result<(), Error> { + if storage::is_reentrancy_locked(env)? { + return Err(Error::ReentrancyDetected); + } + storage::set_reentrancy_locked(env, true); + Ok(()) + } + + fn exit_non_reentrant(env: &Env) { + storage::set_reentrancy_locked(env, false); + } + /// Registers a new vendor pub fn register_vendor( env: Env, @@ -53,6 +65,8 @@ impl VendorRegistryContract { return Err(Error::InvalidName); } + Self::check_non_reentrant(&env)?; + let info = VendorInfo { name: name.clone(), registration_date: env.ledger().timestamp(), @@ -64,6 +78,8 @@ impl VendorRegistryContract { storage::increment_vendor_count(&env)?; events::publish_vendor_registered(&env, vendor, name); + Self::exit_non_reentrant(&env); + Ok(()) } @@ -74,11 +90,16 @@ impl VendorRegistryContract { } access::require_admin(&env, &admin)?; + + Self::check_non_reentrant(&env)?; + let mut info = storage::get_vendor(&env, &vendor)?; info.active = false; storage::set_vendor(&env, &vendor, &info); events::publish_vendor_status(&env, vendor, false); + Self::exit_non_reentrant(&env); + Ok(()) } @@ -89,11 +110,16 @@ impl VendorRegistryContract { } access::require_admin(&env, &admin)?; + + Self::check_non_reentrant(&env)?; + let mut info = storage::get_vendor(&env, &vendor)?; info.active = true; storage::set_vendor(&env, &vendor, &info); events::publish_vendor_status(&env, vendor, true); + Self::exit_non_reentrant(&env); + Ok(()) } @@ -110,11 +136,16 @@ impl VendorRegistryContract { } access::require_admin(&env, &admin)?; + + Self::check_non_reentrant(&env)?; + let mut info = storage::get_vendor(&env, &vendor)?; info.active = active; storage::set_vendor(&env, &vendor, &info); events::publish_vendor_status(&env, vendor, active); + Self::exit_non_reentrant(&env); + Ok(()) } diff --git a/contracts/vendor-registry-contract/src/storage.rs b/contracts/vendor-registry-contract/src/storage.rs index 292eb18..b2b1ff7 100644 --- a/contracts/vendor-registry-contract/src/storage.rs +++ b/contracts/vendor-registry-contract/src/storage.rs @@ -58,6 +58,18 @@ pub fn increment_vendor_count(env: &Env) -> Result<(), Error> { Ok(()) } +pub fn is_reentrancy_locked(env: &Env) -> Result { + Ok(env + .storage() + .instance() + .get(&DataKey::Locked) + .unwrap_or(false)) +} + +pub fn set_reentrancy_locked(env: &Env, locked: bool) { + env.storage().instance().set(&DataKey::Locked, &locked); +} + fn extend_persistent_ttl(env: &Env, key: &DataKey) { env.storage() .persistent() diff --git a/contracts/vendor-registry-contract/src/tests.rs b/contracts/vendor-registry-contract/src/tests.rs index a80b5bb..f607eac 100644 --- a/contracts/vendor-registry-contract/src/tests.rs +++ b/contracts/vendor-registry-contract/src/tests.rs @@ -147,3 +147,137 @@ fn test_set_vendor_status() { let res = client.try_set_vendor_status(&fake_admin, &vendor, &false); assert!(res.is_err()); } + +// ============================================================================ +// Reentrancy Guard Tests +// ============================================================================ + +#[test] +fn test_reentrancy_guard_on_register_vendor() { + let env = Env::default(); + let contract_id = env.register(VendorRegistryContract, ()); + let client = VendorRegistryContractClient::new(&env, &contract_id); + let admin = Address::generate(&env); + let vendor = Address::generate(&env); + + client.initialize(&admin); + env.mock_all_auths(); + + // Lock the contract + env.as_contract(&contract_id, || { + env.storage() + .instance() + .set(&types::DataKey::Locked, &true); + }); + + let name = String::from_str(&env, "Test"); + let res = client.try_register_vendor(&admin, &vendor, &name); + assert_eq!(res, Err(Ok(Error::ReentrancyDetected))); +} + +#[test] +fn test_reentrancy_guard_on_deactivate_vendor() { + let env = Env::default(); + let contract_id = env.register(VendorRegistryContract, ()); + let client = VendorRegistryContractClient::new(&env, &contract_id); + let admin = Address::generate(&env); + let vendor = Address::generate(&env); + + client.initialize(&admin); + env.mock_all_auths(); + + let name = String::from_str(&env, "Test"); + client.register_vendor(&admin, &vendor, &name); + + env.as_contract(&contract_id, || { + env.storage() + .instance() + .set(&types::DataKey::Locked, &true); + }); + + let res = client.try_deactivate_vendor(&admin, &vendor); + assert_eq!(res, Err(Ok(Error::ReentrancyDetected))); +} + +#[test] +fn test_reentrancy_guard_on_activate_vendor() { + let env = Env::default(); + let contract_id = env.register(VendorRegistryContract, ()); + let client = VendorRegistryContractClient::new(&env, &contract_id); + let admin = Address::generate(&env); + let vendor = Address::generate(&env); + + client.initialize(&admin); + env.mock_all_auths(); + + let name = String::from_str(&env, "Test"); + client.register_vendor(&admin, &vendor, &name); + client.deactivate_vendor(&admin, &vendor); + + env.as_contract(&contract_id, || { + env.storage() + .instance() + .set(&types::DataKey::Locked, &true); + }); + + let res = client.try_activate_vendor(&admin, &vendor); + assert_eq!(res, Err(Ok(Error::ReentrancyDetected))); +} + +#[test] +fn test_reentrancy_guard_on_set_vendor_status() { + let env = Env::default(); + let contract_id = env.register(VendorRegistryContract, ()); + let client = VendorRegistryContractClient::new(&env, &contract_id); + let admin = Address::generate(&env); + let vendor = Address::generate(&env); + + client.initialize(&admin); + env.mock_all_auths(); + + let name = String::from_str(&env, "Test"); + client.register_vendor(&admin, &vendor, &name); + + env.as_contract(&contract_id, || { + env.storage() + .instance() + .set(&types::DataKey::Locked, &true); + }); + + let res = client.try_set_vendor_status(&admin, &vendor, &false); + assert_eq!(res, Err(Ok(Error::ReentrancyDetected))); +} + +#[test] +fn test_reentrancy_guard_allows_normal_operations() { + let env = Env::default(); + let (client, admin, vendor) = setup(&env); + env.mock_all_auths(); + + // Normal operations should still succeed + let name = String::from_str(&env, "Test"); + assert!(client.register_vendor(&admin, &vendor, &name).is_ok()); + + assert!(client.deactivate_vendor(&admin, &vendor).is_ok()); + + assert!(client.activate_vendor(&admin, &vendor).is_ok()); + + assert!(client + .set_vendor_status(&admin, &vendor, &false) + .is_ok()); +} + +#[test] +fn test_reentrancy_guard_is_released_after_call() { + let env = Env::default(); + let (client, admin, vendor) = setup(&env); + env.mock_all_auths(); + + // First call should succeed + let name = String::from_str(&env, "Test"); + assert!(client.register_vendor(&admin, &vendor, &name).is_ok()); + + // Lock should be released, second call should also succeed + assert!(client.deactivate_vendor(&admin, &vendor).is_ok()); + assert!(client.activate_vendor(&admin, &vendor).is_ok()); +} diff --git a/contracts/vendor-registry-contract/src/types.rs b/contracts/vendor-registry-contract/src/types.rs index 08e60e0..a58ffc6 100644 --- a/contracts/vendor-registry-contract/src/types.rs +++ b/contracts/vendor-registry-contract/src/types.rs @@ -5,6 +5,7 @@ use soroban_sdk::{contracttype, Address, String}; pub enum DataKey { // Instance storage Admin, + Locked, // Persistent storage Vendor(Address), diff --git a/contracts/vouching-contract/src/errors.rs b/contracts/vouching-contract/src/errors.rs index 5d555e2..9c19a7b 100644 --- a/contracts/vouching-contract/src/errors.rs +++ b/contracts/vouching-contract/src/errors.rs @@ -13,4 +13,5 @@ pub enum VouchingError { VouchNotActive = 7, InvalidBoost = 8, ReputationCallFailed = 9, + ReentrancyDetected = 10, } diff --git a/contracts/vouching-contract/src/lib.rs b/contracts/vouching-contract/src/lib.rs index df9a23c..985e375 100644 --- a/contracts/vouching-contract/src/lib.rs +++ b/contracts/vouching-contract/src/lib.rs @@ -34,6 +34,18 @@ impl VouchingContract { storage::set_vouch_boost(&env, vouch_boost); } + pub fn set_mentor(env: Env, admin: Address, mentor: Address, verified: bool) { + admin.require_auth(); + Self::require_admin(&env, &admin); + + Self::enter_non_reentrant(&env); + + storage::set_mentor(&env, &mentor, verified); + events::emit_mentor_verified(&env, &mentor, verified); + + Self::exit_non_reentrant(&env); + } + pub fn vouch(env: Env, mentor: Address, learner: Address) { mentor.require_auth(); @@ -48,6 +60,8 @@ impl VouchingContract { } } + Self::enter_non_reentrant(&env); + let boost_amount = storage::get_vouch_boost(&env).unwrap_or_else(|err| panic_with_error!(&env, err)); let record = VouchRecord { @@ -62,6 +76,8 @@ impl VouchingContract { storage::add_learner_mentor(&env, &learner, &mentor); Self::add_reputation_boost(&env, &learner, boost_amount); events::emit_mentor_vouched(&env, &mentor, &learner, boost_amount); + + Self::exit_non_reentrant(&env); } pub fn revoke_vouch(env: Env, mentor: Address, learner: Address) { @@ -73,10 +89,14 @@ impl VouchingContract { panic_with_error!(&env, VouchingError::VouchNotActive); } + Self::enter_non_reentrant(&env); + Self::remove_reputation_boost(&env, &learner, record.boost_amount); record.active = false; storage::set_vouch(&env, &record); events::emit_vouch_revoked(&env, &mentor, &learner, record.boost_amount); + + Self::exit_non_reentrant(&env); } pub fn get_vouches(env: Env, learner: Address) -> Vec { @@ -92,14 +112,6 @@ impl VouchingContract { records } - pub fn set_mentor(env: Env, admin: Address, mentor: Address, verified: bool) { - admin.require_auth(); - Self::require_admin(&env, &admin); - - storage::set_mentor(&env, &mentor, verified); - events::emit_mentor_verified(&env, &mentor, verified); - } - pub fn get_admin(env: Env) -> Result { storage::get_admin(&env) } @@ -174,6 +186,19 @@ impl VouchingContract { panic_with_error!(env, VouchingError::NotAdmin); } } + + fn enter_non_reentrant(env: &Env) { + if storage::is_reentrancy_locked(env) + .unwrap_or_else(|err| panic_with_error!(env, err)) + { + panic_with_error!(env, VouchingError::ReentrancyDetected); + } + storage::set_reentrancy_locked(env, true); + } + + fn exit_non_reentrant(env: &Env) { + storage::set_reentrancy_locked(env, false); + } } #[cfg(test)] diff --git a/contracts/vouching-contract/src/storage.rs b/contracts/vouching-contract/src/storage.rs index 5b41fad..7462cc4 100644 --- a/contracts/vouching-contract/src/storage.rs +++ b/contracts/vouching-contract/src/storage.rs @@ -96,6 +96,18 @@ pub fn add_learner_mentor(env: &Env, learner: &Address, mentor: &Address) { } } +pub fn is_reentrancy_locked(env: &Env) -> Result { + Ok(env + .storage() + .instance() + .get(&DataKey::Locked) + .unwrap_or(false)) +} + +pub fn set_reentrancy_locked(env: &Env, locked: bool) { + env.storage().instance().set(&DataKey::Locked, &locked); +} + pub fn extend_vouch_ttl(env: &Env, mentor: &Address, learner: &Address) { extend_persistent_ttl(env, &DataKey::Vouch(mentor.clone(), learner.clone())); } diff --git a/contracts/vouching-contract/src/tests.rs b/contracts/vouching-contract/src/tests.rs index acd224b..987453d 100644 --- a/contracts/vouching-contract/src/tests.rs +++ b/contracts/vouching-contract/src/tests.rs @@ -211,3 +211,137 @@ fn assert_event(env: &Env, expected: Symbol) { panic!("expected event was not emitted"); } + +// ============================================================================ +// Reentrancy Guard Tests +// ============================================================================ + +#[test] +#[should_panic(expected = "Error(Contract, #10)")] +fn test_reentrancy_guard_on_vouch() { + let env = Env::default(); + env.mock_all_auths(); + + let reputation = env.register(MockReputationContract, ()); + let contract_id = env.register(VouchingContract, ()); + let client = VouchingContractClient::new(&env, &contract_id); + let admin = Address::generate(&env); + let mentor = Address::generate(&env); + let learner = Address::generate(&env); + + client.initialize(&admin, &reputation, &DEFAULT_VOUCH_BOOST); + client.set_mentor(&admin, &mentor, &true); + + env.as_contract(&contract_id, || { + env.storage() + .instance() + .set(&crate::types::DataKey::Locked, &true); + }); + + client.vouch(&mentor, &learner); +} + +#[test] +#[should_panic(expected = "Error(Contract, #10)")] +fn test_reentrancy_guard_on_revoke_vouch() { + let env = Env::default(); + env.mock_all_auths(); + + let reputation = env.register(MockReputationContract, ()); + let contract_id = env.register(VouchingContract, ()); + let client = VouchingContractClient::new(&env, &contract_id); + let admin = Address::generate(&env); + let mentor = Address::generate(&env); + let learner = Address::generate(&env); + + client.initialize(&admin, &reputation, &DEFAULT_VOUCH_BOOST); + client.set_mentor(&admin, &mentor, &true); + client.vouch(&mentor, &learner); + + env.as_contract(&contract_id, || { + env.storage() + .instance() + .set(&crate::types::DataKey::Locked, &true); + }); + + client.revoke_vouch(&mentor, &learner); +} + +#[test] +#[should_panic(expected = "Error(Contract, #10)")] +fn test_reentrancy_guard_on_set_mentor() { + let env = Env::default(); + env.mock_all_auths(); + + let reputation = env.register(MockReputationContract, ()); + let contract_id = env.register(VouchingContract, ()); + let client = VouchingContractClient::new(&env, &contract_id); + let admin = Address::generate(&env); + let mentor = Address::generate(&env); + + client.initialize(&admin, &reputation, &DEFAULT_VOUCH_BOOST); + + env.as_contract(&contract_id, || { + env.storage() + .instance() + .set(&crate::types::DataKey::Locked, &true); + }); + + client.set_mentor(&admin, &mentor, &true); +} + +#[test] +fn test_reentrancy_guard_allows_normal_operations() { + let env = Env::default(); + env.mock_all_auths(); + + let reputation = env.register(MockReputationContract, ()); + let contract_id = env.register(VouchingContract, ()); + let client = VouchingContractClient::new(&env, &contract_id); + let admin = Address::generate(&env); + let mentor = Address::generate(&env); + let learner = Address::generate(&env); + + client.initialize(&admin, &reputation, &DEFAULT_VOUCH_BOOST); + client.set_mentor(&admin, &mentor, &true); + client.vouch(&mentor, &learner); + + let record = client.get_vouches(&learner).get_unchecked(0); + assert!(record.active); + + // Normal operations after unlocking should work + let mentor2 = Address::generate(&env); + client.set_mentor(&admin, &mentor2, &true); + assert!(client.is_mentor(&mentor2)); + + client.revoke_vouch(&mentor, &learner); + let record = client.get_vouches(&learner).get_unchecked(0); + assert!(!record.active); +} + +#[test] +fn test_reentrancy_guard_is_released_after_call() { + let env = Env::default(); + env.mock_all_auths(); + + let reputation = env.register(MockReputationContract, ()); + let contract_id = env.register(VouchingContract, ()); + let client = VouchingContractClient::new(&env, &contract_id); + let admin = Address::generate(&env); + let mentor = Address::generate(&env); + let learner = Address::generate(&env); + + client.initialize(&admin, &reputation, &DEFAULT_VOUCH_BOOST); + client.set_mentor(&admin, &mentor, &true); + + // First call should succeed + client.vouch(&mentor, &learner); + + // Lock should be released, second call should also succeed + let learner2 = Address::generate(&env); + // Can't vouch same pair, so create a new learner + let mentor2 = Address::generate(&env); + client.set_mentor(&admin, &mentor2, &true); + client.vouch(&mentor2, &learner2); + assert_eq!(client.get_vouches(&learner2).len(), 1); +} diff --git a/contracts/vouching-contract/src/types.rs b/contracts/vouching-contract/src/types.rs index 7eda96e..0d70929 100644 --- a/contracts/vouching-contract/src/types.rs +++ b/contracts/vouching-contract/src/types.rs @@ -18,6 +18,7 @@ pub enum DataKey { Admin, ReputationContract, VouchBoost, + Locked, Mentor(Address), Vouch(Address, Address), LearnerVouches(Address),