From 5d6cf71e5a40e0c4daae736a26a386d930ca2ee3 Mon Sep 17 00:00:00 2001 From: nanoulloa Date: Thu, 23 Jul 2026 15:47:19 -0600 Subject: [PATCH] fix(security): require admin auth for MintToken in batch operations (#39) `authorize_batch_access` did nothing for `BatchOperation::MintToken`, and the `lib.rs` batch entry points set `caller = None` for a mint-first batch. Together these let anyone mint by submitting a batch whose operation is a `MintToken`, bypassing every KYC and admin guard. Gate any batch that mints behind the contract admin. `authorize_batch_access` now flags `requires_admin` on any `MintToken` op and, once, calls the new `require_batch_admin` helper, which loads the stored admin, enforces `admin.require_auth()`, and re-checks `require_admin` (fail-closed to `admin_req` when no admin is configured). Both `execute_batch_atomic` and `execute_batch_best_effort` route through this helper. - Add regression tests: non-admin mint batch reverts (atomic + best-effort), and mint fails closed when no admin is configured. - Extend scripts/check_kyc_guards.sh to enforce the batch admin gate in CI. Closes #39 Co-Authored-By: Claude Opus 4.8 --- peerx-contracts/counter/batch.rs | 36 +++++++- peerx-contracts/counter/src/kyc_tests.rs | 105 +++++++++++++++++++++++ scripts/check_kyc_guards.sh | 23 +++++ 3 files changed, 163 insertions(+), 1 deletion(-) diff --git a/peerx-contracts/counter/batch.rs b/peerx-contracts/counter/batch.rs index befe75c..96c50e2 100644 --- a/peerx-contracts/counter/batch.rs +++ b/peerx-contracts/counter/batch.rs @@ -137,6 +137,8 @@ fn is_valid_token(token: &Symbol) -> bool { } fn authorize_batch_access(env: &Env, operations: &Vec) -> Result<(), Symbol> { + let mut requires_admin = false; + for i in 0..operations.len() { if let Some(operation) = operations.get(i) { match operation { @@ -147,11 +149,43 @@ fn authorize_batch_access(env: &Env, operations: &Vec) -> Result crate::require_verified_user(env, &user) .map_err(|_| Symbol::new(env, "kyc_req"))?; } - BatchOperation::MintToken(_, _, _) => {} + // `MintToken` is privileged. Defer the admin gate until after the + // loop so it is enforced exactly once for the whole batch, no + // matter how many mint operations it contains. + BatchOperation::MintToken(_, _, _) => { + requires_admin = true; + } } } } + // `MintToken` is an admin-only operation, even inside a batch. If any + // operation mints, the entire batch must be authorized by the configured + // contract admin. This closes the bypass (issue #39) where a batch whose + // first/only operation was a `MintToken` set `caller = None` and skipped + // every KYC and admin guard, letting anyone mint tokens. + if requires_admin { + require_batch_admin(env)?; + } + + Ok(()) +} + +/// Requires that the current batch invocation is authorized by the configured +/// contract admin. Used to gate privileged batch operations such as +/// `MintToken`. +/// +/// Returns `admin_req` when no admin is configured (fail closed) or when the +/// loaded admin does not pass the shared admin check. Callers who are not the +/// admin cannot satisfy `admin.require_auth()` and therefore revert. +fn require_batch_admin(env: &Env) -> Result<(), Symbol> { + let admin = env + .storage() + .persistent() + .get::<_, Address>(&crate::storage::ADMIN_KEY) + .ok_or_else(|| Symbol::new(env, "admin_req"))?; + admin.require_auth(); + crate::admin::require_admin(env, &admin).map_err(|_| Symbol::new(env, "admin_req"))?; Ok(()) } diff --git a/peerx-contracts/counter/src/kyc_tests.rs b/peerx-contracts/counter/src/kyc_tests.rs index ff822c3..dc1c0d5 100644 --- a/peerx-contracts/counter/src/kyc_tests.rs +++ b/peerx-contracts/counter/src/kyc_tests.rs @@ -505,3 +505,108 @@ fn test_sensitive_contract_entry_points_require_verified_kyc() { }); assert_eq!(stake_id, 0); } + +// ===== Batch `MintToken` admin-gate regression tests (issue #39) ===== +// +// Before the fix, `authorize_batch_access` did nothing for `MintToken` and the +// `lib.rs` batch entry points set `caller = None` for a mint-first batch, so a +// batch whose (first) operation was a `MintToken` skipped every KYC and admin +// guard. Anyone could mint tokens by submitting such a batch. These tests pin +// down that a batch containing a `MintToken` is now admin-only. +// +// NB: these tests intentionally do NOT mock auths, so that `admin.require_auth()` +// is actually enforced and an unauthorized (non-admin) batch reverts. + +/// Sets the contract admin directly in persistent storage, matching how the +/// production `set_admin` entry point persists it (`ADMIN_KEY`). +fn set_stored_admin(env: &Env, contract_id: &Address, admin: &Address) { + with_contract(env, contract_id, || { + env.storage() + .persistent() + .set(&crate::storage::ADMIN_KEY, admin); + }); +} + +/// Builds a batch whose only operation mints `amount` of XLM to `to`. +fn mint_only_batch(env: &Env, to: &Address, amount: i128) -> Vec { + let mut operations = Vec::new(env); + operations.push_back(BatchOperation::MintToken( + symbol_short!("XLM"), + to.clone(), + amount, + )); + operations +} + +#[test] +fn test_batch_mint_by_non_admin_reverts_atomic() { + let env = Env::default(); + let contract_id = env.register(CounterContract, ()); + let admin = Address::generate(&env); + let attacker = Address::generate(&env); + + set_stored_admin(&env, &contract_id, &admin); + + // A mint-only atomic batch submitted without the admin's authorization must + // revert (panic on the unmet `admin.require_auth()`), so `attacker` cannot + // mint tokens to itself. + let unauthorized_mint = panic::catch_unwind(AssertUnwindSafe(|| { + with_contract(&env, &contract_id, || { + let operations = mint_only_batch(&env, &attacker, 1_000_000); + CounterContract::execute_batch_atomic(env.clone(), operations) + }) + })); + + assert!( + unauthorized_mint.is_err(), + "non-admin atomic batch containing MintToken must revert" + ); +} + +#[test] +fn test_batch_mint_by_non_admin_reverts_best_effort() { + let env = Env::default(); + let contract_id = env.register(CounterContract, ()); + let admin = Address::generate(&env); + let attacker = Address::generate(&env); + + set_stored_admin(&env, &contract_id, &admin); + + // The best-effort path shares `authorize_batch_access`, so it must also + // reject an unauthorized mint batch before any operation executes. + let unauthorized_mint = panic::catch_unwind(AssertUnwindSafe(|| { + with_contract(&env, &contract_id, || { + let operations = mint_only_batch(&env, &attacker, 1_000_000); + CounterContract::execute_batch_best_effort(env.clone(), operations) + }) + })); + + assert!( + unauthorized_mint.is_err(), + "non-admin best-effort batch containing MintToken must revert" + ); +} + +#[test] +fn test_batch_mint_fails_closed_when_no_admin_configured() { + let env = Env::default(); + let contract_id = env.register(CounterContract, ()); + let attacker = Address::generate(&env); + + // No admin is configured. The mint guard must fail closed: the batch is + // rejected rather than allowed to mint. The atomic entry point converts the + // authorization error into a failed (non-executed) result. + let result: BatchResult = with_contract(&env, &contract_id, || { + let operations = mint_only_batch(&env, &attacker, 1_000_000); + CounterContract::execute_batch_atomic(env.clone(), operations) + }); + + assert_eq!( + result.operations_executed, 0, + "no operation may execute when the mint guard cannot resolve an admin" + ); + assert!( + result.operations_failed >= 1, + "mint batch must be reported as failed when no admin is configured" + ); +} diff --git a/scripts/check_kyc_guards.sh b/scripts/check_kyc_guards.sh index 5916ba2..8ccf680 100755 --- a/scripts/check_kyc_guards.sh +++ b/scripts/check_kyc_guards.sh @@ -66,4 +66,27 @@ if [[ "$(grep -c 'authorize_batch_access(env, &operations)' "$BATCH_FILE")" -lt exit 1 fi +# `MintToken` is a privileged, admin-only batch operation (issue #39). The +# shared authorization helper must gate any batch that mints behind the +# contract admin, so a MintToken op can no longer bypass KYC/admin checks. +if ! grep -q 'fn require_batch_admin' "$BATCH_FILE"; then + echo "Batch guard is missing the admin gate for privileged operations (MintToken)" >&2 + exit 1 +fi + +if ! grep -q 'require_batch_admin(env)' "$BATCH_FILE"; then + echo "Batch authorization helper does not invoke the MintToken admin gate" >&2 + exit 1 +fi + +if ! grep -q 'admin.require_auth();' "$BATCH_FILE"; then + echo "Batch admin gate is missing admin authentication enforcement" >&2 + exit 1 +fi + +if ! grep -q 'crate::admin::require_admin(env, &admin)' "$BATCH_FILE"; then + echo "Batch admin gate is missing the shared require_admin check" >&2 + exit 1 +fi + echo "Verified KYC guard coverage for sensitive contract entry points."