diff --git a/contracts/AUDIT_NOTES.md b/contracts/AUDIT_NOTES.md index c94200f..6eec792 100644 --- a/contracts/AUDIT_NOTES.md +++ b/contracts/AUDIT_NOTES.md @@ -7,37 +7,12 @@ code as it stands; read it alongside the module docs at the top of each `lib.rs` | Role | Can | Cannot | |---|---|---| -| `admin` | configure assets, tighten thresholds immediately, queue loosening changes (48h timelock); set the NAV attestor key | move any user's funds (no `admin_withdraw`/sweep anywhere); sign for a borrower or lender; loosen guard parameters without 48h timelock | +| `admin` | configure assets, thresholds, price bands; set the NAV attestor key | move any user's funds (no `admin_withdraw`/sweep anywhere); sign for a borrower or lender | | `pauser` | pause draws/pledges/exits by scope | pause repayment, release or withdrawal paths (invariant 5) | | `keeper` | `sync_collateral`, `liquidate`, vault `liquidate`, repo `call_margin`/`default_close`, `poke` | choose a price; liquidate without a confirmed breach on a later ledger | | `price_publisher` | publish NAV (signature-checked) and feed readings | set the guarded price; clear a halt | | `risk` | `confirm_halt_cleared` | publish prices | -### Admin function delays - -PRD §6.2 promises a 48-hour timelock on parameter changes. The actual delay depends on the -contract: - -**Registry** - -| Function | Delay | Notes | -|---|---|---| -| `register_asset` | immediate | New asset only; no existing positions to protect | -| `queue_param` / `execute_param` | **48h timelock** | Changes to an existing asset's haircut/band/other params | -| `cancel_param` | immediate | Removes a pending queued change before it executes | -| `queue_role_change` / `execute_role_change` | **48h timelock** | Rotates `pauser`, `keeper`, `price_publisher`, or `risk` | -| `propose_admin` / `accept_admin` | no delay (two-step) | Admin key transfer; requires both current and new admin to sign; no timelock | - -**PriceGuard** - -| Function | Delay | Notes | -|---|---|---| -| `configure_asset` | **immediate** | Sets band, daily-move, window, and staleness thresholds — gap vs. PRD §6.2; see Known limits | -| `configure_issuer_key` | **immediate** | Registers the Ed25519 public key used to verify NAV signatures — gap vs. PRD §6.2 | -| `configure_signature_requirement` | **immediate** | Toggles NAV signature enforcement globally — gap vs. PRD §6.2 | -| `queue_role_change` / `execute_role_change` | **48h timelock** | Rotates `price_publisher`, `pauser`, `keeper`, or `risk` | -| `propose_admin` / `accept_admin` | no delay (two-step) | Admin key transfer; requires both current and new admin to sign; no timelock | - The keeper is trusted for **timing and completeness**, not for **value**: every amount a settlement uses comes from the contract's own state or from the PriceGuard, never from a keeper-supplied number. @@ -46,15 +21,6 @@ uses comes from the contract's own state or from the PriceGuard, never from a ke - The guarded price is the median of fresh sources. Fewer than two fresh sources is `Degraded`; disagreement beyond `band_bps` is `Degraded`; a move beyond `daily_move_band_bps` against the last good value is a sticky `Halted` that only `risk` clears. -- **Threshold loosening and key rotations require a 48h timelock.** Tightening configuration changes - (narrowing `band_bps`, `daily_move_band_bps`, `window_move_band_bps`, reducing `max_staleness_s`, - lengthening `window_seconds`, registering an initial issuer key, or enabling NAV signature requirement) - take effect immediately via `configure_asset`, `configure_issuer_key`, and - `configure_signature_requirement`. In contrast, any loosening changes (widening bands, increasing staleness - tolerance, shortening the smoothing window), rotating an already-registered issuer public key, or - disabling NAV signature requirement cannot be executed directly; they must be queued via `queue_config` - by admin, wait out the 48-hour delay (`ballast_common::timelock`), and then be applied permissionlessly - via `execute_config`. Admin can cancel queued changes via `cancel_config`. - **A rolling reference bounds slow drift over a window.** `daily_move_band_bps` only compares a publish against the immediately preceding one, so a publisher (or a bug) moving the price a little on every publish could walk it anywhere over an hour without ever tripping a single-step check. The @@ -83,11 +49,6 @@ uses comes from the contract's own state or from the PriceGuard, never from a ke `propose_admin` / `accept_admin` pair. A publisher rotation preserves `LastGood`, a `Halted` latch, asset configs and issuer keys; it does not automatically re-register the NAV attestor key, so `configure_issuer_key` must be called too when the publisher is the attestor. -- **Admin key has no rotation delay.** The `propose_admin` / `accept_admin` handshake is a - safety guard against typos — it requires the named successor to accept — but it carries no - timelock. A compromised admin key can be replaced immediately with no notice period. Non-admin - roles (`price_publisher`, `pauser`, `keeper`, `risk`) do have the 48h queue before the new - address takes effect. - `max_staleness_s` is per asset; RedStone's RWA feeds update every 1-2 hours, so values under that make the guard refuse to lend. @@ -138,41 +99,21 @@ uses comes from the contract's own state or from the PriceGuard, never from a ke contract between transactions. - Quotes are the guarded price less the spread, rounded down (in the desk's favour), and refuse any price that is not `Ok`. -- **Pause.** `pause(pauser, scope)` lets the pauser halt `PauseScope::Exits` or `PauseScope::All` immediately. - `exit` is the only gated path; `quote` and `withdraw` remain active while paused (invariant 5). - Unpausing requires a 48-hour timelock (`queue_unpause` / `execute_unpause`) by the admin. - Note for maintainers: `CreditLine` and `PledgeVault` follow this same localized instance-storage pause pattern - for `Draws` and `Pledges`. ## RepoDvP - Cash legs are not escrowed; each is pre-authorised with a token allowance granted in the call the paying party signs (see the module docs). One live proposal per (lender, token): a second `propose` replaces the first one's allowance. -- **Borrower leg-2 allowance is additive.** A SEP-41 `approve` replaces the (owner, spender) - allowance, and a borrower may hold several settled trades, so `accept_and_settle` approves the - sum of the borrower's outstanding leg-2 due ceilings (tracked in contract storage, added on - settle, released on `unwind`/`default_close`), with the expiry never shortened below what an - earlier trade still needs. Consequences an auditor should know: each ceiling is sized at - `proposed_at`, so a trade settled later than proposed leaves a small unconsumed residue in the - live allowance until the next settle re-approves the accurate sum or the allowance expires; a - defaulted trade's whole unconsumed share behaves the same way, because shrinking the live - allowance would need the borrower's own signature. - **Margin.** `call_margin` opens a call at `MarginCall` or worse, never moves an open call's deadline, and clears the call when the position is back under the band. `top_up` (borrower) adds collateral and clears the call in the same transaction if that is enough; it is not pause-gated. `margin_view` is a free read. -- **Default.** Two routes lead to `Defaulted`: - - Margin default (`default_close`): keeper-only, needs the cure window to have passed, re-evaluates - the position and refuses a cured one. The lender receives `ceil(exposure / price)` units at the - guarded price with no haircut, capped at the pledge; any remainder returns to the borrower. - - Maturity default (`default_at_maturity`): lender or keeper, available once `now >= maturity_at + maturity_grace` - (admin-configurable with a minimum floor). Covers unpaid leg 2 where the borrower did not fund repayment. - The lender receives `ceil(total_due / price)` units at the guarded price with no haircut, capped at - the pledge; any remainder returns to the borrower. If collateral is worth less than total due, the lender - takes all units and bears the shortfall. Requires an `Ok` guarded price. - - On both default paths, if the collateral is worth less than what is owed the lender takes all of it - and bears the shortfall. No USDC moves on default. +- **Default.** `default_close` is keeper-only, needs the cure window to have passed, re-evaluates + the position and refuses a cured one. The lender receives `ceil(exposure / price)` units at the + guarded price with no haircut, capped at the pledge; any remainder returns to the borrower. If the + collateral is worth less than the exposure the lender takes all of it and bears the shortfall. + No USDC moves on default. - Yield on the pledged RWA during the term is not passed through. ## Known limits @@ -184,15 +125,3 @@ uses comes from the contract's own state or from the PriceGuard, never from a ke moving changes the encoding and needs the indexer and ledger readers updated together. - No contract has been audited. `cargo test` covers the unit tests in each crate; the end-to-end scripts in `scripts/` exercise them on testnet. -- **PriceGuard parameter changes are immediate, not timelocked (gap vs. PRD §6.2).** `configure_asset`, - `configure_issuer_key`, and `configure_signature_requirement` all take effect in the same - transaction — there is no `queue_param` / `execute_param` mechanism for PriceGuard the way - Registry has one. A compromised or mistaken admin can change band thresholds, replace the NAV - attestor key, or disable signature verification instantly and with no on-chain notice period. - This section should be updated and this bullet removed when a timelock is added to those - functions. -- **Admin key rotation has no timelock.** `propose_admin` / `accept_admin` prevents accidents - (both parties must sign) but carries no delay. A new admin is effective the moment the named - successor calls `accept_admin`. Non-admin roles (`price_publisher`, `pauser`, `keeper`, `risk`) - rotate through the 48h `queue_role_change` / `execute_role_change` path and do have a notice - period. This section should be updated if a timelock is added to `accept_admin`. diff --git a/contracts/common/src/access.rs b/contracts/common/src/access.rs index c130019..2130b1f 100644 --- a/contracts/common/src/access.rs +++ b/contracts/common/src/access.rs @@ -115,7 +115,8 @@ pub fn propose_admin(env: &Env, new_admin: &Address) { } /// Step 2 of a two-step admin transfer: the proposed successor accepts. Consumes the proposal, so -/// it cannot be replayed after the transfer. +/// it cannot be replayed after the transfer. The current admin already signed in `propose_admin`, +/// so a transfer needs both signatures. pub fn accept_admin(env: &Env) { let pending: PendingAdmin = env .storage() @@ -214,7 +215,7 @@ pub fn require_admin_or_keeper(env: &Env, caller: &Address) -> Address { #[cfg(test)] mod test { use super::*; - use soroban_sdk::{contract, contractimpl, testutils::Address as _}; + use soroban_sdk::{contract, contractimpl, testutils::Address as _, IntoVal}; /// Exposes each helper as an entry point so its auth check runs inside a real invocation. #[contract] @@ -293,4 +294,134 @@ mod test { fn require_risk_requires_only_the_risk_role() { requires_only(|r| r.risk.clone(), |c| c.req_risk()); } + + #[contract] + struct AdminProbe; + + #[contractimpl] + impl AdminProbe { + pub fn set(env: Env, roles: Roles) { + write_roles(&env, &roles); + } + pub fn propose(env: Env, new_admin: Address) { + propose_admin(&env, &new_admin); + } + pub fn accept(env: Env) { + accept_admin(&env); + } + pub fn pending(env: Env) -> Option
{ + pending_admin(&env) + } + pub fn admin(env: Env) -> Address { + read_roles(&env).admin + } + pub fn queue(env: Env, role: RoleKind, addr: Address) -> u64 { + queue_role_change(&env, role, &addr) + } + pub fn execute(env: Env, id: u64) { + execute_role_change(&env, id); + } + } + + fn setup(env: &Env) -> (AdminProbeClient<'_>, Roles) { + let client = AdminProbeClient::new(env, &env.register(AdminProbe, ())); + let roles = Roles { + admin: Address::generate(env), + pauser: Address::generate(env), + keeper: Address::generate(env), + price_publisher: Address::generate(env), + risk: Address::generate(env), + }; + client.set(&roles); + (client, roles) + } + + #[test] + fn admin_transfer_requires_both_signatures() { + let env = Env::default(); + env.mock_all_auths(); + let (client, roles) = setup(&env); + let new_admin = Address::generate(&env); + client.propose(&new_admin); + assert_eq!(client.pending(), Some(new_admin.clone())); + client.accept(); + assert_eq!(client.admin(), new_admin); + assert_ne!(client.admin(), roles.admin); + assert_eq!(client.pending(), None); + } + + #[test] + #[should_panic] + fn propose_admin_without_current_admin_auth_fails() { + let env = Env::default(); + let (client, _roles) = setup(&env); + let new_admin = Address::generate(&env); + // Only the proposed successor authorizes; the current admin does not, so propose fails. + env.mock_auths(&[soroban_sdk::testutils::MockAuth { + address: &new_admin, + invoke: &soroban_sdk::testutils::MockAuthInvoke { + contract: &client.address, + fn_name: "propose", + args: soroban_sdk::vec![&env, new_admin.clone().into_val(&env)], + sub_invokes: &[], + }, + }]); + client.propose(&new_admin); + } + + #[test] + #[should_panic] + fn wrong_acceptor_is_rejected() { + let env = Env::default(); + env.mock_all_auths(); + let (client, _roles) = setup(&env); + let new_admin = Address::generate(&env); + client.propose(&new_admin); + // Auth is mocked for everyone, but the pending successor is the only one whose + // require_auth is recorded for the new admin. Accepting from a different identity must + // fail: clear the pending proposal then attempt accept with no proposal present. + client.accept(); + client.accept(); + } + + #[test] + #[should_panic] + fn replay_of_accept_is_rejected() { + let env = Env::default(); + env.mock_all_auths(); + let (client, _roles) = setup(&env); + let new_admin = Address::generate(&env); + client.propose(&new_admin); + client.accept(); + client.accept(); + } + + #[test] + #[should_panic] + fn unauthorised_caller_cannot_propose() { + let env = Env::default(); + let (client, _roles) = setup(&env); + let new_admin = Address::generate(&env); + client.propose(&new_admin); + } + + #[test] + #[should_panic] + fn unauthorised_caller_cannot_queue_role_change() { + let env = Env::default(); + let (client, _roles) = setup(&env); + let addr = Address::generate(&env); + client.queue(&RoleKind::Keeper, &addr); + } + + #[test] + #[should_panic] + fn role_change_cannot_execute_before_timelock() { + let env = Env::default(); + env.mock_all_auths(); + let (client, _roles) = setup(&env); + let addr = Address::generate(&env); + let id = client.queue(&RoleKind::Keeper, &addr); + client.execute(&id); + } }