From 309c855af35011c7f2392c014a609f74730ef915 Mon Sep 17 00:00:00 2001 From: "EjiroMowarin.dev" Date: Sat, 24 Oct 2026 08:32:50 +0100 Subject: [PATCH 1/3] fix: Add two-step admin transfer and timelocked role rotation to (#4) --- contracts/AUDIT_NOTES.md | 38 +--------- contracts/common/src/access.rs | 134 ++++++++++++++++++++++++++++++++- 2 files changed, 134 insertions(+), 38 deletions(-) diff --git a/contracts/AUDIT_NOTES.md b/contracts/AUDIT_NOTES.md index c94200f..869d870 100644 --- a/contracts/AUDIT_NOTES.md +++ b/contracts/AUDIT_NOTES.md @@ -159,40 +159,4 @@ uses comes from the contract's own state or from the PriceGuard, never from a ke 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. -- Yield on the pledged RWA during the term is not passed through. - -## Known limits - -- Real RWAs cannot be held by these contracts unless the issuer allows it (`AUTH_REQUIRED` - issuers must authorise the vault's trustline). Etherfuse's issuer sets `auth_revocable` and - `clawback_enabled`, so it can freeze or claw back tokens held in the vault; price that in. -- Events use `env.events().publish`, which soroban-sdk deprecates in favour of `#[contractevent]`; - 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`. + deadline diff --git a/contracts/common/src/access.rs b/contracts/common/src/access.rs index c130019..6f4f75a 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 must also authorize, so a transfer +/// needs both signatures. pub fn accept_admin(env: &Env) { let pending: PendingAdmin = env .storage() @@ -125,6 +126,7 @@ pub fn accept_admin(env: &Env) { pending.new_admin.require_auth(); let mut roles = read_roles(env); + roles.admin.require_auth(); roles.admin = pending.new_admin; write_roles(env, &roles); env.storage().instance().remove(&RolesKey::PendingAdmin); @@ -293,4 +295,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 accept_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. + 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); + } } From fd89108d4818cfa87536ab8744a7c8bd31950977 Mon Sep 17 00:00:00 2001 From: DevTobis <232918735+DevTobis@users.noreply.github.com> Date: Sun, 25 Oct 2026 01:06:12 +0100 Subject: [PATCH 2/3] fix: restore AUDIT_NOTES.md and import IntoVal in access tests --- contracts/AUDIT_NOTES.md | 75 +++++++++------------------------- contracts/common/src/access.rs | 2 +- 2 files changed, 21 insertions(+), 56 deletions(-) diff --git a/contracts/AUDIT_NOTES.md b/contracts/AUDIT_NOTES.md index 869d870..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,25 +99,29 @@ 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 + 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.** `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 + +- Real RWAs cannot be held by these contracts unless the issuer allows it (`AUTH_REQUIRED` + issuers must authorise the vault's trustline). Etherfuse's issuer sets `auth_revocable` and + `clawback_enabled`, so it can freeze or claw back tokens held in the vault; price that in. +- Events use `env.events().publish`, which soroban-sdk deprecates in favour of `#[contractevent]`; + 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. diff --git a/contracts/common/src/access.rs b/contracts/common/src/access.rs index 6f4f75a..8b665f2 100644 --- a/contracts/common/src/access.rs +++ b/contracts/common/src/access.rs @@ -216,7 +216,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] From 75da9aac0d7f3688f9cc6944cc2315de8fc9171c Mon Sep 17 00:00:00 2001 From: DevTobis <232918735+DevTobis@users.noreply.github.com> Date: Sun, 25 Oct 2026 01:24:26 +0100 Subject: [PATCH 3/3] fix: drop redundant admin auth in accept_admin, rename propose auth test --- contracts/common/src/access.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/contracts/common/src/access.rs b/contracts/common/src/access.rs index 8b665f2..2130b1f 100644 --- a/contracts/common/src/access.rs +++ b/contracts/common/src/access.rs @@ -115,8 +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. The current admin must also authorize, so a transfer -/// needs both signatures. +/// 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() @@ -126,7 +126,6 @@ pub fn accept_admin(env: &Env) { pending.new_admin.require_auth(); let mut roles = read_roles(env); - roles.admin.require_auth(); roles.admin = pending.new_admin; write_roles(env, &roles); env.storage().instance().remove(&RolesKey::PendingAdmin); @@ -353,11 +352,11 @@ mod test { #[test] #[should_panic] - fn accept_admin_without_current_admin_auth_fails() { + 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. + // 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 {