From 8b1bb317943f5975da46b207257d489687820103 Mon Sep 17 00:00:00 2001 From: JuliobaCR Date: Sun, 28 Jun 2026 09:47:17 -0600 Subject: [PATCH] fix: reject duplicate member addresses across forms and pool contracts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit initialize() on the rotational/target/flexible pools had no uniqueness check on the members list (only add_member did), so a duplicate Stellar address could be stored twice. For Rotational this lets one address claim two round-robin payout slots and inflates trigger_payout's deposit_count (since it's tallied per-slot against a shared HasDeposited(address) flag), overpaying the round beneficiary beyond what was actually deposited. For Flexible, distribute_yield iterates the raw members vec, so a duplicate slot grants that address two yield shares per distribution. The reputation tracker keys everything by address so it has no duplicate-storage bug itself, but it has no idempotency guard either — Rotational's per-slot report_missed_round/report_payout calls would double-count for a duplicated address, so closing the gap at initialize fixes that too. Add a has_duplicate_members guard to initialize on all three pool types, mirroring add_member's existing check. Add client-side duplicate detection (findDuplicateAddresses) wired into all three creation forms and BulkImport.tsx so duplicates show a clear inline error and block submission before they ever reach the chain. Closes #143 --- .../components/create-group/BulkImport.tsx | 7 ++++ .../components/create-group/flexible-form.tsx | 17 ++++++--- .../create-group/rotational-form.tsx | 23 +++++------ .../components/create-group/target-form.tsx | 17 ++++++--- frontend/lib/form-validation.test.ts | 38 +++++++++++++++++++ frontend/lib/form-validation.ts | 23 +++++++++++ frontend/package.json | 2 +- smartcontract/contracts/flexible/src/lib.rs | 15 ++++++++ smartcontract/contracts/flexible/src/tests.rs | 37 ++++++++++++++++++ smartcontract/contracts/rotational/src/lib.rs | 16 ++++++++ .../contracts/rotational/src/tests.rs | 38 +++++++++++++++++++ smartcontract/contracts/target/src/lib.rs | 15 ++++++++ smartcontract/contracts/target/src/tests.rs | 26 +++++++++++++ 13 files changed, 248 insertions(+), 26 deletions(-) create mode 100644 frontend/lib/form-validation.test.ts diff --git a/frontend/components/create-group/BulkImport.tsx b/frontend/components/create-group/BulkImport.tsx index 8288205..c4d79c1 100644 --- a/frontend/components/create-group/BulkImport.tsx +++ b/frontend/components/create-group/BulkImport.tsx @@ -35,6 +35,7 @@ export default function BulkImport({ onMembersChange }: BulkImportProps) { complete: (results) => { const parsed: Member[] = []; const errorLines: string[] = []; + const firstLineForAddress = new Map(); results.data.forEach((row, idx) => { const lineNum = idx + 1; const address = row[0]?.trim() ?? ""; @@ -47,6 +48,12 @@ export default function BulkImport({ onMembersChange }: BulkImportProps) { errorLines.push(`Line ${lineNum}: invalid Stellar address`); return; } + const firstLine = firstLineForAddress.get(address); + if (firstLine !== undefined) { + errorLines.push(`Line ${lineNum}: duplicate address (already added on line ${firstLine})`); + return; + } + firstLineForAddress.set(address, lineNum); parsed.push({ address, name, line: lineNum }); }); if (parsed.length > MAX_MEMBERS) { diff --git a/frontend/components/create-group/flexible-form.tsx b/frontend/components/create-group/flexible-form.tsx index d9a4faf..9538775 100644 --- a/frontend/components/create-group/flexible-form.tsx +++ b/frontend/components/create-group/flexible-form.tsx @@ -19,6 +19,7 @@ import { validateStellarAddress, validatePositiveAmount, validateWithdrawalFee, + findDuplicateAddresses, } from "@/lib/form-validation" function isValidStellarAddress(addr: string) { @@ -35,7 +36,6 @@ export function FlexibleForm() { const { address } = useStellar() const [token, setToken] = useState({ address: "native", symbol: "XLM", decimals: 7 }) const [members, setMembers] = useState([""]) - const [memberErrors, setMemberErrors] = useState([""]) const [error, setError] = useState("") const [step, setStep] = useState<"idle" | "deploying" | "initializing" | "registering" | "saving">("idle") const [formData, setFormData] = useState({ @@ -55,6 +55,14 @@ export function FlexibleForm() { const allMembers = address ? [address, ...members] : members const validMembers = Array.from(new Set(allMembers.filter(isValidStellarAddress))) + const duplicateIndices = findDuplicateAddresses(allMembers) + const memberErrors = members.map((m, i) => { + if (!m) return "" + const format = validateStellarAddress(m) + if (!format.valid) return format.message + const allMembersIndex = address ? i + 1 : i + return duplicateIndices.has(allMembersIndex) ? "Duplicate address — already in this pool's member list" : "" + }) const isCreating = step !== "idle" const validateField = useCallback((name: keyof FieldErrors, value: string) => { @@ -72,15 +80,11 @@ export function FlexibleForm() { const updateMember = (i: number, v: string) => { const n = [...members]; n[i] = v; setMembers(n) - const errs = [...memberErrors] - errs[i] = v ? (validateStellarAddress(v).valid ? "" : validateStellarAddress(v).message) : "" - setMemberErrors(errs) } - const addMember = () => { setMembers([...members, ""]); setMemberErrors([...memberErrors, ""]) } + const addMember = () => { setMembers([...members, ""]) } const removeMember = (i: number) => { setMembers(members.filter((_, idx) => idx !== i)) - setMemberErrors(memberErrors.filter((_, idx) => idx !== i)) } const handleSubmit = async (e: React.FormEvent) => { @@ -98,6 +102,7 @@ export function FlexibleForm() { }) if (!address) return setError("Please connect your wallet first") + if (duplicateIndices.size > 0) return setError("Duplicate member addresses found — please remove duplicates before continuing") if (validMembers.length < 2) return setError("Need at least 2 valid Stellar addresses (you + 1 other)") if (!nameResult.valid || !depositResult.valid || !feeResult.valid) return diff --git a/frontend/components/create-group/rotational-form.tsx b/frontend/components/create-group/rotational-form.tsx index 5f5d903..ab93c4d 100644 --- a/frontend/components/create-group/rotational-form.tsx +++ b/frontend/components/create-group/rotational-form.tsx @@ -19,6 +19,7 @@ import { validateGroupName, validateStellarAddress, validatePositiveAmount, + findDuplicateAddresses, } from "@/lib/form-validation" import type { DuplicatePrefill } from "@/app/dashboard/create/[type]/page" @@ -48,9 +49,6 @@ export function RotationalForm({ prefill }: { prefill?: DuplicatePrefill }) { const [members, setMembers] = useState( initialMembers.length > 0 ? initialMembers : [""] ) - const [memberErrors, setMemberErrors] = useState( - initialMembers.length > 0 ? initialMembers.map(() => "") : [""] - ) const [error, setError] = useState("") const [step, setStep] = useState<"idle" | "deploying" | "initializing" | "registering" | "saving">("idle") const [formData, setFormData] = useState({ @@ -75,6 +73,14 @@ export function RotationalForm({ prefill }: { prefill?: DuplicatePrefill }) { // Always include creator as first member const allMembers = address ? [address, ...members] : members const validMembers = Array.from(new Set(allMembers.filter(isValidStellarAddress))) + const duplicateIndices = findDuplicateAddresses(allMembers) + const memberErrors = members.map((m, i) => { + if (!m) return "" + const format = validateStellarAddress(m) + if (!format.valid) return format.message + const allMembersIndex = address ? i + 1 : i + return duplicateIndices.has(allMembersIndex) ? "Duplicate address — already in this pool's member list" : "" + }) const isCreating = step !== "idle" const validateField = useCallback((name: keyof FieldErrors, value: string) => { @@ -92,24 +98,14 @@ export function RotationalForm({ prefill }: { prefill?: DuplicatePrefill }) { const next = [...members] next[i] = v setMembers(next) - const errs = [...memberErrors] - if (v) { - const r = validateStellarAddress(v) - errs[i] = r.valid ? "" : r.message - } else { - errs[i] = "" - } - setMemberErrors(errs) } const addMember = () => { setMembers([...members, ""]) - setMemberErrors([...memberErrors, ""]) } const removeMember = (i: number) => { setMembers(members.filter((_, idx) => idx !== i)) - setMemberErrors(memberErrors.filter((_, idx) => idx !== i)) } const handleSubmit = async (e: React.FormEvent) => { @@ -126,6 +122,7 @@ export function RotationalForm({ prefill }: { prefill?: DuplicatePrefill }) { }) if (!address) return setError("Please connect your wallet first") + if (duplicateIndices.size > 0) return setError("Duplicate member addresses found — please remove duplicates before continuing") if (validMembers.length < 2) return setError("Need at least 2 valid Stellar addresses (you + 1 other)") if (!nameResult.valid || !amountResult.valid) return diff --git a/frontend/components/create-group/target-form.tsx b/frontend/components/create-group/target-form.tsx index d31318d..d0bee00 100644 --- a/frontend/components/create-group/target-form.tsx +++ b/frontend/components/create-group/target-form.tsx @@ -19,6 +19,7 @@ import { validateStellarAddress, validatePositiveAmount, validateDeadline, + findDuplicateAddresses, } from "@/lib/form-validation" function isValidStellarAddress(addr: string) { @@ -43,7 +44,6 @@ export function TargetForm() { const { address } = useStellar() const [token, setToken] = useState({ address: "native", symbol: "XLM", decimals: 7 }) const [members, setMembers] = useState([""]) - const [memberErrors, setMemberErrors] = useState([""]) const [error, setError] = useState("") const [step, setStep] = useState<"idle" | "deploying" | "initializing" | "registering" | "saving">("idle") const [formData, setFormData] = useState({ name: "", description: "", targetAmount: "", deadline: "" }) @@ -61,6 +61,14 @@ export function TargetForm() { const allMembers = address ? [address, ...members] : members const validMembers = Array.from(new Set(allMembers.filter(isValidStellarAddress))) + const duplicateIndices = findDuplicateAddresses(allMembers) + const memberErrors = members.map((m, i) => { + if (!m) return "" + const format = validateStellarAddress(m) + if (!format.valid) return format.message + const allMembersIndex = address ? i + 1 : i + return duplicateIndices.has(allMembersIndex) ? "Duplicate address — already in this pool's member list" : "" + }) const isCreating = step !== "idle" const validateField = useCallback((name: keyof FieldErrors, value: string) => { @@ -78,15 +86,11 @@ export function TargetForm() { const updateMember = (i: number, v: string) => { const next = [...members]; next[i] = v; setMembers(next) - const errs = [...memberErrors] - errs[i] = v ? (validateStellarAddress(v).valid ? "" : validateStellarAddress(v).message) : "" - setMemberErrors(errs) } - const addMember = () => { setMembers([...members, ""]); setMemberErrors([...memberErrors, ""]) } + const addMember = () => { setMembers([...members, ""]) } const removeMember = (i: number) => { setMembers(members.filter((_, idx) => idx !== i)) - setMemberErrors(memberErrors.filter((_, idx) => idx !== i)) } const handleSubmit = async (e: React.FormEvent) => { @@ -104,6 +108,7 @@ export function TargetForm() { }) if (!address) return setError("Please connect your wallet first") + if (duplicateIndices.size > 0) return setError("Duplicate member addresses found — please remove duplicates before continuing") if (validMembers.length < 2) return setError("Need at least 2 valid Stellar addresses (you + 1 other)") if (!nameResult.valid || !amountResult.valid || !deadlineResult.valid) return diff --git a/frontend/lib/form-validation.test.ts b/frontend/lib/form-validation.test.ts new file mode 100644 index 0000000..23d641c --- /dev/null +++ b/frontend/lib/form-validation.test.ts @@ -0,0 +1,38 @@ +// Unit tests for member-address validation, focused on duplicate detection. +import { test } from 'node:test'; +import assert from 'node:assert'; +import { findDuplicateAddresses, validateStellarAddress } from './form-validation'; + +const A = 'GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA'; +const B = 'GBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBA'; +const C = 'GCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCA'; + +test('findDuplicateAddresses - no duplicates returns an empty set', () => { + const result = findDuplicateAddresses([A, B, C]); + assert.strictEqual(result.size, 0); +}); + +test('findDuplicateAddresses - flags both the first and later occurrence', () => { + const result = findDuplicateAddresses([A, B, A]); + assert.deepStrictEqual(result, new Set([0, 2])); +}); + +test('findDuplicateAddresses - flags every member of a repeated group', () => { + const result = findDuplicateAddresses([A, A, A]); + assert.deepStrictEqual(result, new Set([0, 1, 2])); +}); + +test('findDuplicateAddresses - ignores empty entries (covered by other validation)', () => { + const result = findDuplicateAddresses(['', A, '', B]); + assert.strictEqual(result.size, 0); +}); + +test('findDuplicateAddresses - trims whitespace before comparing', () => { + const result = findDuplicateAddresses([A, ` ${A} `]); + assert.deepStrictEqual(result, new Set([0, 1])); +}); + +test('validateStellarAddress - still rejects malformed addresses independently of duplicate checks', () => { + assert.strictEqual(validateStellarAddress('not-an-address').valid, false); + assert.strictEqual(validateStellarAddress(A).valid, true); +}); diff --git a/frontend/lib/form-validation.ts b/frontend/lib/form-validation.ts index 719d884..f9b3b0b 100644 --- a/frontend/lib/form-validation.ts +++ b/frontend/lib/form-validation.ts @@ -44,3 +44,26 @@ export function validateWithdrawalFee(value: string): ValidationResult { if (num > 10) return err("Fee cannot exceed 10%") return ok } + +/** + * Returns the indices of entries in `addresses` that share a value with at + * least one other entry (both the first and later occurrences are flagged, + * so every duplicate row can show an inline error). Empty entries are + * ignored since they're caught by other field validation. + */ +export function findDuplicateAddresses(addresses: string[]): Set { + const firstSeenAt = new Map() + const duplicates = new Set() + addresses.forEach((raw, i) => { + const value = raw.trim() + if (!value) return + const seenAt = firstSeenAt.get(value) + if (seenAt !== undefined) { + duplicates.add(seenAt) + duplicates.add(i) + } else { + firstSeenAt.set(value, i) + } + }) + return duplicates +} diff --git a/frontend/package.json b/frontend/package.json index d04f326..7f5907c 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -7,7 +7,7 @@ "dev": "next dev --webpack", "lint": "next lint", "start": "next start", - "test:unit": "tsx --test lib/csv-export.test.ts lib/analytics.test.ts lib/pool-health.test.ts hooks/use-keyboard-shortcuts.test.ts", + "test:unit": "tsx --test lib/csv-export.test.ts lib/analytics.test.ts lib/pool-health.test.ts lib/form-validation.test.ts hooks/use-keyboard-shortcuts.test.ts", "test:e2e": "playwright test", "test:e2e:ui": "playwright test --ui", "test:e2e:report": "playwright show-report" diff --git a/smartcontract/contracts/flexible/src/lib.rs b/smartcontract/contracts/flexible/src/lib.rs index b64c6b9..595cd31 100644 --- a/smartcontract/contracts/flexible/src/lib.rs +++ b/smartcontract/contracts/flexible/src/lib.rs @@ -41,6 +41,7 @@ impl FlexiblePool { treasury_fee_bps: u32, ) { assert!(members.len() >= 2, "need >=2 members"); + assert!(!Self::has_duplicate_members(&members), "duplicate member address"); assert!(minimum_deposit > 0, "minimum must be > 0"); // Validate the token is a real SEP-41 contract by reading its decimals @@ -487,6 +488,20 @@ impl FlexiblePool { } false } + + /// O(n^2) pairwise scan — member lists are small, so this is cheaper + /// than maintaining a separate index just to dedupe at init time. + fn has_duplicate_members(members: &Vec
) -> bool { + for i in 0..members.len() { + let a = members.get(i).unwrap(); + for j in (i + 1)..members.len() { + if a == members.get(j).unwrap() { + return true; + } + } + } + false + } } #[cfg(test)] diff --git a/smartcontract/contracts/flexible/src/tests.rs b/smartcontract/contracts/flexible/src/tests.rs index 807d6f5..9cc0708 100644 --- a/smartcontract/contracts/flexible/src/tests.rs +++ b/smartcontract/contracts/flexible/src/tests.rs @@ -55,6 +55,43 @@ fn test_token_decimals_recorded() { assert_eq!(client.token_decimals(), 7); } +#[test] +#[should_panic(expected = "duplicate member address")] +fn test_initialize_rejects_duplicate_member() { + let env = Env::default(); + env.mock_all_auths(); + + let contract_id = env.register_contract(None, FlexiblePool); + let client = FlexiblePoolClient::new(&env, &contract_id); + + let token_admin = Address::generate(&env); + let token_contract = env.register_stellar_asset_contract_v2(token_admin.clone()); + let token_address = token_contract.address(); + + let admin = Address::generate(&env); + let treasury = Address::generate(&env); + let member_a = Address::generate(&env); + let member_b = Address::generate(&env); + + // member_a appears twice — distribute_yield iterates the raw members + // vec, so a duplicate slot would grant member_a a double yield share. + let mut members = Vec::new(&env); + members.push_back(member_a.clone()); + members.push_back(member_b.clone()); + members.push_back(member_a.clone()); + + client.initialize( + &token_address, + &admin, + &members, + &10i128, + &0u32, + &false, + &treasury, + &0u32, + ); +} + #[test] #[should_panic(expected = "below minimum deposit")] fn test_minimum_deposit_rejection() { diff --git a/smartcontract/contracts/rotational/src/lib.rs b/smartcontract/contracts/rotational/src/lib.rs index 24bc119..3c71175 100644 --- a/smartcontract/contracts/rotational/src/lib.rs +++ b/smartcontract/contracts/rotational/src/lib.rs @@ -48,6 +48,7 @@ impl RotationalPool { treasury: Address, ) { assert!(members.len() >= 2, "need >=2 members"); + assert!(!Self::has_duplicate_members(&members), "duplicate member address"); assert!(deposit_amount > 0, "deposit must be > 0"); assert!(round_duration > 0, "round_duration must be > 0"); @@ -446,6 +447,21 @@ impl RotationalPool { false } + /// O(n^2) pairwise scan — member lists are small (capped well below + /// the resource limits that would make this costly), so this is cheaper + /// than maintaining a separate index just to dedupe at init time. + fn has_duplicate_members(members: &Vec
) -> bool { + for i in 0..members.len() { + let a = members.get(i).unwrap(); + for j in (i + 1)..members.len() { + if a == members.get(j).unwrap() { + return true; + } + } + } + false + } + fn member_index(members: &Vec
, who: &Address) -> Option { let mut index = 0u32; for m in members.iter() { diff --git a/smartcontract/contracts/rotational/src/tests.rs b/smartcontract/contracts/rotational/src/tests.rs index b17b4d9..c31c6d9 100644 --- a/smartcontract/contracts/rotational/src/tests.rs +++ b/smartcontract/contracts/rotational/src/tests.rs @@ -144,6 +144,44 @@ fn test_non_member_deposit_rejection() { client.deposit(&non_member); } +#[test] +#[should_panic(expected = "duplicate member address")] +fn test_initialize_rejects_duplicate_member() { + let env = Env::default(); + env.mock_all_auths(); + + let contract_id = env.register_contract(None, RotationalPool); + let client = RotationalPoolClient::new(&env, &contract_id); + + let token_admin = Address::generate(&env); + let token_contract = env.register_stellar_asset_contract_v2(token_admin.clone()); + let token_address = token_contract.address(); + + let treasury = Address::generate(&env); + let admin = Address::generate(&env); + let member_a = Address::generate(&env); + let member_b = Address::generate(&env); + + // member_a appears twice — this must be rejected, since the round-robin + // payout indexes directly into the members vec and would otherwise let + // member_a claim two payout rounds while member_b only ever claims one. + let mut members = Vec::new(&env); + members.push_back(member_a.clone()); + members.push_back(member_b.clone()); + members.push_back(member_a.clone()); + + client.initialize( + &token_address, + &admin, + &members, + &100i128, + &100u64, + &0u32, + &0u32, + &treasury, + ); +} + #[test] #[should_panic(expected = "already deposited this round")] fn test_duplicate_deposit_rejection() { diff --git a/smartcontract/contracts/target/src/lib.rs b/smartcontract/contracts/target/src/lib.rs index 8eb2f74..4931c97 100644 --- a/smartcontract/contracts/target/src/lib.rs +++ b/smartcontract/contracts/target/src/lib.rs @@ -36,6 +36,7 @@ impl TargetPool { deadline: u32, ) { assert!(members.len() >= 2, "need >=2 members"); + assert!(!Self::has_duplicate_members(&members), "duplicate member address"); assert!(target_amount > 0, "target must be > 0"); // Validate the token is a real SEP-41 contract by reading its decimals @@ -378,6 +379,20 @@ impl TargetPool { } false } + + /// O(n^2) pairwise scan — member lists are small, so this is cheaper + /// than maintaining a separate index just to dedupe at init time. + fn has_duplicate_members(members: &Vec
) -> bool { + for i in 0..members.len() { + let a = members.get(i).unwrap(); + for j in (i + 1)..members.len() { + if a == members.get(j).unwrap() { + return true; + } + } + } + false + } } #[cfg(test)] diff --git a/smartcontract/contracts/target/src/tests.rs b/smartcontract/contracts/target/src/tests.rs index 4a470e5..5ff94b7 100644 --- a/smartcontract/contracts/target/src/tests.rs +++ b/smartcontract/contracts/target/src/tests.rs @@ -53,6 +53,32 @@ fn test_unlock_on_target() { assert!(client.is_unlocked()); } +#[test] +#[should_panic(expected = "duplicate member address")] +fn test_initialize_rejects_duplicate_member() { + let env = Env::default(); + env.mock_all_auths(); + + let contract_id = env.register_contract(None, TargetPool); + let client = TargetPoolClient::new(&env, &contract_id); + + let token_admin = Address::generate(&env); + let token_contract = env.register_stellar_asset_contract_v2(token_admin.clone()); + let token_address = token_contract.address(); + + let admin = Address::generate(&env); + let member_a = Address::generate(&env); + let member_b = Address::generate(&env); + + // member_a appears twice in the member list. + let mut members = Vec::new(&env); + members.push_back(member_a.clone()); + members.push_back(member_b.clone()); + members.push_back(member_a.clone()); + + client.initialize(&token_address, &admin, &members, &100i128, &1000u32); +} + #[test] fn test_proportional_withdraw() { let env = Env::default();