From 25ba1b9c33dcc66b31ed4f92513f4d2960825538 Mon Sep 17 00:00:00 2001 From: vicajohn Date: Fri, 26 Jun 2026 04:43:30 +0100 Subject: [PATCH] Harden signature verification and ownership checks --- Documents/signature-verification-audit.md | 46 ++++++++++ .../hello-world/src/autoshare_logic.rs | 41 ++++++++- .../hello-world/src/interfaces/autoshare.rs | 14 ++- contract/contracts/hello-world/src/lib.rs | 16 +++- .../hello-world/src/tests/autoshare_test.rs | 91 ++++++++++++++++++- .../hello-world/src/tests/pause_test.rs | 2 +- 6 files changed, 196 insertions(+), 14 deletions(-) create mode 100644 Documents/signature-verification-audit.md diff --git a/Documents/signature-verification-audit.md b/Documents/signature-verification-audit.md new file mode 100644 index 0000000..b0731f4 --- /dev/null +++ b/Documents/signature-verification-audit.md @@ -0,0 +1,46 @@ +# Signature Verification Audit + +## Scope + +This audit reviewed the contract-side auth workflow around AutoShare creation, membership changes, and usage reduction. + +## Findings + +### 1. `add_group_member` accepted unauthenticated calls + +The original implementation updated a group without requiring a signer check from the group owner. That meant any caller could add members to any group if they knew the group ID. + +Impact: +- Unauthorized membership changes +- Potential dilution or hijacking of payout splits +- Weak ownership enforcement for state-changing actions + +Fix: +- Added `caller.require_auth()` +- Verified the caller matches the stored group creator +- Rejected changes to inactive groups + +### 2. `reduce_usage` had no signer ownership gate + +The original implementation let any caller decrement remaining usages. That made the usage counter easy to exhaust or skew. + +Impact: +- Unauthorized usage depletion +- Possible denial of service for legitimate group owners +- No ownership binding between the action and the group + +Fix: +- Added `caller.require_auth()` +- Verified the caller matches the stored group creator +- Rejected reductions for inactive groups + +## Edge Cases Covered + +- Unauthorized caller attempts to modify group membership +- Unauthorized caller attempts to reduce usage +- Usage reduction when no usages remain +- Membership changes on inactive groups + +## Result + +The contract now enforces signer ownership on the mutating paths most relevant to creation and acknowledgment-style usage updates. The added tests cover the expected failure modes and the zero-usage edge case. diff --git a/contract/contracts/hello-world/src/autoshare_logic.rs b/contract/contracts/hello-world/src/autoshare_logic.rs index bcd78e3..0b8e3e4 100644 --- a/contract/contracts/hello-world/src/autoshare_logic.rs +++ b/contract/contracts/hello-world/src/autoshare_logic.rs @@ -172,9 +172,12 @@ pub fn get_group_members(env: Env, id: BytesN<32>) -> Result, E pub fn add_group_member( env: Env, id: BytesN<32>, + caller: Address, address: Address, percentage: u32, ) -> Result<(), Error> { + caller.require_auth(); + // Check if contract is paused if get_paused_status(&env) { return Err(Error::ContractPaused); @@ -187,6 +190,14 @@ pub fn add_group_member( .get(&key) .ok_or(Error::NotFound)?; + if details.creator != caller { + return Err(Error::Unauthorized); + } + + if !details.is_active { + return Err(Error::GroupInactive); + } + // Check if already a member for member in details.members.iter() { if member.address == address { @@ -194,9 +205,11 @@ pub fn add_group_member( } } + let member_address = address.clone(); + // Add new member details.members.push_back(GroupMember { - address, + address: member_address.clone(), percentage, }); @@ -205,6 +218,20 @@ pub fn add_group_member( // Save updated details env.storage().persistent().set(&key, &details); + + let members_key = DataKey::GroupMembers(id); + let mut stored_members: Vec = env + .storage() + .persistent() + .get(&members_key) + .unwrap_or(Vec::new(&env)); + stored_members.push_back(GroupMember { + address: member_address, + percentage, + }); + env.storage() + .persistent() + .set(&members_key, &stored_members); Ok(()) } @@ -546,7 +573,9 @@ pub fn get_total_usages_paid(env: Env, id: BytesN<32>) -> Result { Ok(details.total_usages_paid) } -pub fn reduce_usage(env: Env, id: BytesN<32>) -> Result<(), Error> { +pub fn reduce_usage(env: Env, id: BytesN<32>, caller: Address) -> Result<(), Error> { + caller.require_auth(); + let key = DataKey::AutoShare(id); let mut details: AutoShareDetails = env .storage() @@ -554,6 +583,14 @@ pub fn reduce_usage(env: Env, id: BytesN<32>) -> Result<(), Error> { .get(&key) .ok_or(Error::NotFound)?; + if details.creator != caller { + return Err(Error::Unauthorized); + } + + if !details.is_active { + return Err(Error::GroupInactive); + } + if details.usage_count == 0 { return Err(Error::NoUsagesRemaining); } diff --git a/contract/contracts/hello-world/src/interfaces/autoshare.rs b/contract/contracts/hello-world/src/interfaces/autoshare.rs index af766ba..e0b481c 100644 --- a/contract/contracts/hello-world/src/interfaces/autoshare.rs +++ b/contract/contracts/hello-world/src/interfaces/autoshare.rs @@ -69,7 +69,14 @@ pub trait AutoShareTrait { fn get_group_members(env: Env, id: BytesN<32>) -> Vec; /// Adds a member to a group with specified percentage. - fn add_group_member(env: Env, id: BytesN<32>, address: Address, percentage: u32); + /// Only the creator can call this. + fn add_group_member( + env: Env, + id: BytesN<32>, + caller: Address, + address: Address, + percentage: u32, + ); /// Deactivates a group. Only the creator can deactivate. fn deactivate_group(env: Env, id: BytesN<32>, caller: Address); @@ -139,6 +146,7 @@ pub trait AutoShareTrait { /// Returns the total usages paid for a group. fn get_total_usages_paid(env: Env, id: BytesN<32>) -> u32; - /// Reduces the usage count by 1 (dummy function for testing). - fn reduce_usage(env: Env, id: BytesN<32>); + /// Reduces the usage count by 1. + /// Only the creator can call this. + fn reduce_usage(env: Env, id: BytesN<32>, caller: Address); } diff --git a/contract/contracts/hello-world/src/lib.rs b/contract/contracts/hello-world/src/lib.rs index 9f3ca43..37b4c32 100644 --- a/contract/contracts/hello-world/src/lib.rs +++ b/contract/contracts/hello-world/src/lib.rs @@ -102,8 +102,14 @@ impl AutoShareContract { } /// Adds a member to a group with specified percentage. - pub fn add_group_member(env: Env, id: BytesN<32>, address: Address, percentage: u32) { - autoshare_logic::add_group_member(env, id, address, percentage).unwrap(); + pub fn add_group_member( + env: Env, + id: BytesN<32>, + caller: Address, + address: Address, + percentage: u32, + ) { + autoshare_logic::add_group_member(env, id, caller, address, percentage).unwrap(); } /// Deactivates a group. Only the creator can deactivate. @@ -223,9 +229,9 @@ impl AutoShareContract { autoshare_logic::get_total_usages_paid(env, id).unwrap() } - /// Reduces the usage count by 1 (dummy function for testing). - pub fn reduce_usage(env: Env, id: BytesN<32>) { - autoshare_logic::reduce_usage(env, id).unwrap(); + /// Reduces the usage count by 1. + pub fn reduce_usage(env: Env, id: BytesN<32>, caller: Address) { + autoshare_logic::reduce_usage(env, id, caller).unwrap(); } } diff --git a/contract/contracts/hello-world/src/tests/autoshare_test.rs b/contract/contracts/hello-world/src/tests/autoshare_test.rs index 317d91d..53f8549 100644 --- a/contract/contracts/hello-world/src/tests/autoshare_test.rs +++ b/contract/contracts/hello-world/src/tests/autoshare_test.rs @@ -602,6 +602,31 @@ fn test_add_group_member_success() { assert_eq!(final_members.get(2).unwrap().percentage, 34); } +#[test] +fn test_add_group_member_updates_member_index() { + let test_env = setup_test_env(); + let client = AutoShareContractClient::new(&test_env.env, &test_env.autoshare_contract); + + let creator = test_env.users.get(0).unwrap().clone(); + let id = BytesN::from_array(&test_env.env, &[7u8; 32]); + let name = String::from_str(&test_env.env, "Membership Index Test"); + let empty_members = Vec::new(&test_env.env); + + create_helper(&client, &id, &name, &creator, &empty_members, &test_env); + + let member1 = Address::generate(&test_env.env); + let member2 = Address::generate(&test_env.env); + + client.add_group_member(&id, &creator, &member1, &60); + client.add_group_member(&id, &creator, &member2, &40); + + assert!(client.is_group_member(&id, &member1)); + assert!(client.is_group_member(&id, &member2)); + + let members = client.get_group_members(&id); + assert_eq!(members.len(), 2); +} + #[test] #[should_panic] // AlreadyExists fn test_add_duplicate_member() { @@ -622,7 +647,7 @@ fn test_add_duplicate_member() { create_helper(&client, &id, &name, &creator, &members, &test_env); // Try to add the same member again - should fail - client.add_group_member(&id, &member1, &50); + client.add_group_member(&id, &creator, &member1, &50); } #[test] @@ -631,10 +656,11 @@ fn test_add_member_to_non_existent_group() { let test_env = setup_test_env(); let client = AutoShareContractClient::new(&test_env.env, &test_env.autoshare_contract); + let creator = test_env.users.get(0).unwrap().clone(); let id = BytesN::from_array(&test_env.env, &[99u8; 32]); let member = Address::generate(&test_env.env); - client.add_group_member(&id, &member, &50); + client.add_group_member(&id, &creator, &member, &50); } #[test] @@ -659,7 +685,7 @@ fn test_add_member_invalid_total_percentage() { // Try to add another member with 50% (total would be 150%) - should fail let member2 = Address::generate(&test_env.env); - client.add_group_member(&id, &member2, &50); + client.add_group_member(&id, &creator, &member2, &50); } #[test] @@ -1612,3 +1638,62 @@ fn test_create_group_with_payment() { assert_eq!(details.usage_count, usage_count); assert_eq!(details.total_usages_paid, usage_count); } + +#[test] +#[should_panic] +fn test_add_group_member_requires_creator_auth() { + let test_env = setup_test_env(); + let client = AutoShareContractClient::new(&test_env.env, &test_env.autoshare_contract); + + let creator = test_env.users.get(0).unwrap().clone(); + let attacker = test_env.users.get(1).unwrap().clone(); + let id = BytesN::from_array(&test_env.env, &[1u8; 32]); + let name = String::from_str(&test_env.env, "Auth Guard Test"); + + let member = Address::generate(&test_env.env); + let mut members = Vec::new(&test_env.env); + members.push_back(GroupMember { + address: member.clone(), + percentage: 100, + }); + + create_helper(&client, &id, &name, &creator, &members, &test_env); + + client.add_group_member(&id, &attacker, &Address::generate(&test_env.env), &1u32); +} + +#[test] +#[should_panic] +fn test_reduce_usage_rejects_unauthorized_caller() { + let test_env = setup_test_env(); + let client = AutoShareContractClient::new(&test_env.env, &test_env.autoshare_contract); + + let creator = test_env.users.get(0).unwrap().clone(); + let attacker = test_env.users.get(1).unwrap().clone(); + let token = test_env.mock_tokens.get(0).unwrap().clone(); + + let id = BytesN::from_array(&test_env.env, &[9u8; 32]); + let name = String::from_str(&test_env.env, "Usage Guard Test"); + crate::test_utils::mint_tokens(&test_env.env, &token, &creator, 10000000); + client.create(&id, &name, &creator, &2u32, &token); + + client.reduce_usage(&id, &attacker); +} + +#[test] +#[should_panic] +fn test_reduce_usage_fails_when_usage_exhausted() { + let test_env = setup_test_env(); + let client = AutoShareContractClient::new(&test_env.env, &test_env.autoshare_contract); + + let creator = test_env.users.get(0).unwrap().clone(); + let token = test_env.mock_tokens.get(0).unwrap().clone(); + + let id = BytesN::from_array(&test_env.env, &[8u8; 32]); + let name = String::from_str(&test_env.env, "Exhausted Usage Test"); + crate::test_utils::mint_tokens(&test_env.env, &token, &creator, 10000000); + client.create(&id, &name, &creator, &1u32, &token); + + client.reduce_usage(&id, &creator); + client.reduce_usage(&id, &creator); +} diff --git a/contract/contracts/hello-world/src/tests/pause_test.rs b/contract/contracts/hello-world/src/tests/pause_test.rs index 5005b07..60d6c94 100644 --- a/contract/contracts/hello-world/src/tests/pause_test.rs +++ b/contract/contracts/hello-world/src/tests/pause_test.rs @@ -181,7 +181,7 @@ fn test_add_member_fails_when_paused() { token_admin_client.mint(&creator, &10000000); client.create(&id, &name, &creator, &100u32, &token_address); client.pause(&admin); - client.add_group_member(&id, &member, &50u32); + client.add_group_member(&id, &creator, &member, &50u32); } #[test]