feat: check the whole wiring before setup closes

README.md

@@ -139,12 +139,12 @@ Output of a real run from a fresh clone. Contract IDs differ on every run.
 deploying to local (http://localhost:8000/rpc)
 account keys in .env.keys.local (13 new)
 funded 13 accounts; 0 already existed
-asset contract CDKWXRAHPQVFOAEBOOWWR62ZXAE2IV4V47PATFN7IX4XDW5QIUUL7D2B
-compliance CDGKQCYSMTDRBO7SYV3V72H6AMCQ3UNQFKUU4AD5GSU2I32XDYFS6F3U
-identity_verifier CCZE76VWEWJ47NSFMIXCKU3L55CZZTI5JJY4CSYAWPO3WXPDFXUCA5G6
-share_token CD6IA3BOPFX7VNADJ3GFK7IZGPVPNZA6HBCF425CNPSQ2GN7QZ3UUCP2
-nav_oracle CAHD5ZKMIDD67ZEJUB5ACR62FMAYMQJ2KCRV2MBDNXWT7WEZ2VV3MRDD
-async_vault CDQNMAZP7D5IA7AXZGK3XWYPFD2LDR6V5HQD2OLXZJYNQN4BMNN6SXWJ
+asset contract CCW7OKVOXBBBX3YK6O7F6TZPNIFG32OG4UY7R2K6NSWZUI4R75QCEPVT
+compliance CDPCD2BQQKVQBQFAFI4YBLN3TRX34J5NZUQQMUBGHPYNY64LCEM63DTL
+identity_verifier CC4SRUIMKOBZSEO2GKGVQMDWPWCBJBBIMYYE7BBCUEZE6VMD3SFQDAMW
+share_token CDC7SRTDZIMJSLC5AONUD3FIFYZGMFOHRAS7LLYV2ATJRT7RMTMQO2DG
+nav_oracle CCUUBP3RGOIZZL4DUGRL4FBOSQJZKBPNQ7VUWIN6UOOYBJYYKGNT5E52
+async_vault CB2KVM2WHLXDZRHGCYPTGE7PZM6OORXCNVBUMC3L7GY5KX4BA6NMADXE
 clients generated
 vault holds share_token manager role
 share_token bound to the vault, setup finished
@@ -153,7 +153,7 @@ share_token bound to compliance
 compliance bound to the vault, setup finished
 nav_oracle bound to the vault, setup finished
 vault setup finished
-health {"allowlisted":true,"compliance_bound":true,"feed_valid":false,"freshness_covers_notice":true,"oracle_bound":true,"setup_open":false,"sole_token_manager":true,"token_bound":true,"vault_unfrozen":true}
+health {"addresses_distinct":true,"allowlisted":true,"compliance_bound":true,"custodian_set":true,"decimals_match":true,"feed_answers":true,"feed_valid":false,"freshness_covers_notice":true,"oracle_bound":true,"setup_open":false,"sole_token_manager":true,"token_bound":true,"vault_unfrozen":true}
 wrote deployed.local.json
 ```
 

contracts/async-vault/src/error.rs

@@ -126,7 +126,8 @@ pub enum VaultError {
     /// Setup has finished, and it never reopens.
     SetupClosed = 6066,
     /// A wiring check `health()` reports is false (token, oracle and compliance
-    /// bound, sole token manager, allowlisted, not frozen) or the token has
+    /// bound, sole token manager, allowlisted, not frozen, matching decimals,
+    /// distinct addresses, a feed that answers, a custodian) or the token has
     /// shares outstanding, so setup cannot finish.
     SetupIncomplete = 6067,
     /// The holder fails the share token's identity check, so the vault takes no
@@ -141,8 +142,10 @@ pub enum VaultError {
     /// returned or moved, until compliance lifts the freeze. A partial freeze
     /// refuses only what would touch frozen shares.
     Frozen = 6071,
-    /// The proposed feed does not name this vault as its own, or its setup is
-    /// still open. Its admin could then change its limits with no delay.
+    /// The proposed feed does not name this vault as its own, its setup is
+    /// still open, or it does not answer `state()` and `freshness_duration()`.
+    /// Its admin could then change its limits with no delay, or no epoch could
+    /// be priced from it.
     FeedNotBound = 6072,
     /// The holder may not receive shares (delisted, frozen or refused by the
     /// compliance rules), so the escrowed shares stay with the vault and the
@@ -152,4 +155,7 @@ pub enum VaultError {
     /// once it is accepted, cancelled or expired, so no nominee from setup can
     /// accept after changes start to wait.
     AdminTransferPending = 6074,
+    /// The custodian would be the vault itself. Capital deployed to it would
+    /// never leave, and the books would count it as both held and deployed.
+    InvalidCustodian = 6075,
 }

contracts/async-vault/src/governance.rs

@@ -1,5 +1,6 @@
 use soroban_sdk::{
-    contracttype, panic_with_error, symbol_short, Address, BytesN, Env, Symbol, Vec,
+    contracttype, panic_with_error, symbol_short, token::TokenClient, Address, BytesN, Env, Symbol,
+    Vec,
 };
 use stellar_access::{
     access_control::{self, AccessControlClient, AccessControlStorageKey},
@@ -71,6 +72,10 @@ pub(crate) struct Wiring {
     pub allowlisted: bool,
     pub vault_unfrozen: bool,
     pub compliance_bound: bool,
+    pub decimals_match: bool,
+    pub addresses_distinct: bool,
+    pub custodian_set: bool,
+    pub feed_answers: bool,
 }
 
 impl Wiring {
@@ -82,6 +87,10 @@ impl Wiring {
             && self.allowlisted
             && self.vault_unfrozen
             && self.compliance_bound
+            && self.decimals_match
+            && self.addresses_distinct
+            && self.custodian_set
+            && self.feed_answers
     }
 }
 
@@ -121,6 +130,13 @@ pub(crate) fn bound_here(e: &Env, contract: &Address) -> bool {
         && matches!(binding.try_setup_open(), Ok(Ok(false)))
 }
 
+/// True when `feed` answers the views the vault prices from. A new feed has no
+/// report yet, so this asks only for answers, not for a valid price.
+pub(crate) fn answers_as_feed(e: &Env, feed: &Address) -> bool {
+    let feed = OracleFeedClient::new(e, feed);
+    matches!(feed.try_state(), Ok(Ok(_))) && matches!(feed.try_freshness_duration(), Ok(Ok(_)))
+}
+
 fn compliance_bound(e: &Env, compliance: &Address, token: &Address) -> bool {
     let linked = matches!(
         TokenBinderClient::new(e, compliance).try_linked_tokens(),
@@ -135,12 +151,21 @@ fn compliance_bound(e: &Env, compliance: &Address, token: &Address) -> bool {
     linked && on_this_clock
 }
 
+fn all_different(addresses: &[&Address]) -> bool {
+    addresses
+        .iter()
+        .enumerate()
+        .all(|(i, a)| addresses[i + 1..].iter().all(|b| a != b))
+}
+
 pub(crate) fn wiring(e: &Env) -> Wiring {
     let me = e.current_contract_address();
+    let asset = state::get_addr(e, &DataKey::Asset);
     let token = state::get_addr(e, &DataKey::ShareToken);
     let oracle = state::get_addr(e, &DataKey::Oracle);
     let roles = AccessControlClient::new(e, &token);
     let rwa = RWATokenClient::new(e, &token);
+    let shares = FungibleTokenClient::new(e, &token);
     Wiring {
         token_bound: bound_here(e, &token),
         oracle_bound: bound_here(e, &oracle),
@@ -151,10 +176,7 @@ pub(crate) fn wiring(e: &Env) -> Wiring {
             roles.try_get_role_member(&TOKEN_MANAGER_ROLE, &0),
             Ok(Ok(holder)) if holder == me
         ),
-        no_shares_outstanding: matches!(
-            FungibleTokenClient::new(e, &token).try_total_supply(),
-            Ok(Ok(0))
-        ),
+        no_shares_outstanding: matches!(shares.try_total_supply(), Ok(Ok(0))),
         allowlisted: match rwa.try_identity_verifier() {
             Ok(Ok(verifier)) => matches!(
                 IdentityVerifierClient::new(e, &verifier).try_verify_identity(&me),
@@ -167,6 +189,14 @@ pub(crate) fn wiring(e: &Env) -> Wiring {
             Ok(Ok(compliance)) => compliance_bound(e, &compliance, &token),
             _ => false,
         },
+        decimals_match: matches!(
+            (TokenClient::new(e, &asset).try_decimals(), shares.try_decimals()),
+            (Ok(Ok(of_asset)), Ok(Ok(of_shares))) if of_asset == of_shares
+        ),
+        addresses_distinct: all_different(&[&asset, &token, &oracle, &me]),
+        custodian_set: state::get_addr_opt(e, &DataKey::Custodian)
+            .is_some_and(|custodian| custodian != me),
+        feed_answers: answers_as_feed(e, &oracle),
     }
 }
 
@@ -223,11 +253,7 @@ pub(crate) fn apply(e: &Env, id: u32) {
             access_control::revoke_role_no_auth(e, account, role, &admin)
         }
         VaultChange::Admin(next) => offer_admin(e, &admin, next),
-        VaultChange::Oracle(feed) => {
-            // A feed that cannot answer would stall every epoch after the swap.
-            OracleFeedClient::new(e, feed).state();
-            state::set_addr(e, &DataKey::Oracle, feed);
-        }
+        VaultChange::Oracle(feed) => state::set_addr(e, &DataKey::Oracle, feed),
     }
     ChangeApplied { id, change }.publish(e);
 }

contracts/async-vault/src/guards.rs

@@ -148,8 +148,15 @@ pub(crate) fn upgrade_delay_in_bounds(e: &Env, secs: u64) {
     }
 }
 
+pub(crate) fn custodian_not_the_vault(e: &Env, custodian: &Address) {
+    if *custodian == e.current_contract_address() {
+        panic_with_error!(e, VaultError::InvalidCustodian);
+    }
+}
+
+/// A feed that cannot answer would stall every epoch after the swap.
 pub(crate) fn feed_bound_here(e: &Env, feed: &Address) {
-    if !governance::bound_here(e, feed) {
+    if !governance::bound_here(e, feed) || !governance::answers_as_feed(e, feed) {
         panic_with_error!(e, VaultError::FeedNotBound);
     }
 }
@@ -162,8 +169,8 @@ pub(crate) fn change_in_bounds(e: &Env, change: &VaultChange) {
         VaultChange::Notice(secs) => notice_in_bounds(e, *secs),
         VaultChange::WindDownDelay(secs) => wind_down_delay_in_bounds(e, *secs),
         VaultChange::Oracle(feed) => feed_bound_here(e, feed),
+        VaultChange::Custodian(custodian) => custodian_not_the_vault(e, custodian),
         VaultChange::Upgrade(_)
-        | VaultChange::Custodian(_)
         | VaultChange::GrantRole(..)
         | VaultChange::RevokeRole(..)
         | VaultChange::Admin(_) => {}

contracts/async-vault/src/health.rs

@@ -20,6 +20,14 @@ pub struct Health {
     pub allowlisted: bool,
     pub vault_unfrozen: bool,
     pub compliance_bound: bool,
+    pub decimals_match: bool,
+    /// The asset, the share token, the oracle and the vault are four
+    /// different addresses.
+    pub addresses_distinct: bool,
+    /// A custodian is set, and it is not the vault.
+    pub custodian_set: bool,
+    /// The oracle answers `state` and `freshness_duration`, report or not.
+    pub feed_answers: bool,
     pub feed_valid: bool,
     /// A report outlives the notice, so an epoch's cancellation window does
     /// not reopen on its own while the notice runs.
@@ -38,6 +46,10 @@ pub(crate) fn health(e: &Env) -> Health {
         allowlisted: wiring.allowlisted,
         vault_unfrozen: wiring.vault_unfrozen,
         compliance_bound: wiring.compliance_bound,
+        decimals_match: wiring.decimals_match,
+        addresses_distinct: wiring.addresses_distinct,
+        custodian_set: wiring.custodian_set,
+        feed_answers: wiring.feed_answers,
         feed_valid: matches!(feed.try_state(), Ok(Ok(OracleState::Valid))),
         freshness_covers_notice: matches!(
             feed.try_freshness_duration(),

contracts/async-vault/src/lib.rs

@@ -190,9 +190,10 @@ impl AsyncVault {
     }
 
     /// Instant during setup; afterwards the custodian changes through the
-    /// timelock.
+    /// timelock. Never the vault itself (`InvalidCustodian`).
     #[only_admin]
     pub fn set_custodian(e: &Env, custodian: Address) {
+        guards::custodian_not_the_vault(e, &custodian);
         guards::direct_change_allowed(e);
         treasury::set_custodian(e, &custodian);
     }
@@ -270,7 +271,8 @@ impl AsyncVault {
 
     /// Ends the window in which admin changes are instant. Needs every wiring
     /// check `health()` reports (token, oracle and compliance bound, sole token
-    /// manager, allowlisted, not frozen), no shares outstanding and no admin
+    /// manager, allowlisted, not frozen, matching decimals, distinct addresses,
+    /// a feed that answers, a custodian), no shares outstanding and no admin
     /// hand-over pending. Deposits open from here on, and it never reopens.
     #[only_admin]
     pub fn finish_setup(e: &Env) {

contracts/async-vault/src/test/governance.rs

@@ -48,6 +48,26 @@ fn applying_a_custodian_change_publishes_both_events() {
     );
 }
 
+#[test]
+fn a_custodian_change_to_the_vault_is_refused_when_proposed_and_when_applied() {
+    let f = setup();
+    let change = VaultChange::Custodian(f.vault.address.clone());
+
+    refused(
+        f.vault.try_propose_change(&change),
+        VaultError::InvalidCustodian,
+    );
+
+    // Skips the proposal's check, as a change queued by an earlier version would.
+    let id = f.e.as_contract(&f.vault.address, || {
+        strata_timelock::queue::propose::<AsyncVault, _>(&f.e, &change).unwrap()
+    });
+    f.advance(MIN_UPGRADE_DELAY);
+
+    refused(f.vault.try_apply_change(&id), VaultError::InvalidCustodian);
+    assert_eq!(f.vault.custodian(), Some(f.custodian.clone()));
+}
+
 #[test]
 fn raising_the_notice_is_instant_and_lowering_waits() {
     let f = setup();

contracts/async-vault/src/test/guards.rs

@@ -272,7 +272,16 @@ fn upgrade_delay_in_bounds_refuses_a_delay_outside_the_bounds_or_below_the_notic
 }
 
 #[test]
-fn feed_bound_here_refuses_a_feed_not_bound_to_this_vault() {
+fn custodian_not_the_vault_refuses_the_vault_itself() {
+    let f = setup();
+    holds(&f, || guards::custodian_not_the_vault(&f.e, &f.custodian));
+    refuses(&f, VaultError::InvalidCustodian, || {
+        guards::custodian_not_the_vault(&f.e, &f.vault.address)
+    });
+}
+
+#[test]
+fn feed_bound_here_refuses_an_unbound_feed_or_a_contract_that_is_not_a_feed() {
     let f = setup();
     holds(&f, || guards::feed_bound_here(&f.e, &f.oracle.address));
 
@@ -285,9 +294,12 @@ fn feed_bound_here_refuses_a_feed_not_bound_to_this_vault() {
             open_feed(),
         ),
     );
-    refuses(&f, VaultError::FeedNotBound, || {
-        guards::feed_bound_here(&f.e, &unbound)
-    });
+    // The compliance contract is bound to this vault but answers no feed view.
+    for not_a_feed_here in [unbound, f.share.compliance()] {
+        refuses(&f, VaultError::FeedNotBound, || {
+            guards::feed_bound_here(&f.e, &not_a_feed_here)
+        });
+    }
 }
 
 #[test]
@@ -325,6 +337,14 @@ fn change_in_bounds_checks_the_bounds_of_each_change() {
             VaultChange::Oracle(stranger.clone()),
             VaultError::FeedNotBound,
         ),
+        (
+            VaultChange::Oracle(f.share.compliance()),
+            VaultError::FeedNotBound,
+        ),
+        (
+            VaultChange::Custodian(f.vault.address.clone()),
+            VaultError::InvalidCustodian,
+        ),
     ] {
         refuses(&f, error, || guards::change_in_bounds(&f.e, &change));
     }

contracts/async-vault/src/test/health.rs

@@ -10,6 +10,10 @@ fn all_healthy() -> Health {
         allowlisted: true,
         vault_unfrozen: true,
         compliance_bound: true,
+        decimals_match: true,
+        addresses_distinct: true,
+        custodian_set: true,
+        feed_answers: true,
         feed_valid: true,
         freshness_covers_notice: true,
     }
@@ -103,6 +107,43 @@ fn each_missing_wiring_step_shows_in_its_own_flag() {
                 ..all_healthy()
             },
         ),
+        (
+            Miswire::AssetDecimalsDiffer,
+            Health {
+                decimals_match: false,
+                ..all_healthy()
+            },
+        ),
+        (
+            Miswire::AssetIsShareToken,
+            Health {
+                addresses_distinct: false,
+                ..all_healthy()
+            },
+        ),
+        (
+            Miswire::OracleNotAFeed,
+            Health {
+                feed_answers: false,
+                feed_valid: false,
+                freshness_covers_notice: false,
+                ..all_healthy()
+            },
+        ),
+        (
+            Miswire::NoCustodian,
+            Health {
+                custodian_set: false,
+                ..all_healthy()
+            },
+        ),
+        (
+            Miswire::CustodianIsVault,
+            Health {
+                custodian_set: false,
+                ..all_healthy()
+            },
+        ),
     ];
     for (miswire, expected) in cases {
         let f = miswired(miswire);
@@ -113,7 +154,8 @@ fn each_missing_wiring_step_shows_in_its_own_flag() {
             Health {
                 setup_open: true,
                 ..expected
-            }
+            },
+            "{miswire:?}"
         );
     }
 }