From 6e21505a7f8a6b07e7996ecae9d3eae5b1baf958 Mon Sep 17 00:00:00 2001 From: CleverWild Date: Mon, 7 Sep 2026 15:18:31 +0200 Subject: [PATCH] fix(operator): soft-revoke wallet access instead of deleting the row --- .../2026-02-14-171124-0000_init/up.sql | 1 + .../arbiter-server/src/actors/evm/mod.rs | 10 + server/crates/arbiter-server/src/db/mod.rs | 19 +- server/crates/arbiter-server/src/db/models.rs | 7 +- server/crates/arbiter-server/src/db/schema.rs | 1 + server/crates/arbiter-server/src/evm/mod.rs | 4 + .../src/evm/policies/ether_transfer/tests.rs | 1 + .../src/evm/policies/token_transfers/tests.rs | 1 + .../src/peers/operator/session/handlers.rs | 380 +++++++++++++++--- 9 files changed, 358 insertions(+), 66 deletions(-) diff --git a/server/crates/arbiter-server/migrations/2026-02-14-171124-0000_init/up.sql b/server/crates/arbiter-server/migrations/2026-02-14-171124-0000_init/up.sql index 36a5310..42c7756 100644 --- a/server/crates/arbiter-server/migrations/2026-02-14-171124-0000_init/up.sql +++ b/server/crates/arbiter-server/migrations/2026-02-14-171124-0000_init/up.sql @@ -109,6 +109,7 @@ create table if not exists evm_wallet_access ( id integer not null primary key, wallet_id integer not null references evm_wallet (id) on delete cascade, client_id integer not null references program_client (id) on delete cascade, + revoked_at integer, -- unix timestamp when revoked, null = still active created_at integer not null default(unixepoch ('now')) ) STRICT; diff --git a/server/crates/arbiter-server/src/actors/evm/mod.rs b/server/crates/arbiter-server/src/actors/evm/mod.rs index 9b2c073..59d91c8 100644 --- a/server/crates/arbiter-server/src/actors/evm/mod.rs +++ b/server/crates/arbiter-server/src/actors/evm/mod.rs @@ -226,6 +226,7 @@ impl EvmActor { .select(models::EvmWalletAccess::as_select()) .filter(schema::evm_wallet_access::wallet_id.eq(wallet.id)) .filter(schema::evm_wallet_access::client_id.eq(client_id)) + .filter(schema::evm_wallet_access::revoked_at.is_null()) .first(&mut conn) .await .optional() @@ -261,6 +262,7 @@ impl EvmActor { .select(models::EvmWalletAccess::as_select()) .filter(schema::evm_wallet_access::wallet_id.eq(wallet.id)) .filter(schema::evm_wallet_access::client_id.eq(client_id)) + .filter(schema::evm_wallet_access::revoked_at.is_null()) .first(&mut conn) .await .optional() @@ -323,11 +325,19 @@ impl EvmActor { ) -> Result<(), Error> { let mut conn = self.db.get().await.map_err(DatabaseError::from)?; + // Revives a previously revoked row instead of conflicting on it forever: + // `uniq_wallet_access` is a unique index on `(wallet_id, client_id)`. insert_into(schema::evm_wallet_access::table) .values(( schema::evm_wallet_access::wallet_id.eq(EvmWalletId::from_raw(settings.wallet_id)), schema::evm_wallet_access::client_id.eq(settings.client_id), )) + .on_conflict(( + schema::evm_wallet_access::wallet_id, + schema::evm_wallet_access::client_id, + )) + .do_update() + .set(schema::evm_wallet_access::revoked_at.eq(None::)) .execute(&mut conn) .await .map_err(DatabaseError::from)?; diff --git a/server/crates/arbiter-server/src/db/mod.rs b/server/crates/arbiter-server/src/db/mod.rs index da564ba..01d1b87 100644 --- a/server/crates/arbiter-server/src/db/mod.rs +++ b/server/crates/arbiter-server/src/db/mod.rs @@ -164,7 +164,11 @@ pub async fn create_test_pool() -> DatabasePool { #[cfg(test)] mod tests { use super::*; - use diesel::{ExpressionMethods as _, dsl::insert_into}; + use diesel::{ + ExpressionMethods as _, + dsl::insert_into, + result::{DatabaseErrorKind, Error as DieselError}, + }; use diesel_async::RunQueryDsl; /// `operator.id` references `operator_identity(id)`. Without `PRAGMA foreign_keys = ON` @@ -184,9 +188,18 @@ mod tests { .execute(&mut conn) .await; + // Specifically a foreign-key violation, not any error: a `NOT NULL` failure or a + // renamed column would also make `result.is_err()` true without proving the pragma + // is what rejected the insert. assert!( - result.is_err(), - "insert with a dangling operator_identity reference was accepted" + matches!( + result, + Err(DieselError::DatabaseError( + DatabaseErrorKind::ForeignKeyViolation, + _ + )) + ), + "expected a foreign-key violation for a dangling operator_identity reference, got {result:?}" ); } } diff --git a/server/crates/arbiter-server/src/db/models.rs b/server/crates/arbiter-server/src/db/models.rs index 4eae389..4bd004b 100644 --- a/server/crates/arbiter-server/src/db/models.rs +++ b/server/crates/arbiter-server/src/db/models.rs @@ -263,19 +263,22 @@ pub struct EvmWallet { #[view( NewEvmWalletAccess, derive(Insertable), - omit(id, created_at), + omit(id, created_at, revoked_at), attributes_with = "deriveless" )] #[view( CoreEvmWalletAccess, derive(Insertable), - omit(created_at), + omit(created_at, revoked_at), attributes_with = "deriveless" )] pub struct EvmWalletAccess { pub id: i32, pub wallet_id: EvmWalletId, pub client_id: i32, + // Grants, transaction logs, and persistent-grant proposals reference this row + // `on delete restrict`, so revocation cannot delete it -- it marks it revoked instead. + pub revoked_at: Option, pub created_at: SqliteTimestamp, } diff --git a/server/crates/arbiter-server/src/db/schema.rs b/server/crates/arbiter-server/src/db/schema.rs index 8123290..c71717c 100644 --- a/server/crates/arbiter-server/src/db/schema.rs +++ b/server/crates/arbiter-server/src/db/schema.rs @@ -135,6 +135,7 @@ diesel::table! { id -> Integer, wallet_id -> Integer, client_id -> Integer, + revoked_at -> Nullable, created_at -> Integer, } } diff --git a/server/crates/arbiter-server/src/evm/mod.rs b/server/crates/arbiter-server/src/evm/mod.rs index 9b34bc3..f557270 100644 --- a/server/crates/arbiter-server/src/evm/mod.rs +++ b/server/crates/arbiter-server/src/evm/mod.rs @@ -381,6 +381,7 @@ mod tests { id: WALLET_ACCESS_ID, wallet_id: EvmWalletId::from_raw(5), client_id: 20, + revoked_at: None, created_at: SqliteTimestamp(Utc::now()), }, chain: CHAIN_ID, @@ -478,6 +479,9 @@ mod tests { conn: &mut DatabaseConnection, shared: &SharedGrantSettings, ) -> EvmBasicGrant { + // The seeded id deliberately wins over `shared.wallet_access_id`: every other field + // below is read from `shared`, but a caller-supplied access id would almost never + // reference a row that actually exists under foreign-key enforcement. let wallet_access_id = seed_wallet_access(conn).await; #[expect( diff --git a/server/crates/arbiter-server/src/evm/policies/ether_transfer/tests.rs b/server/crates/arbiter-server/src/evm/policies/ether_transfer/tests.rs index 4d2cfa5..19ea8be 100644 --- a/server/crates/arbiter-server/src/evm/policies/ether_transfer/tests.rs +++ b/server/crates/arbiter-server/src/evm/policies/ether_transfer/tests.rs @@ -35,6 +35,7 @@ fn ctx(to: Address, value: U256) -> EvalContext { id: WALLET_ACCESS_ID, wallet_id: EvmWalletId::from_raw(10), client_id: 20, + revoked_at: None, created_at: SqliteTimestamp(Utc::now()), }, chain: CHAIN_ID, diff --git a/server/crates/arbiter-server/src/evm/policies/token_transfers/tests.rs b/server/crates/arbiter-server/src/evm/policies/token_transfers/tests.rs index 4f6afd0..a4a587e 100644 --- a/server/crates/arbiter-server/src/evm/policies/token_transfers/tests.rs +++ b/server/crates/arbiter-server/src/evm/policies/token_transfers/tests.rs @@ -48,6 +48,7 @@ fn ctx(to: Address, calldata: Bytes) -> EvalContext { id: WALLET_ACCESS_ID, wallet_id: EvmWalletId::from_raw(10), client_id: 20, + revoked_at: None, created_at: SqliteTimestamp(Utc::now()), }, chain: CHAIN_ID, diff --git a/server/crates/arbiter-server/src/peers/operator/session/handlers.rs b/server/crates/arbiter-server/src/peers/operator/session/handlers.rs index 299254a..60cc16e 100644 --- a/server/crates/arbiter-server/src/peers/operator/session/handlers.rs +++ b/server/crates/arbiter-server/src/peers/operator/session/handlers.rs @@ -177,20 +177,7 @@ impl OperatorSession { entries: Vec, ) -> Result<(), Error> { let mut conn = self.props.db.get().await?; - conn.transaction(async |conn| { - use crate::db::schema::evm_wallet_access; - - for entry in entries { - diesel::insert_into(evm_wallet_access::table) - .values(&entry) - .on_conflict_do_nothing() - .execute(&mut *conn) - .await?; - } - - Result::<_, Error>::Ok(()) - }) - .await?; + grant_wallet_access(&mut conn, entries).await?; Ok(()) } @@ -211,6 +198,7 @@ impl OperatorSession { use crate::db::schema::evm_wallet_access; let mut conn = self.props.db.get().await?; let access_entries = evm_wallet_access::table + .filter(evm_wallet_access::revoked_at.is_null()) .select(EvmWalletAccess::as_select()) .load::<_>(&mut conn) .await?; @@ -218,16 +206,47 @@ impl OperatorSession { } } -/// Deletes access rows by their own id. The wire carries `WalletAccessEntry.id` values, so -/// filtering by `wallet_id` here would revoke every client's access to that wallet. +/// Grants access, reviving a previously revoked row rather than leaving it shadowed: +/// `uniq_wallet_access` is a unique index on `(wallet_id, client_id)`, so a plain insert +/// would conflict forever on a row that was revoked but never deleted. +pub(crate) async fn grant_wallet_access( + conn: &mut crate::db::DatabaseConnection, + entries: Vec, +) -> Result<(), diesel::result::Error> { + use crate::db::{models::SqliteTimestamp, schema::evm_wallet_access}; + + conn.transaction(async |conn| { + for entry in entries { + diesel::insert_into(evm_wallet_access::table) + .values(&entry) + .on_conflict((evm_wallet_access::wallet_id, evm_wallet_access::client_id)) + .do_update() + .set(evm_wallet_access::revoked_at.eq(None::)) + .execute(&mut *conn) + .await?; + } + + Ok(()) + }) + .await +} + +/// Marks access rows revoked by their own id rather than deleting them. The wire carries +/// `WalletAccessEntry.id` values, so filtering by `wallet_id` here would revoke every +/// client's access to that wallet. Deleting is not an option: `evm_basic_grant`, +/// `evm_transaction_log`, and `proposal_persistent_grant` all reference this row +/// `on delete restrict`, so an access that was ever granted, signed with, or proposed +/// against can never be deleted -- only marked revoked. pub(crate) async fn revoke_wallet_access( conn: &mut crate::db::DatabaseConnection, ids: &[i32], ) -> Result { - use crate::db::schema::evm_wallet_access; + use crate::db::{models::SqliteTimestamp, schema::evm_wallet_access}; - diesel::delete(evm_wallet_access::table) + diesel::update(evm_wallet_access::table) .filter(evm_wallet_access::id.eq_any(ids)) + .filter(evm_wallet_access::revoked_at.is_null()) + .set(evm_wallet_access::revoked_at.eq(SqliteTimestamp::now())) .execute(conn) .await } @@ -385,18 +404,15 @@ impl OperatorSession { #[cfg(test)] mod tests { - use super::revoke_wallet_access; + use super::{grant_wallet_access, revoke_wallet_access}; use crate::db::{self, models, schema}; use diesel::{ExpressionMethods as _, QueryDsl as _, dsl::insert_into}; use diesel_async::RunQueryDsl; - /// Two clients share one wallet. Revoking one access row must leave the other alone. - #[tokio::test] - async fn revoking_one_access_leaves_the_other_client_alone() { - let pool = db::create_test_pool().await; - let mut conn = pool.get().await.unwrap(); - + /// Inserts a fresh root key, an aead-encrypted wallet secret, and the wallet itself. + /// Returns the wallet's id and its 20-byte address. + async fn seed_wallet(conn: &mut db::DatabaseConnection) -> (models::EvmWalletId, Vec) { let root_key_id: models::RootKeyHistoryId = insert_into(schema::root_key_history::table) .values(&models::NewRootKeyHistory { ciphertext: vec![0u8; 32], @@ -407,7 +423,7 @@ mod tests { salt: vec![0u8; 16], }) .returning(schema::root_key_history::id) - .get_result(&mut conn) + .get_result(conn) .await .unwrap(); @@ -421,81 +437,323 @@ mod tests { created_at: chrono::Utc::now().into(), }) .returning(schema::aead_encrypted::id) - .get_result(&mut conn) + .get_result(conn) .await .unwrap(); + let address = rand::random::<[u8; 20]>().to_vec(); let wallet_id: models::EvmWalletId = insert_into(schema::evm_wallet::table) .values(( - schema::evm_wallet::address.eq(vec![0u8; 20]), + schema::evm_wallet::address.eq(address.clone()), schema::evm_wallet::aead_encrypted_id.eq(aead_id), )) .returning(schema::evm_wallet::id) - .get_result(&mut conn) + .get_result(conn) .await .unwrap(); - let metadata_id: i32 = insert_into(schema::client_metadata::table) + (wallet_id, address) + } + + async fn seed_client_metadata(conn: &mut db::DatabaseConnection) -> i32 { + insert_into(schema::client_metadata::table) .values(schema::client_metadata::name.eq("test")) .returning(schema::client_metadata::id) - .get_result(&mut conn) + .get_result(conn) .await - .unwrap(); + .unwrap() + } - let first_client: i32 = insert_into(schema::program_client::table) + /// Inserts a `program_client` row under the given `client_metadata` row, keyed by its + /// own random public key. + async fn seed_client(conn: &mut db::DatabaseConnection, metadata_id: i32) -> i32 { + insert_into(schema::program_client::table) .values(( - schema::program_client::public_key.eq(vec![1u8; 32]), + schema::program_client::public_key.eq(rand::random::<[u8; 32]>().to_vec()), schema::program_client::metadata_id.eq(metadata_id), )) .returning(schema::program_client::id) - .get_result(&mut conn) + .get_result(conn) .await - .unwrap(); + .unwrap() + } - let second_client: i32 = insert_into(schema::program_client::table) - .values(( - schema::program_client::public_key.eq(vec![2u8; 32]), - schema::program_client::metadata_id.eq(metadata_id), - )) - .returning(schema::program_client::id) - .get_result(&mut conn) - .await - .unwrap(); - - let first_access: i32 = insert_into(schema::evm_wallet_access::table) + /// Inserts an access row directly, for fixtures that need one to already exist. + /// Production code grants access through [`grant_wallet_access`]. + async fn insert_wallet_access( + conn: &mut db::DatabaseConnection, + wallet_id: models::EvmWalletId, + client_id: i32, + ) -> i32 { + insert_into(schema::evm_wallet_access::table) .values(( schema::evm_wallet_access::wallet_id.eq(wallet_id), - schema::evm_wallet_access::client_id.eq(first_client), + schema::evm_wallet_access::client_id.eq(client_id), )) .returning(schema::evm_wallet_access::id) - .get_result(&mut conn) + .get_result(conn) .await - .unwrap(); + .unwrap() + } - let _second_access: i32 = insert_into(schema::evm_wallet_access::table) - .values(( - schema::evm_wallet_access::wallet_id.eq(wallet_id), - schema::evm_wallet_access::client_id.eq(second_client), - )) - .returning(schema::evm_wallet_access::id) - .get_result(&mut conn) - .await - .unwrap(); + /// Two clients share one wallet. Revoking one access row must leave the other alone, + /// and must mark the row revoked rather than deleting it. + #[tokio::test] + async fn revoking_one_access_leaves_the_other_client_alone() { + let pool = db::create_test_pool().await; + let mut conn = pool.get().await.unwrap(); + + let (wallet_id, _address) = seed_wallet(&mut conn).await; + let metadata_id = seed_client_metadata(&mut conn).await; + let first_client = seed_client(&mut conn, metadata_id).await; + let second_client = seed_client(&mut conn, metadata_id).await; + + let first_access = insert_wallet_access(&mut conn, wallet_id, first_client).await; + let _second_access = insert_wallet_access(&mut conn, wallet_id, second_client).await; let removed = revoke_wallet_access(&mut conn, &[first_access]) .await .unwrap(); assert_eq!(removed, 1); - let survivors: Vec = schema::evm_wallet_access::table + let active: Vec = schema::evm_wallet_access::table + .filter(schema::evm_wallet_access::revoked_at.is_null()) .select(schema::evm_wallet_access::client_id) .load(&mut conn) .await .unwrap(); assert_eq!( - survivors, + active, vec![second_client], "revoking one access row removed another client's access" ); + + let total: i64 = schema::evm_wallet_access::table + .count() + .get_result(&mut conn) + .await + .unwrap(); + assert_eq!( + total, 2, + "revoking an access row must mark it revoked, not delete it" + ); + } + + /// The bug this round fixes: once an access has been used for a grant, a signed + /// transaction, or a proposed persistent grant, three tables reference + /// `evm_wallet_access` `on delete restrict`, so deleting the row is no longer possible + /// once foreign keys are enforced. Revoking must still succeed by marking it revoked. + #[tokio::test] + async fn revoking_an_access_with_grant_log_and_proposal_succeeds() { + use crate::db::proposal::{Proposal as _, persistent_grant, persistent_grant::PersistentGrant}; + + let pool = db::create_test_pool().await; + let mut conn = pool.get().await.unwrap(); + + let (wallet_id, _address) = seed_wallet(&mut conn).await; + let metadata_id = seed_client_metadata(&mut conn).await; + let client_id = seed_client(&mut conn, metadata_id).await; + let access_id = insert_wallet_access(&mut conn, wallet_id, client_id).await; + + // A grant against this access... + let grant_id: i32 = insert_into(schema::evm_basic_grant::table) + .values(models::NewEvmBasicGrant { + wallet_access_id: access_id, + chain_id: 1u64.into(), + valid_from: None, + valid_until: None, + max_gas_fee_per_gas: None, + max_priority_fee_per_gas: None, + rate_limit_count: None, + rate_limit_window_secs: None, + revoked_at: None, + }) + .returning(schema::evm_basic_grant::id) + .get_result(&mut conn) + .await + .unwrap(); + + // ...a signed transaction against that grant... + insert_into(schema::evm_transaction_log::table) + .values(models::NewEvmTransactionLog { + grant_id, + wallet_access_id: access_id, + chain_id: 1u64.into(), + eth_value: vec![0u8; 32], + signed_at: models::SqliteTimestamp(chrono::Utc::now()), + }) + .execute(&mut conn) + .await + .unwrap(); + + // ...and a persistent-grant proposal that named this access before it was voted on. + let operator_id: models::OperatorIdentityId = + insert_into(schema::operator_identity::table) + .values(schema::operator_identity::public_key.eq(rand::random::<[u8; 32]>().to_vec())) + .returning(schema::operator_identity::id) + .get_result(&mut conn) + .await + .unwrap(); + let proposal_id: models::ProposalId = insert_into(schema::proposal::table) + .values(&models::NewProposal { + kind: db::proposal::ProposalKindTag::ApprovePersistentGrant, + initiator_id: operator_id, + expires_at: models::SqliteTimestamp(chrono::Utc::now() + chrono::Duration::days(1)), + }) + .returning(schema::proposal::id) + .get_result(&mut conn) + .await + .unwrap(); + PersistentGrant::insert( + proposal_id, + &persistent_grant::Settings { + wallet_access_id: access_id, + chain_id: 1, + valid_from_secs: None, + valid_until_secs: None, + max_gas_fee_per_gas: None, + max_priority_fee_per_gas: None, + rate_limit: None, + specific: persistent_grant::Specific::EtherTransfer { + targets: vec![[0u8; 20]], + limit: persistent_grant::VolumeLimit { + max_volume: [0u8; 32], + window_secs: 3600, + }, + }, + }, + &mut conn, + ) + .await + .unwrap(); + + // Before this round's fix, this would fail with a foreign-key violation. + let removed = revoke_wallet_access(&mut conn, &[access_id]).await.unwrap(); + assert_eq!(removed, 1); + + let revoked_at: Option = schema::evm_wallet_access::table + .find(access_id) + .select(schema::evm_wallet_access::revoked_at) + .first(&mut conn) + .await + .unwrap(); + assert!( + revoked_at.is_some(), + "the row must be marked revoked, not deleted" + ); + } + + /// A revoked access must no longer resolve through the lookup `shared_analyze_transaction` + /// and `client_sign_transaction` share -- otherwise the SDK client keeps signing after + /// the operator believes it has been cut off. + #[tokio::test] + async fn revoked_access_no_longer_authorizes_signing() { + use crate::actors::{ + GlobalActors, + evm::{EvmActor, SignTransactionError}, + vault::Vault, + }; + use alloy::{ + consensus::TxEip1559, + eips::eip2930::AccessList, + primitives::{Address, Bytes, TxKind, U256}, + }; + use kameo::actor::Spawn as _; + + let pool = db::create_test_pool().await; + let mut conn = pool.get().await.unwrap(); + + let (wallet_id, address) = seed_wallet(&mut conn).await; + let metadata_id = seed_client_metadata(&mut conn).await; + let client_id = seed_client(&mut conn, metadata_id).await; + let access_id = insert_wallet_access(&mut conn, wallet_id, client_id).await; + + revoke_wallet_access(&mut conn, &[access_id]).await.unwrap(); + drop(conn); + + let vault = Vault::spawn( + Vault::new(pool.clone(), GlobalActors::spawn_message_bus()) + .await + .unwrap(), + ); + let mut evm_actor = EvmActor::new(vault, pool.clone()); + + let transaction = TxEip1559 { + chain_id: 1, + nonce: 0, + gas_limit: 21_000, + max_fee_per_gas: 0, + max_priority_fee_per_gas: 0, + to: TxKind::Call(Address::ZERO), + value: U256::ZERO, + input: Bytes::new(), + access_list: AccessList::default(), + }; + let wallet_address = Address::from_slice(&address); + + // Both lookups resolve access the same way; both must reject the revoked row before + // ever touching the vault (neither call bootstraps one). + let analyze_result = evm_actor + .shared_analyze_transaction(client_id, wallet_address, transaction.clone()) + .await; + assert!( + matches!(analyze_result, Err(SignTransactionError::WalletNotFound)), + "a revoked access must not authorize shared_analyze_transaction: {analyze_result:?}" + ); + + let sign_result = evm_actor + .client_sign_transaction(client_id, wallet_address, transaction) + .await; + assert!( + matches!(sign_result, Err(SignTransactionError::WalletNotFound)), + "a revoked access must not authorize client_sign_transaction: {sign_result:?}" + ); + } + + /// Re-granting a revoked access must restore it rather than silently doing nothing: + /// `uniq_wallet_access` is a unique index on `(wallet_id, client_id)`, so a plain insert + /// would conflict on the revoked row forever. + #[tokio::test] + async fn regranting_a_revoked_access_restores_it() { + let pool = db::create_test_pool().await; + let mut conn = pool.get().await.unwrap(); + + let (wallet_id, _address) = seed_wallet(&mut conn).await; + let metadata_id = seed_client_metadata(&mut conn).await; + let client_id = seed_client(&mut conn, metadata_id).await; + let access_id = insert_wallet_access(&mut conn, wallet_id, client_id).await; + + revoke_wallet_access(&mut conn, &[access_id]).await.unwrap(); + + grant_wallet_access( + &mut conn, + vec![models::NewEvmWalletAccess { + wallet_id, + client_id, + }], + ) + .await + .unwrap(); + + let revoked_at: Option = schema::evm_wallet_access::table + .find(access_id) + .select(schema::evm_wallet_access::revoked_at) + .first(&mut conn) + .await + .unwrap(); + assert!( + revoked_at.is_none(), + "re-granting a revoked access must clear revoked_at" + ); + + let total: i64 = schema::evm_wallet_access::table + .count() + .get_result(&mut conn) + .await + .unwrap(); + assert_eq!( + total, 1, + "re-granting a revoked access must revive the existing row, not add a second one" + ); } }