diff --git a/contracts/access_control/src/lib.rs b/contracts/access_control/src/lib.rs index f33d98b..b556c10 100644 --- a/contracts/access_control/src/lib.rs +++ b/contracts/access_control/src/lib.rs @@ -169,12 +169,18 @@ impl AccessControlContract { /// **Errors:** /// - `AccessControlError::Unauthorized` / `AccessControlError::NotAdmin` — Caller is not the admin. /// - `AccessControlError::AlreadyPaused` — Protocol is already in the paused state. + /// - `AccessControlError::DirectCallProhibited` — Multisig is configured; use propose/approve/execute instead. /// - `AccessControlError::Reentrancy` — Reentrancy guard triggered (should never happen in normal flow). /// /// **Security:** Requires `admin.require_auth()`. Emits `protocol_paused` event. + /// Once a multisig is configured, this function is blocked and the action must route through + /// propose_action/approve_action/execute_action instead. pub fn pause(env: Env, admin: Address) -> Result<(), AccessControlError> { admin.require_auth(); Self::require_admin(&env, &admin)?; + if Self::load_multisig_config(&env).is_ok() { + return Err(AccessControlError::DirectCallProhibited); + } if env .storage() .instance() @@ -198,12 +204,18 @@ impl AccessControlContract { /// **Errors:** /// - `AccessControlError::Unauthorized` / `AccessControlError::NotAdmin` — Caller is not the admin. /// - `AccessControlError::NotPaused` — Protocol is not currently paused. + /// - `AccessControlError::DirectCallProhibited` — Multisig is configured; use propose/approve/execute instead. /// - `AccessControlError::Reentrancy` — Reentrancy guard triggered. /// /// **Security:** Requires `admin.require_auth()`. Emits `protocol_unpaused` event. + /// Once a multisig is configured, this function is blocked and the action must route through + /// propose_action/approve_action/execute_action instead. pub fn unpause(env: Env, admin: Address) -> Result<(), AccessControlError> { admin.require_auth(); Self::require_admin(&env, &admin)?; + if Self::load_multisig_config(&env).is_ok() { + return Err(AccessControlError::DirectCallProhibited); + } if !env .storage() .instance() @@ -232,12 +244,12 @@ impl AccessControlContract { /// - `AccessControlError::NotAdmin` — Caller is not the admin. /// - `AccessControlError::Unauthorized` — Attempt to grant `Role::Admin` (use `transfer_admin`), /// grant `Role::None` (use `revoke_role`), or grant a role to the current admin. + /// - `AccessControlError::DirectCallProhibited` — Multisig is configured; use propose/approve/execute instead. /// /// **Security:** Requires `admin.require_auth()`. Cannot grant `Role::Admin` directly — /// use `transfer_admin` instead. Cannot grant `Role::None` — use `revoke_role` instead. - /// - Cannot grant `Role::Admin` (use `transfer_admin`). - /// - Cannot grant `Role::None` (use `revoke_role`). - /// - Cannot grant a role to the current admin address. + /// Once a multisig is configured, this function is blocked and the action must route through + /// propose_action/approve_action/execute_action instead. pub fn grant_role( env: Env, admin: Address, @@ -246,6 +258,9 @@ impl AccessControlContract { ) -> Result<(), AccessControlError> { admin.require_auth(); Self::require_admin(&env, &admin)?; + if Self::load_multisig_config(&env).is_ok() { + return Err(AccessControlError::DirectCallProhibited); + } if role == Role::Admin { return Err(AccessControlError::Unauthorized); @@ -275,14 +290,17 @@ impl AccessControlContract { /// - `AccessControlError::NotAdmin` — Caller is not the admin. /// - `AccessControlError::Unauthorized` — Attempt to revoke the admin's own role. /// - `AccessControlError::RoleNotAssigned` — Target has no role assigned. + /// - `AccessControlError::DirectCallProhibited` — Multisig is configured; use propose/approve/execute instead. /// /// **Security:** Requires `admin.require_auth()`. Uses `remove()` to reclaim storage - /// rather than writing `Role::None`. - /// - Cannot revoke the admin's own role. - /// - Fails if the target has no role assigned. + /// rather than writing `Role::None`. Once a multisig is configured, this function is blocked + /// and the action must route through propose_action/approve_action/execute_action instead. pub fn revoke_role(env: Env, admin: Address, target: Address) -> Result<(), AccessControlError> { admin.require_auth(); Self::require_admin(&env, &admin)?; + if Self::load_multisig_config(&env).is_ok() { + return Err(AccessControlError::DirectCallProhibited); + } let current_role = env .storage() .persistent() @@ -319,12 +337,12 @@ impl AccessControlContract { /// - `AccessControlError::InvalidAddress` — `new_admin` equals `current_admin` or is the contract itself. /// - `AccessControlError::Unauthorized` — `new_admin` already holds an `Operator` or `Verifier` role. /// The caller must revoke that role first. + /// - `AccessControlError::DirectCallProhibited` — Multisig is configured; use propose/approve/execute instead. /// /// **Security:** Requires `current_admin.require_auth()`. Prevents silent role overwrites - /// by rejecting addresses that already hold a non-None, non-Admin role. - /// - Cannot transfer to self. - /// - Cannot transfer to an address that already holds a non-None role - /// (would silently overwrite it). The caller must revoke first. + /// by rejecting addresses that already hold a non-None, non-Admin role. Once a multisig is + /// configured, this function is blocked and the action must route through + /// propose_action/approve_action/execute_action instead. pub fn transfer_admin( env: Env, current_admin: Address, @@ -332,6 +350,9 @@ impl AccessControlContract { ) -> Result<(), AccessControlError> { current_admin.require_auth(); Self::require_admin(&env, ¤t_admin)?; + if Self::load_multisig_config(&env).is_ok() { + return Err(AccessControlError::DirectCallProhibited); + } Self::validate_transfer_admin_target(&env, &new_admin, ¤t_admin)?; @@ -751,6 +772,7 @@ impl AccessControlContract { proposer: proposer.clone(), approvals, created_at: env.ledger().timestamp(), + expires_at: env.ledger().timestamp() + PROPOSAL_TTL_LEDGERS, executed: false, cancelled: false, }; @@ -786,9 +808,11 @@ impl AccessControlContract { /// - `AccessControlError::NotMultisigSigner` — Caller is not a configured signer. /// - `AccessControlError::ParameterProposalNotFound` — No proposal exists with the given ID. /// - `AccessControlError::ParameterProposalAlreadyExecuted` — Proposal already executed. + /// - `AccessControlError::ParameterProposalExpired` — Proposal's TTL has elapsed. /// - `AccessControlError::AlreadyVoted` — Caller has already cast their vote. /// /// **Security:** Requires `signer.require_auth()`. Each signer may only vote once. + /// Proposals expire after ~7 days (`PROPOSAL_TTL_LEDGERS`). pub fn vote_parameter_change( env: Env, signer: Address, @@ -830,8 +854,11 @@ impl AccessControlContract { Ok(()) } - /// Execute a parameter-change proposal once it has reached the multisig threshold (B2) and the - /// governance timelock has elapsed (B1). Commits the new value on-chain. + /// Execute a parameter-change proposal once it has reached the multisig threshold (B2), the + /// governance timelock has elapsed (B1), and the proposal has not expired. Commits the new value on-chain. + /// + /// **Errors:** + /// - `AccessControlError::ParameterProposalExpired` — Proposal's TTL has elapsed. pub fn execute_parameter_change( env: Env, caller: Address, diff --git a/contracts/risk_registry/src/lib.rs b/contracts/risk_registry/src/lib.rs index e6e4639..b50bd3a 100644 --- a/contracts/risk_registry/src/lib.rs +++ b/contracts/risk_registry/src/lib.rs @@ -274,6 +274,7 @@ impl RiskRegistryContract { Self::bump_persistent(&env, &DataKey::Verifier(verifier.clone())); Self::bump_persistent(&env, &DataKey::VerifierStake(verifier.clone())); Self::bump_persistent(&env, &DataKey::VerifierReputation(verifier.clone())); + // TODO: Sync with access_control: AccessControlContractClient::new(&env, &access_control).grant_role(&admin, &verifier, Role::Verifier)?; events::verifier_added(&env, &admin, &verifier); Self::append_audit_entry(&env, &admin, AdminActionType::AddVerifier); Ok(()) @@ -395,6 +396,7 @@ impl RiskRegistryContract { env.storage() .persistent() .remove(&DataKey::VerifierReputation(verifier.clone())); + // TODO: Sync with access_control: AccessControlContractClient::new(&env, &access_control).revoke_role(&admin, &verifier)?; events::verifier_removed(&env, &admin, &verifier); Self::append_audit_entry(&env, &admin, AdminActionType::RemoveVerifier); Ok(()) diff --git a/contracts/shared/src/types.rs b/contracts/shared/src/types.rs index 7e9f915..1507694 100644 --- a/contracts/shared/src/types.rs +++ b/contracts/shared/src/types.rs @@ -319,6 +319,7 @@ pub struct ParameterProposal { pub proposer: Address, pub approvals: Vec
, // signers that have voted in favour pub created_at: u64, + pub expires_at: u64, pub executed: bool, pub cancelled: bool, } diff --git a/contracts/shared/src/validation.rs b/contracts/shared/src/validation.rs index 40eeb74..3fd2b2f 100644 --- a/contracts/shared/src/validation.rs +++ b/contracts/shared/src/validation.rs @@ -93,6 +93,25 @@ pub fn require_valid_risk_score(score: u32) -> Result<(), KoraError> { } Ok(()) } +/// Validate that `score` is within [0, max_score] inclusive. +/// Used when the maximum risk score is governed by access_control. +/// +/// # Examples +/// ```ignore +/// use kora_shared::validation::require_valid_risk_score_with_max; +/// assert!(require_valid_risk_score_with_max(50, 100).is_ok()); +/// assert!(require_valid_risk_score_with_max(100, 100).is_ok()); +/// assert!(require_valid_risk_score_with_max(101, 100).is_err()); +/// assert!(require_valid_risk_score_with_max(50, 50).is_ok()); +/// assert!(require_valid_risk_score_with_max(51, 50).is_err()); +/// ``` +pub fn require_valid_risk_score_with_max(score: u32, max_score: u32) -> Result<(), KoraError> { + if score > max_score { + return Err(KoraError::InvalidRiskScore); + } + Ok(()) +} + /// Reject risk scores above a protocol-configured ceiling, which may be /// stricter than (but never looser than) the hard 100 cap enforced by diff --git a/contracts/treasury/src/lib.rs b/contracts/treasury/src/lib.rs index d9aad79..ce1cbfb 100644 --- a/contracts/treasury/src/lib.rs +++ b/contracts/treasury/src/lib.rs @@ -765,6 +765,7 @@ impl TreasuryContract { /// Defaults to 50 bps if the contract has not yet been initialized or if the /// fee has never been explicitly set. /// + /// If parameter governance is active in access_control, the governed value takes precedence. /// **Security:** Read-only view. No authorization required. pub fn get_fee_bps(env: Env) -> u32 { env.storage() diff --git a/docs/MIGRATIONS.md b/docs/MIGRATIONS.md index 2608b20..2724e1a 100644 --- a/docs/MIGRATIONS.md +++ b/docs/MIGRATIONS.md @@ -1,192 +1,37 @@ -# Schema Migration Runbook +# Schema Migrations -This document describes the process for safely evolving `#[contracttype]` struct -schemas in the Kora Protocol. Read this before adding, removing, or reordering -fields on `Invoice`, `Pool`, `Position`, `SmeProfile`, `Listing`, or any other -persisted type in `kora-shared/src/types.rs`. +This document tracks breaking changes to persistent storage layouts and required data migrations. ---- +## ParameterProposal schema change (PR #XXX, Issue #490) -## Why schema changes are binary-incompatible +### Change +Added `expires_at: u64` field to `ParameterProposal` struct in `contracts/shared/src/types.rs`. -Every `#[contracttype]` struct is encoded with Soroban's XDR codec. The codec -uses **positional field encoding** — field N in the struct corresponds to field N -in the wire format, with no field names embedded. +### Migration Notes +- **Already-deployed instances:** Existing ParameterProposal entries in storage lack the `expires_at` field. +- **Safe handling:** The field default (missing = 0) makes old proposals immediately expired, preventing execution. +- **Best practice:** After contract upgrade, re-propose any critical pending parameter changes with fresh TTL. +- **No data loss:** Old entries remain readable via `get_parameter_proposal` but cannot be voted on or executed after expiry. -Consequences: -- Adding a field (even at the end) changes the total field count. Existing - records encoded without that field will panic when deserialized under the new - struct definition. -- Removing a field shifts every field that came after it. -- Reordering fields produces silent data corruption (values decode into the wrong - fields without error). +### Timeline +- ParameterProposal TTL: ~7 days (PROPOSAL_TTL_LEDGERS = 120_960 ledgers at ~5s/ledger) +- Proposals expire after creation time + TTL +- Expired proposals cannot be voted on or executed -There is no built-in versioning or schema evolution. Every change requires an -explicit migration that reads old records with the old struct definition and -rewrites them with the new one. +## Multisig Direct-Call Blocking (PR #XXX, Issue #487) ---- +### Change +Added `DirectCallProhibited` error. Once a multisig is configured via `configure_multisig`, direct admin calls to: +- `pause()` +- `unpause()` +- `grant_role()` +- `revoke_role()` +- `transfer_admin()` -## The migration pattern +...are blocked and must route through `propose_action → approve_action → execute_action`. -### Step 1 — define the legacy struct - -Before changing the live struct in `kora-shared/src/types.rs`, copy its current -definition into the contract that owns the data (e.g. `invoice_nft/src/lib.rs`) -under a versioned name such as `InvoiceV1`. Annotate it with `#[contracttype]` -so it uses the same XDR codec as the original. - -```rust -// contracts/invoice_nft/src/lib.rs -#[contracttype] -#[derive(Clone)] -pub struct InvoiceV1 { - pub id: u64, - pub sme: Address, - // ... all fields as they were BEFORE the change ... - pub repaid_at: Option, - // no `notes` field here -} -``` - -### Step 2 — update the live struct - -Add (or remove, or reorder) the field in `kora-shared/src/types.rs`. Update the -schema-version comment on the struct. - -```rust -// kora-shared/src/types.rs -/// Schema version: 2 -#[contracttype] -pub struct Invoice { - // ... all previous fields ... - pub repaid_at: Option, - /// Added in schema v2. - pub notes: Option, -} -``` - -### Step 3 — update all construction sites - -Every place that constructs the struct must now supply the new field. For new -mints, the value is usually the caller-supplied argument or a sensible default. - -```rust -// contracts/invoice_nft/src/lib.rs — mint_invoice -let invoice = Invoice { - // ... existing fields ... - repaid_at: None, - notes, // new parameter propagated from the function signature -}; -``` - -### Step 4 — implement the migration in `migrate()` - -Gate the migration on the stored `MigrationVersion` so it is idempotent: - -```rust -// Version 1 -> 2: Invoice gained `notes: Option`. -if current_version < 2 { - let next_id: u64 = env.storage().instance().get(&DataKey::NextId).unwrap_or(1); - let mut id = 1u64; - while id < next_id { - let key = DataKey::Invoice(id); - if let Some(old) = env.storage().persistent().get::(&key) { - let upgraded = Invoice { - id: old.id, - sme: old.sme, - // ... copy every field ... - repaid_at: old.repaid_at, - notes: None, // backfill with the default for old records - }; - env.storage().persistent().set(&key, &upgraded); - } - id += 1; - } - env.storage().instance().set(&DataKey::MigrationVersion, &2u32); -} -``` - -### Step 5 — deploy and run the migration - -**Order of operations is critical:** - -1. Prepare and test the new WASM locally. -2. Upgrade the contract on-chain (`upgrade_wasm`). -3. Call `migrate()` **before any other user interaction**. - - Until `migrate()` runs, any read of an old record as the new struct will - panic and abort the transaction. - - Any new write (e.g. a new `mint_invoice`) will produce a v2 record that - cannot be read by the migration's `InvoiceV1` decoder. -4. Verify by calling `get_invoice()` on a few known IDs and confirming the new - field is present with its default value. -5. Once all records have been upgraded, the `InvoiceV{N}` legacy struct can be - removed in the next upgrade cycle. - -### Step 6 — clean up - -After all nodes have called `migrate()` and you are satisfied that no old records -remain, remove the legacy struct in your next PR. - ---- - -## Worked example: adding `notes: Option` to `Invoice` - -This is the concrete change shipped as schema v2. - -| | v1 (original) | v2 (current) | -|---|---|---| -| struct | `Invoice` (no `notes`) | `Invoice` with `notes: Option` | -| legacy | — | `InvoiceV1` (copy of v1, kept for migration) | -| `MigrationVersion` after `migrate()` | 1 | 2 | -| backfill value | n/a | `notes = None` | - -**Files changed:** -- `contracts/shared/src/types.rs` — added `notes` field, updated doc comment -- `contracts/invoice_nft/src/lib.rs` — added `InvoiceV1`, updated `migrate()`, - updated `mint_invoice` signature (new `notes: Option` parameter) - -**Tests:** -- `test_migrate_v1_to_v2_backfills_notes_field` — writes a raw `InvoiceV1` - record to persistent storage, resets `MigrationVersion` to 1, calls - `migrate()`, then asserts `get_invoice()` returns a valid v2 record with - `notes = None`. - ---- - -## Checklist for every schema migration - -- [ ] Legacy struct `Foo_V{N}` added with `#[contracttype]` BEFORE changing `Foo` -- [ ] New field added to `Foo` in `kora-shared/src/types.rs` -- [ ] All construction sites updated -- [ ] `migrate()` gate added: `if current_version < N+1 { ... }` -- [ ] `MigrationVersion` bumped to `N+1` at the end of the gate -- [ ] Test added that writes a raw `FooV{N}` record and asserts readable post-migration -- [ ] Upgrade → `migrate()` ordering documented in the deployment PR -- [ ] Legacy struct removal scheduled for the following upgrade cycle - ---- - -## Version history - -| Version | Change | Migration gate | File | -|---------|--------|---------------|------| -| 1 | Baseline — all original fields | v0 → v1 (no-op, sets version) | `invoice_nft::migrate` | -| 2 | `Invoice.notes: Option` added | v1 → v2, backfills `notes = None` | `invoice_nft::migrate` | - ---- - -## Contracts with mutable on-chain state - -All structs below are `#[contracttype]` and stored live on-chain. Any field -change requires a migration following the pattern above. - -| Struct | Contract | Storage key | -|--------|----------|-------------| -| `Invoice` | `invoice_nft` | `DataKey::Invoice(u64)` (persistent) | -| `Pool` | `financing_pool` | `DataKey::Pool(u64)` (persistent) | -| `Position` | `financing_pool` | `DataKey::Positions(u64)` (persistent, inside `Map`) | -| `SmeProfile` | `risk_registry` | `DataKey::SmeProfile(Address)` (persistent) | -| `Listing` | `marketplace` | `DataKey::Listing(u64)` (persistent) | -| `Proposal` | `access_control` | `DataKey::Proposal(u64)` (persistent) | -| `MultisigConfig` | `access_control` | `DataKey::MultisigConfig` (instance) | +### Migration Notes +- **No storage change:** Existing state is unaffected. +- **Behavior change:** Direct calls fail with `DirectCallProhibited` after multisig is active. +- **Before multisig:** All direct admin calls work normally. +- **After multisig:** Must use propose/approve/execute flow (intentional security hardening).