diff --git a/contract/contracts/agent_registry/src/agent.rs b/contract/contracts/agent_registry/src/agent.rs index 7ea529b8..6b70d78c 100644 --- a/contract/contracts/agent_registry/src/agent.rs +++ b/contract/contracts/agent_registry/src/agent.rs @@ -118,6 +118,11 @@ pub fn rate_agent( rater.require_auth(); + // An agent can never rate itself, even if it somehow appears as a party. + if rater == agent { + return Err(AgentError::SelfRatingNotAllowed); + } + if !(1..=5).contains(&score) { return Err(AgentError::InvalidRatingScore); } @@ -197,6 +202,7 @@ pub fn get_agent_count(env: &Env) -> u32 { pub fn register_transaction( env: &Env, + caller: Address, transaction_id: String, agent: Address, parties: Vec
, @@ -207,11 +213,51 @@ pub fn register_transaction( check_paused(env)?; + // Authenticate the caller. Without this, anyone could fabricate transaction + // records naming an arbitrary agent and parties. + caller.require_auth(); + let agent_key = DataKey::Agent(agent.clone()); if !env.storage().persistent().has(&agent_key) { return Err(AgentError::AgentNotFound); } + // Validate the parties list: it must be non-empty, all entries distinct, and + // none of them may be the agent. Parties are the counterparties who can later + // rate the agent, so keeping the agent out of this list forecloses self-rating + // at the source. + let len = parties.len(); + if len == 0 { + return Err(AgentError::InvalidParties); + } + for i in 0..len { + let party = parties.get(i).unwrap(); + if party == agent { + return Err(AgentError::InvalidParties); + } + for j in (i + 1)..len { + if party == parties.get(j).unwrap() { + return Err(AgentError::InvalidParties); + } + } + } + + // The caller must be a genuine participant of the transaction: either the + // agent itself or one of the listed parties. This stops a third party from + // manufacturing reputation-bearing records for others. + let mut caller_is_participant = caller == agent; + if !caller_is_participant { + for party in parties.iter() { + if party == caller { + caller_is_participant = true; + break; + } + } + } + if !caller_is_participant { + return Err(AgentError::NotTransactionParty); + } + let txn_key = DataKey::Transaction(transaction_id.clone()); let transaction = AgentTransaction { @@ -253,6 +299,17 @@ pub fn complete_transaction( return Err(AgentError::Unauthorized); } + // Only the agent named on the transaction may mark it complete. Without this + // auth check anyone could complete on the agent's behalf and inflate the + // agent's completed_agreements counter. + agent.require_auth(); + + // Guard against double-completion, which would otherwise bump + // completed_agreements more than once for a single transaction. + if transaction.completed { + return Err(AgentError::TransactionAlreadyCompleted); + } + transaction.completed = true; env.storage().persistent().set(&txn_key, &transaction); diff --git a/contract/contracts/agent_registry/src/errors.rs b/contract/contracts/agent_registry/src/errors.rs index 640ad094..f5b9d600 100644 --- a/contract/contracts/agent_registry/src/errors.rs +++ b/contract/contracts/agent_registry/src/errors.rs @@ -22,4 +22,7 @@ pub enum AgentError { NotPaused = 16, NoPendingAdmin = 17, NotPendingAdmin = 18, + SelfRatingNotAllowed = 19, + InvalidParties = 20, + TransactionAlreadyCompleted = 21, } diff --git a/contract/contracts/agent_registry/src/lib.rs b/contract/contracts/agent_registry/src/lib.rs index 7909128e..d6659200 100644 --- a/contract/contracts/agent_registry/src/lib.rs +++ b/contract/contracts/agent_registry/src/lib.rs @@ -113,6 +113,7 @@ impl AgentRegistryContract { /// * `TransactionNotFound` - If the transaction doesn't exist /// * `TransactionNotCompleted` - If the transaction is not marked as completed /// * `NotTransactionParty` - If the rater wasn't part of the transaction + /// * `SelfRatingNotAllowed` - If the rater is the agent being rated /// * `AlreadyRated` - If the rater has already rated this agent pub fn rate_agent( env: Env, @@ -147,6 +148,8 @@ impl AgentRegistryContract { /// This is called when a rent agreement or property transaction is created. /// /// # Arguments + /// * `caller` - The participant registering the transaction; must be the + /// agent or one of the parties and must authorize the call /// * `transaction_id` - Unique identifier for the transaction /// * `agent` - The agent involved in the transaction /// * `parties` - Vector of addresses involved (tenant, landlord, etc.) @@ -154,13 +157,16 @@ impl AgentRegistryContract { /// # Errors /// * `NotInitialized` - If the contract hasn't been initialized /// * `AgentNotFound` - If the agent doesn't exist + /// * `InvalidParties` - If parties is empty, has duplicates, or includes the agent + /// * `NotTransactionParty` - If the caller is neither the agent nor a party pub fn register_transaction( env: Env, + caller: Address, transaction_id: String, agent: Address, parties: Vec, ) -> Result<(), AgentError> { - agent::register_transaction(&env, transaction_id, agent, parties) + agent::register_transaction(&env, caller, transaction_id, agent, parties) } /// Mark a transaction as completed. @@ -174,6 +180,7 @@ impl AgentRegistryContract { /// * `NotInitialized` - If the contract hasn't been initialized /// * `TransactionNotFound` - If the transaction doesn't exist /// * `Unauthorized` - If the caller is not the agent for this transaction + /// * `TransactionAlreadyCompleted` - If the transaction was already completed pub fn complete_transaction( env: Env, transaction_id: String, diff --git a/contract/contracts/agent_registry/src/tests.rs b/contract/contracts/agent_registry/src/tests.rs index 6aa6be57..4c49d0c1 100644 --- a/contract/contracts/agent_registry/src/tests.rs +++ b/contract/contracts/agent_registry/src/tests.rs @@ -268,7 +268,7 @@ fn test_register_and_complete_transaction() { let txn_id = String::from_str(&env, "TXN-001"); let parties = vec![&env, tenant.clone(), landlord.clone()]; - let result = client.try_register_transaction(&txn_id, &agent, &parties); + let result = client.try_register_transaction(&agent, &txn_id, &agent, &parties); assert!(result.is_ok()); let result = client.try_complete_transaction(&txn_id, &agent); @@ -299,7 +299,7 @@ fn test_rate_agent_success() { let txn_id = String::from_str(&env, "TXN-001"); let parties = vec![&env, tenant.clone(), landlord.clone()]; - client.register_transaction(&txn_id, &agent, &parties); + client.register_transaction(&agent, &txn_id, &agent, &parties); client.complete_transaction(&txn_id, &agent); let result = client.try_rate_agent(&tenant, &agent, &5, &txn_id); @@ -332,7 +332,7 @@ fn test_multiple_ratings_average() { let txn_id = String::from_str(&env, "TXN-001"); let parties = vec![&env, tenant.clone(), landlord.clone()]; - client.register_transaction(&txn_id, &agent, &parties); + client.register_transaction(&agent, &txn_id, &agent, &parties); client.complete_transaction(&txn_id, &agent); client.rate_agent(&tenant, &agent, &5, &txn_id); @@ -366,7 +366,7 @@ fn test_rate_agent_fails_with_invalid_score_low() { let txn_id = String::from_str(&env, "TXN-001"); let parties = vec![&env, tenant.clone(), landlord.clone()]; - client.register_transaction(&txn_id, &agent, &parties); + client.register_transaction(&agent, &txn_id, &agent, &parties); client.complete_transaction(&txn_id, &agent); client.rate_agent(&tenant, &agent, &0, &txn_id); @@ -394,7 +394,7 @@ fn test_rate_agent_fails_with_invalid_score_high() { let txn_id = String::from_str(&env, "TXN-001"); let parties = vec![&env, tenant.clone(), landlord.clone()]; - client.register_transaction(&txn_id, &agent, &parties); + client.register_transaction(&agent, &txn_id, &agent, &parties); client.complete_transaction(&txn_id, &agent); client.rate_agent(&tenant, &agent, &6, &txn_id); @@ -421,7 +421,7 @@ fn test_rate_agent_fails_when_agent_not_verified() { let txn_id = String::from_str(&env, "TXN-001"); let parties = vec![&env, tenant.clone(), landlord.clone()]; - client.register_transaction(&txn_id, &agent, &parties); + client.register_transaction(&agent, &txn_id, &agent, &parties); client.complete_transaction(&txn_id, &agent); client.rate_agent(&tenant, &agent, &5, &txn_id); @@ -472,7 +472,7 @@ fn test_rate_agent_fails_when_transaction_not_completed() { let txn_id = String::from_str(&env, "TXN-001"); let parties = vec![&env, tenant.clone(), landlord.clone()]; - client.register_transaction(&txn_id, &agent, &parties); + client.register_transaction(&agent, &txn_id, &agent, &parties); client.rate_agent(&tenant, &agent, &5, &txn_id); } @@ -500,7 +500,7 @@ fn test_rate_agent_fails_when_not_transaction_party() { let txn_id = String::from_str(&env, "TXN-001"); let parties = vec![&env, tenant.clone(), landlord.clone()]; - client.register_transaction(&txn_id, &agent, &parties); + client.register_transaction(&agent, &txn_id, &agent, &parties); client.complete_transaction(&txn_id, &agent); client.rate_agent(&stranger, &agent, &5, &txn_id); @@ -528,9 +528,198 @@ fn test_rate_agent_fails_when_already_rated() { let txn_id = String::from_str(&env, "TXN-001"); let parties = vec![&env, tenant.clone(), landlord.clone()]; - client.register_transaction(&txn_id, &agent, &parties); + client.register_transaction(&agent, &txn_id, &agent, &parties); client.complete_transaction(&txn_id, &agent); client.rate_agent(&tenant, &agent, &5, &txn_id); client.rate_agent(&tenant, &agent, &4, &txn_id); } + +// --------------------------------------------------------------------------- +// Security regression tests for issue #77: register_transaction / +// complete_transaction were unauthenticated, letting reputation be fabricated. +// --------------------------------------------------------------------------- + +#[test] +#[should_panic] +fn test_register_transaction_requires_caller_auth() { + let env = Env::default(); + let client = create_contract(&env); + + let admin = Address::generate(&env); + let agent = Address::generate(&env); + let tenant = Address::generate(&env); + let landlord = Address::generate(&env); + + env.mock_all_auths(); + client.initialize(&admin); + let profile_hash = String::from_str(&env, "QmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco"); + client.register_agent(&agent, &profile_hash); + + let txn_id = String::from_str(&env, "TXN-001"); + let parties = vec![&env, tenant.clone(), landlord.clone()]; + + // No mocked auths: a caller that does not authorize the call cannot register. + env.mock_auths(&[]); + client.register_transaction(&tenant, &txn_id, &agent, &parties); +} + +#[test] +#[should_panic(expected = "Error(Contract, #12)")] +fn test_register_transaction_rejects_non_participant_caller() { + let env = Env::default(); + let client = create_contract(&env); + + let admin = Address::generate(&env); + let agent = Address::generate(&env); + let tenant = Address::generate(&env); + let landlord = Address::generate(&env); + let stranger = Address::generate(&env); + + env.mock_all_auths(); + client.initialize(&admin); + let profile_hash = String::from_str(&env, "QmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco"); + client.register_agent(&agent, &profile_hash); + + let txn_id = String::from_str(&env, "TXN-001"); + let parties = vec![&env, tenant.clone(), landlord.clone()]; + + // Stranger is neither the agent nor a listed party. + client.register_transaction(&stranger, &txn_id, &agent, &parties); +} + +#[test] +fn test_register_transaction_allows_party_caller() { + let env = Env::default(); + let client = create_contract(&env); + + let admin = Address::generate(&env); + let agent = Address::generate(&env); + let tenant = Address::generate(&env); + let landlord = Address::generate(&env); + + env.mock_all_auths(); + client.initialize(&admin); + let profile_hash = String::from_str(&env, "QmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco"); + client.register_agent(&agent, &profile_hash); + + let txn_id = String::from_str(&env, "TXN-001"); + let parties = vec![&env, tenant.clone(), landlord.clone()]; + + // A genuine party (not the agent) may register the transaction. + let result = client.try_register_transaction(&tenant, &txn_id, &agent, &parties); + assert!(result.is_ok()); +} + +#[test] +#[should_panic(expected = "Error(Contract, #20)")] +fn test_register_transaction_rejects_agent_in_parties() { + let env = Env::default(); + let client = create_contract(&env); + + let admin = Address::generate(&env); + let agent = Address::generate(&env); + let tenant = Address::generate(&env); + + env.mock_all_auths(); + client.initialize(&admin); + let profile_hash = String::from_str(&env, "QmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco"); + client.register_agent(&agent, &profile_hash); + + let txn_id = String::from_str(&env, "TXN-001"); + // Agent listed as its own party would enable self-rating; must be rejected. + let parties = vec![&env, tenant.clone(), agent.clone()]; + + client.register_transaction(&agent, &txn_id, &agent, &parties); +} + +#[test] +#[should_panic(expected = "Error(Contract, #20)")] +fn test_register_transaction_rejects_duplicate_parties() { + let env = Env::default(); + let client = create_contract(&env); + + let admin = Address::generate(&env); + let agent = Address::generate(&env); + let tenant = Address::generate(&env); + + env.mock_all_auths(); + client.initialize(&admin); + let profile_hash = String::from_str(&env, "QmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco"); + client.register_agent(&agent, &profile_hash); + + let txn_id = String::from_str(&env, "TXN-001"); + let parties = vec![&env, tenant.clone(), tenant.clone()]; + + client.register_transaction(&agent, &txn_id, &agent, &parties); +} + +#[test] +#[should_panic(expected = "Error(Contract, #20)")] +fn test_register_transaction_rejects_empty_parties() { + let env = Env::default(); + let client = create_contract(&env); + + let admin = Address::generate(&env); + let agent = Address::generate(&env); + + env.mock_all_auths(); + client.initialize(&admin); + let profile_hash = String::from_str(&env, "QmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco"); + client.register_agent(&agent, &profile_hash); + + let txn_id = String::from_str(&env, "TXN-001"); + let parties = vec![&env]; + + client.register_transaction(&agent, &txn_id, &agent, &parties); +} + +#[test] +#[should_panic] +fn test_complete_transaction_requires_agent_auth() { + let env = Env::default(); + let client = create_contract(&env); + + let admin = Address::generate(&env); + let agent = Address::generate(&env); + let tenant = Address::generate(&env); + let landlord = Address::generate(&env); + + env.mock_all_auths(); + client.initialize(&admin); + let profile_hash = String::from_str(&env, "QmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco"); + client.register_agent(&agent, &profile_hash); + + let txn_id = String::from_str(&env, "TXN-001"); + let parties = vec![&env, tenant.clone(), landlord.clone()]; + client.register_transaction(&agent, &txn_id, &agent, &parties); + + // Without the agent's authorization, the transaction cannot be completed. + env.mock_auths(&[]); + client.complete_transaction(&txn_id, &agent); +} + +#[test] +#[should_panic(expected = "Error(Contract, #21)")] +fn test_complete_transaction_rejects_double_completion() { + let env = Env::default(); + let client = create_contract(&env); + + let admin = Address::generate(&env); + let agent = Address::generate(&env); + let tenant = Address::generate(&env); + let landlord = Address::generate(&env); + + env.mock_all_auths(); + client.initialize(&admin); + let profile_hash = String::from_str(&env, "QmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco"); + client.register_agent(&agent, &profile_hash); + + let txn_id = String::from_str(&env, "TXN-001"); + let parties = vec![&env, tenant.clone(), landlord.clone()]; + client.register_transaction(&agent, &txn_id, &agent, &parties); + + client.complete_transaction(&txn_id, &agent); + // Completing again would otherwise inflate completed_agreements a second time. + client.complete_transaction(&txn_id, &agent); +}