diff --git a/.github/ISSUE_TEMPLATE/feature_request.md b/.github/ISSUE_TEMPLATE/feature_request.md new file mode 100644 index 0000000..aec8ed3 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/feature_request.md @@ -0,0 +1,29 @@ +--- +name: Feature Request +about: Propose a new feature or enhancement for the smart contracts +title: "[Feature]: " +labels: enhancement +assignees: "" +--- + +## Contract affected + +Please specify the contract or module this feature applies to. + +Example: `token`, `escrow`, `payment`, `refund`, `staking`, `governance` + +## Motivating use case + +Describe the problem or use case that motivates this feature. Who needs it, and why? + +## Proposed solution + +Describe the feature or change you'd like to see. Include relevant function signatures, storage changes, or events if applicable. + +## Alternatives considered + +Describe any alternative solutions or workarounds you've considered, and why they fall short. + +## Additional context + +Add any other context, references, or screenshots that may help evaluate this proposal. diff --git a/contracts/escrow/README.md b/contracts/escrow/README.md index 5687118..d6022c8 100644 --- a/contracts/escrow/README.md +++ b/contracts/escrow/README.md @@ -11,6 +11,61 @@ Multi-party escrows can require a minimum approval weight before funds are relea - The minimum safe threshold is `1` bps, but in practice you should use a higher value for any escrow that holds meaningful funds. Setting the threshold too low can let a small group of signers release funds with weak consensus, which increases the risk of misuse or compromise. - Recommended defaults are `10000` for high-value or sensitive escrows, and `5000` or higher for majority-based approval policies. +## Dispute lifecycle + +An escrow moves through dispute states via `dispute_escrow`, `escalate_dispute`, `file_dispute_appeal`, `resolve_appeal`, `resolve_dispute`/`auto_resolve_dispute`, and the timeout paths `trigger_timeout_resolution` / `process_escalation_timeouts`. + +```mermaid +stateDiagram-v2 + [*] --> Locked: create_escrow_with_multisig / create_conditional_escrow + Locked --> Released: release_escrow + Locked --> Disputed: dispute_escrow (customer or merchant) + Locked --> Cancelled: cancel_escrow + + Disputed --> Resolved: resolve_dispute / auto_resolve_dispute (favors customer) + Disputed --> Released: auto_resolve_dispute (favors merchant) + Disputed --> Escalated: escalate_dispute (customer or merchant) + + Escalated --> Resolved: trigger_timeout_resolution (favor: Customer) + Escalated --> Released: trigger_timeout_resolution (favor: Merchant) + Escalated --> Escalated: process_escalation_timeouts (batch sweep, no state change until deadline hit) + + Resolved --> Appealed: file_dispute_appeal (within 72h appeal window, Initial round only) + Released --> Appealed: file_dispute_appeal (within 72h appeal window, Initial round only) + Appealed --> Resolved: resolve_appeal + Appealed --> Released: resolve_appeal + + Released --> [*] + Resolved --> [*] + Cancelled --> [*] +``` + +Notes: +- Only the escrow's `customer` or `merchant` can open a dispute, escalate it, or file an appeal. +- Each escrow allows at most one appeal round (`DisputeRound::Initial` → `Appeal` → `Final`); a third round is rejected. +- The appeal window is fixed at 72 hours (259200 seconds) from `dispute_started_at`; filing after that window returns `InvalidStatus`. +- Escalation timeouts are configured per-contract via `set_escalation_config` (timeout duration + which party auto-resolution favors) and enforced by `trigger_timeout_resolution` (single escrow) or `process_escalation_timeouts` (batch sweep of the escalation queue). + +## Observer role + +Observers get time-limited, read-only visibility into a single escrow — useful for auditors, support staff, or dispute mediators who need to inspect an escrow without being a party to it. + +- **Granting access** — `add_observer(granter, escrow_id, observer, duration_seconds)` grants `observer` read access to `escrow_id` until `now + duration_seconds`. `granter` must be the escrow's `customer`, its `merchant`, or an address in the admin multisig (`AdminMultiSig`). +- **Revoking access** — `remove_observer(granter, escrow_id, observer)` removes an observer entry early. Same caller restriction as granting. +- **Checking access** — `verify_observer_access(escrow_id, observer)` returns `true` only if the address was granted observer status and `expires_at` is still in the future; expired grants are not deleted automatically but read as inactive. +- **Listing** — `get_observers(escrow_id)` returns all observer entries for an escrow, including expired ones (callers should check `expires_at` themselves). + +Access split: + +| Function | Observer access | +|----------|------------------| +| `get_escrow_details(caller, escrow_id)` | Readable — succeeds for the customer, merchant, or any active (non-expired) observer. | +| `get_observers`, `verify_observer_access`, `get_dispute_round`, `get_appeal`, `is_escrow_disputed` | Public/unauthenticated reads, not gated by observer status. | +| `add_observer`, `remove_observer` | Restricted — customer, merchant, or admin only. Observers cannot grant or revoke observer access, including their own. | +| `dispute_escrow`, `escalate_dispute`, `file_dispute_appeal`, `release_escrow`, `resolve_dispute`, and all fund-moving or state-changing calls | Restricted — observers have no write access; these remain limited to the customer, merchant, arbitrators, or admins as applicable. | + +An observer is purely a read-only role: it never satisfies `require_auth` checks for customer/merchant/admin-gated functions, so granting observer access cannot be used to bypass dispute or fund-release authorization. + ## Events All events are emitted via `#[contractevent]` structs published through the Soroban event system. Each event's **topic** is a `Symbol` matching the struct name. Off-chain consumers (Horizon, indexers) can subscribe to these topics. diff --git a/contracts/refund/src/lib.rs b/contracts/refund/src/lib.rs index 4c14b05..e232b2c 100644 --- a/contracts/refund/src/lib.rs +++ b/contracts/refund/src/lib.rs @@ -85,6 +85,8 @@ pub enum DataKey { CustomerTierPolicy(Address, u32), StrictTierPolicy(Address), AppealWindowSeconds, + // Issue #389: two-step admin rotation + PendingAdmin, } #[derive(Clone, Debug, PartialEq)] @@ -285,6 +287,9 @@ pub enum ExtError { // Issue #370: Customer tier policy errors TierPolicyNotFound = 57, SchemaAlreadyAtTarget = 58, + // Issue #389: two-step admin rotation errors + NoPendingAdmin = 59, + NotPendingAdmin = 60, } #[derive(Clone, Copy, Debug, PartialEq)] @@ -1308,6 +1313,22 @@ pub struct RateLimitUpdated { pub effective_at: u64, } +/// Event emitted when the current admin proposes a new admin. +#[contractevent] +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct AdminRotationProposed { + pub current_admin: Address, + pub pending_admin: Address, +} + +/// Event emitted when a proposed admin accepts the role, completing rotation. +#[contractevent] +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct AdminRotationAccepted { + pub previous_admin: Address, + pub new_admin: Address, +} + #[contract] pub struct RefundContract; @@ -1401,6 +1422,86 @@ impl RefundContract { Ok(()) } + /// Propose a new admin, starting a two-step rotation (Issue #389). + /// + /// The current admin designates `new_admin` as pending. The rotation only + /// completes once `new_admin` calls [`Self::accept_admin`], so a typo'd or + /// unreachable address can never brick admin control of the contract. + /// + /// # Arguments + /// * `admin` - The current admin (must be authorized and match stored admin). + /// * `new_admin` - The address to propose as the next admin. + /// + /// # Errors + /// Returns `Unauthorized` if the caller is not the current admin. + pub fn propose_admin(env: Env, admin: Address, new_admin: Address) -> Result<(), Error> { + admin.require_auth(); + let stored_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .ok_or(Error::Core(CoreError::Unauthorized))?; + if admin != stored_admin { + return Err(Error::Core(CoreError::Unauthorized)); + } + + env.storage() + .instance() + .set(&DataKey::PendingAdmin, &new_admin); + + (AdminRotationProposed { + current_admin: admin, + pending_admin: new_admin, + }) + .publish(&env); + + Ok(()) + } + + /// Accept a pending admin rotation, finalizing the transition (Issue #389). + /// + /// Must be called by the address previously proposed via + /// [`Self::propose_admin`]. Replaces `DataKey::Admin` with the caller and + /// clears the pending slot. + /// + /// # Errors + /// Returns `NoPendingAdmin` if no rotation has been proposed. + /// Returns `NotPendingAdmin` if the caller is not the proposed admin. + pub fn accept_admin(env: Env, new_admin: Address) -> Result<(), Error> { + new_admin.require_auth(); + + let pending: Address = env + .storage() + .instance() + .get(&DataKey::PendingAdmin) + .ok_or(Error::Ext(ExtError::NoPendingAdmin))?; + if pending != new_admin { + return Err(Error::Ext(ExtError::NotPendingAdmin)); + } + + let previous_admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .ok_or(Error::Core(CoreError::Unauthorized))?; + + env.storage().instance().set(&DataKey::Admin, &new_admin); + env.storage().instance().remove(&DataKey::PendingAdmin); + + (AdminRotationAccepted { + previous_admin, + new_admin, + }) + .publish(&env); + + Ok(()) + } + + /// Get the address currently proposed as the next admin, if any. + pub fn get_pending_admin(env: Env) -> Option
{ + env.storage().instance().get(&DataKey::PendingAdmin) + } + /// Request a refund for a payment. /// /// Creates a new refund request with status `Requested` (or `Approved` if auto-approval @@ -8472,3 +8573,6 @@ mod schema_version_test; #[cfg(test)] mod test_merchant_override_and_error_codes; + +#[cfg(test)] +mod test_admin_rotation; diff --git a/contracts/refund/src/test_admin_rotation.rs b/contracts/refund/src/test_admin_rotation.rs new file mode 100644 index 0000000..35d1cc1 --- /dev/null +++ b/contracts/refund/src/test_admin_rotation.rs @@ -0,0 +1,105 @@ +#![cfg(test)] + +// Issue #389: two-step admin rotation for the refund contract. + +use super::*; +use soroban_sdk::testutils::Address as _; +use soroban_sdk::Env; + +#[test] +fn test_propose_and_accept_admin_rotates_admin() { + let env = Env::default(); + env.mock_all_auths(); + let contract_id = env.register(RefundContract, ()); + let client = RefundContractClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + let new_admin = Address::generate(&env); + client.initialize(&admin); + + assert_eq!(client.get_pending_admin(), None); + + client.propose_admin(&admin, &new_admin); + assert_eq!(client.get_pending_admin(), Some(new_admin.clone())); + + client.accept_admin(&new_admin); + assert_eq!(client.get_pending_admin(), None); + + // The rotated-in admin can now perform an admin-gated action. + let target_version = client.get_schema_version() + 1; + client.migrate_schema(&new_admin, &target_version); + assert_eq!(client.get_schema_version(), target_version); +} + +#[test] +fn test_propose_admin_rejects_non_admin_caller() { + let env = Env::default(); + env.mock_all_auths(); + let contract_id = env.register(RefundContract, ()); + let client = RefundContractClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + let not_admin = Address::generate(&env); + let new_admin = Address::generate(&env); + client.initialize(&admin); + + let result = client.try_propose_admin(¬_admin, &new_admin); + assert_eq!(result, Err(Ok(Error::Core(CoreError::Unauthorized)))); +} + +#[test] +fn test_accept_admin_rejects_wrong_caller() { + let env = Env::default(); + env.mock_all_auths(); + let contract_id = env.register(RefundContract, ()); + let client = RefundContractClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + let new_admin = Address::generate(&env); + let imposter = Address::generate(&env); + client.initialize(&admin); + + client.propose_admin(&admin, &new_admin); + + let result = client.try_accept_admin(&imposter); + assert_eq!(result, Err(Ok(Error::Ext(ExtError::NotPendingAdmin)))); + + // Old admin still retains control since rotation never completed. + let target_version = client.get_schema_version() + 1; + client.migrate_schema(&admin, &target_version); + assert_eq!(client.get_schema_version(), target_version); +} + +#[test] +fn test_accept_admin_without_proposal_fails() { + let env = Env::default(); + env.mock_all_auths(); + let contract_id = env.register(RefundContract, ()); + let client = RefundContractClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + let stranger = Address::generate(&env); + client.initialize(&admin); + + let result = client.try_accept_admin(&stranger); + assert_eq!(result, Err(Ok(Error::Ext(ExtError::NoPendingAdmin)))); +} + +#[test] +fn test_compromised_admin_key_cannot_be_used_after_rotation() { + let env = Env::default(); + env.mock_all_auths(); + let contract_id = env.register(RefundContract, ()); + let client = RefundContractClient::new(&env, &contract_id); + + let compromised_admin = Address::generate(&env); + let safe_admin = Address::generate(&env); + client.initialize(&compromised_admin); + + client.propose_admin(&compromised_admin, &safe_admin); + client.accept_admin(&safe_admin); + + // The old (compromised) admin address can no longer perform admin actions. + let result = client.try_migrate_schema(&compromised_admin, &2); + assert_eq!(result, Err(Ok(Error::Core(CoreError::Unauthorized)))); +}