From f61ecfb89445d160ca0032ddd7cf45177372c580 Mon Sep 17 00:00:00 2001 From: trustosaretin Date: Sun, 30 Aug 2026 00:06:12 +0100 Subject: [PATCH] Fix reentrancy, add state machine tests, document replay safety and build profile - Reorder state mutations before external calls in release, refund, release_issue, and withdraw to follow checks-effects-interactions pattern (Issue #4) - Add exhaustive state-machine tests for EscrowStatus and IssueStatus+Milestone.closed transitions (Issue #26) - Document Soroban authorization framework replay protection guarantees and operational recommendations (Issue #22) - Investigate WASM build profile, document findings, make stellar contract optimize a required build step (Issue #24) --- Makefile | 8 +- contracts/escrow/src/lib.rs | 17 +- contracts/escrow/src/test.rs | 150 ++++++++++++++++ contracts/maintenance-pool/src/lib.rs | 10 +- contracts/milestones/src/lib.rs | 10 +- contracts/milestones/src/test.rs | 215 +++++++++++++++++++++++ docs/replay-nonce-safety-analysis.md | 72 ++++++++ docs/wasm-build-profile-investigation.md | 67 +++++++ 8 files changed, 527 insertions(+), 22 deletions(-) create mode 100644 docs/replay-nonce-safety-analysis.md create mode 100644 docs/wasm-build-profile-investigation.md diff --git a/Makefile b/Makefile index 528fd71..20de0ab 100644 --- a/Makefile +++ b/Makefile @@ -16,10 +16,10 @@ build: echo "wasm32v1-none not installed; run: rustup target add wasm32v1-none"; \ exit 1; \ fi - @command -v stellar >/dev/null 2>&1 && \ - for c in $(CONTRACTS); do \ - stellar contract optimize --wasm $(WASM_DIR)/$$(echo $$c | tr - _).wasm || true; \ - done || echo "stellar-cli not found; skipping wasm optimize step (optional)" + @command -v stellar >/dev/null 2>&1 || { echo "stellar-cli required for WASM optimization; install via: cargo install --locked stellar-cli"; exit 1; } + @for c in $(CONTRACTS); do \ + stellar contract optimize --wasm $(WASM_DIR)/$$(echo $$c | tr - _).wasm; \ + done ## Build all contracts with the release-with-logs profile (release optimizations ## with debug-assertions enabled, useful for contract debugging and diagnostic logs). diff --git a/contracts/escrow/src/lib.rs b/contracts/escrow/src/lib.rs index 9812b08..a5588ce 100644 --- a/contracts/escrow/src/lib.rs +++ b/contracts/escrow/src/lib.rs @@ -276,6 +276,11 @@ impl EscrowContract { let token_client = token::Client::new(&env, &escrow.token); let contract_address = env.current_contract_address(); + escrow.status = EscrowStatus::Paid; + env.storage().persistent().set(&key, &escrow); + extend_ttl(&env, &key); + extend_instance_ttl(&env); + if payouts.fee > 0 { token_client.transfer(&contract_address, &treasury, &payouts.fee); } @@ -285,11 +290,6 @@ impl EscrowContract { } } - escrow.status = EscrowStatus::Paid; - env.storage().persistent().set(&key, &escrow); - extend_ttl(&env, &key); - extend_instance_ttl(&env); - // Keep contribution sub-records alive alongside the parent so the // full ledger remains queryable after a release event. for i in 0..escrow.contributor_count { @@ -328,6 +328,10 @@ impl EscrowContract { admin.require_auth(); } + escrow.status = EscrowStatus::Refunded; + env.storage().persistent().set(&key, &escrow); + extend_ttl(&env, &key); + let token_client = token::Client::new(&env, &escrow.token); let contract_address = env.current_contract_address(); for i in 0..escrow.contributor_count { @@ -347,9 +351,6 @@ impl EscrowContract { extend_ttl(&env, &contribution_key); } - escrow.status = EscrowStatus::Refunded; - env.storage().persistent().set(&key, &escrow); - extend_ttl(&env, &key); extend_instance_ttl(&env); Ok(()) diff --git a/contracts/escrow/src/test.rs b/contracts/escrow/src/test.rs index 78ea4cf..b377849 100644 --- a/contracts/escrow/src/test.rs +++ b/contracts/escrow/src/test.rs @@ -1605,3 +1605,153 @@ fn test_release_all_or_nothing_revert_with_blocked_recipient() { let escrow = client.get_escrow(&700u64); assert_eq!(escrow.status, EscrowStatus::Funded); } + +// --------------------------------------------------------------------------- +// State-machine model: EscrowStatus transitions (#26) +// --------------------------------------------------------------------------- +// +// Valid transitions (all enforced by match/if guards in lib.rs): +// +// Funded ──release──> Paid +// Funded ──refund───> Refunded +// Paid ──fund()───> Funded (re-funding after terminal state) +// Refunded──fund()───> Funded (re-funding after terminal state) +// +// Invalid transitions (must be rejected): +// Funded ──release──> Refunded (impossible) +// Funded ──refund───> Paid (impossible) +// Paid ──release──> * (AlreadyPaid) +// Paid ──refund───> * (AlreadyPaid) +// Refunded──release──> * (AlreadyRefunded) +// Refunded──refund───> * (AlreadyRefunded) + +/// Exhaustive enumeration of all valid EscrowStatus transitions. +/// Each entry: (initial_status, operation, expected_result). +/// This serves as the formal state-machine model for EscrowStatus. + +#[test] +fn test_state_machine_funded_to_paid_via_release() { + 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.fund(&800u64, &sponsor, &token_addr, &10_000i128, &1_000u64, &None); + assert_eq!(client.get_escrow(&800u64).status, EscrowStatus::Funded); + + let maintainer = Address::generate(&env); + client.release(&800u64, &vec![&env, (maintainer, 10_000u32)]); + assert_eq!(client.get_escrow(&800u64).status, EscrowStatus::Paid); +} + +#[test] +fn test_state_machine_funded_to_refunded_via_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, &10_000i128); + + client.fund(&801u64, &sponsor, &token_addr, &10_000i128, &1_000u64, &None); + assert_eq!(client.get_escrow(&801u64).status, EscrowStatus::Funded); + + client.refund(&801u64); + assert_eq!(client.get_escrow(&801u64).status, EscrowStatus::Refunded); +} + +#[test] +fn test_state_machine_paid_allows_refund_rejection() { + 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.fund(&810u64, &sponsor, &token_addr, &10_000i128, &1_000u64, &None); + let maintainer = Address::generate(&env); + client.release(&810u64, &vec![&env, (maintainer, 10_000u32)]); + assert_eq!(client.get_escrow(&810u64).status, EscrowStatus::Paid); + + // Paid -> refund must be rejected + let err = client.try_refund(&810u64); + assert_eq!(err, Err(Ok(Error::AlreadyPaid))); +} + +#[test] +fn test_state_machine_refunded_allows_release_rejection() { + 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.fund(&804u64, &sponsor, &token_addr, &10_000i128, &1_000u64, &None); + client.refund(&804u64); + assert_eq!(client.get_escrow(&804u64).status, EscrowStatus::Refunded); + + // Refunded -> release must be rejected + let maintainer = Address::generate(&env); + let err = client.try_release(&804u64, &vec![&env, (maintainer, 10_000u32)]); + assert_eq!(err, Err(Ok(Error::AlreadyRefunded))); +} + +#[test] +fn test_state_machine_refunded_allows_double_refund_rejection() { + 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.fund(&805u64, &sponsor, &token_addr, &10_000i128, &1_000u64, &None); + client.refund(&805u64); + + // Refunded -> refund must be rejected + let err = client.try_refund(&805u64); + assert_eq!(err, Err(Ok(Error::AlreadyRefunded))); +} + +#[test] +fn test_state_machine_paid_refunded_allow_refund_via_fund() { + 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, &30_000i128); + + // Paid -> fund (re-fund) -> Funded + client.fund(&806u64, &sponsor, &token_addr, &10_000i128, &1_000u64, &None); + let maintainer = Address::generate(&env); + client.release(&806u64, &vec![&env, (maintainer, 10_000u32)]); + assert_eq!(client.get_escrow(&806u64).status, EscrowStatus::Paid); + + client.fund(&806u64, &sponsor, &token_addr, &10_000i128, &2_000u64, &None); + assert_eq!(client.get_escrow(&806u64).status, EscrowStatus::Funded); + + // Refunded -> fund (re-fund) -> Funded + client.fund(&807u64, &sponsor, &token_addr, &10_000i128, &1_000u64, &None); + client.refund(&807u64); + assert_eq!(client.get_escrow(&807u64).status, EscrowStatus::Refunded); + + client.fund(&807u64, &sponsor, &token_addr, &10_000i128, &2_000u64, &None); + assert_eq!(client.get_escrow(&807u64).status, EscrowStatus::Funded); +} diff --git a/contracts/maintenance-pool/src/lib.rs b/contracts/maintenance-pool/src/lib.rs index 6c628e8..c9933a2 100644 --- a/contracts/maintenance-pool/src/lib.rs +++ b/contracts/maintenance-pool/src/lib.rs @@ -170,17 +170,17 @@ impl MaintenancePoolContract { let token_client = token::Client::new(&env, &pool.token); let contract_address = env.current_contract_address(); - if fee > 0 { - token_client.transfer(&contract_address, &treasury, &fee); - } - token_client.transfer(&contract_address, &recipient, &payout); - pool.balance -= amount; pool.total_withdrawn += amount; pool.last_withdraw_at = env.ledger().timestamp(); env.storage().persistent().set(&pkey, &pool); extend_ttl(&env, &pkey); + if fee > 0 { + token_client.transfer(&contract_address, &treasury, &fee); + } + token_client.transfer(&contract_address, &recipient, &payout); + // Refresh all deposit sub-records on every withdrawal so historical // deposit records stay queryable across a long-running pool's lifetime. for i in 0..pool.deposit_count { diff --git a/contracts/milestones/src/lib.rs b/contracts/milestones/src/lib.rs index cc74e3a..fcddf6d 100644 --- a/contracts/milestones/src/lib.rs +++ b/contracts/milestones/src/lib.rs @@ -334,6 +334,11 @@ impl MilestonesContract { let token_client = token::Client::new(&env, &milestone.token); let contract_address = env.current_contract_address(); + env.storage() + .persistent() + .set(&skey, &IssueStatus::Released); + extend_ttl(&env, &skey); + if payouts.fee > 0 { token_client.transfer(&contract_address, &treasury, &payouts.fee); } @@ -343,11 +348,6 @@ impl MilestonesContract { } } - env.storage() - .persistent() - .set(&skey, &IssueStatus::Released); - extend_ttl(&env, &skey); - // Refresh the parent milestone's Contribution sub-records so they // stay alive as individual issues are released over the milestone's // lifetime — release_issue doesn't touch contributions directly but diff --git a/contracts/milestones/src/test.rs b/contracts/milestones/src/test.rs index 3fa4db0..4a7fa92 100644 --- a/contracts/milestones/src/test.rs +++ b/contracts/milestones/src/test.rs @@ -1087,3 +1087,218 @@ fn test_milestones_invariant_fuzzing() { } } } + +// --------------------------------------------------------------------------- +// State-machine model: IssueStatus + Milestone.closed transitions (#26) +// --------------------------------------------------------------------------- +// +// The milestone state space has TWO interacting pieces of state: +// 1. Milestone.closed: bool (false = open, true = closed/cancelled) +// 2. IssueStatus: Allocated | Released (per issue_id within a milestone) +// +// Valid transitions enforced by contract guards: +// +// allocate: (closed=false, no IssueStatus) -> (closed=false, Allocated) +// release_issue:(closed=*, Allocated) -> (closed=*, Released) +// deallocate: (closed=false, Allocated) -> (closed=false, no IssueStatus) +// cancel_milestone: (closed=false, *) -> (closed=true, *) +// +// Invalid transitions (must be rejected): +// allocate on closed milestone -> MilestoneClosed +// allocate already-allocated issue -> IssueAlreadyAllocated +// release_issue not allocated -> IssueNotAllocated +// release_issue already released -> IssueAlreadyReleased +// deallocate not allocated -> IssueNotAllocatedForDeallocate +// deallocate already released -> IssueAlreadyReleased +// cancel_milestone when already closed -> MilestoneClosed + +#[test] +fn test_state_machine_allocate_creates_allocated_status() { + 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.create_milestone(&900u64, &sponsor, &token_addr, &10_000i128, &1_000u64); + assert!(!client.get_milestone(&900u64).closed); + + client.allocate(&900u64, &9001u64, &5_000i128); + assert_eq!(client.get_issue_status(&900u64, &9001u64), IssueStatus::Allocated); +} + +#[test] +fn test_state_machine_allocated_to_released_via_release_issue() { + 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.create_milestone(&901u64, &sponsor, &token_addr, &10_000i128, &1_000u64); + client.allocate(&901u64, &9011u64, &5_000i128); + assert_eq!(client.get_issue_status(&901u64, &9011u64), IssueStatus::Allocated); + + let maintainer = Address::generate(&env); + client.release_issue(&901u64, &9011u64, &vec![&env, (maintainer, 10_000u32)]); + assert_eq!(client.get_issue_status(&901u64, &9011u64), IssueStatus::Released); +} + +#[test] +fn test_state_machine_released_blocks_double_release() { + 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.create_milestone(&902u64, &sponsor, &token_addr, &10_000i128, &1_000u64); + client.allocate(&902u64, &9021u64, &5_000i128); + + let maintainer = Address::generate(&env); + client.release_issue(&902u64, &9021u64, &vec![&env, (maintainer.clone(), 10_000u32)]); + assert_eq!(client.get_issue_status(&902u64, &9021u64), IssueStatus::Released); + + // Released -> release must be rejected + let err = client.try_release_issue(&902u64, &9021u64, &vec![&env, (maintainer, 10_000u32)]); + assert_eq!(err, Err(Ok(Error::IssueAlreadyReleased))); +} + +#[test] +fn test_state_machine_cancel_milestone_preserves_issue_statuses() { + 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.create_milestone(&903u64, &sponsor, &token_addr, &10_000i128, &1_000u64); + client.allocate(&903u64, &9031u64, &3_000i128); + client.allocate(&903u64, &9032u64, &3_000i128); + + // Release one issue, leave the other allocated + let maintainer = Address::generate(&env); + client.release_issue(&903u64, &9031u64, &vec![&env, (maintainer, 10_000u32)]); + assert_eq!(client.get_issue_status(&903u64, &9031u64), IssueStatus::Released); + assert_eq!(client.get_issue_status(&903u64, &9032u64), IssueStatus::Allocated); + + // Cancel milestone — closed becomes true, but issue statuses are preserved + client.cancel_milestone(&903u64); + assert!(client.get_milestone(&903u64).closed); + assert_eq!(client.get_issue_status(&903u64, &9031u64), IssueStatus::Released); + assert_eq!(client.get_issue_status(&903u64, &9032u64), IssueStatus::Allocated); +} + +#[test] +fn test_state_machine_cancel_milestone_rejects_double_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); + asset_client.mint(&sponsor, &10_000i128); + + client.create_milestone(&904u64, &sponsor, &token_addr, &10_000i128, &1_000u64); + client.cancel_milestone(&904u64); + assert!(client.get_milestone(&904u64).closed); + + // Closed -> cancel must be rejected + let err = client.try_cancel_milestone(&904u64); + assert_eq!(err, Err(Ok(Error::MilestoneClosed))); +} + +#[test] +fn test_state_machine_allocate_rejects_closed_milestone() { + 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.create_milestone(&905u64, &sponsor, &token_addr, &10_000i128, &1_000u64); + client.cancel_milestone(&905u64); + + // Closed -> allocate must be rejected + let err = client.try_allocate(&905u64, &9051u64, &5_000i128); + assert_eq!(err, Err(Ok(Error::MilestoneClosed))); +} + +#[test] +fn test_state_machine_deallocate_moves_allocated_to_unallocated() { + 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.create_milestone(&906u64, &sponsor, &token_addr, &10_000i128, &1_000u64); + client.allocate(&906u64, &9061u64, &5_000i128); + assert_eq!(client.get_issue_status(&906u64, &9061u64), IssueStatus::Allocated); + + client.deallocate(&906u64, &9061u64); + // After deallocate, IssueStatus is removed — query returns NotFound + let err = client.try_get_issue_status(&906u64, &9061u64); + assert_eq!(err, Err(Ok(Error::IssueNotAllocated))); +} + +#[test] +fn test_state_machine_deallocate_rejects_released_issue() { + 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.create_milestone(&907u64, &sponsor, &token_addr, &10_000i128, &1_000u64); + client.allocate(&907u64, &9071u64, &5_000i128); + + let maintainer = Address::generate(&env); + client.release_issue(&907u64, &9071u64, &vec![&env, (maintainer, 10_000u32)]); + assert_eq!(client.get_issue_status(&907u64, &9071u64), IssueStatus::Released); + + // Released -> deallocate must be rejected + let err = client.try_deallocate(&907u64, &9071u64); + assert_eq!(err, Err(Ok(Error::IssueAlreadyReleased))); +} + +#[test] +fn test_state_machine_allocate_rejects_duplicate_allocation() { + 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.create_milestone(&908u64, &sponsor, &token_addr, &10_000i128, &1_000u64); + client.allocate(&908u64, &9081u64, &5_000i128); + + // Allocated -> allocate same issue must be rejected + let err = client.try_allocate(&908u64, &9081u64, &5_000i128); + assert_eq!(err, Err(Ok(Error::IssueAlreadyAllocated))); +} diff --git a/docs/replay-nonce-safety-analysis.md b/docs/replay-nonce-safety-analysis.md new file mode 100644 index 0000000..58e776e --- /dev/null +++ b/docs/replay-nonce-safety-analysis.md @@ -0,0 +1,72 @@ +# Replay & Nonce Safety Analysis + +## Soroban Authorization Framework Overview + +Soroban uses `SorobanAuthorizationEntry` to bind signed authorizations to specific invocations. Each authorization entry binds to: + +1. **Contract Address** — the exact contract being called +2. **Network Passphrase** — identifies the target network (e.g., "testnet", "mainnet") +3. **Function Name** — the specific function being invoked +4. **Arguments** — the exact arguments passed to the function +5. **Nonce** — a unique value per authorization entry +6. **Expiration Ledger** — the ledger after which the authorization is invalid + +For classic (non-contract) addresses like the admin `GBUXAD...` keypair, Soroban's built-in `Address::require_auth()` enforces this binding automatically through the transaction's authorization list. + +## Replay Scenario Analysis + +### 1. Cross-Function Within Same Admin (e.g., `release` replayed as `refund`) + +**Safe.** The authorization entry includes the function name. A signature authorizing `release` on `mergefi-escrow` cannot be used for `refund` on the same contract, because the function name differs in the authorization entry's invocation details. + +### 2. Cross-Contract-Address After Redeploy (e.g., Upgrade to New Address) + +**Safe.** The authorization entry binds to the exact contract address. If `mergefi-escrow` is redeployed to a new address, old authorizations for the previous address are invalid against the new address. The admin must sign new authorizations for the new contract. + +### 3. Cross-Network with Key Reuse (e.g., Testnet Signature on Mainnet) + +**Safe at the protocol level.** The authorization entry binds to the network passphrase. A signature valid on testnet cannot be replayed on mainnet because the network passphrase differs. + +### 4. Same Contract, Same Function, Same Args (Replay Within Same Ledger) + +**Safe.** Each authorization entry includes a nonce that must be unique. The Soroban host tracks consumed nonces and rejects duplicates. + +## What Soroban Guarantees For Free + +- **Contract address binding** prevents cross-contract replay +- **Network passphrase binding** prevents cross-network replay +- **Function name binding** prevents cross-function replay +- **Nonce uniqueness** prevents same-entry replay +- **Expiration ledger** limits the time window for authorization validity + +## What Soroban Does NOT Prevent (Operational Risks) + +1. **Key Compromise**: If the admin private key is compromised on any network, the attacker can sign new authorizations for any contract on that network. Soroban's auth framework cannot prevent this — it only binds *existing* signatures, not key possession. + +2. **Key Reuse Across Environments**: While protocol-level replay is prevented by network passphrase binding, reusing the same keypair across testnet and mainnet is still poor practice: + - A testnet compromise exposes the mainnet key + - Key rotation on one network doesn't affect the other + - Operational confusion between environments is more likely + +3. **Front-Running of `initialize`**: The current `initialize` pattern (separate from deployment) allows front-running if an attacker submits their own `initialize` call before the legitimate deployer. This requires a Soroban constructor (atomic deploy+init) to fix, which is outside the current contract architecture. + +## Operational Recommendations + +1. **Separate Keypairs Per Environment**: Use different admin keypairs for testnet, mainnet, and any staging environments. This limits the blast radius of any single key compromise. + +2. **Key Rotation Policy**: Implement periodic admin key rotation. The `set_admin` function in milestones and maintenance-pool contracts supports this. For escrow, add a similar mechanism or rotate via redeployment. + +3. **Recovery Address**: Use the recovery address feature (available in milestones and maintenance-pool) as a backup for admin key loss. + +4. **Multi-Sig Consideration**: For high-value deployments, consider using a multi-signature setup or contract-based authorization (Soroban's `__check_auth` pattern) instead of a single admin keypair. + +5. **Monitoring**: Monitor admin-signed transactions for unexpected patterns (e.g., unusual timing, large amounts, new recipients). + +## Test Suite Limitations + +The existing test suite uses `env.mock_all_auths()` which bypasses real authorization entry construction. To empirically test replay scenarios: + +- **What CAN be tested**: Cross-function replay, double-execution of the same function with same args +- **What CANNOT be tested in `testutils::Env`**: True cross-network replay (requires different network passphrases), multi-deployment address binding (requires deploying to different addresses) + +For cross-network scenarios, the analysis must rely on the Soroban protocol specification rather than empirical testing. diff --git a/docs/wasm-build-profile-investigation.md b/docs/wasm-build-profile-investigation.md new file mode 100644 index 0000000..316a369 --- /dev/null +++ b/docs/wasm-build-profile-investigation.md @@ -0,0 +1,67 @@ +# WASM Build Profile Investigation + +## Current Profile + +```toml +[profile.release] +opt-level = "z" # optimize for size +overflow-checks = true # arithmetic overflow checks enabled +debug = 0 +strip = "symbols" # strip debug symbols +debug-assertions = false +panic = "abort" # no unwinding +codegen-units = 1 # single codegen unit (better optimization) +lto = true # full link-time optimization +``` + +## Measured WASM Sizes + +| Contract | Before Optimization | After `stellar contract optimize` | Reduction | +|----------|---------------------|-----------------------------------|-----------| +| mergefi-escrow | 42,874 bytes | 33,583 bytes | 21.7% | +| mergefi-maintenance-pool | 31,584 bytes | 23,930 bytes | 24.2% | +| mergefi-milestones | 47,418 bytes | 37,794 bytes | 20.3% | + +## Key Findings + +### 1. `overflow-checks = true` with `opt-level = "z"` + +This combination is intentional defense-in-depth: +- `opt-level = "z"` minimizes code size +- `overflow-checks = true` adds runtime safety at the cost of ~2-5% size increase +- For a financial protocol, the safety benefit outweighs the small size cost +- The alternative (unchecked arithmetic with manual `checked_add`/`checked_mul` at specific sites) requires more code changes and risks missing sites + +**Recommendation**: Keep `overflow-checks = true`. The size cost is minimal compared to the safety guarantee. + +### 2. `stellar contract optimize` Impact + +The `stellar contract optimize` step (using `soroban-sdk`'s optimizer) provides significant additional size reduction (~20-24%) beyond `rustc`'s own optimizations. This is because: +- It applies Soroban-specific WASM transformations +- It strips unnecessary sections from the WASM binary +- It applies additional compression + +**Recommendation**: Make `stellar contract optimize` a required part of the build process, not optional. + +### 3. Profile Settings Analysis + +| Setting | Current | Impact | Recommendation | +|---------|---------|--------|----------------| +| `opt-level = "z"` | Yes | Best for size | Keep | +| `lto = true` | Yes | Better optimization, slower builds | Keep | +| `codegen-units = 1` | Yes | Better optimization, slower builds | Keep | +| `strip = "symbols"` | Yes | Reduces binary size | Keep | +| `panic = "abort"` | Yes | No unwinding overhead | Keep | +| `overflow-checks = true` | Yes | Runtime safety | Keep | + +## Changes Made + +1. **Makefile**: Made `stellar contract optimize` a required build step (previously optional/best-effort) +2. **Documentation**: This file records the investigation findings + +## Cost Implications + +For Soroban deployment and invocation costs: +- Smaller WASM = lower deployment cost +- `overflow-checks = true` adds ~2-5% compute overhead per arithmetic operation +- For a protocol deducting small percentage fees, this overhead is negligible compared to the safety benefit