diff --git a/Makefile b/Makefile index 5984132..ec6e6fb 100644 --- a/Makefile +++ b/Makefile @@ -3,6 +3,9 @@ WASM_TARGET := wasm32v1-none WASM_DIR := target/$(WASM_TARGET)/release NETWORK ?= testnet SOURCE_ACCOUNT ?= mergefi-admin +ADMIN ?= +TREASURY ?= +FEE_BPS ?= 250 .PHONY: build test test-verbose fmt clean deploy-escrow deploy-milestones deploy-maintenance-pool deploy @@ -37,23 +40,31 @@ clean: ## Example deploy targets. Requires `stellar` (formerly `soroban`) CLI and a ## funded identity named $(SOURCE_ACCOUNT) (see: stellar keys generate). -## Usage: make deploy-escrow NETWORK=testnet SOURCE_ACCOUNT=mergefi-admin +## Constructor arguments are mandatory and deployment is atomic. +## Usage: make deploy-escrow NETWORK=testnet SOURCE_ACCOUNT=mergefi-admin \ +## ADMIN=G... TREASURY=G... FEE_BPS=250 deploy-escrow: build + @test -n "$(ADMIN)" -a -n "$(TREASURY)" || (echo "ADMIN and TREASURY are required"; exit 1) stellar contract deploy \ --wasm $(WASM_DIR)/mergefi_escrow.wasm \ --source $(SOURCE_ACCOUNT) \ - --network $(NETWORK) + --network $(NETWORK) -- \ + --admin $(ADMIN) --treasury $(TREASURY) --fee_bps $(FEE_BPS) deploy-milestones: build + @test -n "$(ADMIN)" -a -n "$(TREASURY)" || (echo "ADMIN and TREASURY are required"; exit 1) stellar contract deploy \ --wasm $(WASM_DIR)/mergefi_milestones.wasm \ --source $(SOURCE_ACCOUNT) \ - --network $(NETWORK) + --network $(NETWORK) -- \ + --admin $(ADMIN) --treasury $(TREASURY) --fee_bps $(FEE_BPS) deploy-maintenance-pool: build + @test -n "$(ADMIN)" -a -n "$(TREASURY)" || (echo "ADMIN and TREASURY are required"; exit 1) stellar contract deploy \ --wasm $(WASM_DIR)/mergefi_maintenance_pool.wasm \ --source $(SOURCE_ACCOUNT) \ - --network $(NETWORK) + --network $(NETWORK) -- \ + --admin $(ADMIN) --treasury $(TREASURY) --fee_bps $(FEE_BPS) deploy: deploy-escrow deploy-milestones deploy-maintenance-pool diff --git a/README.md b/README.md index ea0627a..9d86ee1 100644 --- a/README.md +++ b/README.md @@ -144,7 +144,7 @@ systematically rewarding the final recipient. Core single-issue bounty escrow. ```rust -fn initialize(env, admin: Address, treasury: Address, fee_bps: u32) -> Result<(), Error>; +fn __constructor(env, admin: Address, treasury: Address, fee_bps: u32) -> Result<(), Error>; fn fund(env, issue_id: u64, sponsor: Address, token: Address, amount: i128, deadline: u64) -> 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>; @@ -205,7 +205,7 @@ fn get_fee_bps(env) -> Result; Lump-sum budget shared across the issues in a release. ```rust -fn initialize(env, admin: Address, treasury: Address, fee_bps: u32) -> Result<(), Error>; +fn __constructor(env, admin: Address, treasury: Address, fee_bps: u32) -> 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>; @@ -257,7 +257,7 @@ fn get_contribution(env, milestone_id: u64, index: u32) -> Result Result<(), Error>; +fn __constructor(env, admin: Address, treasury: Address, fee_bps: u32) -> 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 get_pool(env, pool_id: u64) -> Result; @@ -338,8 +338,8 @@ archival, tuned for a multi-month bounty/release lifecycle). ## Security model -- **Admin / oracle authorization.** One `Address` (`admin`), set once at - `initialize` and immutable thereafter, represents the `mergefi-backend` +- **Admin / oracle authorization.** One `Address` (`admin`), set atomically by + `__constructor` during deployment 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 @@ -350,21 +350,15 @@ archival, tuned for a multi-month bounty/release lifecycle). 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 - 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 - does **not** prevent an attacker from front-running the legitimate - deployer's `initialize` call by naming *themselves* as admin 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 - constructor) rather than an in-contract check. +- **Atomic construction; no re-initialization surface.** All three contracts + expose `__constructor(admin, treasury, fee_bps)` instead of `initialize`. + Soroban runs it in the same host operation that creates the instance, so + the contract is never callable in an uninitialized state and there is no + first-caller-wins race. Constructors cannot be invoked again after creation. + The constructor also calls `admin.require_auth()`, preventing deployment + with an unwilling third party named as admin. - **Fee mechanics.** `fee_bps` is basis points (1/100 of a percent) out of - 10000, validated `<= 10000` at `initialize`. It's deducted from the top + 10000, validated `<= 10000` by the constructor. It's deducted from the top of every payout (`release`, `release_issue`, `withdraw`) before the remainder is split among recipients — the treasury is paid in the same transaction as the recipients, so there's no separate "sweep fees" @@ -454,7 +448,7 @@ integration points: ```sh make build # cargo build --target wasm32v1-none --release, all 3 contracts make test # cargo test --workspace (native target, no wasm needed) -make deploy # example stellar contract deploy calls, see Makefile +make deploy ADMIN=G... TREASURY=G... FEE_BPS=250 # atomic deploy + construction ``` Or directly: @@ -465,8 +459,8 @@ cargo build --target wasm32v1-none --release \ -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 +Verified in this session: `cargo test --workspace` — **56/56 tests pass** +(30 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 `soroban_sdk::testutils` (`Env::default()`, `Address::generate`, @@ -476,48 +470,51 @@ compile to `.wasm` in `target/wasm32v1-none/release/`. ### Deployed on Stellar testnet -All three contracts are deployed and initialized on testnet as of this -writing. `stellar-cli`'s HTTP client couldn't reach the RPC endpoint from +The contract IDs below are legacy, already-initialized testnet deployments +from before the constructor migration. They do **not** contain the atomic +construction fix and must not be treated as current deployments. A constructor +cannot be retrofitted onto an existing instance, so each contract must be +redeployed under a new contract ID and every backend/frontend configuration +must be updated to the replacement IDs. + +`stellar-cli`'s HTTP client couldn't reach the RPC endpoint from the environment this was deployed from (a local TLS/cert issue, not a Stellar-side problem), so `scripts/deploy.mjs` and `scripts/invoke.mjs` (thin wrappers around `@stellar/stellar-sdk`) were used instead to -perform the same upload → create-contract → initialize flow the CLI -would otherwise do. +perform the upload and deployment operations. -| Contract | Contract ID | +| Contract | Legacy contract ID (redeployment required) | |---|---| | `mergefi-escrow` | `CAY77D2SFDVQYONSPYHOEWARE3UIWQDYHWWI2WXNPFBLBKR2Q4GEWXFB` | | `mergefi-milestones` | `CBBRLSL6TM6XCNP2XBVT4GFHJ3NNPFKI2BCZQJ4U3TI7GV7DO2F2HG6F` | | `mergefi-maintenance-pool` | `CD46U7WTEM2I77TXQI2VIBRQXOHEFEYYR2XFA7OVGTXX5M2F7Z3ZQOX2` | -All three were initialized with the same admin/treasury address +All three legacy instances were initialized with the same admin/treasury address (`GBUXADZJ7O4NM7S7CDZYVXGP37M772D2TYMFBT2QFH2JSRCFEJPAVW5N`, a throwaway testnet-only account) and a 250 bps (2.5%) treasury fee. View them on [Stellar Expert](https://stellar.expert/explorer/testnet/contract/CAY77D2SFDVQYONSPYHOEWARE3UIWQDYHWWI2WXNPFBLBKR2Q4GEWXFB). -To redeploy (e.g. after a contract change), once `stellar-cli` has -working network access: +Redeploy all three with constructor arguments in the deployment transaction, +record the new IDs here, update all consumers, and smoke-test their config view +functions before retiring the legacy IDs. With working `stellar-cli` access: ```sh stellar keys generate mergefi-admin --network testnet --fund stellar contract deploy \ --wasm target/wasm32v1-none/release/mergefi_escrow.wasm \ --source mergefi-admin \ - --network testnet -# then, e.g. -stellar contract invoke \ - --id --source mergefi-admin --network testnet \ - -- initialize --admin --treasury --fee_bps 250 + --network testnet -- \ + --admin --treasury --fee_bps 250 ``` Or, in an environment where the CLI's own network calls are blocked but 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 +node scripts/deploy.mjs \ + target/wasm32v1-none/release/mergefi_escrow.wasm \ + 250 escrow ``` ## Roadmap @@ -529,7 +526,7 @@ node scripts/invoke.mjs initialize \ 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 + separate higher-trust deployment/admin-rotation key) once the contracts move past initial testnet iteration. - Support partial milestone/pool refunds and issue re-allocation (currently `allocate` is one-shot per issue). diff --git a/contracts/escrow/src/lib.rs b/contracts/escrow/src/lib.rs index 7b2da8f..762bf23 100644 --- a/contracts/escrow/src/lib.rs +++ b/contracts/escrow/src/lib.rs @@ -35,19 +35,15 @@ pub struct EscrowContract; #[contractimpl] impl EscrowContract { - /// One-time setup. `admin` is the mergefi-backend oracle address that is + /// Atomic deployment-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%. /// - /// 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`. - pub fn initialize( + /// The host invokes this constructor in the contract-creation operation, + /// so no callable, uninitialized instance can exist. The named admin must + /// also authorize the deployment. + pub fn __constructor( env: Env, admin: Address, treasury: Address, @@ -55,9 +51,6 @@ impl EscrowContract { ) -> Result<(), Error> { admin.require_auth(); - if env.storage().instance().has(&DataKey::Admin) { - return Err(Error::AlreadyInitialized); - } if fee_bps as i128 > BPS_DENOMINATOR { return Err(Error::InvalidFee); } @@ -182,7 +175,7 @@ impl EscrowContract { /// Releases escrowed funds to one or more recipients. `recipients` is a /// list of (address, basis_points) pairs that must sum to exactly /// `BPS_DENOMINATOR` (10000 = 100%). A protocol fee (`fee_bps`, - /// configured at `initialize`) is deducted from the total and sent to + /// configured at deployment) 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. diff --git a/contracts/escrow/src/test.rs b/contracts/escrow/src/test.rs index 982244e..c7ed2a3 100644 --- a/contracts/escrow/src/test.rs +++ b/contracts/escrow/src/test.rs @@ -22,19 +22,22 @@ fn create_token<'a>( fn setup(env: &Env) -> (Address, Address, Address, EscrowContractClient<'_>) { let admin = Address::generate(env); let treasury = Address::generate(env); - let contract_id = env.register(EscrowContract, ()); + let contract_id = env.register( + EscrowContract, + EscrowContractArgs::__constructor(&admin, &treasury, &500u32), + ); let client = EscrowContractClient::new(env, &contract_id); - client.initialize(&admin, &treasury, &500u32); // 5% fee (contract_id, admin, treasury, client) } #[test] -fn test_initialize_rejects_double_init() { +fn test_constructor_sets_configuration() { let env = Env::default(); env.mock_all_auths(); let (_, admin, treasury, client) = setup(&env); - let err = client.try_initialize(&admin, &treasury, &500u32); - assert_eq!(err, Err(Ok(Error::AlreadyInitialized))); + assert_eq!(client.get_admin(), admin); + assert_eq!(client.get_treasury(), treasury); + assert_eq!(client.get_fee_bps(), 500u32); } #[test] @@ -172,7 +175,7 @@ fn test_double_release_rejected() { #[test] fn test_unauthorized_release_rejected() { let env = Env::default(); - // initialize/fund both need auth too now, so mock broadly up front and + // Construction/fund both need auth, so mock broadly up front and // turn it off only for the specific unauthorized call under test below. env.mock_all_auths(); let (_, _admin, _treasury, client) = setup(&env); @@ -243,12 +246,10 @@ fn test_adversarial_ordering_resistance() { // 1. Setup contract and environment let admin = 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); - + let contract_id = env.register( + crate::EscrowContract, + crate::EscrowContractArgs::__constructor(&admin, &treasury, &0u32), + ); // 2. Create recipient addresses let dev1 = Address::generate(&env); let dev2 = Address::generate(&env); @@ -307,16 +308,16 @@ fn test_adversarial_ordering_resistance() { // --------------------------------------------------------------------------- #[test] -fn test_initialize_requires_admin_auth() { +#[should_panic] +fn test_constructor_requires_admin_auth() { let env = Env::default(); // No auths mocked at all. let admin = 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); - assert!(result.is_err()); + env.register( + EscrowContract, + EscrowContractArgs::__constructor(&admin, &treasury, &500u32), + ); } #[test] @@ -421,7 +422,7 @@ fn test_extend_deadline_pushes_out_the_permissionless_window() { // Old deadline (200) has now passed, but the extended one (500) hasn't: // refund must still require admin auth, proving the extension actually // re-closed the permissionless window. - env.ledger().set_timestamp(300); + env.ledger().set_timestamp(200 + crate::GRACE_PERIOD); env.set_auths(&[]); let result = client.try_refund(&13u64); assert!(result.is_err()); @@ -515,8 +516,8 @@ fn test_multi_sponsor_refund_returns_exact_contributions_to_each_sponsor() { assert_eq!(escrow.amount, 11_500i128); assert_eq!(escrow.contributor_count, 3); - // Past the deadline: permissionless refund. - env.ledger().set_timestamp(300); + // Past the deadline and grace period: permissionless refund. + env.ledger().set_timestamp(200 + crate::GRACE_PERIOD); env.set_auths(&[]); client.refund(&100u64); @@ -749,7 +750,7 @@ fn test_release_succeeds_in_grace_period() { // Pass the nominal deadline but stay within the grace period. env.ledger().set_timestamp(200 + crate::GRACE_PERIOD - 1); - + // Permissionless refund is still rejected. env.set_auths(&[]); let result = client.try_refund(&200u64); @@ -785,14 +786,14 @@ fn test_release_loses_race_to_refund_at_grace_period_boundary() { env.set_auths(&[]); client.refund(&201u64); assert_eq!(token_client.balance(&sponsor), 10_000_000_000i128); - + // The backend's subsequently-landing release call fails. env.mock_all_auths(); let contributor = Address::generate(&env); let recipients = vec![&env, (contributor.clone(), 10_000u32)]; let err = client.try_release(&201u64, &recipients); assert_eq!(err, Err(Ok(Error::AlreadyRefunded))); - + // The would-be recipient gets nothing. assert_eq!(token_client.balance(&contributor), 0); } diff --git a/contracts/maintenance-pool/src/lib.rs b/contracts/maintenance-pool/src/lib.rs index a23f759..da42af4 100644 --- a/contracts/maintenance-pool/src/lib.rs +++ b/contracts/maintenance-pool/src/lib.rs @@ -25,11 +25,10 @@ pub struct MaintenancePoolContract; #[contractimpl] impl MaintenancePoolContract { - /// One-time setup. Requires `admin`'s own authorization, so nobody can - /// name a third-party address as admin without that address's consent - /// — see `docs/access-control-audit.md` for what this does and does - /// not protect against (it does not stop initializer front-running). - pub fn initialize( + /// Atomic deployment-time setup. The host invokes this constructor in + /// the contract-creation operation, so no callable, uninitialized + /// instance can exist. The named admin must authorize the deployment. + pub fn __constructor( env: Env, admin: Address, treasury: Address, @@ -37,9 +36,6 @@ impl MaintenancePoolContract { ) -> Result<(), Error> { admin.require_auth(); - if env.storage().instance().has(&DataKey::Admin) { - return Err(Error::AlreadyInitialized); - } if fee_bps as i128 > BPS_DENOMINATOR { return Err(Error::InvalidFee); } diff --git a/contracts/maintenance-pool/src/test.rs b/contracts/maintenance-pool/src/test.rs index 6330bb9..2883692 100644 --- a/contracts/maintenance-pool/src/test.rs +++ b/contracts/maintenance-pool/src/test.rs @@ -19,9 +19,11 @@ fn create_token<'a>( fn setup(env: &Env) -> (Address, Address, MaintenancePoolContractClient<'_>) { let admin = Address::generate(env); let treasury = Address::generate(env); - let contract_id = env.register(MaintenancePoolContract, ()); + let contract_id = env.register( + MaintenancePoolContract, + MaintenancePoolContractArgs::__constructor(&admin, &treasury, &1_000u32), + ); let client = MaintenancePoolContractClient::new(env, &contract_id); - client.initialize(&admin, &treasury, &1_000u32); // 10% fee (admin, treasury, client) } @@ -119,15 +121,15 @@ fn test_deposit_rejects_token_mismatch() { // --------------------------------------------------------------------------- #[test] -fn test_initialize_requires_admin_auth() { +#[should_panic] +fn test_constructor_requires_admin_auth() { let env = Env::default(); let admin = 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); - assert!(result.is_err()); + env.register( + MaintenancePoolContract, + MaintenancePoolContractArgs::__constructor(&admin, &treasury, &1_000u32), + ); } #[test] diff --git a/contracts/milestones/src/lib.rs b/contracts/milestones/src/lib.rs index 8a640f8..64018ab 100644 --- a/contracts/milestones/src/lib.rs +++ b/contracts/milestones/src/lib.rs @@ -35,11 +35,10 @@ pub struct MilestonesContract; #[contractimpl] impl MilestonesContract { - /// One-time setup. Requires `admin`'s own authorization, so nobody can - /// name a third-party address as admin without that address's consent - /// — see `docs/access-control-audit.md` for what this does and does - /// not protect against (it does not stop initializer front-running). - pub fn initialize( + /// Atomic deployment-time setup. The host invokes this constructor in + /// the contract-creation operation, so no callable, uninitialized + /// instance can exist. The named admin must authorize the deployment. + pub fn __constructor( env: Env, admin: Address, treasury: Address, @@ -47,9 +46,6 @@ impl MilestonesContract { ) -> Result<(), Error> { admin.require_auth(); - if env.storage().instance().has(&DataKey::Admin) { - return Err(Error::AlreadyInitialized); - } if fee_bps as i128 > BPS_DENOMINATOR { return Err(Error::InvalidFee); } diff --git a/contracts/milestones/src/test.rs b/contracts/milestones/src/test.rs index 1a1c5fa..efd143d 100644 --- a/contracts/milestones/src/test.rs +++ b/contracts/milestones/src/test.rs @@ -19,9 +19,11 @@ fn create_token<'a>( fn setup(env: &Env) -> (Address, Address, MilestonesContractClient<'_>) { let admin = Address::generate(env); let treasury = Address::generate(env); - let contract_id = env.register(MilestonesContract, ()); + let contract_id = env.register( + MilestonesContract, + MilestonesContractArgs::__constructor(&admin, &treasury, &500u32), + ); let client = MilestonesContractClient::new(env, &contract_id); - client.initialize(&admin, &treasury, &500u32); // 5% fee (admin, treasury, client) } @@ -169,15 +171,15 @@ fn test_cancel_milestone_refunds_remaining_budget() { // --------------------------------------------------------------------------- #[test] -fn test_initialize_requires_admin_auth() { +#[should_panic] +fn test_constructor_requires_admin_auth() { let env = Env::default(); let admin = 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); - assert!(result.is_err()); + env.register( + MilestonesContract, + MilestonesContractArgs::__constructor(&admin, &treasury, &500u32), + ); } #[test] diff --git a/docs/access-control-audit.md b/docs/access-control-audit.md index 7503107..345628b 100644 --- a/docs/access-control-audit.md +++ b/docs/access-control-audit.md @@ -14,7 +14,7 @@ signature requirement on any particular address. | Function | Intended access | Actual (before this PR) | Actual (after this PR) | Verdict | |---|---|---|---|---| -| `initialize` | Deployer/authorized setup only (implicit — not written down anywhere) | none | `admin.require_auth()` | **Mismatch, fixed** — see "`initialize` has no access control" below | +| `__constructor` (formerly `initialize`) | Deployer/authorized setup only | none | atomic construction + `admin.require_auth()` | **Mismatch, fixed in #30/#33** — see below | | `fund` | Sponsor-only | `sponsor.require_auth()` | unchanged | Match | | `release` | Admin-only | `require_admin(&env)?.require_auth()` | unchanged | Match | | `refund` (before `deadline`) | Admin-only | `require_admin(&env)?.require_auth()` | unchanged | Match | @@ -29,7 +29,7 @@ signature requirement on any particular address. | Function | Intended access | Actual (before this PR) | Actual (after this PR) | Verdict | |---|---|---|---|---| -| `initialize` | Deployer/authorized setup only (implicit) | none | `admin.require_auth()` | **Mismatch, fixed** | +| `__constructor` (formerly `initialize`) | Deployer/authorized setup only | none | atomic construction + `admin.require_auth()` | **Mismatch, fixed in #30/#33** | | `create_milestone` | Sponsor-only | `sponsor.require_auth()` | unchanged | Match | | `allocate` | Admin-only | `require_admin(&env)?.require_auth()` | unchanged | Match | | `release_issue` | Admin-only | `require_admin(&env)?.require_auth()` | unchanged | Match — access control itself is correct; see note below on the *separate* state-machine gap tracked in #5 | @@ -41,7 +41,7 @@ signature requirement on any particular address. | Function | Intended access | Actual (before this PR) | Actual (after this PR) | Verdict | |---|---|---|---|---| -| `initialize` | Deployer/authorized setup only (implicit) | none | `admin.require_auth()` | **Mismatch, fixed** | +| `__constructor` (formerly `initialize`) | Deployer/authorized setup only | none | atomic construction + `admin.require_auth()` | **Mismatch, fixed in #30/#33** | | `deposit` | Sponsor-only | `sponsor.require_auth()` | unchanged | Match | | `withdraw` | Admin-only | `require_admin(&env)?.require_auth()` | unchanged | Match | | `get_pool` | Permissionless (view) | none | unchanged | Match | @@ -49,7 +49,7 @@ signature requirement on any particular address. ## Findings -### 1. `initialize` has no access control in any of the three contracts +### 1. Legacy `initialize` had no access control in any contract Before this PR, `initialize(admin, treasury, fee_bps)` performed **zero** `require_auth()` calls in all three contracts — the only guard is @@ -61,6 +61,14 @@ contract and name itself (or anyone) as `admin`/`treasury`. **Fix applied in this PR:** all three `initialize` functions now call `admin.require_auth()` before writing any state. +**Follow-up resolution in #33:** `initialize` has now been removed from all +three public APIs and replaced by `__constructor(admin, treasury, fee_bps)`. +Soroban executes the constructor atomically with instance creation, eliminating +the deployment/initialization transaction-ordering window described below. +The constructor retains `admin.require_auth()` so an unwilling third party +cannot be named as admin. The remainder of this section is preserved as the +historical reasoning that motivated #33. + **What this fix does and does not solve — read carefully, this is not a complete fix:** @@ -80,7 +88,8 @@ complete fix:** lands first, wins" — there is no address to check a signature *against* until that transaction has already executed. - The actual fix for this class of bug is structural: use Soroban's + The actual fix for this class of bug is structural and is now implemented: + use Soroban's native constructor (`__constructor`, supported since roughly soroban-sdk 21/22 — this repo is on 26.1.0) so that contract creation and initialization happen atomically in a single host operation, with @@ -89,8 +98,8 @@ complete fix:** touches `scripts/deploy.mjs`, the `Makefile` deploy targets, and the README's deploy instructions, and it's not backward compatible with the already-initialized testnet deployments listed in the README) — - too large and orthogonal to bundle into an access-control audit PR. - Filed as + too large and orthogonal to bundle into the original access-control audit PR. + Tracked and resolved as [#33](https://github.com/MergeFi/contracts/issues/33) to track the constructor migration separately. diff --git a/scripts/deploy.mjs b/scripts/deploy.mjs index 7e1048c..fe655de 100644 --- a/scripts/deploy.mjs +++ b/scripts/deploy.mjs @@ -8,6 +8,7 @@ import { BASE_FEE, Operation, Address, + nativeToScVal, rpc, } from "@stellar/stellar-sdk"; @@ -18,14 +19,34 @@ const NETWORK_PASSPHRASE = Networks.TESTNET; const server = new rpc.Server(RPC_URL); const deployerSecret = process.argv[2]; const wasmPath = process.argv[3]; -const contractName = process.argv[4] ?? "contract"; +const admin = process.argv[4]; +const treasury = process.argv[5]; +const feeBps = process.argv[6]; +const contractName = process.argv[7] ?? "contract"; -if (!deployerSecret || !wasmPath) { - console.error("Usage: node deploy.mjs [name]"); +if (!deployerSecret || !wasmPath || !admin || !treasury || feeBps === undefined) { + console.error( + "Usage: node deploy.mjs [name]", + ); process.exit(1); } const kp = Keypair.fromSecret(deployerSecret); +const parsedFeeBps = Number.parseInt(feeBps, 10); +if (!Number.isInteger(parsedFeeBps) || parsedFeeBps < 0 || parsedFeeBps > 10_000) { + throw new Error("fee-bps must be an integer between 0 and 10000"); +} +if (admin !== kp.publicKey()) { + throw new Error( + "admin must match the supplied secret key because the constructor requires admin authorization", + ); +} + +const constructorArgs = [ + nativeToScVal(new Address(admin), { type: "address" }), + nativeToScVal(new Address(treasury), { type: "address" }), + nativeToScVal(parsedFeeBps, { type: "u32" }), +]; async function submitAndWait(tx) { const prepared = await server.prepareTransaction(tx); @@ -63,7 +84,7 @@ async function main() { const wasmHash = uploadResult.returnValue.bytes(); console.log(`[${contractName}] wasm uploaded, hash: ${wasmHash.toString("hex")}`); - // 2. Create the contract instance from that wasm hash. + // 2. Create and initialize the instance atomically via __constructor. const account2 = await server.getAccount(kp.publicKey()); const createTx = new TransactionBuilder(account2, { fee: BASE_FEE, @@ -73,6 +94,7 @@ async function main() { Operation.createCustomContract({ address: new Address(kp.publicKey()), wasmHash, + constructorArgs, salt: Buffer.from( Array.from({ length: 32 }, () => Math.floor(Math.random() * 256)), ), diff --git a/scripts/invoke.mjs b/scripts/invoke.mjs index b7b912b..a64d47d 100644 --- a/scripts/invoke.mjs +++ b/scripts/invoke.mjs @@ -19,6 +19,12 @@ if (!secret || !contractId || !method) { process.exit(1); } +if (method === "initialize" || method === "__constructor") { + throw new Error( + "configuration is constructor-only; pass admin, treasury, and fee-bps to deploy.mjs", + ); +} + function parseArg(raw) { const [type, value] = raw.split(":"); if (type === "address") return nativeToScVal(new Address(value), { type: "address" });