fix(contracts): close front-runnable adapter initialize() with __constructor

apps/docs/architecture/vault-contract.md

@@ -116,6 +116,19 @@ pub trait YieldAdapterInterface {
 
 Two adapters exist today, at `packages/contracts/blend-adapter/src/lib.rs` and `packages/contracts/defindex-adapter/src/lib.rs`.
 
+### Adapter deployment and initialization
+
+Both adapters set their vault, protocol, and USDC addresses in a `__constructor`, which the host runs inside the `CreateContract` operation that deploys the contract. Pass the arguments to `stellar contract deploy` after a `--` separator:
+
+```bash
+stellar contract deploy --network testnet --source $DEPLOYER --wasm-hash $HASH \
+  -- --vault $VAULT_ID --pool $BLEND_POOL_ID --usdc $USDC_ID
+```
+
+There is no separate initialization transaction, deliberately. An adapter's `initialize()` cannot authenticate its caller (there is no deployer identity in storage yet to check against), so while deploy and initialize were two transactions, anyone watching the ledger could land `initialize()` first with their own address as `vault` and become the only party able to move funds through that adapter (#505). Adding `require_auth()` to `initialize()` would not have closed this, since it would only prove the racer controls the address they passed in. A constructor removes the intervening ledger, so there is nothing to race.
+
+`initialize()` still exists on both adapters, so the ABI of adapters deployed from earlier WASM is unchanged and they can still be initialized by hand. On anything deployed from current WASM it always returns `AlreadyInitialized`, because the constructor has already written `VAULT_KEY`.
+
 ### BlendAdapter
 
 Supplies USDC into a Blend lending pool as collateral. `deposit()` calls the pool's `submit()` with a `REQUEST_SUPPLY` request; `withdraw()` calls `submit()` with a `REQUEST_WITHDRAW` request and has Blend deliver USDC straight to the recipient.

apps/docs/operations/testnet-deployment.md

@@ -42,7 +42,7 @@ DEPLOYER=my-deployer ADMIN=$ADMIN_ADDR ADMIN_KEY=my-admin bash scripts/deploy-te
 
 This builds all three contract crates (`vault`, `blend-adapter`, `defindex-adapter`), uploads and deploys the vault and a `BlendAdapter`, deploys a fresh mUSDC Stellar Asset Contract, and wires everything together:
 
-1. Initializes the `BlendAdapter` with the vault address, Blend's testnet pool, and USDC.
+1. Deploys the `BlendAdapter` with the vault address, Blend's testnet pool, and USDC passed as **constructor arguments**, so the adapter is wired inside the transaction that creates it. There is no separate adapter `initialize()` step: that gap was front-runnable (#505). See "Adapter deployment and initialization" in [`architecture/vault-contract.md`](../architecture/vault-contract.md).
 2. Initializes the vault with `admin`, `usdc`, `musdc`, and `adapter` (the just-deployed `BlendAdapter`), signed by `DEPLOYER` when `ADMIN` defaulted to it, or by `ADMIN_KEY` when `ADMIN` is a separate address. Without `ADMIN_KEY` this step is printed for the `ADMIN` key holder to run instead, leaving the vault claimable until they do (see "The `DEPLOYER` / `ADMIN` split" above).
 3. Sets the vault as mUSDC's admin, so it can mint/burn shares autonomously.
 
@@ -101,10 +101,12 @@ This is exactly the call chain the frontend uses to discover live APY (`vault.ge
 Adapter contracts have no in-place upgrade path. To get new adapter code (a bug fix, a new feature) onto an already-live vault, deploy a fresh adapter and swap the vault onto it:
 
 ```bash
+# VAULT_ID is required: the vault address is a constructor argument, baked into
+# the adapter permanently by the deploying transaction.
 VAULT_ID=$VAULT_CONTRACT_ID DEPLOYER=my-deployer bash scripts/redeploy-blend-adapter.sh
 ```
 
-This builds and deploys a new `BlendAdapter`, initializes it against the same vault/pool/USDC, and then **prints, but does not run**, the final `set_adapter` command:
+This builds and deploys a new `BlendAdapter`, wired to the same vault/pool/USDC through its constructor arguments, and then **prints, but does not run**, the final `set_adapter` command:
 
 ```bash
 stellar contract invoke --network testnet --source $ADMIN \

packages/contracts/blend-adapter/src/lib.rs

@@ -152,21 +152,53 @@ pub struct MeridianBlendAdapter;
 
 #[contractimpl]
 impl MeridianBlendAdapter {
-    /// Called once after deployment. Links the adapter to its vault, Blend pool,
-    /// and USDC token.
+    /// Links the adapter to its vault, Blend pool, and USDC token.
+    ///
+    /// Runs inside the `CreateContract` host operation that deploys this
+    /// adapter, in the same transaction, so the adapter is never observable
+    /// on-ledger in an uninitialized state.
+    ///
+    /// This is what closes the front-running window in #505. `initialize()`
+    /// below has no authorization check by design (there is no deployer
+    /// identity in storage yet to check against), so for as long as deploy
+    /// and initialize were two separate transactions, anyone watching the
+    /// ledger could land `initialize()` first with their own address as
+    /// `vault`, becoming the only party able to move funds through the
+    /// adapter. Adding an auth check to `initialize()` would not have helped:
+    /// it would only prove the racer controls the address they chose to pass
+    /// in. Removing the intervening ledger is the fix.
+    pub fn __constructor(env: Env, vault: Address, pool: Address, usdc: Address) {
+        Self::init_state(&env, &vault, &pool, &usdc);
+    }
+
+    /// Retained so the ABI of adapters already deployed from earlier WASM is
+    /// unchanged, and so an old adapter can still be initialized by hand.
+    ///
+    /// On any adapter deployed from this WASM it is unreachable:
+    /// `__constructor` has already set `VAULT_KEY`, so every call returns
+    /// `AlreadyInitialized`. That is the intended behaviour, not a leftover.
+    /// An attacker calling this against a freshly deployed adapter is
+    /// rejected instead of served.
     pub fn initialize(
         env: Env,
         vault: Address,
         pool: Address,
         usdc: Address,
     ) -> Result<(), ContractError> {
         require_not_initialized(&env)?;
-        store_vault_and_usdc(&env, &vault, &usdc);
-        env.storage().instance().set(&POOL_KEY, &pool);
-        env.storage().instance().set(&TOTAL_KEY, &0_i128);
+        Self::init_state(&env, &vault, &pool, &usdc);
         Ok(())
     }
 
+    /// The write half of initialization, shared by `__constructor` and
+    /// `initialize` so the two can never set up different state. Not exported
+    /// (no `pub`), so it is not callable from outside the contract.
+    fn init_state(env: &Env, vault: &Address, pool: &Address, usdc: &Address) {
+        store_vault_and_usdc(env, vault, usdc);
+        env.storage().instance().set(&POOL_KEY, pool);
+        env.storage().instance().set(&TOTAL_KEY, &0_i128);
+    }
+
     /// Called by the vault after transferring `amount` USDC to this adapter.
     /// Supplies the USDC to the Blend lending pool as collateral and returns
     /// the real bTokens credited, measured from Blend's own ledger rather
@@ -501,9 +533,14 @@ mod tests {
         let pool = MockBlendPoolClient::new(&env, &pool_id);
         pool.initialize(&SCALAR, &RESERVE_INDEX);
 
-        let adapter_id = env.register(MeridianBlendAdapter, ());
+        // Registered with constructor arguments, which is how every real
+        // deployment of this contract is now wired: there is no
+        // deploy-then-initialize path left to exercise.
+        let adapter_id = env.register(
+            MeridianBlendAdapter,
+            (vault.clone(), pool_id.clone(), usdc_id.clone()),
+        );
         let adapter = MeridianBlendAdapterClient::new(&env, &adapter_id);
-        adapter.initialize(&vault, &pool_id, &usdc_id);
 
         // Fund the vault (the caller of deposit) with USDC, then act as the
         // vault transferring into the adapter, matching real vault behaviour.
@@ -674,6 +711,44 @@ mod tests {
         assert_eq!(result, Err(Ok(ContractError::AlreadyInitialized)));
     }
 
+    #[test]
+    fn constructor_sets_vault_pool_and_usdc() {
+        let (env, vault, usdc_id, adapter, pool) = setup();
+
+        // No initialize() call happened in setup(): every one of these was
+        // written by __constructor during registration.
+        assert_eq!(adapter.get_pool(), pool.address);
+        assert_eq!(
+            env.as_contract(&adapter.address, || adapter_common::get_vault(&env)),
+            Some(vault)
+        );
+        assert_eq!(
+            env.as_contract(&adapter.address, || adapter_common::get_usdc(&env)),
+            usdc_id
+        );
+        assert_eq!(adapter.total_assets(), 0);
+    }
+
+    #[test]
+    fn initialize_cannot_hijack_a_constructor_deployed_adapter() {
+        // The #505 front-run, run against the fixed contract. An attacker
+        // watching the ledger calls initialize() with their own address as
+        // vault, hoping to land before the deployer's own call. There is no
+        // longer a window to land in: __constructor already ran inside the
+        // deploying transaction, so the attempt is rejected and the adapter
+        // stays bound to the real vault.
+        let (env, vault, usdc_id, adapter, pool) = setup();
+        let attacker = Address::generate(&env);
+
+        let result = adapter.try_initialize(&attacker, &pool.address, &usdc_id);
+        assert_eq!(result, Err(Ok(ContractError::AlreadyInitialized)));
+
+        assert_eq!(
+            env.as_contract(&adapter.address, || adapter_common::get_vault(&env)),
+            Some(vault)
+        );
+    }
+
     #[test]
     #[should_panic]
     fn deposit_requires_vault_auth() {
@@ -687,9 +762,11 @@ mod tests {
             .address();
         let pool_id = env.register(MockBlendPool, ());
         MockBlendPoolClient::new(&env, &pool_id).initialize(&SCALAR, &RESERVE_INDEX);
-        let adapter_id = env.register(MeridianBlendAdapter, ());
+        let adapter_id = env.register(
+            MeridianBlendAdapter,
+            (vault.clone(), pool_id.clone(), usdc_id.clone()),
+        );
         let adapter = MeridianBlendAdapterClient::new(&env, &adapter_id);
-        adapter.initialize(&vault, &pool_id, &usdc_id);
 
         adapter.deposit(&100_0000000_i128);
     }
@@ -705,9 +782,11 @@ mod tests {
             .address();
         let pool_id = env.register(MockBlendPool, ());
         MockBlendPoolClient::new(&env, &pool_id).initialize(&SCALAR, &RESERVE_INDEX);
-        let adapter_id = env.register(MeridianBlendAdapter, ());
+        let adapter_id = env.register(
+            MeridianBlendAdapter,
+            (vault.clone(), pool_id.clone(), usdc_id.clone()),
+        );
         let adapter = MeridianBlendAdapterClient::new(&env, &adapter_id);
-        adapter.initialize(&vault, &pool_id, &usdc_id);
 
         let recipient = Address::generate(&env);
         adapter.withdraw(&100_0000000_i128, &recipient);

packages/contracts/defindex-adapter/src/lib.rs

@@ -71,20 +71,53 @@ pub struct MeridianDefindexAdapter;
 
 #[contractimpl]
 impl MeridianDefindexAdapter {
-    /// Called once after deployment. Links the adapter to its vault, DeFindex
-    /// vault contract, and USDC token.
+    /// Links the adapter to its vault, DeFindex vault contract, and USDC
+    /// token.
+    ///
+    /// Runs inside the `CreateContract` host operation that deploys this
+    /// adapter, in the same transaction, so the adapter is never observable
+    /// on-ledger in an uninitialized state.
+    ///
+    /// This is what closes the front-running window in #505. `initialize()`
+    /// below has no authorization check by design (there is no deployer
+    /// identity in storage yet to check against), so for as long as deploy
+    /// and initialize were two separate transactions, anyone watching the
+    /// ledger could land `initialize()` first with their own address as
+    /// `vault`, becoming the only party able to move funds through the
+    /// adapter. Adding an auth check to `initialize()` would not have helped:
+    /// it would only prove the racer controls the address they chose to pass
+    /// in. Removing the intervening ledger is the fix.
+    pub fn __constructor(env: Env, vault: Address, defindex_vault: Address, usdc: Address) {
+        Self::init_state(&env, &vault, &defindex_vault, &usdc);
+    }
+
+    /// Retained so the ABI of adapters already deployed from earlier WASM is
+    /// unchanged, and so an old adapter can still be initialized by hand.
+    ///
+    /// On any adapter deployed from this WASM it is unreachable:
+    /// `__constructor` has already set `VAULT_KEY`, so every call returns
+    /// `AlreadyInitialized`. That is the intended behaviour, not a leftover.
+    /// An attacker calling this against a freshly deployed adapter is
+    /// rejected instead of served.
     pub fn initialize(
         env: Env,
         vault: Address,
         defindex_vault: Address,
         usdc: Address,
     ) -> Result<(), ContractError> {
         require_not_initialized(&env)?;
-        store_vault_and_usdc(&env, &vault, &usdc);
-        env.storage().instance().set(&DFX_VAULT, &defindex_vault);
+        Self::init_state(&env, &vault, &defindex_vault, &usdc);
         Ok(())
     }
 
+    /// The write half of initialization, shared by `__constructor` and
+    /// `initialize` so the two can never set up different state. Not exported
+    /// (no `pub`), so it is not callable from outside the contract.
+    fn init_state(env: &Env, vault: &Address, defindex_vault: &Address, usdc: &Address) {
+        store_vault_and_usdc(env, vault, usdc);
+        env.storage().instance().set(&DFX_VAULT, defindex_vault);
+    }
+
     /// Called by the vault after transferring `amount` USDC to this adapter.
     /// Deposits USDC into the DeFindex vault on behalf of the adapter and
     /// returns the dfToken shares received.
@@ -272,9 +305,11 @@ mod tests {
         let dfx = MockDefindexVaultClient::new(&env, &dfx_id);
         dfx.initialize(&usdc_id);
 
-        let adapter_id = env.register(MeridianDefindexAdapter, ());
+        let adapter_id = env.register(
+            MeridianDefindexAdapter,
+            (vault.clone(), dfx_id.clone(), usdc_id.clone()),
+        );
         let adapter = MeridianDefindexAdapterClient::new(&env, &adapter_id);
-        adapter.initialize(&vault, &dfx_id, &usdc_id);
 
         // Fund the vault (the caller of deposit) with USDC, then act as the
         // vault transferring into the adapter, matching real vault behaviour.
@@ -369,6 +404,43 @@ mod tests {
         assert_eq!(result, Err(Ok(ContractError::AlreadyInitialized)));
     }
 
+    #[test]
+    fn constructor_sets_vault_defindex_vault_and_usdc() {
+        let (env, vault, usdc_id, adapter, dfx) = setup();
+
+        // No initialize() call happened in setup(): every one of these was
+        // written by __constructor during registration.
+        assert_eq!(adapter.get_pool(), dfx.address);
+        assert_eq!(
+            env.as_contract(&adapter.address, || adapter_common::get_vault(&env)),
+            Some(vault)
+        );
+        assert_eq!(
+            env.as_contract(&adapter.address, || adapter_common::get_usdc(&env)),
+            usdc_id
+        );
+    }
+
+    #[test]
+    fn initialize_cannot_hijack_a_constructor_deployed_adapter() {
+        // The #505 front-run, run against the fixed contract. An attacker
+        // watching the ledger calls initialize() with their own address as
+        // vault, hoping to land before the deployer's own call. There is no
+        // longer a window to land in: __constructor already ran inside the
+        // deploying transaction, so the attempt is rejected and the adapter
+        // stays bound to the real vault.
+        let (env, vault, usdc_id, adapter, dfx) = setup();
+        let attacker = Address::generate(&env);
+
+        let result = adapter.try_initialize(&attacker, &dfx.address, &usdc_id);
+        assert_eq!(result, Err(Ok(ContractError::AlreadyInitialized)));
+
+        assert_eq!(
+            env.as_contract(&adapter.address, || adapter_common::get_vault(&env)),
+            Some(vault)
+        );
+    }
+
     #[test]
     #[should_panic]
     fn deposit_requires_vault_auth() {
@@ -382,9 +454,11 @@ mod tests {
             .address();
         let dfx_id = env.register(MockDefindexVault, ());
         MockDefindexVaultClient::new(&env, &dfx_id).initialize(&usdc_id);
-        let adapter_id = env.register(MeridianDefindexAdapter, ());
+        let adapter_id = env.register(
+            MeridianDefindexAdapter,
+            (vault.clone(), dfx_id.clone(), usdc_id.clone()),
+        );
         let adapter = MeridianDefindexAdapterClient::new(&env, &adapter_id);
-        adapter.initialize(&vault, &dfx_id, &usdc_id);
 
         adapter.deposit(&100_0000000_i128);
     }
@@ -400,9 +474,11 @@ mod tests {
             .address();
         let dfx_id = env.register(MockDefindexVault, ());
         MockDefindexVaultClient::new(&env, &dfx_id).initialize(&usdc_id);
-        let adapter_id = env.register(MeridianDefindexAdapter, ());
+        let adapter_id = env.register(
+            MeridianDefindexAdapter,
+            (vault.clone(), dfx_id.clone(), usdc_id.clone()),
+        );
         let adapter = MeridianDefindexAdapterClient::new(&env, &adapter_id);
-        adapter.initialize(&vault, &dfx_id, &usdc_id);
 
         let recipient = Address::generate(&env);
         adapter.withdraw(&100_0000000_i128, &recipient);

scripts/deploy-testnet.sh

@@ -79,8 +79,12 @@ WASM_BLEND_ADAPTER="$WASM_DIR/meridian_blend_adapter.wasm"
 upload() {
   stellar contract upload --network "$NETWORK" --source "$DEPLOYER" --wasm "$1"
 }
+# Any arguments after the wasm hash are forwarded to `stellar contract deploy`,
+# which is how constructor arguments are passed: `deploy "$hash" -- --a 1`.
 deploy() {
-  stellar contract deploy --network "$NETWORK" --source "$DEPLOYER" --wasm-hash "$1"
+  local hash="$1"
+  shift
+  stellar contract deploy --network "$NETWORK" --source "$DEPLOYER" --wasm-hash "$hash" "$@"
 }
 
 echo "Uploading vault WASM..."
@@ -92,8 +96,12 @@ echo "Deploying vault..."
 VAULT_ID=$(deploy "$VAULT_HASH")
 echo "vault contract ID: $VAULT_ID"
 
-echo "Deploying blend-adapter..."
-BLEND_ADAPTER_ID=$(deploy "$BLEND_ADAPTER_HASH")
+# The adapter's vault/pool/USDC wiring is passed as constructor arguments, so
+# it is set inside this same CreateContract operation. There is deliberately no
+# separate initialize() step: that gap was front-runnable (#505).
+echo "Deploying blend-adapter (vault=$VAULT_ID, pool=$BLEND_POOL_ID, usdc=$USDC_ID)..."
+BLEND_ADAPTER_ID=$(deploy "$BLEND_ADAPTER_HASH" \
+  -- --vault "$VAULT_ID" --pool "$BLEND_POOL_ID" --usdc "$USDC_ID")
 echo "blend-adapter contract ID: $BLEND_ADAPTER_ID"
 
 echo "Deploying mUSDC share token (Stellar Asset Contract)..."
@@ -103,11 +111,6 @@ MUSDC_ID=$(stellar contract asset deploy \
   --asset "MUSDC:$DEPLOYER_ADDRESS")
 echo "mUSDC contract ID: $MUSDC_ID"
 
-echo "Initializing blend-adapter (pool=$BLEND_POOL_ID, usdc=$USDC_ID)..."
-stellar contract invoke \
-  --network "$NETWORK" --source "$DEPLOYER" --id "$BLEND_ADAPTER_ID" \
-  -- initialize --vault "$VAULT_ID" --pool "$BLEND_POOL_ID" --usdc "$USDC_ID"
-
 # Whichever key signs initialize(), it has to be the one that controls
 # ADMIN_ADDRESS: initialize() calls admin.require_auth() on the address it is
 # handed. When ADMIN defaults to DEPLOYER's own address, DEPLOYER's signature