Description
MeridianVault::initialize() (packages/contracts/vault/src/lib.rs:129-147) has no caller
authorization beyond require_auth() on the admin address the caller themselves supplies:
pub fn initialize(env: Env, admin: Address, usdc: Address, musdc: Address, adapter: Address) -> Result<(), ContractError> {
if env.storage().instance().has(&ADMIN) {
return Err(ContractError::AlreadyInitialized);
}
admin.require_auth();
env.storage().instance().set(&ADMIN, &admin);
...
}
require_auth() only proves the caller controls the address they chose to pass in as admin,
it says nothing about who is entitled to be the admin. Since deploy and initialize() are two
separate transactions (scripts/deploy-testnet.sh deploys the vault, then calls initialize()
in a later transaction), anyone watching the ledger can land their own initialize() call first,
self-signing as the admin they supply, and become the permanent admin of the vault:
set_admin, set_paused, set_adapter, migrate_adapter.
This is the same bug class as #505, fixed for both adapters in #550 by moving state-setting into
a __constructor that runs inside the CreateContract deploy operation, removing the
intervening ledger where a race is possible. The vault was explicitly left out of #550's scope.
See the unanswered review thread on #544: #544 (comment)
Steps to Reproduce
- Deploy a fresh
MeridianVault (stellar contract deploy), without calling initialize() in
the same transaction.
- Before the legitimate deployer submits their
initialize(admin, usdc, musdc, adapter) call,
an attacker submits their own initialize() call with their own address as admin,
self-authorizing via require_auth().
- The attacker's transaction lands first.
Expected Behavior
Only the deploying party should be able to become the vault's admin. No transaction ordering
should be able to hand admin control to an unrelated address.
Actual Behavior
The attacker's initialize() call succeeds and permanently sets them as admin. The legitimate
deployer's subsequent initialize() call fails with AlreadyInitialized, and the vault
(set_admin, set_paused, set_adapter, migrate_adapter) is now controlled by the attacker.
Environment
| Field |
Value |
| Network |
testnet |
| Wallet |
N/A |
| Protocol affected |
None |
| Browser (if frontend) |
N/A |
| Node.js version |
N/A |
| pnpm version |
N/A |
Possible Cause / Fix
Apply the same __constructor pattern #550 used for the adapters: move initialize()'s state
writes into a __constructor(admin, usdc, musdc, adapter) that runs inside the deploying
CreateContract transaction, and keep initialize() around only so it always returns
AlreadyInitialized on any vault deployed from the new WASM.
This is not a mechanical port. The vault's test suite has several tests that deliberately
register an uninitialized vault on purpose (get_admin_fails_before_initialize,
deposit_fails_before_initialize, and others), and a constructor makes that state unreachable,
so those tests and the NotInitialized error path they exercise need to be rethought as part of
this fix, not just carried over.
scripts/deploy-testnet.sh's current ADMIN_KEY convention (calling initialize() immediately
within the same script run when the admin key is available) is the interim mitigation and stays
in place until this lands.
Additional Context
Found during review of #550 (the adapter-side fix for the identical bug, #505). The contributor
on #550 raised this exact gap during #544's review and it went unaddressed before #544 merged.
Description
MeridianVault::initialize()(packages/contracts/vault/src/lib.rs:129-147) has no callerauthorization beyond
require_auth()on theadminaddress the caller themselves supplies:require_auth()only proves the caller controls the address they chose to pass in asadmin,it says nothing about who is entitled to be the admin. Since deploy and
initialize()are twoseparate transactions (
scripts/deploy-testnet.shdeploys the vault, then callsinitialize()in a later transaction), anyone watching the ledger can land their own
initialize()call first,self-signing as the
adminthey supply, and become the permanent admin of the vault:set_admin,set_paused,set_adapter,migrate_adapter.This is the same bug class as #505, fixed for both adapters in #550 by moving state-setting into
a
__constructorthat runs inside theCreateContractdeploy operation, removing theintervening ledger where a race is possible. The vault was explicitly left out of #550's scope.
See the unanswered review thread on #544: #544 (comment)
Steps to Reproduce
MeridianVault(stellar contract deploy), without callinginitialize()inthe same transaction.
initialize(admin, usdc, musdc, adapter)call,an attacker submits their own
initialize()call with their own address asadmin,self-authorizing via
require_auth().Expected Behavior
Only the deploying party should be able to become the vault's admin. No transaction ordering
should be able to hand admin control to an unrelated address.
Actual Behavior
The attacker's
initialize()call succeeds and permanently sets them as admin. The legitimatedeployer's subsequent
initialize()call fails withAlreadyInitialized, and the vault(
set_admin,set_paused,set_adapter,migrate_adapter) is now controlled by the attacker.Environment
Possible Cause / Fix
Apply the same
__constructorpattern #550 used for the adapters: moveinitialize()'s statewrites into a
__constructor(admin, usdc, musdc, adapter)that runs inside the deployingCreateContracttransaction, and keepinitialize()around only so it always returnsAlreadyInitializedon any vault deployed from the new WASM.This is not a mechanical port. The vault's test suite has several tests that deliberately
register an uninitialized vault on purpose (
get_admin_fails_before_initialize,deposit_fails_before_initialize, and others), and a constructor makes that state unreachable,so those tests and the
NotInitializederror path they exercise need to be rethought as part ofthis fix, not just carried over.
scripts/deploy-testnet.sh's currentADMIN_KEYconvention (callinginitialize()immediatelywithin the same script run when the admin key is available) is the interim mitigation and stays
in place until this lands.
Additional Context
Found during review of #550 (the adapter-side fix for the identical bug, #505). The contributor
on #550 raised this exact gap during #544's review and it went unaddressed before #544 merged.