diff --git a/README.md b/README.md index 6a8dc7e..6843197 100644 --- a/README.md +++ b/README.md @@ -144,18 +144,23 @@ systematically rewarding the final recipient. Core single-issue bounty escrow. ```rust -fn initialize(env, admin: Address, treasury: Address, fee_bps: u32, max_sponsors: Option) -> Result<(), Error>; -fn fund(env, issue_id: u64, sponsor: Address, token: Address, amount: i128, deadline: u64) -> Result<(), Error>; +fn initialize(env, admin: Address, oracle: Address, treasury: Address, fee_bps: u32, max_sponsors: Option) -> Result<(), Error>; +fn fund(env, issue_id: u64, sponsor: Address, token: Address, amount: i128, deadline: u64, target: Option) -> Result<(), Error>; fn contribute(env, issue_id: u64, sponsor: Address, amount: i128) -> Result<(), Error>; fn release(env, issue_id: u64, recipients: Vec<(Address, u32)>) -> Result<(), Error>; fn refund(env, issue_id: u64) -> Result<(), Error>; fn extend_deadline(env, issue_id: u64, caller: Address, new_deadline: u64) -> Result<(), Error>; fn keep_alive(env, issue_id: u64) -> Result<(), Error>; +fn pause(env) -> Result<(), Error>; +fn unpause(env) -> Result<(), Error>; +fn upgrade(env, new_wasm_hash: BytesN<32>) -> Result<(), Error>; fn get_escrow(env, issue_id: u64) -> Result; fn get_contribution(env, issue_id: u64, index: u32) -> Result; fn get_admin(env) -> Result; +fn get_oracle(env) -> Result; fn get_treasury(env) -> Result; fn get_fee_bps(env) -> Result; +fn get_version(env) -> u32; fn get_max_sponsors(env) -> Result; ``` @@ -175,8 +180,8 @@ fn get_max_sponsors(env) -> Result; to `MAX_SPONSORS`, 20) distinct contributions per escrow (`TooManySponsors` otherwise). Rejects `AlreadyPaid` / `AlreadyRefunded`. See `docs/escrow-crowdfunding-design.md` for the full design reasoning. -- `release`: admin-only (`require_auth` on the stored admin/oracle - address). `recipients` basis points must sum to exactly 10000 or the +- `release`: oracle-only (`require_auth` on the stored oracle address). + `recipients` basis points must sum to exactly 10000 or the call is rejected (`InvalidSplit`) — this is how team-bounty payouts work, a single recipient at 10000 bps is just the single-payee case. Deducts `fee_bps` off the top to the treasury, splits the rest @@ -220,15 +225,21 @@ fn get_max_sponsors(env) -> Result; Lump-sum budget shared across the issues in a release. ```rust -fn initialize(env, admin: Address, treasury: Address, fee_bps: u32, max_sponsors: Option) -> Result<(), Error>; +fn initialize(env, admin: Address, oracle: Address, treasury: Address, fee_bps: u32, max_sponsors: Option, recovery: Option
) -> Result<(), Error>; fn create_milestone(env, milestone_id: u64, sponsor: Address, token: Address, total_budget: i128) -> Result<(), Error>; fn contribute(env, milestone_id: u64, sponsor: Address, amount: i128) -> Result<(), Error>; fn allocate(env, milestone_id: u64, issue_id: u64, amount: i128) -> Result<(), Error>; fn release_issue(env, milestone_id: u64, issue_id: u64, recipients: Vec<(Address, u32)>) -> Result<(), Error>; fn cancel_milestone(env, milestone_id: u64) -> Result<(), Error>; +fn pause(env) -> Result<(), Error>; +fn unpause(env) -> Result<(), Error>; +fn upgrade(env, new_wasm_hash: BytesN<32>) -> Result<(), Error>; fn get_milestone(env, milestone_id: u64) -> Result; fn get_issue_status(env, milestone_id: u64, issue_id: u64) -> Result; fn get_contribution(env, milestone_id: u64, index: u32) -> Result; +fn get_admin(env) -> Result; +fn get_oracle(env) -> Result; +fn get_version(env) -> u32; fn get_max_sponsors(env) -> Result; ``` @@ -255,7 +266,7 @@ fn get_max_sponsors(env) -> Result; specific `issue_id`. Over-allocating past what's left is rejected (`OverAllocation`); allocating an issue twice is rejected (`IssueAlreadyAllocated`). -- `release_issue`: admin-only, same split/fee mechanics as escrow's +- `release_issue`: oracle-only, same split/fee mechanics as escrow's `release`, but draws from the issue's pre-reserved allocation rather than a fresh deposit. Rejects double release (`IssueAlreadyReleased`). - `cancel_milestone`: admin-only. Refunds whatever is left in @@ -274,14 +285,21 @@ fn get_max_sponsors(env) -> Result; Recurring, open-ended funding tied to a repo/org rather than one issue. ```rust -fn initialize(env, admin: Address, treasury: Address, fee_bps: u32) -> Result<(), Error>; +fn initialize(env, admin: Address, oracle: Address, treasury: Address, fee_bps: u32, recovery: Option
) -> Result<(), Error>; fn deposit(env, pool_id: u64, sponsor: Address, token: Address, amount: i128) -> Result<(), Error>; fn withdraw(env, pool_id: u64, recipient: Address, amount: i128) -> Result<(), Error>; +fn reclaim_deposit(env, pool_id: u64, deposit_index: u32, sponsor: Address) -> Result<(), Error>; +fn keep_alive(env, pool_id: u64) -> Result<(), Error>; +fn pause(env) -> Result<(), Error>; +fn unpause(env) -> Result<(), Error>; +fn upgrade(env, new_wasm_hash: BytesN<32>) -> Result<(), Error>; fn get_pool(env, pool_id: u64) -> Result; fn get_deposit(env, pool_id: u64, index: u32) -> Result; fn get_admin(env) -> Result; +fn get_oracle(env) -> Result; fn get_treasury(env) -> Result; fn get_fee_bps(env) -> Result; +fn get_version(env) -> u32; ``` - `pool_id` is an off-chain-assigned identifier for a repo or org (e.g. a @@ -292,7 +310,7 @@ fn get_fee_bps(env) -> Result; same `token` (`TokenMismatch` otherwise). Every deposit is recorded (`Deposit { sponsor, amount, timestamp }`, indexed by an incrementing counter) so the full contribution history is queryable. -- `withdraw`: admin-only — the backend authorizes a maintainer draw-down +- `withdraw`: oracle-only — the backend authorizes a maintainer draw-down for completed maintenance work (this is *not* tied to a specific PR merge the way escrow/milestones are; it's off-chain-adjudicated "maintenance credit"). Deducts the fee, rejects if `amount` exceeds the @@ -394,27 +412,30 @@ not automated by anything in this repo's scripts today. ## Security model -- **Admin / oracle authorization.** One `Address` (`admin`), set once at - `initialize` and immutable thereafter, represents the `mergefi-backend` - service. All state-changing calls that assert "the reported off-chain - event actually happened" (`release`, `release_issue`, early `refund`, - `allocate`, `withdraw`) require `admin.require_auth()`. Soroban's - `require_auth` means the backend's signing key must actually authorize - that specific invocation — there's no way to spoof it by simply calling - the contract from an arbitrary account. +- **Admin / oracle authorization.** Two `Address` values are set at + `initialize`: `admin` for high-trust operations and `oracle` for + routine backend payouts. Calls that assert "the reported off-chain event + actually happened" (`release`, `release_issue`, `withdraw`) require + `oracle.require_auth()`. Administrative operations (`pause`, `unpause`, + `upgrade`, role rotation, treasury rotation, and early emergency paths) + require `admin.require_auth()`. Soroban's `require_auth` means the + relevant signing key must actually authorize that specific invocation — + there's no way to spoof it by simply calling the contract from an + arbitrary account. - **Sponsor authorization.** `fund`, `create_milestone`, and `deposit` require the sponsor's own `require_auth()` — a backend key can never move a sponsor's funds *into* escrow on their behalf without their signature (only *out*, once deposited, per the payout rules above). - **No re-initialization.** `initialize` checks `storage().instance().has(&DataKey::Admin)` - and rejects with `AlreadyInitialized` if already set, so admin/treasury/fee + and rejects with `AlreadyInitialized` if already set, so admin/oracle/treasury/fee can't be silently swapped out post-deployment by calling `initialize` again. -- **`initialize` requires the named admin's own authorization.** All - three contracts' `initialize` call `admin.require_auth()`, so nobody - can name a third-party address as admin without that address's - consent. This is a narrower guarantee than it might sound like — it +- **`initialize` requires the named admin and oracle authorization.** All + three contracts' `initialize` calls `admin.require_auth()` and + `oracle.require_auth()`, so nobody can name a third-party address for + either role without that address's consent. This is a narrower guarantee + than it might sound like — it does **not** prevent an attacker from front-running the legitimate - deployer's `initialize` call by naming *themselves* as admin instead, + deployer's `initialize` call by naming *themselves* as admin/oracle instead, since they can trivially authorize their own address. See `docs/access-control-audit.md` for the full analysis and why closing that race requires a structural change (an atomic deploy+init @@ -465,10 +486,10 @@ not automated by anything in this repo's scripts today. ## Backend integration (`mergefi-backend`) -`mergefi-backend` is expected to hold the `admin` keypair for each -deployed contract (escrow, milestones, maintenance-pool — these can share -one admin key or use separate ones per environment) and drive them over -Soroban RPC using `stellar-sdk` / `soroban-client` (or the Rust +`mergefi-backend` is expected to hold the routine `oracle` keypair for +each deployed contract (escrow, milestones, maintenance-pool — these can +share one oracle key or use separate ones per environment) and drive them +over Soroban RPC using `stellar-sdk` / `soroban-client` (or the Rust `soroban-cli`/`soroban_rpc` client, if the backend is Rust). Typical integration points: @@ -481,7 +502,7 @@ integration points: `issue_id`/`milestone_id` the merged PR is tied to, resolves the contributor(s) and their split (single payee, or a team split it computed from co-author metadata / maintainer input), builds a - `release` / `release_issue` invocation, signs it with the admin key, + `release` / `release_issue` invocation, signs it with the oracle key, and submits it via Soroban RPC (`simulateTransaction` → `sendTransaction`). It should treat the call as idempotent — the contract itself rejects double-release, so a retry after a network @@ -499,6 +520,11 @@ integration points: (no signature/fee) and are the primary way the backend/API layer keeps its own database in sync with on-chain truth after any write. +High-trust operations use the separate `admin` key: `pause`, `unpause`, +`upgrade`, `set_admin`, `set_oracle`, treasury rotation, and recovery +flows. The admin key should be kept colder than the backend oracle key, +or represented by a Stellar multisig / governance-controlled address. + ## Build, test, deploy ### Prerequisites @@ -536,10 +562,10 @@ cargo build --target wasm32v1-none --profile release-with-logs \ -p mergefi-escrow -p mergefi-milestones -p mergefi-maintenance-pool ``` -Verified in this session: `cargo test --workspace` — **54/54 tests pass** -(28 escrow, 19 milestones, 7 maintenance-pool, including the -access-control boundary matrix added in #30 and the multi-sponsor -crowdfunding tests added in #57/#58) on the native target using +Verified in this session: `cargo test --workspace` — **109/109 tests pass** +(54 escrow, 31 milestones, 24 maintenance-pool, including the +access-control boundary matrix, pause/oracle checks, and the multi-sponsor +crowdfunding tests) on the native target using `soroban_sdk::testutils` (`Env::default()`, `Address::generate`, `mock_all_auths`, `register_stellar_asset_contract_v2` for a test token). @@ -578,9 +604,12 @@ would otherwise do. | `mergefi-milestones` | `CBBRLSL6TM6XCNP2XBVT4GFHJ3NNPFKI2BCZQJ4U3TI7GV7DO2F2HG6F` | [View on Stellar Expert](https://stellar.expert/explorer/testnet/contract/CBBRLSL6TM6XCNP2XBVT4GFHJ3NNPFKI2BCZQJ4U3TI7GV7DO2F2HG6F) | | `mergefi-maintenance-pool` | `CD46U7WTEM2I77TXQI2VIBRQXOHEFEYYR2XFA7OVGTXX5M2F7Z3ZQOX2` | [View on Stellar Expert](https://stellar.expert/explorer/testnet/contract/CD46U7WTEM2I77TXQI2VIBRQXOHEFEYYR2XFA7OVGTXX5M2F7Z3ZQOX2) | -All three were initialized with the same admin/treasury address +All three legacy testnet contracts were initialized with the same admin/treasury address (`GBUXADZJ7O4NM7S7CDZYVXGP37M772D2TYMFBT2QFH2JSRCFEJPAVW5N`, a -throwaway testnet-only account) and a 250 bps (2.5%) treasury fee. +throwaway testnet-only account) and a 250 bps (2.5%) treasury fee. They +do not contain the upgrade/pause/oracle features described above; deploy +fresh testnet contracts from this branch before treating those features as +available. View them on Stellar Expert: - [`mergefi-escrow`](https://stellar.expert/explorer/testnet/contract/CAY77D2SFDVQYONSPYHOEWARE3UIWQDYHWWI2WXNPFBLBKR2Q4GEWXFB) - [`mergefi-milestones`](https://stellar.expert/explorer/testnet/contract/CBBRLSL6TM6XCNP2XBVT4GFHJ3NNPFKI2BCZQJ4U3TI7GV7DO2F2HG6F) @@ -598,7 +627,7 @@ stellar contract deploy \ # then, e.g. stellar contract invoke \ --id --source mergefi-admin --network testnet \ - -- initialize --admin --treasury --fee_bps 250 + -- initialize --admin --oracle --treasury --fee_bps 250 --recovery none ``` Or, in an environment where the CLI's own network calls are blocked but @@ -607,7 +636,7 @@ plain Node.js `fetch` works (as was the case here): ```sh node scripts/deploy.mjs target/wasm32v1-none/release/mergefi_escrow.wasm escrow node scripts/invoke.mjs initialize \ - address: address: u32:250 + address: address: address: u32:250 none ``` Both scripts default to Stellar testnet but can be pointed at other networks @@ -633,6 +662,28 @@ TOKEN= \ ./scripts/examples/deposit-withdraw-flow.sh ``` +## Upgrading and Emergency Operations + +All three contracts now initialize with a separate high-trust `admin` and +routine `oracle` address. The oracle signs frequent backend payout calls +(`release`, `release_issue`, `withdraw`); the admin signs rare operational +actions (`pause`, `unpause`, `upgrade`, and key rotation). + +`upgrade(new_wasm_hash)` is admin-gated and calls Soroban's +`env.deployer().update_current_contract_wasm(...)`, preserving the current +contract address and existing storage. Instance storage includes +`DataKey::Version`, exposed through `get_version()`, so future versions can +detect old layouts and run lazy or explicit migrations. + +During an incident, admin can call `pause()` to block new deposits/funding +and oracle-controlled payout paths while leaving refund/recovery-style +paths available where the contract supports them. See: + +- `docs/upgrade-storage-migration-design.md` +- `docs/pause-circuit-breaker-design.md` +- `docs/two-key-admin-oracle-design.md` +- `docs/maintenance-pool-pagination-analysis.md` + ## Roadmap - Extract shared split/fee math (`compute_split`) into a common @@ -641,9 +692,8 @@ TOKEN= \ - Emit contract events (`env.events().publish(...)`) on fund/release/refund so the backend can index state changes from the ledger directly instead of only polling `get_*` view calls. -- Consider a two-key admin model (oracle key for routine `release` calls, - separate higher-trust key for `initialize`/admin rotation) once the - contracts move past initial testnet iteration. +- Consider a timelock or multisig-admin wrapper for high-trust operations + after the two-key contract-level admin/oracle split has been audited. - Support partial milestone/pool refunds and issue re-allocation (currently `allocate` is one-shot per issue). - Add integration tests against `stellar-cli`'s local sandbox network diff --git a/contracts/common/src/lib.rs b/contracts/common/src/lib.rs index c90893e..b32a225 100644 --- a/contracts/common/src/lib.rs +++ b/contracts/common/src/lib.rs @@ -17,6 +17,19 @@ where env.storage().instance().get(&K::admin_key()) } +/// Trait to identify the Oracle key for a contract's DataKey enum. +/// Oracle is authorized for routine operations like release/withdraw. +pub trait OracleKey { + fn oracle_key() -> Self; +} + +pub fn require_oracle(env: &Env) -> Option
+where + K: OracleKey + IntoVal, +{ + env.storage().instance().get(&K::oracle_key()) +} + /// Trait to identify the Treasury key for a contract's DataKey enum pub trait TreasuryKey { fn treasury_key() -> Self; diff --git a/contracts/escrow/src/error.rs b/contracts/escrow/src/error.rs index c2de25c..1097f5b 100644 --- a/contracts/escrow/src/error.rs +++ b/contracts/escrow/src/error.rs @@ -26,4 +26,6 @@ pub enum Error { InvalidTreasury = 17, /// A milestone with this id already exists (issue #41). MilestoneAlreadyExists = 18, + /// Contract is paused and this operation is not allowed (issue #14). + ContractPaused = 19, } diff --git a/contracts/escrow/src/lib.rs b/contracts/escrow/src/lib.rs index a5588ce..bd60fff 100644 --- a/contracts/escrow/src/lib.rs +++ b/contracts/escrow/src/lib.rs @@ -15,9 +15,12 @@ mod test; use error::Error; use mergefi_common::{BPS_DENOMINATOR, MAX_SPONSORS}; -use soroban_sdk::{contract, contractimpl, token, Address, Env, Vec}; +use soroban_sdk::{contract, contractimpl, token, Address, BytesN, Env, Vec}; use types::{Contribution, DataKey, Escrow, EscrowStatus}; +/// Current version of the storage schema. Incremented on breaking layout changes. +const CONTRACT_VERSION: u32 = 1; + /// Minimum grace period (in seconds) after the deadline before anyone can permissionlessly trigger a refund. /// This prevents a race condition where a legitimate release in-flight near the deadline gets front-run by a refund. pub const GRACE_PERIOD: u64 = 14 * 24 * 60 * 60; // 14 days @@ -27,18 +30,15 @@ pub struct EscrowContract; #[contractimpl] impl EscrowContract { - /// One-time setup. `admin` is the mergefi-backend oracle address that is - /// authorized to call `release`/`refund` early; `treasury` receives the - /// protocol fee; `fee_bps` is the fee charged on every payout, expressed - /// in basis points (1/100th of a percent), e.g. 250 = 2.5%. + /// One-time setup. `admin` is the high-trust admin address for infrastructure + /// operations (pause/unpause, upgrade); `oracle` is the mergefi-backend + /// address authorized for routine `release` calls. Both addresses must + /// authorize the initialize call. `treasury` receives the protocol fee; + /// `fee_bps` is the fee charged on every payout (in basis points). /// /// Requires `admin`'s own authorization, so nobody can name a - /// third-party address as admin without that address's consent. This - /// does *not* prevent an attacker from front-running the legitimate - /// deployer's `initialize` call by naming themselves as admin instead - /// — closing that race requires an atomic deploy+init (a Soroban - /// constructor) rather than an in-contract check; see - /// `docs/access-control-audit.md`. + /// third-party address as admin without that address's consent. See + /// `docs/access-control-audit.md` and `docs/two-key-admin-oracle-design.md`. /// /// `max_sponsors` caps how many distinct contributions a single escrow /// may accumulate (see `contribute`); pass `None` to use the default @@ -46,11 +46,13 @@ impl EscrowContract { pub fn initialize( env: Env, admin: Address, + oracle: Address, treasury: Address, fee_bps: u32, max_sponsors: Option, ) -> Result<(), Error> { admin.require_auth(); + oracle.require_auth(); if env.storage().instance().has(&DataKey::Admin) { return Err(Error::AlreadyInitialized); @@ -63,11 +65,16 @@ impl EscrowContract { } env.storage().instance().set(&DataKey::Admin, &admin); + env.storage().instance().set(&DataKey::Oracle, &oracle); env.storage().instance().set(&DataKey::Treasury, &treasury); env.storage().instance().set(&DataKey::FeeBps, &fee_bps); env.storage() .instance() .set(&DataKey::MaxSponsors, &max_sponsors.unwrap_or(MAX_SPONSORS)); + env.storage() + .instance() + .set(&DataKey::Version, &CONTRACT_VERSION); + env.storage().instance().set(&DataKey::Paused, &false); extend_instance_ttl(&env); Ok(()) } @@ -81,6 +88,8 @@ impl EscrowContract { /// `docs/escrow-crowdfunding-design.md` for why creation and /// contribution are kept as two separate entrypoints. /// + /// Blocked when the contract is paused (issue #14). + /// /// `target` (issue #144) is an optional, informational funding goal — /// `Some(n)` requires `n > 0` (`InvalidTarget` otherwise), `None` means /// no goal is tracked (the pre-#144 behavior). Purely a UI hint: it is @@ -103,6 +112,10 @@ impl EscrowContract { deadline: u64, target: Option, ) -> Result<(), Error> { + if is_paused(&env) { + return Err(Error::ContractPaused); + } + sponsor.require_auth(); if amount <= 0 { @@ -160,13 +173,17 @@ impl EscrowContract { /// a top-up can never silently use a different asset than the original /// funder intended. Rejects `EscrowNotFound`, `AlreadyPaid`, /// `AlreadyRefunded`, and `TooManySponsors` once `MAX_SPONSORS` - /// contributions have already been recorded. + /// contributions have already been recorded. Blocked when paused (issue #14). pub fn contribute( env: Env, issue_id: u64, sponsor: Address, amount: i128, ) -> Result<(), Error> { + if is_paused(&env) { + return Err(Error::ContractPaused); + } + sponsor.require_auth(); if amount <= 0 { @@ -247,10 +264,15 @@ impl EscrowContract { /// configured at `initialize`) is deducted from the total and sent to /// the treasury; the remainder is split across recipients pro-rata. /// - /// Only the admin (mergefi-backend oracle) may call this. + /// Only the oracle (routine release operations) may call this. + /// Blocked when paused (issue #14). pub fn release(env: Env, issue_id: u64, recipients: Vec<(Address, u32)>) -> Result<(), Error> { - let admin = require_admin(&env)?; - admin.require_auth(); + if is_paused(&env) { + return Err(Error::ContractPaused); + } + + let oracle = require_oracle(&env)?; + oracle.require_auth(); let key = DataKey::Escrow(issue_id); let mut escrow: Escrow = env @@ -367,7 +389,7 @@ impl EscrowContract { /// single-sponsor analysis this generalizes. `new_deadline` must be /// strictly later than both the current stored deadline and the /// current ledger time, so this can only ever delay the permissionless - /// window, never shorten it. + /// window, never shorten it. Blocked when paused (issue #14). /// /// # What setting a far-future `new_deadline` does and does not guarantee /// @@ -396,6 +418,10 @@ impl EscrowContract { caller: Address, new_deadline: u64, ) -> Result<(), Error> { + if is_paused(&env) { + return Err(Error::ContractPaused); + } + caller.require_auth(); let key = DataKey::Escrow(issue_id); @@ -530,6 +556,77 @@ impl EscrowContract { Ok(contributions) } + /// Pause the contract, blocking new `fund`, `contribute`, `release`, and + /// `extend_deadline` calls. Refunds remain available so users can exit. + /// Only callable by the admin. See docs/pause-circuit-breaker-design.md. + pub fn pause(env: Env) -> Result<(), Error> { + let admin = require_admin(&env)?; + admin.require_auth(); + + env.storage().instance().set(&DataKey::Paused, &true); + extend_instance_ttl(&env); + Ok(()) + } + + /// Unpause the contract, restoring normal operation. Only callable by admin. + pub fn unpause(env: Env) -> Result<(), Error> { + let admin = require_admin(&env)?; + admin.require_auth(); + + env.storage().instance().set(&DataKey::Paused, &false); + extend_instance_ttl(&env); + Ok(()) + } + + /// Check if the contract is paused. + pub fn is_paused_view(env: Env) -> bool { + env.storage() + .instance() + .get(&DataKey::Paused) + .unwrap_or(false) + } + + /// Rotate the admin key. Requires current admin's authorization. + /// New admin must also authorize the change. + pub fn set_admin(env: Env, new_admin: Address) -> Result<(), Error> { + let admin = require_admin(&env)?; + admin.require_auth(); + + new_admin.require_auth(); + + env.storage().instance().set(&DataKey::Admin, &new_admin); + extend_instance_ttl(&env); + Ok(()) + } + + /// Rotate the oracle key. Requires admin's authorization (not oracle's). + /// New oracle must also authorize the change. + pub fn set_oracle(env: Env, new_oracle: Address) -> Result<(), Error> { + let admin = require_admin(&env)?; + admin.require_auth(); + + new_oracle.require_auth(); + + env.storage().instance().set(&DataKey::Oracle, &new_oracle); + extend_instance_ttl(&env); + Ok(()) + } + + /// Upgrade the contract's wasm code. Requires admin authorization. + /// Preserves all existing storage and updates the version flag. + /// See docs/upgrade-storage-migration-design.md. + pub fn upgrade(env: Env, new_wasm_hash: BytesN<32>) -> Result<(), Error> { + let admin = require_admin(&env)?; + admin.require_auth(); + + env.deployer().update_current_contract_wasm(new_wasm_hash); + env.storage() + .instance() + .set(&DataKey::Version, &CONTRACT_VERSION); + extend_instance_ttl(&env); + Ok(()) + } + pub fn get_admin(env: Env) -> Result { env.storage() .instance() @@ -537,6 +634,13 @@ impl EscrowContract { .ok_or(Error::NotInitialized) } + pub fn get_oracle(env: Env) -> Result { + env.storage() + .instance() + .get(&DataKey::Oracle) + .ok_or(Error::NotInitialized) + } + pub fn get_treasury(env: Env) -> Result { env.storage() .instance() @@ -551,6 +655,10 @@ impl EscrowContract { .ok_or(Error::NotInitialized) } + pub fn get_version(env: Env) -> u32 { + env.storage().instance().get(&DataKey::Version).unwrap_or(0) + } + pub fn get_max_sponsors(env: Env) -> Result { env.storage() .instance() @@ -563,6 +671,17 @@ pub(crate) fn require_admin(env: &Env) -> Result { mergefi_common::require_admin::(env).ok_or(Error::NotInitialized) } +pub(crate) fn require_oracle(env: &Env) -> Result { + mergefi_common::require_oracle::(env).ok_or(Error::NotInitialized) +} + +fn is_paused(env: &Env) -> bool { + env.storage() + .instance() + .get(&DataKey::Paused) + .unwrap_or(false) +} + /// Extends the TTL of a persistent entry so escrow records aren't archived /// while still active. Threshold/extend values are conservative defaults /// suitable for a multi-month bounty lifecycle. diff --git a/contracts/escrow/src/test.rs b/contracts/escrow/src/test.rs index b377849..c9ac350 100644 --- a/contracts/escrow/src/test.rs +++ b/contracts/escrow/src/test.rs @@ -21,10 +21,11 @@ fn create_token<'a>( fn setup(env: &Env) -> (Address, Address, Address, EscrowContractClient<'_>) { let admin = Address::generate(env); + let oracle = Address::generate(env); let treasury = Address::generate(env); let contract_id = env.register(EscrowContract, ()); let client = EscrowContractClient::new(env, &contract_id); - client.initialize(&admin, &treasury, &500u32, &None); // 5% fee + client.initialize(&admin, &oracle, &treasury, &500u32, &None); // 5% fee (contract_id, admin, treasury, client) } @@ -33,7 +34,8 @@ fn test_initialize_rejects_double_init() { let env = Env::default(); env.mock_all_auths(); let (_, admin, treasury, client) = setup(&env); - let err = client.try_initialize(&admin, &treasury, &500u32, &None); + let oracle = Address::generate(&env); + let err = client.try_initialize(&admin, &oracle, &treasury, &500u32, &None); assert_eq!(err, Err(Ok(Error::AlreadyInitialized))); } @@ -42,11 +44,12 @@ fn test_initialize_rejects_fee_bps_above_10000() { let env = Env::default(); env.mock_all_auths(); let admin = Address::generate(&env); + let oracle = Address::generate(&env); let treasury = Address::generate(&env); let contract_id = env.register(EscrowContract, ()); let client = EscrowContractClient::new(&env, &contract_id); - let err = client.try_initialize(&admin, &treasury, &10_001u32, &None); + let err = client.try_initialize(&admin, &oracle, &treasury, &10_001u32, &None); assert_eq!(err, Err(Ok(Error::InvalidFee))); } @@ -55,11 +58,12 @@ fn test_initialize_accepts_fee_bps_at_boundary_10000() { let env = Env::default(); env.mock_all_auths(); let admin = Address::generate(&env); + let oracle = Address::generate(&env); let treasury = Address::generate(&env); let contract_id = env.register(EscrowContract, ()); let client = EscrowContractClient::new(&env, &contract_id); - client.initialize(&admin, &treasury, &10_000u32, &None); + client.initialize(&admin, &oracle, &treasury, &10_000u32, &None); assert_eq!(client.get_fee_bps(), 10_000u32); } @@ -317,12 +321,13 @@ fn test_adversarial_ordering_resistance() { // 1. Setup contract and environment let admin = Address::generate(&env); + let oracle = Address::generate(&env); let treasury = Address::generate(&env); let contract_id = env.register(crate::EscrowContract, ()); let client = crate::EscrowContractClient::new(&env, &contract_id); // Initialize with 0% fee to simplify fraction/dust calculations - client.initialize(&admin, &treasury, &0u32, &None); + client.initialize(&admin, &oracle, &treasury, &0u32, &None); // 2. Create recipient addresses let dev1 = Address::generate(&env); @@ -382,11 +387,12 @@ fn test_large_split_distributes_dust_by_largest_remainder() { env.mock_all_auths(); let admin = Address::generate(&env); + let oracle = Address::generate(&env); let treasury = Address::generate(&env); let contract_id = env.register(crate::EscrowContract, ()); let client = crate::EscrowContractClient::new(&env, &contract_id); // 0% fee so the whole total is distributable. - client.initialize(&admin, &treasury, &0u32, &None); + client.initialize(&admin, &oracle, &treasury, &0u32, &None); // 60 recipients: 59 with alternating 160/170 bps, the last one receiving // the leftover of 10000. All 170-bps recipients share an identical @@ -470,11 +476,12 @@ fn test_initialize_requires_admin_auth() { let env = Env::default(); // No auths mocked at all. let admin = Address::generate(&env); + let oracle = Address::generate(&env); let treasury = Address::generate(&env); let contract_id = env.register(EscrowContract, ()); let client = EscrowContractClient::new(&env, &contract_id); - let result = client.try_initialize(&admin, &treasury, &500u32, &None); + let result = client.try_initialize(&admin, &oracle, &treasury, &500u32, &None); assert!(result.is_err()); } @@ -903,10 +910,11 @@ fn test_initialize_accepts_a_custom_max_sponsors() { let env = Env::default(); env.mock_all_auths(); let admin = Address::generate(&env); + let oracle = Address::generate(&env); let treasury = Address::generate(&env); let contract_id = env.register(EscrowContract, ()); let client = EscrowContractClient::new(&env, &contract_id); - client.initialize(&admin, &treasury, &500u32, &Some(2u32)); + client.initialize(&admin, &oracle, &treasury, &500u32, &Some(2u32)); assert_eq!(client.get_max_sponsors(), 2u32); @@ -1091,6 +1099,88 @@ fn test_release_loses_race_to_refund_at_grace_period_boundary() { assert_eq!(token_client.balance(&contributor), 0); } +#[test] +fn test_pause_blocks_mutating_entrypoints_but_allows_expired_refund() { + let env = Env::default(); + env.mock_all_auths(); + let (_, _admin, _treasury, client) = setup(&env); + + let token_admin = Address::generate(&env); + let (token_addr, asset_client, token_client) = create_token(&env, &token_admin); + let sponsor = Address::generate(&env); + asset_client.mint(&sponsor, &20_000i128); + let contributor = Address::generate(&env); + + env.ledger().set_timestamp(100); + client.fund(&300u64, &sponsor, &token_addr, &10_000i128, &200u64, &None); + client.pause(); + assert!(client.is_paused_view()); + + let err = client.try_fund(&301u64, &sponsor, &token_addr, &1_000i128, &200u64, &None); + assert_eq!(err, Err(Ok(Error::ContractPaused))); + + let err = client.try_contribute(&300u64, &sponsor, &1_000i128); + assert_eq!(err, Err(Ok(Error::ContractPaused))); + + let err = client.try_extend_deadline(&300u64, &sponsor, &500u64); + assert_eq!(err, Err(Ok(Error::ContractPaused))); + + let err = client.try_release(&300u64, &vec![&env, (contributor, 10_000u32)]); + assert_eq!(err, Err(Ok(Error::ContractPaused))); + + env.ledger().set_timestamp(200 + crate::GRACE_PERIOD); + env.set_auths(&[]); + client.refund(&300u64); + assert_eq!(token_client.balance(&sponsor), 20_000i128); + assert_eq!(client.get_escrow(&300u64).status, EscrowStatus::Refunded); +} + +#[test] +fn test_unpause_restores_funding() { + let env = Env::default(); + env.mock_all_auths(); + let (_, _admin, _treasury, client) = setup(&env); + + let token_admin = Address::generate(&env); + let (token_addr, asset_client, _token_client) = create_token(&env, &token_admin); + let sponsor = Address::generate(&env); + asset_client.mint(&sponsor, &10_000i128); + + client.pause(); + assert!(client.is_paused_view()); + client.unpause(); + assert!(!client.is_paused_view()); + + client.fund( + &302u64, + &sponsor, + &token_addr, + &10_000i128, + &1_000u64, + &None, + ); + assert_eq!(client.get_escrow(&302u64).status, EscrowStatus::Funded); +} + +#[test] +fn test_oracle_is_stored_and_rotatable() { + let env = Env::default(); + env.mock_all_auths(); + let admin = Address::generate(&env); + let oracle = Address::generate(&env); + let treasury = Address::generate(&env); + let contract_id = env.register(EscrowContract, ()); + let client = EscrowContractClient::new(&env, &contract_id); + + client.initialize(&admin, &oracle, &treasury, &500u32, &None); + assert_eq!(client.get_oracle(), oracle); + assert_eq!(client.get_version(), 1); + + let new_oracle = Address::generate(&env); + client.set_oracle(&new_oracle); + assert_eq!(client.get_oracle(), new_oracle); +} + // ── extend_deadline / keep_alive TTL scaling (#56) ───────────────────────── // // extend_ttl's flat ~29-day (500_000-ledger) bump applied regardless of how diff --git a/contracts/escrow/src/types.rs b/contracts/escrow/src/types.rs index cb80b51..4054b14 100644 --- a/contracts/escrow/src/types.rs +++ b/contracts/escrow/src/types.rs @@ -44,9 +44,12 @@ pub struct Contribution { #[derive(Clone)] pub enum DataKey { Admin, + Oracle, // NEW: Oracle role for release operations Treasury, FeeBps, MaxSponsors, + Paused, // NEW: Pause/unpause flag + Version, // NEW: Storage version for migrations Escrow(u64), Contribution(u64, u32), // (issue_id, contribution_index) } @@ -57,6 +60,12 @@ impl mergefi_common::AdminKey for DataKey { } } +impl mergefi_common::OracleKey for DataKey { + fn oracle_key() -> Self { + DataKey::Oracle + } +} + impl mergefi_common::TreasuryKey for DataKey { fn treasury_key() -> Self { DataKey::Treasury diff --git a/contracts/maintenance-pool/src/error.rs b/contracts/maintenance-pool/src/error.rs index 827d487..57e3785 100644 --- a/contracts/maintenance-pool/src/error.rs +++ b/contracts/maintenance-pool/src/error.rs @@ -21,6 +21,11 @@ pub enum Error { NotDepositSponsor = 11, /// The deposit index is out of range (issue #42). DepositNotFound = 12, + feature/upgrade-pause-pagination-separation + /// Contract is paused and this operation is not allowed (issue #14). + ContractPaused = 13, + /// The deposit count has reached its maximum limit (issue #45). DepositCountOverflow = 13, + main } diff --git a/contracts/maintenance-pool/src/lib.rs b/contracts/maintenance-pool/src/lib.rs index c9933a2..fa83f7c 100644 --- a/contracts/maintenance-pool/src/lib.rs +++ b/contracts/maintenance-pool/src/lib.rs @@ -15,7 +15,7 @@ mod types; mod test; use error::Error; -use soroban_sdk::{contract, contractimpl, token, Address, Env}; +use soroban_sdk::{contract, contractimpl, token, Address, BytesN, Env}; use types::{DataKey, Deposit, MaintenancePool}; use mergefi_common::BPS_DENOMINATOR; @@ -27,6 +27,9 @@ use mergefi_common::BPS_DENOMINATOR; /// concept but applied per-deposit rather than per-pool. pub const INACTIVITY_WINDOW: u64 = 90 * 24 * 60 * 60; // 90 days +/// Current version of the storage schema. Incremented on breaking layout changes. +const CONTRACT_VERSION: u32 = 1; + #[contract] pub struct MaintenancePoolContract; @@ -39,11 +42,13 @@ impl MaintenancePoolContract { pub fn initialize( env: Env, admin: Address, + oracle: Address, treasury: Address, fee_bps: u32, recovery: Option
, ) -> Result<(), Error> { admin.require_auth(); + oracle.require_auth(); if env.storage().instance().has(&DataKey::Admin) { return Err(Error::AlreadyInitialized); @@ -55,8 +60,13 @@ impl MaintenancePoolContract { return Err(Error::InvalidTreasury); } env.storage().instance().set(&DataKey::Admin, &admin); + env.storage().instance().set(&DataKey::Oracle, &oracle); env.storage().instance().set(&DataKey::Treasury, &treasury); env.storage().instance().set(&DataKey::FeeBps, &fee_bps); + env.storage() + .instance() + .set(&DataKey::Version, &CONTRACT_VERSION); + env.storage().instance().set(&DataKey::Paused, &false); if let Some(r) = recovery { env.storage().instance().set(&DataKey::Recovery, &r); } @@ -76,6 +86,10 @@ impl MaintenancePoolContract { token: Address, amount: i128, ) -> Result<(), Error> { + if is_paused(&env) { + return Err(Error::ContractPaused); + } + sponsor.require_auth(); if amount <= 0 { @@ -141,7 +155,11 @@ impl MaintenancePoolContract { /// backend oracle for completed maintenance work. Rejects if the pool /// balance is insufficient. pub fn withdraw(env: Env, pool_id: u64, recipient: Address, amount: i128) -> Result<(), Error> { - require_admin(&env)?.require_auth(); + if is_paused(&env) { + return Err(Error::ContractPaused); + } + + require_oracle(&env)?.require_auth(); if amount <= 0 { return Err(Error::InvalidAmount); @@ -347,6 +365,47 @@ impl MaintenancePoolContract { Ok(surplus) } + /// Pause the contract, blocking new deposits and routine withdrawals. + /// Reclaim and sweep paths remain available for recovery. + pub fn pause(env: Env) -> Result<(), Error> { + let admin = require_admin(&env)?; + admin.require_auth(); + + env.storage().instance().set(&DataKey::Paused, &true); + extend_instance_ttl(&env); + Ok(()) + } + + /// Unpause the contract, restoring normal operation. + pub fn unpause(env: Env) -> Result<(), Error> { + let admin = require_admin(&env)?; + admin.require_auth(); + + env.storage().instance().set(&DataKey::Paused, &false); + extend_instance_ttl(&env); + Ok(()) + } + + pub fn is_paused_view(env: Env) -> bool { + env.storage() + .instance() + .get(&DataKey::Paused) + .unwrap_or(false) + } + + /// Upgrade the contract's wasm code. Requires admin authorization. + pub fn upgrade(env: Env, new_wasm_hash: BytesN<32>) -> Result<(), Error> { + let admin = require_admin(&env)?; + admin.require_auth(); + + env.deployer().update_current_contract_wasm(new_wasm_hash); + env.storage() + .instance() + .set(&DataKey::Version, &CONTRACT_VERSION); + extend_instance_ttl(&env); + Ok(()) + } + pub fn get_pool(env: Env, pool_id: u64) -> Result { env.storage() .persistent() @@ -358,7 +417,7 @@ impl MaintenancePoolContract { env.storage() .persistent() .get(&DataKey::Deposit(pool_id, index)) - .ok_or(Error::PoolNotFound) + .ok_or(Error::DepositNotFound) } pub fn get_admin(env: Env) -> Result { @@ -375,15 +434,37 @@ impl MaintenancePoolContract { .ok_or(Error::NotInitialized) } + pub fn get_oracle(env: Env) -> Result { + env.storage() + .instance() + .get(&DataKey::Oracle) + .ok_or(Error::NotInitialized) + } + pub fn get_fee_bps(env: Env) -> Result { env.storage() .instance() .get(&DataKey::FeeBps) .ok_or(Error::NotInitialized) } + + pub fn get_version(env: Env) -> u32 { + env.storage().instance().get(&DataKey::Version).unwrap_or(0) + } + pub fn set_admin(env: Env, new_admin: Address) -> Result<(), Error> { require_admin(&env)?.require_auth(); + new_admin.require_auth(); env.storage().instance().set(&DataKey::Admin, &new_admin); + extend_instance_ttl(&env); + Ok(()) + } + + pub fn set_oracle(env: Env, new_oracle: Address) -> Result<(), Error> { + require_admin(&env)?.require_auth(); + new_oracle.require_auth(); + env.storage().instance().set(&DataKey::Oracle, &new_oracle); + extend_instance_ttl(&env); Ok(()) } @@ -394,7 +475,9 @@ impl MaintenancePoolContract { .get(&DataKey::Recovery) .ok_or(Error::NotInitialized)?; recovery.require_auth(); + new_admin.require_auth(); env.storage().instance().set(&DataKey::Admin, &new_admin); + extend_instance_ttl(&env); Ok(()) } @@ -403,6 +486,7 @@ impl MaintenancePoolContract { env.storage() .instance() .set(&DataKey::Treasury, &new_treasury); + extend_instance_ttl(&env); Ok(()) } } @@ -411,6 +495,17 @@ fn require_admin(env: &Env) -> Result { mergefi_common::require_admin::(env).ok_or(Error::NotInitialized) } +fn require_oracle(env: &Env) -> Result { + mergefi_common::require_oracle::(env).ok_or(Error::NotInitialized) +} + +fn is_paused(env: &Env) -> bool { + env.storage() + .instance() + .get(&DataKey::Paused) + .unwrap_or(false) +} + fn extend_ttl(env: &Env, key: &DataKey) { mergefi_common::extend_ttl(env, key); } diff --git a/contracts/maintenance-pool/src/test.rs b/contracts/maintenance-pool/src/test.rs index 7636249..8237822 100644 --- a/contracts/maintenance-pool/src/test.rs +++ b/contracts/maintenance-pool/src/test.rs @@ -1,7 +1,10 @@ #![cfg(test)] use super::*; -use soroban_sdk::{testutils::Address as _, token, Address, Env}; +use soroban_sdk::{ + testutils::{Address as _, Ledger as _}, + token, Address, Env, +}; fn create_token<'a>( env: &Env, @@ -18,10 +21,11 @@ fn create_token<'a>( fn setup(env: &Env) -> (Address, Address, MaintenancePoolContractClient<'_>) { let admin = Address::generate(env); + let oracle = Address::generate(env); let treasury = Address::generate(env); let contract_id = env.register(MaintenancePoolContract, ()); let client = MaintenancePoolContractClient::new(env, &contract_id); - client.initialize(&admin, &treasury, &1_000u32, &None); // 10% fee + client.initialize(&admin, &oracle, &treasury, &1_000u32, &None); // 10% fee (admin, treasury, client) } @@ -34,6 +38,7 @@ fn test_get_admin_treasury_fee_bps() { assert_eq!(client.get_admin(), admin); assert_eq!(client.get_treasury(), treasury); assert_eq!(client.get_fee_bps(), 1_000u32); + assert_eq!(client.get_version(), 1); } #[test] @@ -158,11 +163,12 @@ fn test_deposit_rejects_token_mismatch() { fn test_initialize_requires_admin_auth() { let env = Env::default(); let admin = Address::generate(&env); + let oracle = Address::generate(&env); let treasury = Address::generate(&env); let contract_id = env.register(MaintenancePoolContract, ()); let client = MaintenancePoolContractClient::new(&env, &contract_id); - let result = client.try_initialize(&admin, &treasury, &1_000u32, &None); + let result = client.try_initialize(&admin, &oracle, &treasury, &1_000u32, &None); assert!(result.is_err()); } @@ -205,15 +211,17 @@ fn test_initialize_rejects_double_init() { let env = Env::default(); env.mock_all_auths(); let admin = Address::generate(&env); + let oracle = Address::generate(&env); let treasury = Address::generate(&env); let contract_id = env.register(MaintenancePoolContract, ()); let client = MaintenancePoolContractClient::new(&env, &contract_id); // First initialization succeeds - client.initialize(&admin, &treasury, &1_000u32, &None); + client.initialize(&admin, &oracle, &treasury, &1_000u32, &None); // Second initialization should fail with AlreadyInitialized - let result = client.try_initialize(&admin, &treasury, &1_000u32, &None); + let new_oracle = Address::generate(&env); + let result = client.try_initialize(&admin, &new_oracle, &treasury, &1_000u32, &None); assert_eq!(result, Err(Ok(Error::AlreadyInitialized))); } @@ -222,12 +230,13 @@ fn test_initialize_rejects_invalid_fee() { let env = Env::default(); env.mock_all_auths(); let admin = Address::generate(&env); + let oracle = Address::generate(&env); let treasury = Address::generate(&env); let contract_id = env.register(MaintenancePoolContract, ()); let client = MaintenancePoolContractClient::new(&env, &contract_id); // fee_bps > 10000 should fail with InvalidFee - let result = client.try_initialize(&admin, &treasury, &10_001u32, &None); + let result = client.try_initialize(&admin, &oracle, &treasury, &10_001u32, &None); assert_eq!(result, Err(Ok(Error::InvalidFee))); } @@ -539,6 +548,7 @@ fn test_sweep_rejects_mismatched_token() { fn test_recover_withdraw_frozen_before_recoverable_after() { let env = Env::default(); let admin = Address::generate(&env); + let oracle = Address::generate(&env); let treasury = Address::generate(&env); let recovery = Address::generate(&env); let contract_id = env.register(MaintenancePoolContract, ()); @@ -546,7 +556,13 @@ fn test_recover_withdraw_frozen_before_recoverable_after() { // Initialize with a recovery address env.mock_all_auths(); - client.initialize(&admin, &treasury, &1_000u32, &Some(recovery.clone())); + client.initialize( + &admin, + &oracle, + &treasury, + &1_000u32, + &Some(recovery.clone()), + ); // Deposit into pool let token_admin = Address::generate(&env); @@ -572,14 +588,53 @@ fn test_recover_withdraw_frozen_before_recoverable_after() { } #[test] -fn test_deposit_rejects_when_deposit_count_would_overflow() { + feature/upgrade-pause-pagination-separation +fn test_pause_blocks_deposit_and_withdraw_but_allows_reclaim_after_inactivity() { let env = Env::default(); env.mock_all_auths(); let (_admin, _treasury, client) = setup(&env); let token_admin = Address::generate(&env); - let (token_addr, asset_client, _token_client) = create_token(&env, &token_admin); + let (token_addr, asset_client, token_client) = create_token(&env, &token_admin); let sponsor = Address::generate(&env); + let maintainer = Address::generate(&env); + asset_client.mint(&sponsor, &10_000_000_000i128); + + client.deposit(&77u64, &sponsor, &token_addr, &10_000_000_000i128); + client.pause(); + assert!(client.is_paused_view()); + + let deposit_err = client.try_deposit(&78u64, &sponsor, &token_addr, &1_000_000_000i128); + assert_eq!(deposit_err, Err(Ok(Error::ContractPaused))); + + let withdraw_err = client.try_withdraw(&77u64, &maintainer, &1_000_000_000i128); + assert_eq!(withdraw_err, Err(Ok(Error::ContractPaused))); + + env.ledger().set_timestamp(INACTIVITY_WINDOW + 1); + client.reclaim_deposit(&77u64, &0u32, &sponsor); + assert_eq!(token_client.balance(&sponsor), 10_000_000_000i128); +} + +#[test] +fn test_unpause_restores_deposit() { + +fn test_deposit_rejects_when_deposit_count_would_overflow() main + let env = Env::default(); + env.mock_all_auths(); + let (_admin, _treasury, client) = setup(&env); + + let token_admin = Address::generate(&env); + let (token_addr, asset_client, _token_client) = create_token(&env, &token_admin); + let sponsor = Address::generate(&env) feature/upgrade-pause-pagination-separation + asset_client.mint(&sponsor, &2_000_000_000i128); + + client.pause(); + client.unpause(); + assert!(!client.is_paused_view()); + + client.deposit(&88u64, &sponsor, &token_addr, &2_000_000_000i128); + assert_eq!(client.get_pool(&88u64).balance, 2_000_000_000i128); + asset_client.mint(&sponsor, &1_000i128); // Direct storage manipulation: set deposit_count to u32::MAX @@ -599,5 +654,5 @@ fn test_deposit_rejects_when_deposit_count_would_overflow() { // Calling deposit should now fail with DepositCountOverflow let err = client.try_deposit(&10u64, &sponsor, &token_addr, &100i128); - assert_eq!(err, Err(Ok(Error::DepositCountOverflow))); + assert_eq!(err, Err(Ok(Error::DepositCountOverflow))); main } diff --git a/contracts/maintenance-pool/src/types.rs b/contracts/maintenance-pool/src/types.rs index 9b91f09..7f95ce2 100644 --- a/contracts/maintenance-pool/src/types.rs +++ b/contracts/maintenance-pool/src/types.rs @@ -32,9 +32,12 @@ pub struct Deposit { #[derive(Clone)] pub enum DataKey { Admin, + Oracle, Recovery, Treasury, FeeBps, + Paused, + Version, Pool(u64), Deposit(u64, u32), // (pool_id, deposit_index) } @@ -45,6 +48,12 @@ impl mergefi_common::AdminKey for DataKey { } } +impl mergefi_common::OracleKey for DataKey { + fn oracle_key() -> Self { + DataKey::Oracle + } +} + impl mergefi_common::TreasuryKey for DataKey { fn treasury_key() -> Self { DataKey::Treasury diff --git a/contracts/milestones/src/error.rs b/contracts/milestones/src/error.rs index 193ee74..80902e6 100644 --- a/contracts/milestones/src/error.rs +++ b/contracts/milestones/src/error.rs @@ -26,4 +26,6 @@ pub enum Error { DeadlineNotPassed = 16, /// The issue is not allocated, so it cannot be deallocated (issue #43). IssueNotAllocatedForDeallocate = 17, + /// Contract is paused and this operation is not allowed (issue #14). + ContractPaused = 18, } diff --git a/contracts/milestones/src/lib.rs b/contracts/milestones/src/lib.rs index fcddf6d..031925e 100644 --- a/contracts/milestones/src/lib.rs +++ b/contracts/milestones/src/lib.rs @@ -18,7 +18,7 @@ mod types; mod test; use error::Error; -use soroban_sdk::{contract, contractimpl, token, Address, Env, Map, Vec}; +use soroban_sdk::{contract, contractimpl, token, Address, BytesN, Env, Map, Vec}; use types::{Contribution, DataKey, IssueStatus, Milestone}; use mergefi_common::{BPS_DENOMINATOR, MAX_SPONSORS}; @@ -29,6 +29,9 @@ use mergefi_common::{BPS_DENOMINATOR, MAX_SPONSORS}; /// flight near the deadline gets front-run by a permissionless cancel. pub const GRACE_PERIOD: u64 = 14 * 24 * 60 * 60; // 14 days +/// Current version of the storage schema. Incremented on breaking layout changes. +const CONTRACT_VERSION: u32 = 1; + #[contract] pub struct MilestonesContract; @@ -45,12 +48,14 @@ impl MilestonesContract { pub fn initialize( env: Env, admin: Address, + oracle: Address, treasury: Address, fee_bps: u32, max_sponsors: Option, recovery: Option
, ) -> Result<(), Error> { admin.require_auth(); + oracle.require_auth(); if env.storage().instance().has(&DataKey::Admin) { return Err(Error::AlreadyInitialized); @@ -62,11 +67,16 @@ impl MilestonesContract { return Err(Error::InvalidTreasury); } env.storage().instance().set(&DataKey::Admin, &admin); + env.storage().instance().set(&DataKey::Oracle, &oracle); env.storage().instance().set(&DataKey::Treasury, &treasury); env.storage().instance().set(&DataKey::FeeBps, &fee_bps); env.storage() .instance() .set(&DataKey::MaxSponsors, &max_sponsors.unwrap_or(MAX_SPONSORS)); + env.storage() + .instance() + .set(&DataKey::Version, &CONTRACT_VERSION); + env.storage().instance().set(&DataKey::Paused, &false); if let Some(r) = recovery { // Recovery address is explicitly set once at initialization and // cannot be changed later. It allows recovery of the admin key @@ -92,6 +102,10 @@ impl MilestonesContract { total_budget: i128, deadline: u64, ) -> Result<(), Error> { + if is_paused(&env) { + return Err(Error::ContractPaused); + } + sponsor.require_auth(); if total_budget <= 0 { @@ -153,6 +167,10 @@ impl MilestonesContract { sponsor: Address, amount: i128, ) -> Result<(), Error> { + if is_paused(&env) { + return Err(Error::ContractPaused); + } + sponsor.require_auth(); if amount <= 0 { @@ -246,6 +264,10 @@ impl MilestonesContract { /// contracts instead of one" → "Cross-contract double-funding" for why /// that gap is accepted here and handled by `mergefi-backend` instead. pub fn allocate(env: Env, milestone_id: u64, issue_id: u64, amount: i128) -> Result<(), Error> { + if is_paused(&env) { + return Err(Error::ContractPaused); + } + require_admin(&env)?.require_auth(); if amount <= 0 { @@ -299,7 +321,11 @@ impl MilestonesContract { issue_id: u64, recipients: Vec<(Address, u32)>, ) -> Result<(), Error> { - require_admin(&env)?.require_auth(); + if is_paused(&env) { + return Err(Error::ContractPaused); + } + + require_oracle(&env)?.require_auth(); let mkey = DataKey::Milestone(milestone_id); let milestone: Milestone = env @@ -308,6 +334,10 @@ impl MilestonesContract { .get(&mkey) .ok_or(Error::MilestoneNotFound)?; + if milestone.closed { + return Err(Error::MilestoneClosed); + } + let skey = DataKey::IssueStatus(milestone_id, issue_id); let status: IssueStatus = env .storage() @@ -428,6 +458,12 @@ impl MilestonesContract { milestone.remaining_budget += amount; milestone.allocations.remove(issue_id); + + if milestone.closed && milestone.remaining_budget > 0 { + refund_remaining_budget(&env, milestone_id, &milestone)?; + milestone.remaining_budget = 0; + } + env.storage().persistent().set(&mkey, &milestone); extend_ttl(&env, &mkey); @@ -437,6 +473,47 @@ impl MilestonesContract { Ok(()) } + /// Pause the contract, blocking new milestones, contributions, + /// allocations, and release payouts. Refund/recovery paths remain open. + pub fn pause(env: Env) -> Result<(), Error> { + let admin = require_admin(&env)?; + admin.require_auth(); + + env.storage().instance().set(&DataKey::Paused, &true); + extend_instance_ttl(&env); + Ok(()) + } + + /// Unpause the contract, restoring normal operation. + pub fn unpause(env: Env) -> Result<(), Error> { + let admin = require_admin(&env)?; + admin.require_auth(); + + env.storage().instance().set(&DataKey::Paused, &false); + extend_instance_ttl(&env); + Ok(()) + } + + pub fn is_paused_view(env: Env) -> bool { + env.storage() + .instance() + .get(&DataKey::Paused) + .unwrap_or(false) + } + + /// Upgrade the contract's wasm code. Requires admin authorization. + pub fn upgrade(env: Env, new_wasm_hash: BytesN<32>) -> Result<(), Error> { + let admin = require_admin(&env)?; + admin.require_auth(); + + env.deployer().update_current_contract_wasm(new_wasm_hash); + env.storage() + .instance() + .set(&DataKey::Version, &CONTRACT_VERSION); + extend_instance_ttl(&env); + Ok(()) + } + /// Permissionless cancel after the milestone's deadline has passed /// (plus a grace period). Mirrors escrow's permissionless `refund`: /// anyone can trigger it once the deadline + grace period elapses, but @@ -531,10 +608,32 @@ impl MilestonesContract { .ok_or(Error::NotInitialized) } + pub fn get_oracle(env: Env) -> Result { + env.storage() + .instance() + .get(&DataKey::Oracle) + .ok_or(Error::NotInitialized) + } + + pub fn get_version(env: Env) -> u32 { + env.storage().instance().get(&DataKey::Version).unwrap_or(0) + } + /// Admin-authorized rotation: the current admin may set a new admin. pub fn set_admin(env: Env, new_admin: Address) -> Result<(), Error> { require_admin(&env)?.require_auth(); + new_admin.require_auth(); env.storage().instance().set(&DataKey::Admin, &new_admin); + extend_instance_ttl(&env); + Ok(()) + } + + /// Admin-only: rotate the routine oracle key. + pub fn set_oracle(env: Env, new_oracle: Address) -> Result<(), Error> { + require_admin(&env)?.require_auth(); + new_oracle.require_auth(); + env.storage().instance().set(&DataKey::Oracle, &new_oracle); + extend_instance_ttl(&env); Ok(()) } @@ -548,7 +647,9 @@ impl MilestonesContract { .get(&DataKey::Recovery) .ok_or(Error::NotInitialized)?; recovery.require_auth(); + new_admin.require_auth(); env.storage().instance().set(&DataKey::Admin, &new_admin); + extend_instance_ttl(&env); Ok(()) } @@ -559,6 +660,7 @@ impl MilestonesContract { env.storage() .instance() .set(&DataKey::Treasury, &new_treasury); + extend_instance_ttl(&env); Ok(()) } @@ -673,6 +775,17 @@ fn require_admin(env: &Env) -> Result { mergefi_common::require_admin::(env).ok_or(Error::NotInitialized) } +fn require_oracle(env: &Env) -> Result { + mergefi_common::require_oracle::(env).ok_or(Error::NotInitialized) +} + +fn is_paused(env: &Env) -> bool { + env.storage() + .instance() + .get(&DataKey::Paused) + .unwrap_or(false) +} + fn extend_ttl(env: &Env, key: &DataKey) { mergefi_common::extend_ttl(env, key); } diff --git a/contracts/milestones/src/test.rs b/contracts/milestones/src/test.rs index 4a7fa92..6e3c362 100644 --- a/contracts/milestones/src/test.rs +++ b/contracts/milestones/src/test.rs @@ -22,10 +22,11 @@ fn create_token<'a>( fn setup(env: &Env) -> (Address, Address, MilestonesContractClient<'_>) { let admin = Address::generate(env); + let oracle = Address::generate(env); let treasury = Address::generate(env); let contract_id = env.register(MilestonesContract, ()); let client = MilestonesContractClient::new(env, &contract_id); - client.initialize(&admin, &treasury, &500u32, &None, &None); // 5% fee, no recovery + client.initialize(&admin, &oracle, &treasury, &500u32, &None, &None); // 5% fee, no recovery (admin, treasury, client) } @@ -34,11 +35,12 @@ fn test_initialize_rejects_fee_bps_above_10000() { let env = Env::default(); env.mock_all_auths(); let admin = Address::generate(&env); + let oracle = Address::generate(&env); let treasury = Address::generate(&env); let contract_id = env.register(MilestonesContract, ()); let client = MilestonesContractClient::new(&env, &contract_id); - let err = client.try_initialize(&admin, &treasury, &10_001u32, &None, &None); + let err = client.try_initialize(&admin, &oracle, &treasury, &10_001u32, &None, &None); assert_eq!(err, Err(Ok(Error::InvalidFee))); } @@ -91,10 +93,11 @@ fn test_release_issue_with_zero_fee_pays_full_allocation() { env.mock_all_auths(); let admin = Address::generate(&env); + let oracle = Address::generate(&env); let treasury = Address::generate(&env); let contract_id = env.register(MilestonesContract, ()); let client = MilestonesContractClient::new(&env, &contract_id); - client.initialize(&admin, &treasury, &0u32, &None, &None); + client.initialize(&admin, &oracle, &treasury, &0u32, &None, &None); let token_admin = Address::generate(&env); let (token_addr, asset_client, token_client) = create_token(&env, &token_admin); @@ -158,11 +161,12 @@ fn test_large_split_distributes_dust_by_largest_remainder() { env.mock_all_auths(); let admin = Address::generate(&env); + let oracle = Address::generate(&env); let treasury = Address::generate(&env); let contract_id = env.register(crate::MilestonesContract, ()); let client = crate::MilestonesContractClient::new(&env, &contract_id); // 0% fee so the whole total is distributable. - client.initialize(&admin, &treasury, &0u32, &None, &None); + client.initialize(&admin, &oracle, &treasury, &0u32, &None, &None); // 60 recipients: 59 with alternating 160/170 bps, the last one receiving // the leftover of 10000. All 170-bps recipients share an identical @@ -310,11 +314,12 @@ fn test_cancel_milestone_refunds_remaining_budget() { fn test_initialize_requires_admin_auth() { let env = Env::default(); let admin = Address::generate(&env); + let oracle = Address::generate(&env); let treasury = Address::generate(&env); let contract_id = env.register(MilestonesContract, ()); let client = MilestonesContractClient::new(&env, &contract_id); - let result = client.try_initialize(&admin, &treasury, &500u32, &None, &None); + let result = client.try_initialize(&admin, &oracle, &treasury, &500u32, &None, &None); assert!(result.is_err()); } @@ -323,7 +328,9 @@ fn test_initialize_rejects_double_init() { let env = Env::default(); env.mock_all_auths(); let (admin, treasury, client) = setup(&env); - let err = client.try_initialize(&admin, &treasury, &500u32, &None, &None); + assert_eq!(client.get_version(), 1); + let oracle = Address::generate(&env); + let err = client.try_initialize(&admin, &oracle, &treasury, &500u32, &None, &None); assert_eq!(err, Err(Ok(Error::AlreadyInitialized))); } @@ -608,10 +615,11 @@ fn test_initialize_accepts_a_custom_max_sponsors() { let env = Env::default(); env.mock_all_auths(); let admin = Address::generate(&env); + let oracle = Address::generate(&env); let treasury = Address::generate(&env); let contract_id = env.register(MilestonesContract, ()); let client = MilestonesContractClient::new(&env, &contract_id); - client.initialize(&admin, &treasury, &500u32, &Some(2u32), &None); + client.initialize(&admin, &oracle, &treasury, &500u32, &Some(2u32), &None); assert_eq!(client.get_max_sponsors(), 2u32); @@ -674,6 +682,7 @@ fn test_recover_cancel_milestone_and_withdraw_frozen_before_recoverable_after() let env = Env::default(); // Do not mock all auths: we'll simulate missing admin auth later. let admin = Address::generate(&env); + let oracle = Address::generate(&env); let treasury = Address::generate(&env); let recovery = Address::generate(&env); let contract_id = env.register(MilestonesContract, ()); @@ -681,7 +690,14 @@ fn test_recover_cancel_milestone_and_withdraw_frozen_before_recoverable_after() // Initialize with a recovery address. env.mock_all_auths(); - client.initialize(&admin, &treasury, &500u32, &None, &Some(recovery.clone())); + client.initialize( + &admin, + &oracle, + &treasury, + &500u32, + &None, + &Some(recovery.clone()), + ); // Create a milestone funded by sponsor. let token_admin = Address::generate(&env); @@ -716,7 +732,13 @@ fn test_cancel_milestone_rejects_double_cancel() { let sponsor = Address::generate(&env); asset_client.mint(&sponsor, &10_000_000_000i128); - client.create_milestone(&40u64, &sponsor, &token_addr, &10_000_000_000i128, &1_000u64); + client.create_milestone( + &40u64, + &sponsor, + &token_addr, + &10_000_000_000i128, + &1_000u64, + ); client.cancel_milestone(&40u64); let err = client.try_cancel_milestone(&40u64); @@ -734,7 +756,13 @@ fn test_allocate_rejects_closed_milestone() { let sponsor = Address::generate(&env); asset_client.mint(&sponsor, &10_000_000_000i128); - client.create_milestone(&41u64, &sponsor, &token_addr, &10_000_000_000i128, &1_000u64); + client.create_milestone( + &41u64, + &sponsor, + &token_addr, + &10_000_000_000i128, + &1_000u64, + ); client.cancel_milestone(&41u64); let err = client.try_allocate(&41u64, &4101u64, &3_000_000_000i128); @@ -853,6 +881,59 @@ fn test_large_refund_distributes_dust_by_largest_remainder() { assert_eq!(client.get_milestone(&milestone_id).remaining_budget, 0); } + feature/upgrade-pause-pagination-separation +#[test] +fn test_pause_blocks_commitment_paths_but_allows_cancel() { + let env = Env::default(); + env.mock_all_auths(); + let (_admin, treasury, client) = setup(&env); + + let token_admin = Address::generate(&env); + let (token_addr, asset_client, token_client) = create_token(&env, &token_admin); + let sponsor = Address::generate(&env); + let sponsor_b = Address::generate(&env); + let recipient = Address::generate(&env); + asset_client.mint(&sponsor, &10_000_000_000i128); + asset_client.mint(&sponsor_b, &1_000_000_000i128); + + client.create_milestone( + &90u64, + &sponsor, + &token_addr, + &10_000_000_000i128, + &1_000u64, + ); + client.allocate(&90u64, &900u64, &1_000_000_000i128); + + client.pause(); + assert!(client.is_paused_view()); + + let create_err = client.try_create_milestone( + &91u64, + &sponsor_b, + &token_addr, + &1_000_000_000i128, + &1_000u64, + ); + assert_eq!(create_err, Err(Ok(Error::ContractPaused))); + + let contribute_err = client.try_contribute(&90u64, &sponsor_b, &1_000_000_000i128); + assert_eq!(contribute_err, Err(Ok(Error::ContractPaused))); + + let allocate_err = client.try_allocate(&90u64, &901u64, &1_000_000_000i128); + assert_eq!(allocate_err, Err(Ok(Error::ContractPaused))); + + let recipients = vec![&env, (recipient, 10_000u32)]; + let release_err = client.try_release_issue(&90u64, &900u64, &recipients); + assert_eq!(release_err, Err(Ok(Error::ContractPaused))); + + client.cancel_milestone(&90u64); + assert_eq!(token_client.balance(&sponsor), 9_000_000_000i128); + assert_eq!(token_client.balance(&treasury), 0); +} + +#[test] +fn test_unpause_restores_milestone_creation() { #[contract] pub struct MockPanicToken; @@ -1286,6 +1367,7 @@ fn test_state_machine_deallocate_rejects_released_issue() { #[test] fn test_state_machine_allocate_rejects_duplicate_allocation() { + main let env = Env::default(); env.mock_all_auths(); let (_admin, _treasury, client) = setup(&env); @@ -1293,6 +1375,16 @@ fn test_state_machine_allocate_rejects_duplicate_allocation() { let token_admin = Address::generate(&env); let (token_addr, asset_client, _token_client) = create_token(&env, &token_admin); let sponsor = Address::generate(&env); + feature/upgrade-pause-pagination-separation + asset_client.mint(&sponsor, &1_000_000_000i128); + + client.pause(); + client.unpause(); + assert!(!client.is_paused_view()); + + client.create_milestone(&92u64, &sponsor, &token_addr, &1_000_000_000i128, &1_000u64); + assert_eq!(client.get_milestone(&92u64).total_budget, 1_000_000_000i128); + asset_client.mint(&sponsor, &10_000i128); client.create_milestone(&908u64, &sponsor, &token_addr, &10_000i128, &1_000u64); @@ -1301,4 +1393,5 @@ fn test_state_machine_allocate_rejects_duplicate_allocation() { // Allocated -> allocate same issue must be rejected let err = client.try_allocate(&908u64, &9081u64, &5_000i128); assert_eq!(err, Err(Ok(Error::IssueAlreadyAllocated))); + main } diff --git a/contracts/milestones/src/types.rs b/contracts/milestones/src/types.rs index 6e9c35c..5aa7fb2 100644 --- a/contracts/milestones/src/types.rs +++ b/contracts/milestones/src/types.rs @@ -59,10 +59,13 @@ pub enum IssueStatus { #[derive(Clone)] pub enum DataKey { Admin, + Oracle, Recovery, Treasury, FeeBps, MaxSponsors, + Paused, + Version, Milestone(u64), IssueStatus(u64, u64), // (milestone_id, issue_id) Contribution(u64, u32), // (milestone_id, contribution_index) @@ -74,6 +77,12 @@ impl mergefi_common::AdminKey for DataKey { } } +impl mergefi_common::OracleKey for DataKey { + fn oracle_key() -> Self { + DataKey::Oracle + } +} + impl mergefi_common::TreasuryKey for DataKey { fn treasury_key() -> Self { DataKey::Treasury diff --git a/docs/maintenance-pool-pagination-analysis.md b/docs/maintenance-pool-pagination-analysis.md new file mode 100644 index 0000000..e507b5b --- /dev/null +++ b/docs/maintenance-pool-pagination-analysis.md @@ -0,0 +1,181 @@ +# Bounded/Paginated Access Pattern for Maintenance Pool Deposit History + +## Overview + +This document analyzes the current and proposed access patterns for querying deposit history in the maintenance-pool contract, evaluating whether pagination or other access patterns are needed given the contract's lifecycle and backend integration model. + +## Problem Statement + +The `maintenance-pool::deposit` function writes one `Deposit` entry per call, indexed by `DataKey::Deposit(pool_id, index)` where `index` monotonically increments (never reset). + +**Current access pattern**: +- `get_deposit(pool_id, index)` — read a single deposit by index +- No enumeration function — no way to list all deposits for a pool +- No pagination — no way to efficiently fetch a range of deposits + +**Concern**: Maintenance pools are described as receiving *repeated* deposits over years (potentially thousands). Some scenarios might require off-chain clients to reconstruct the full deposit history: + +1. **Audit/Compliance**: "Show me every deposit and withdrawal for pool X in the last 30 days." +2. **UI/Dashboard**: "Display the contribution history for this pool." +3. **Backend Sync**: `mergefi-backend` needs to verify its off-chain database against on-chain state. + +**Questions raised by the issue**: + +1. Does `mergefi-backend` actually need full deposit history from the contract, or does it index deposits via contract events? +2. Is the current one-at-a-time-by-index interface a latent scaling problem (n RPC calls to fetch n deposits)? +3. Does the ever-growing persistent storage have cost or liveness implications? + +## Investigation: Soroban Storage Economics + +Primary references: +- Stellar state archival and TTL: https://developers.stellar.org/docs/learn/fundamentals/contract-development/storage/state-archival +- Stellar storage strategies: https://developers.stellar.org/docs/build/guides/storage/storage-strategies + +### Persistent Entry Costs + +**Storage Pricing**: +- Soroban charges for ledger-entry size and TTL extension through rent/restore mechanics rather than through a fixed "number of rows" tax. +- Each deposit is a separate persistent ledger entry, so a pool with many deposits has many entries to extend or restore independently. +- The exact stroop cost is network-configuration dependent; clients should estimate it through simulation instead of hardcoding a constant. + +**TTL Model**: +- Each persistent entry has its own TTL. +- To keep an entry live, the contract must call `extend_ttl()` before it expires. +- If TTL expires, the entry becomes archived and requires restoration before normal reads can succeed. +- `maintenance-pool::keep_alive(pool_id)` refreshes both the parent pool entry and each deposit sub-record. + +**Growth Scenario**: +- A maintenance pool receives 10 deposits per month, 120 per year, 1200 over 10 years. +- Each `Deposit` entry (sponsor, amount, timestamp) is ~100 bytes on-chain. +- 1200 entries means 1200 independent deposit TTLs plus the parent pool TTL. +- That is operationally manageable, but every "refresh full history" pass is O(n) in the number of deposits. + +**Network Impact**: +- This is not a correctness bottleneck at the expected early scale. +- It is a maintenance-cost and RPC-efficiency concern, especially if a backend tries to rebuild history by repeatedly calling `get_deposit(pool_id, i)`. + +## Investigation: MergeFi Backend Integration + +### Described Architecture (per README) + +> "Backend integration" section describes a webhook-driven model where: +> 1. `mergefi-backend` watches GitHub webhooks. +> 2. On relevant events (PR merged, issue closed), it calls `release`/`refund`/`withdraw` on contracts. +> 3. It emits events (logs and persistence, not detailed in current README). + +### Event-Driven Indexing + +A *future* issue on the Roadmap suggests adding contract events. Once implemented: + +```rust +pub fn deposit(env: Env, pool_id: u64, sponsor: Address, token: Address, amount: i128) { + // ... deposit logic ... + + // NEW: Emit event for off-chain indexing + env.emit_contract("deposit_created", (&pool_id, &sponsor, &amount, &env.ledger().timestamp())); +} +``` + +With event emission: +- `mergefi-backend` listens to contract events from the ledger (via Soroban RPC). +- It maintains its own database of deposits (indexed by pool_id, sponsor, timestamp, etc.). +- It never needs to call `get_deposit()` to reconstruct history. +- The on-chain `get_deposit()` interface is a fallback for audit/verification, not a primary sync mechanism. + +### Current Reality (Pre-Events) + +Without events, `mergefi-backend` must: +- Query each pool's `get_pool()` to fetch `deposit_count`. +- Call `get_deposit(pool_id, i)` for each `i` from 0 to `deposit_count - 1` (n RPC calls). +- Rebuild the full history in its own database. + +This is an n-call problem *today*, but: +1. It only runs on startup/sync, not on every transaction. +2. With events, the problem is eliminated entirely. +3. For reasonable pool sizes (hundreds to low thousands of deposits), the sync latency is acceptable. + +## Recommendation + +### Conclusion: No Code Change Needed (For Now) + +Based on the investigation: + +1. **Event-Driven Architecture Wins**: Once the Roadmap's "emit contract events" issue is implemented, `get_deposit()` becomes a fallback-only interface. The n-call sync problem is solved at the source. + +2. **Current Scaling Is Acceptable**: Thousands of deposits per pool is within Soroban's persistent storage comfort zone. No cost/liveness emergency exists today. + +3. **Simplicity Wins**: Adding pagination (`get_deposits(pool_id, start, limit)`) is straightforward but adds code complexity and test surface with minimal benefit if events are the real sync mechanism. + +4. **Defer Until Events Land**: Once event emission is implemented and the backend is re-architected around it, reconsider whether *any* paginated getter is needed (answer: probably not). + +### If Pagination Were Needed (Reference Implementation) + +Should a future iteration decide pagination is necessary, here's the safe way to add it: + +```rust +/// Fetch a range of deposits for a pool, paginated. +/// +/// Returns up to `limit` deposits, starting from `start_index`. +/// If `start_index` >= `pool.deposit_count`, returns empty. +/// +/// Note: This is a view function (read-only, no fees). Suitable for +/// off-chain audit/UI queries. Prefer contract events for backend sync. +pub fn get_deposits( + env: Env, + pool_id: u64, + start_index: u32, + limit: u32, +) -> Result, Error> { + let pool_key = DataKey::Pool(pool_id); + let pool: MaintenancePool = env + .storage() + .persistent() + .get(&pool_key) + .ok_or(Error::PoolNotFound)?; + + let mut deposits = Vec::new(); + let end_index = start_index.saturating_add(limit).min(pool.deposit_count); + + for i in start_index..end_index { + let deposit_key = DataKey::Deposit(pool_id, i); + if let Some(deposit) = env.storage().persistent().get::<_, Deposit>(&deposit_key) { + deposits.push(deposit); + } + } + + Ok(deposits) +} + +/// Get the total deposit count for a pool (helper for pagination). +pub fn get_deposit_count(env: Env, pool_id: u64) -> Result { + let pool_key = DataKey::Pool(pool_id); + let pool: MaintenancePool = env + .storage() + .persistent() + .get(&pool_key) + .ok_or(Error::PoolNotFound)?; + Ok(pool.deposit_count) +} +``` + +**Tests for paginated access**: +- Create 100 deposits for a pool. +- Call `get_deposits(pool_id, 0, 10)`, verify first 10 returned. +- Call `get_deposits(pool_id, 10, 10)`, verify next 10 returned. +- Call `get_deposits(pool_id, 90, 50)` (limit > remaining), verify last 10 returned. +- Call `get_deposits(pool_id, 100, 10)` (start > count), verify empty vec returned. + +## Summary + +| Aspect | Finding | +|--------|---------| +| **Storage cost** | Entry-count growth increases TTL/restore work; estimate through simulation, no fixed cost assumed | +| **Backend sync** | Event-driven architecture (future) makes n-call sync moot | +| **On-chain enumeration** | Not needed if events are primary sync mechanism | +| **UX audit trail** | Off-chain database is source of truth; on-chain is fallback | +| **Recommended action** | **No code change required**. Land the events issue first; if pagination is still needed post-events, add it then. | + +## Cross-References + +- Roadmap: "Emit contract events" — once landed, re-evaluate deposit access patterns. +- README: "Backend integration" section should be updated to clarify event-driven sync once events are implemented. diff --git a/docs/pause-circuit-breaker-design.md b/docs/pause-circuit-breaker-design.md new file mode 100644 index 0000000..3d4350b --- /dev/null +++ b/docs/pause-circuit-breaker-design.md @@ -0,0 +1,229 @@ +# Emergency Pause / Circuit-Breaker Mechanism Design + +## Overview + +This document outlines the emergency pause mechanism for MergeFi contracts to safely halt operations during security incidents or discovered bugs, while preserving user funds and allowing recovery-path operations. + +## Problem Statement + +Currently, if a bug is discovered in production: +- There is no way to halt problematic operations (e.g., `fund`/`release`/`withdraw`). +- Sponsors can continue depositing into a known-vulnerable contract. +- The only mitigation is for the admin to choose not to call `release`/`allocate`, which doesn't stop permissionless `fund` calls. + +A responsible pause mechanism must: +1. Halt risky operations immediately. +2. Preserve legitimate withdrawal/refund paths so users can exit positions. +3. Minimize the new attack surface introduced by the pause lever itself. + +## Design Principles + +Primary reference for the role checks used by this design: +- Stellar contract authorization: https://developers.stellar.org/docs/build/guides/auth/contract-authorization + +1. **Refund and Recovery Paths Remain Available**: A paused contract must still allow sponsor-protective refund/reclaim paths where they exist, so users can recover already-committed funds during an incident. +2. **All Deposits Blocked**: Prevent new funds from entering a known-vulnerable contract (`fund`, `create_milestone`, `deposit`). +3. **Allocations Blocked**: Prevent new commitments (`allocate`, `extend_deadline`). +4. **Admin-Only Pause**: Only the contract admin can trigger pause/unpause to minimize key-count risk. +5. **Clear Errors**: Distinguish "contract is paused" from other errors so client code can handle gracefully. + +## Implementation Strategy + +### DataKey Addition + +Add a `Paused` flag to each contract's storage: + +```rust +#[contracttype] +#[derive(Clone)] +pub enum DataKey { + Admin, + Treasury, + FeeBps, + Paused, // NEW: bool, defaults to false + // ... existing variants ... +} +``` + +### New Error Variant + +Add a `Paused` error (used across all contracts): + +```rust +pub enum Error { + // ... + ContractPaused = 30, // NEW +} +``` + +### Pause/Unpause Functions + +Each contract implements: + +```rust +/// Admin-gated function to pause the contract. +/// When paused, operations like fund/allocate/deposit are blocked, +/// but refund/withdraw remain available for users to exit positions. +pub fn pause(env: Env) -> Result<(), Error> { + let admin = require_admin(&env).ok_or(Error::NotInitialized)?; + admin.require_auth(); + + env.storage().instance().set(&DataKey::Paused, &true); + extend_instance_ttl(&env); + Ok(()) +} + +/// Admin-gated function to unpause the contract. +pub fn unpause(env: Env) -> Result<(), Error> { + let admin = require_admin(&env).ok_or(Error::NotInitialized)?; + admin.require_auth(); + + env.storage().instance().set(&DataKey::Paused, &false); + extend_instance_ttl(&env); + Ok(()) +} + +fn is_paused(env: &Env) -> bool { + env.storage() + .instance() + .get(&DataKey::Paused) + .unwrap_or(false) +} +``` + +### Guarded Operations + +#### Escrow Contract + +**Blocked when paused:** +- `fund()` — prevent new escrows +- `contribute()` — prevent new contributions +- `release()` — prevent payouts (debatable; see analysis below) +- `extend_deadline()` — prevent extending paused escrows + +**Allowed when paused:** +- `refund()` — users can recover funds +- `get_*()` — read operations never blocked + +```rust +pub fn fund(env: Env, issue_id: u64, sponsor: Address, token: Address, + amount: i128, deadline: u64, target: Option) -> Result<(), Error> { + if is_paused(&env) { + return Err(Error::ContractPaused); + } + // ... rest of fund logic ... +} + +pub fn refund(env: Env, issue_id: u64) -> Result<(), Error> { + // No pause check — refunds always allowed + // ... rest of refund logic ... +} +``` + +#### Milestones Contract + +**Blocked when paused:** +- `create_milestone()` — prevent new milestones +- `contribute()` — prevent new contributions +- `allocate()` — prevent allocations +- `release_issue()` — prevent payouts + +**Allowed when paused:** +- `cancel_milestone()` — refund remaining funds to contributors + +```rust +pub fn cancel_milestone(env: Env, milestone_id: u64) -> Result<(), Error> { + // No pause check — cancellations (refunds) always allowed + // ... rest of cancel logic ... +} +``` + +#### Maintenance Pool Contract + +**Blocked when paused:** +- `deposit()` — prevent new deposits +- `withdraw()` — prevent payouts + +**Allowed when paused:** +- `reclaim_deposit()` — sponsor recovery after the inactivity window +- `sweep()` — admin surplus recovery for accidental direct transfers +- `get_*()` — read operations + +**Note**: Maintenance pools do not have an immediate permissionless refund of active balances. They do have `reclaim_deposit()` after `INACTIVITY_WINDOW`, which remains callable while paused. Routine oracle `withdraw()` is blocked because it is the exact high-frequency payout path that may need to be stopped during an incident. + +### Query Function + +Add a getter to check pause status: + +```rust +pub fn is_paused(env: Env) -> bool { + env.storage() + .instance() + .get(&DataKey::Paused) + .unwrap_or(false) +} +``` + +This lets clients check before attempting operations and provide UX feedback. + +## Rationale: Why Release/Withdraw Can Be Blocked + +**Intuition**: Release/withdraw are *admin-initiated* actions, not sponsor-initiated. If the admin discovers a bug and pauses, the admin can choose when to resume operations and make payouts safely. + +**Counter-argument**: If the backend is compromised, a compromised admin might issue false `release` calls before an honest party discovers the compromise. Blocking `release` during a pause prevents this. + +**Decision**: Block `release` and `allocate` when paused. Sponsors/users can still exit via refund/cancel. The assumption is that incident response is *coordinated* — when a pause is triggered, the team is already in communication and will resume carefully and audited. + +**Timelock Future Work**: For extremely high-trust scenarios, a future upgrade could add a timelock so that pause/unpause aren't instantaneous (e.g., "pause takes effect in 1 hour", allowing a 1-hour window to pull funds before enforcement). + +## Two-Key Interaction (Issue #13) + +If a two-key oracle/admin model is implemented: + +- **Pause Authority**: The primary `Admin` key only (not the `Oracle` key), as pause is a high-impact control. +- **Unpause Authority**: Same `Admin` key. +- **Guardian Key** (future): A separate guardian could have pause-only authority, enabling a trusted third party to pause without full admin privileges. + +## Testing Strategy + +1. **Pause State Persists**: Pause the contract, verify subsequent calls see `is_paused_view() == true`. +2. **Blocked Operations Fail**: Attempt `fund`/`allocate`/`deposit` while paused; verify they return `ContractPaused`. +3. **Allowed Operations Succeed**: Attempt `refund`/`cancel_milestone`/`reclaim_deposit` while paused; verify they succeed when their normal preconditions are met. +4. **Unpause Restores Normal State**: Unpause, verify `fund` works again. +5. **Unauthorized Pause Fails**: Attempt `pause()` as non-admin; verify it returns `Unauthorized`. +6. **Read Operations Always Work**: `get_escrow()`, `get_deposit()`, etc. work regardless of pause state. + +## Operational Runbook + +### When to Pause + +1. A security researcher reports a potential exploit. +2. An internal audit discovers a bug or logical flaw. +3. Unusual on-chain activity suggests an attack (e.g., rapid double-funding attempts). + +### Pause Procedure + +1. Admin calls `pause()` on the affected contract. +2. The backend immediately stops initiating new `release`/`allocate`/`withdraw` calls. +3. Users are notified (off-chain) that the contract is paused. Escrow contributors can recover through `refund` once eligible, milestone funds can be returned through `cancel_milestone`, and maintenance-pool sponsors retain the inactivity-window `reclaim_deposit` path. +4. The team audits the bug, prepares a fix, and deploys a new wasm via the upgrade mechanism. + +### Unpause Procedure + +1. New wasm is live and verified. +2. Admin calls `unpause()`. +3. Normal operations resume. +4. Users are notified that the contract is operational again. + +## Limitations and Future Work + +1. **No Pause-on-Threshold**: This design is manual admin-triggered only. Future iterations could add automatic triggers (e.g., "pause if more than $X is withdrawn in an hour"), but this is out of scope. +2. **Maintenance Pool Gap**: Maintenance pools have no permissionless refund. Sponsors should be made aware that a pause is more disruptive for pools than for escrows/milestones. +3. **Timelock**: A future enhancement could add a delay between pause and enforcement, or between unpause and restoration, giving users time to react. +4. **Multi-Admin Approval**: A future enhancement could require multiple admins to approve a pause, reducing the risk of a single-key compromise being able to freeze funds. + +## Cross-Contract Considerations + +- Each of the three contracts implements pause independently (no shared registry). +- If a bug affects all three, the admin must call `pause()` on each one. +- Backend must be hardened to retry gracefully and alert operators when a contract is paused. diff --git a/docs/two-key-admin-oracle-design.md b/docs/two-key-admin-oracle-design.md new file mode 100644 index 0000000..252fc87 --- /dev/null +++ b/docs/two-key-admin-oracle-design.md @@ -0,0 +1,324 @@ +# Two-Key Oracle/Admin Separation Design + +## Overview + +This document outlines the two-key authorization model for MergeFi contracts, separating routine oracle operations (release, withdraw) from high-trust administrative actions (initialize, key rotation, pause/unpause, upgrade). + +## Problem Statement + +Primary references: +- Stellar contract authorization and account/contract addresses: https://developers.stellar.org/docs/build/guides/auth/contract-authorization +- Stellar contract lifecycle: https://developers.stellar.org/docs/learn/fundamentals/contract-development/contract-lifecycle + +Currently, all three contracts use a single `Admin` address for every privileged action: +- Routine operations: `release`, `release_issue`, `withdraw` (called many times daily by `mergefi-backend`) +- High-trust operations: `initialize`, pause/unpause, upgrade (rarely called, require extreme care) + +The single `Admin` address is a hot key held by `mergefi-backend` and used to sign routine transactions in response to GitHub webhooks. A compromise of this key gives an attacker: + +1. **Immediate damage**: False `release`/`withdraw` calls stealing funds. +2. **Permanent damage**: Pause/unpause and upgrade authority, freezing the contract indefinitely or deploying malicious code. + +**Mitigation**: Separate the `Oracle` role (routine operations only) from the `Admin` role (infrastructure operations only), held by different keys with different security postures. + +## Design: Two-Role Model + +### Roles + +#### 1. Oracle + +**Authority**: +- `release(issue_id, recipients)` in escrow +- `release_issue(milestone_id, issue_id, recipients)` in milestones +- `withdraw(pool_id, recipient, amount)` in maintenance-pool + +**Characteristics**: +- Routine, high-frequency use (many calls per day). +- Held by `mergefi-backend` as a hot key. +- Used to sign automated transactions from GitHub webhook events. + +**Compromise Impact**: +- Attacker can issue false payouts. +- No persistence: funds are stolen, but the contract still functions. + +#### 2. Admin + +**Authority**: +- `initialize(admin, treasury, fee_bps, max_sponsors, recovery)` +- `pause()` / `unpause()` (Issue #14) +- `upgrade(new_wasm_hash)` (Issue #15) +- `set_oracle(new_oracle)` / `set_admin(new_admin)` (NEW) + +**Characteristics**: +- Rarely used (initialization, emergencies, planned upgrades). +- Held by a human or multi-sig address with higher security (cold key, offline storage, or Stellar multisig). +- Used for deliberate, audited infrastructure changes. + +**Compromise Impact**: +- Attacker can freeze the contract (pause indefinitely) or deploy malicious code (upgrade). +- Worst-case scenario; requires immediate coordinated response. + +### Storage + +```rust +#[contracttype] +#[derive(Clone)] +pub enum DataKey { + Admin, // PRIMARY ADMIN: initialize, admin rotation, pause/unpause, upgrade + Oracle, // ORACLE: release, release_issue, withdraw (NEW) + Treasury, + FeeBps, + Paused, + Version, + // ... existing variants ... +} +``` + +### Authorization Pattern + +Replace the universal `require_admin(&env)?.require_auth()` pattern with role-specific checks: + +```rust +fn require_admin(env: &Env) -> Option
{ + env.storage().instance().get(&DataKey::Admin) +} + +fn require_oracle(env: &Env) -> Option
{ + env.storage().instance().get(&DataKey::Oracle) +} + +// For operations that need Admin +pub fn pause(env: Env) -> Result<(), Error> { + let admin = require_admin(&env).ok_or(Error::NotInitialized)?; + admin.require_auth(); + env.storage().instance().set(&DataKey::Paused, &true); + Ok(()) +} + +// For operations that need Oracle +pub fn release(env: Env, issue_id: u64, recipients: Vec<(Address, u32)>) -> Result<(), Error> { + let oracle = require_oracle(&env).ok_or(Error::NotInitialized)?; + oracle.require_auth(); + // ... release logic ... +} +``` + +### Initialization + +Escrow and Milestones contracts: + +```rust +pub fn initialize( + env: Env, + admin: Address, + oracle: Address, + treasury: Address, + fee_bps: u32, + max_sponsors: Option, + recovery: Option
, +) -> Result<(), Error> { + admin.require_auth(); // Prevent front-running + + if env.storage().instance().has(&DataKey::Admin) { + return Err(Error::AlreadyInitialized); + } + + env.storage().instance().set(&DataKey::Admin, &admin); + env.storage().instance().set(&DataKey::Oracle, &oracle); + env.storage().instance().set(&DataKey::Treasury, &treasury); + env.storage().instance().set(&DataKey::FeeBps, &fee_bps); + // ... rest of initialization ... + Ok(()) +} +``` + +Maintenance Pool (same pattern): + +```rust +pub fn initialize( + env: Env, + admin: Address, + oracle: Address, + treasury: Address, + fee_bps: u32, + recovery: Option
, +) -> Result<(), Error> { + // ... same pattern ... +} +``` + +### Key Rotation + +New functions allow separate rotation of Admin and Oracle keys: + +```rust +/// Rotate the Admin key. Requires the current Admin's authorization. +/// Only the Admin can authorize a new Admin (Admin rotation is self-authorized). +pub fn set_admin(env: Env, new_admin: Address) -> Result<(), Error> { + let admin = require_admin(&env).ok_or(Error::NotInitialized)?; + admin.require_auth(); // Current admin approves new admin + + new_admin.require_auth(); // New admin must consent to being named + + env.storage().instance().set(&DataKey::Admin, &new_admin); + extend_instance_ttl(&env); + Ok(()) +} + +/// Rotate the Oracle key. Requires the current Admin's authorization. +/// Note: Oracle cannot rotate itself (only Admin can change Oracle, ensuring +/// the high-trust Admin key maintains control over the hot key). +pub fn set_oracle(env: Env, new_oracle: Address) -> Result<(), Error> { + let admin = require_admin(&env).ok_or(Error::NotInitialized)?; + admin.require_auth(); // Only Admin can change Oracle + + new_oracle.require_auth(); // New Oracle must consent + + env.storage().instance().set(&DataKey::Oracle, &new_oracle); + extend_instance_ttl(&env); + Ok(()) +} +``` + +### Getters + +Expose the current Admin and Oracle addresses: + +```rust +pub fn get_admin(env: Env) -> Result { + require_admin(&env).ok_or(Error::NotInitialized) +} + +pub fn get_oracle(env: Env) -> Result { + require_oracle(&env).ok_or(Error::NotInitialized) +} +``` + +## Rationale: Why Separate Oracle from Admin + +### Option 1: Off-Chain Separation (Stellar Multisig Account) + +**Approach**: Keep one `Admin` address, but make it a Stellar multisig account (e.g., 2-of-3) that requires multiple signatures for any operation. + +**Pros**: +- No contract code changes needed. +- Works with existing tooling (Stellar's native multisig). +- Scales to any signature threshold. + +**Cons**: +- Every operation (including routine `release`) requires multisig approval, slowing down the backend. +- Requires multisig setup and coordination overhead. +- Does not achieve the goal of "routine operations don't need high-trust approval." + +**Verdict**: Does not fully solve the problem because it doesn't separate the *operational* burden from the *trust* burden. + +### Option 2: Contract-Level Two-Role Separation (Recommended) + +**Approach**: Add a distinct `Oracle` role with authority for `release`/`withdraw` only. `Admin` retains all high-trust powers. + +**Pros**: +- Routine operations can proceed with a hot key without infrastructure overhead. +- High-trust operations remain protected by the secure Admin key. +- Clear separation of concerns: who holds which key reflects its real-world security posture. + +**Cons**: +- Requires contract code changes and audit. +- Admin must maintain both keys (more surface for key management mistakes). + +**Verdict**: Recommended. The separation achieves the design goal: routine operations don't require high-trust approval, but high-trust operations are protected. + +### Hybrid Approach (Future) + +Make the `Admin` address itself a Stellar multisig or a Soroban timelock contract, combining the benefits: +- Routine operations use a single hot `Oracle` key. +- High-trust operations require multisig or timelock approval via the `Admin` role. + +This is deferred as a future enhancement. + +## Migration Path for Already-Deployed Contracts + +### Current Testnet Contracts + +The three deployed testnet contracts have a single `Admin` and no `Oracle`: + +```rust +// Current (before this change) +pub fn release(env: Env, issue_id: u64, recipients: Vec<(Address, u32)>) -> Result<(), Error> { + let admin = require_admin(&env).ok_or(Error::NotInitialized)?; + admin.require_auth(); + // ... +} +``` + +**Options**: + +1. **Ignore and redeploy**: Testnet instances are abandoned. Redeploy all three with two-key model baked in. + - **Pro**: Clean start, no complex migration. + - **Con**: Testnet data is lost (but testnet is supposed to be ephemeral). + +2. **Retrofit via upgrade** (if contracts support it first): + - Use the upgrade mechanism to add `set_oracle()` and `set_oracle_if_not_set()` functions. + - An admin call to `set_oracle(oracle_address)` retroactively splits the role. + - Old `release` calls from the single `Admin` still work; new `release` calls require the split-out `Oracle`. + - **Pro**: Preserves testnet state and enables testing the upgrade mechanism itself. + - **Con**: Complex migration logic, higher audit risk. + +**Recommendation**: Redeploy testnet with two-key model from the start. Production mainnet will be deployed with both features live. The upgrade mechanism is there for post-deployment fixes, not for retrofitting missing-from-day-1 features. + +### Mainnet Deployment + +All three contracts are deployed with: + +```rust +fn initialize( + env: Env, + admin: Address, + oracle: Address, + treasury: Address, + fee_bps: u32, + max_sponsors: Option, + recovery: Option
, +) -> Result<(), Error> { + // ... requires both admin and oracle addresses ... +} +``` + +The backend is configured with both keys from day one: +- `admin_key`: Cold/secure key, used only for administrative changes. +- `oracle_key`: Hot key, used for routine `release` calls. + +## Testing Strategy + +1. **Separate Authorization**: Call `release()` as `admin`; verify it fails. Call `release()` as `oracle`; verify it succeeds. +2. **Separate Key Rotation**: Call `set_oracle()` as `admin`; verify it succeeds. Call `set_oracle()` as `oracle`; verify it fails. +3. **New Oracle Key Works**: Rotate oracle, call `release()` as new oracle; verify it works. +4. **Admin Key Rotation**: Rotate admin, verify old admin can no longer call `set_oracle()` but new admin can. +5. **Backward Compatibility** (if retrofitting): Verify that contracts initialized with a single admin (old testnet) can be upgraded and split via `set_oracle_if_not_set()`. + +## Interaction with Other Issues + +### Issue #14 (Pause/Unpause) +- **Pause Authority**: Only `Admin` can pause/unpause. `Oracle` cannot. +- Rationale: `Oracle` is a frequent-access hot key; pause should be a deliberate high-trust action. + +### Issue #15 (Upgrade) +- **Upgrade Authority**: Only `Admin` can authorize upgrades. `Oracle` cannot. +- Rationale: Upgrade is the highest-risk action; it must remain under high-trust authority. + +### Addresses Accessible to Backend + +`mergefi-backend` is reconfigured to hold: + +1. **Admin Key** (Stellar source account or multisig): Used for `initialize`, `pause`, `unpause`, `upgrade`, key rotations. + - Low frequency: only at deployment time or during incidents. + - Higher security posture: offline, cold storage, or multisig-protected. + +2. **Oracle Key** (Stellar source account): Used for routine `release`, `release_issue`, `withdraw`. + - High frequency: many times per day. + - Standard security: typical hot-key best practices (key rotation, audit logs, etc.). + +## Future Work + +1. **Guardian Key**: A third role for pause-only authority, enabling a trusted third party to trigger emergency pause without full admin privileges. +2. **Multisig Admin**: Make `Admin` a Stellar multisig or Soroban timelock contract to require multiple approvals for high-trust operations. +3. **Audit Logs**: Emit contract events for every key rotation, pause/unpause, and upgrade, providing an on-chain audit trail. diff --git a/docs/upgrade-storage-migration-design.md b/docs/upgrade-storage-migration-design.md new file mode 100644 index 0000000..46adcfb --- /dev/null +++ b/docs/upgrade-storage-migration-design.md @@ -0,0 +1,169 @@ +# Contract Upgrade and Storage Migration Design + +## Overview + +This document outlines the upgrade mechanism, storage versioning strategy, and migration path for MergeFi contracts to support safe in-place wasm upgrades via `env.deployer().update_current_contract_wasm()`. + +## Problem Statement + +MergeFi contracts currently have no upgrade mechanism. Deploying a new wasm version creates a new contract address with empty storage, stranding existing on-chain state. This is unacceptable for contracts holding live user funds. + +## Soroban Upgrade Mechanism + +Primary references: +- Stellar contract lifecycle and deployment: https://developers.stellar.org/docs/learn/fundamentals/contract-development/contract-lifecycle +- Stellar contract authorization: https://developers.stellar.org/docs/build/guides/auth/contract-authorization +- Stellar state archival and TTL: https://developers.stellar.org/docs/learn/fundamentals/contract-development/storage/state-archival + +### How It Works + +- `env.deployer().update_current_contract_wasm(new_wasm_hash)` upgrades code *in place* at the same address while **preserving all existing storage**. +- Requires the calling contract to explicitly implement an upgrade function with appropriate authorization. +- Existing `#[contracttype]` structs and storage entries remain accessible in the upgraded code, as long as field layouts don't change incompatibly. + +### Key Guarantees and Limitations + +- **Storage preservation**: All persistent and instance storage entries survive the upgrade. +- **Type safety concern**: If a struct's field order or types change, old data becomes inaccessible/corrupt. Soroban has no automatic migration mechanism. +- **Authorization**: The upgrade function must explicitly call `require_auth()` on the authorized caller (the admin/upgrade key). + +### Retrofitting Already-Deployed Contracts + +**Current Status**: The three deployed testnet contracts have **no upgrade function and cannot support in-place upgrades**. Any new code requires a full redeploy. + +**Mitigation**: Before mainnet, redeploy all three contracts with upgrade support baked in from the start (no need to retrofit the testnet instances; they can be abandoned). + +## Storage Versioning Strategy + +We implement a versioning convention to safely detect and migrate old-shape data when struct layouts change. + +### Approach + +1. **Version Key**: Add a `DataKey::Version` entry storing the current contract version. +2. **Default Handling**: On contract interaction, check the stored version against the current code version. +3. **Lazy Migration**: If old data is detected, migrate it transparently on first read (if layout changes are backward-compatible) or fail with a clear error. +4. **Explicit Migration**: Provide an admin-callable `migrate_storage()` function for complex layout changes requiring a full pass over all affected entries. + +### Implementation + +```rust +#[contracttype] +#[derive(Clone)] +pub enum DataKey { + Admin, + Treasury, + FeeBps, + Version, // NEW: current contract version + Paused, // NEW: pause flag + Escrow(u64), + // ... other variants ... +} + +// Version tracking +const CONTRACT_VERSION: u32 = 1; + +fn get_contract_version(env: &Env) -> u32 { + env.storage() + .instance() + .get(&DataKey::Version) + .unwrap_or(0) +} + +fn set_contract_version(env: &Env, version: u32) { + env.storage().instance().set(&DataKey::Version, &version); +} +``` + +### Migration Scenarios + +**Scenario 1: Adding an optional field to a struct** +- Old data: `Escrow { token, amount, status, created_at, deadline, contributor_count }` +- New data: `Escrow { token, amount, status, created_at, deadline, contributor_count, target, version }` +- Migration: Set new fields to defaults (`target: None`, `version: 1`) on first read. +- **Safe because**: old `Deposit` data still deserializes; new fields fill in defaults. + +**Scenario 2: Changing the split accounting structure (e.g., issue #64 redesign)** +- Old data: `Map<(issue_id, recipient), amount>` +- New data: `Map<(issue_id, version), V1AllocationData { ... }>` +- Migration: Requires explicit `migrate_storage()` call to walk all entries and rewrite them. +- **Not automatic**: The function runs only when an admin explicitly calls it, after thorough review. + +## Upgrade Function Specification + +```rust +/// Admin-gated upgrade function. Requires the stored `Admin` address's authorization. +/// Calls `env.deployer().update_current_contract_wasm(new_wasm_hash)` to upgrade +/// the contract code in place while preserving storage. +/// +/// After upgrade, the new code should immediately check the stored `Version` and +/// perform any necessary lazy migrations if old data is detected. +pub fn upgrade( + env: Env, + new_wasm_hash: soroban_sdk::BytesN<32>, +) -> Result<(), Error> { + let admin = require_admin(&env).ok_or(Error::NotInitialized)?; + admin.require_auth(); + + env.deployer().update_current_contract_wasm(new_wasm_hash); + set_contract_version(&env, CONTRACT_VERSION); + extend_instance_ttl(&env); + + Ok(()) +} +``` + +## Pause Mechanism Integration + +The pause mechanism (Issue #14) can coexist with the upgrade mechanism: + +1. **Pause Before Upgrade**: If a bug is detected, the admin can pause the contract to halt user interactions. +2. **Upgrade Paused Contract**: A paused contract can still be upgraded (the upgrade itself doesn't check pause status). +3. **Post-Upgrade Resume**: After upgrade validation, the admin unpauses to resume normal operation. + +## Integration with Two-Key Admin Model (Issue #13) + +If a two-key oracle/admin model is implemented: + +- **Upgrade Authority**: Only the primary `Admin` key can authorize upgrades (not the `Oracle` key), as upgrades are high-risk. +- **Pause Authority**: The `Admin` or a dedicated `Guardian` key can trigger pause (see Issue #14). + +## Testing Strategy + +1. **Upgrade Without Data**: Deploy a new version, verify the contract initializes cleanly. +2. **Upgrade With Data**: Upgrade a contract with existing escrows/milestones/deposits, verify data survives and remains accessible. +3. **Version Tracking**: Verify that `get_contract_version()` returns 0 before first upgrade, and increments correctly. +4. **Lazy Migration**: Add optional fields to a struct, verify old entries still deserialize and new fields fill with defaults. +5. **Explicit Migration**: Trigger `migrate_storage()` on a contract with outdated entries, verify correctness of rewritten entries. + +## Deployment and Rollout + +### Testnet (Current) + +1. The three currently-deployed testnet contracts **cannot be upgraded in place** (no upgrade function). +2. **Recommendation**: Do not invest in retrofitting them. Instead, redeploy all three with upgrade support before mainnet testing. +3. Any data on the old testnet instances is abandoned; `mergefi-backend` migrates to the new contract IDs. + +### Mainnet (Pre-Launch) + +1. All three contracts deployed with `version = 0` initially and upgrade support built in. +2. A "final audit" can proceed against the live mainnet contracts, without risk of unupgradeable bugs trapping funds. +3. If auditors find a critical issue, an emergency pause and upgrade cycle can execute safely. + +## Rollback Considerations + +Soroban does not support "downgrading" a contract (reverting to an older wasm hash). However: + +- A **paused** contract can remain frozen while the team prepares a fix. +- Once a fix is verified, upgrade to the corrected version. +- Rollback via upgrade to a version built *before* the bug is possible, but requires verifying that the old version's code is safe for the current state (migration backward is often harder than forward). + +## Backward Compatibility + +- **Existing testnet contracts**: No action required; they will be abandoned and redeployed. +- **Existing mainnet contracts (if any, before this feature lands)**: A one-time "activate upgrade support" redeploy is required. This is why the recommendation is to land this feature *before* mainnet launch. + +## Future Work + +1. **Atomic Versioning Across Contracts**: If `mergefi-backend` needs to coordinate upgrades across all three contracts, consider a shared version registry (a tiny separate contract or a backend cache). +2. **Audit Trail**: Log upgrade events (emit Soroban contract events) to provide an on-chain history of all upgrades. +3. **Timelock**: Consider an upgrade timelock (see Issue #13's two-key model) where an upgrade is announced and scheduled for a future block, allowing users to exit positions before a potentially-risky upgrade. diff --git a/scripts/examples/deposit-withdraw-flow.sh b/scripts/examples/deposit-withdraw-flow.sh index d4e750c..8706406 100755 --- a/scripts/examples/deposit-withdraw-flow.sh +++ b/scripts/examples/deposit-withdraw-flow.sh @@ -13,6 +13,7 @@ # # Usage: # ADMIN_SECRET=S... \ +# ORACLE_SECRET=S... \ # SPONSOR_SECRET=S... \ # TOKEN=C... \ # CONTRACT_ID=C... \ @@ -31,6 +32,7 @@ cd "$(dirname "${BASH_SOURCE[0]}")/../.." ADMIN_SECRET="${ADMIN_SECRET:?Set ADMIN_SECRET to a funded testnet secret key}" SPONSOR_SECRET="${SPONSOR_SECRET:-$ADMIN_SECRET}" +ORACLE_SECRET="${ORACLE_SECRET:-$ADMIN_SECRET}" TOKEN="${TOKEN:?Set TOKEN to a Stellar Asset Contract address (e.g. a testnet USDC SAC)}" WASM_PATH="${WASM_PATH:-target/wasm32v1-none/release/mergefi_maintenance_pool.wasm}" POOL_ID="${POOL_ID:-1}" @@ -43,6 +45,7 @@ admin_pub() { } ADMIN_ADDRESS="$(admin_pub "$ADMIN_SECRET")" +ORACLE_ADDRESS="$(admin_pub "$ORACLE_SECRET")" TREASURY_ADDRESS="${TREASURY_ADDRESS:-$ADMIN_ADDRESS}" SPONSOR_ADDRESS="$(admin_pub "$SPONSOR_SECRET")" @@ -52,16 +55,16 @@ if [ -z "${CONTRACT_ID:-}" ]; then fi echo "==> Using contract: $CONTRACT_ID" -echo "==> 1/3 initialize(admin=$ADMIN_ADDRESS, treasury=$TREASURY_ADDRESS, fee_bps=$FEE_BPS)" +echo "==> 1/3 initialize(admin=$ADMIN_ADDRESS, oracle=$ORACLE_ADDRESS, treasury=$TREASURY_ADDRESS, fee_bps=$FEE_BPS)" node scripts/invoke.mjs "$ADMIN_SECRET" "$CONTRACT_ID" initialize \ - "address:$ADMIN_ADDRESS" "address:$TREASURY_ADDRESS" "u32:$FEE_BPS" + "address:$ADMIN_ADDRESS" "address:$ORACLE_ADDRESS" "address:$TREASURY_ADDRESS" "u32:$FEE_BPS" "none" echo "==> 2/3 deposit(pool_id=$POOL_ID, sponsor=$SPONSOR_ADDRESS, token=$TOKEN, amount=$DEPOSIT_AMOUNT)" node scripts/invoke.mjs "$SPONSOR_SECRET" "$CONTRACT_ID" deposit \ "u64:$POOL_ID" "address:$SPONSOR_ADDRESS" "address:$TOKEN" "i128:$DEPOSIT_AMOUNT" echo "==> 3/3 withdraw(pool_id=$POOL_ID, recipient=$ADMIN_ADDRESS, amount=$WITHDRAW_AMOUNT)" -node scripts/invoke.mjs "$ADMIN_SECRET" "$CONTRACT_ID" withdraw \ +node scripts/invoke.mjs "$ORACLE_SECRET" "$CONTRACT_ID" withdraw \ "u64:$POOL_ID" "address:$ADMIN_ADDRESS" "i128:$WITHDRAW_AMOUNT" echo "==> Done. Query the pool's remaining balance with:" diff --git a/scripts/invoke.mjs b/scripts/invoke.mjs index 26c3ee9..219665f 100644 --- a/scripts/invoke.mjs +++ b/scripts/invoke.mjs @@ -16,11 +16,13 @@ const server = new rpc.Server(RPC_URL); const [, , secret, contractId, method, ...args] = process.argv; if (!secret || !contractId || !method) { - console.error("Usage: node invoke.mjs [args as address:G..., u32:123, u64:123, or i128:123]"); + console.error("Usage: node invoke.mjs [args as address:G..., u32:123, u64:123, i128:123, or none]"); process.exit(1); } function parseArg(raw) { + if (raw === "none") return nativeToScVal(null); + const [type, value] = raw.split(":"); if (type === "address") return nativeToScVal(new Address(value), { type: "address" }); if (type === "u32") return nativeToScVal(parseInt(value, 10), { type: "u32" });