From 6cea557c9e28767aaebe376e635bfacecd4bc4bc Mon Sep 17 00:00:00 2001 From: Chris Smith <1979423+chris13524@users.noreply.github.com> Date: Wed, 2 Oct 2024 13:00:55 -0400 Subject: [PATCH] fix: Safe bricking (#33) * chore: refactor 3 * fix: bricked account --- .../bundler/models/user_operation_receipt.rs | 7 +- crates/yttrium/src/chain.rs | 24 +- crates/yttrium/src/smart_accounts/safe.rs | 10 +- .../simple_account/sender_address.rs | 2 +- crates/yttrium/src/transaction/send.rs | 1 + .../yttrium/src/transaction/send/safe_test.rs | 219 +++++++++++++----- .../transaction/send/simple_account_test.rs | 2 +- 7 files changed, 186 insertions(+), 79 deletions(-) diff --git a/crates/yttrium/src/bundler/models/user_operation_receipt.rs b/crates/yttrium/src/bundler/models/user_operation_receipt.rs index 834f038..234d7c2 100644 --- a/crates/yttrium/src/bundler/models/user_operation_receipt.rs +++ b/crates/yttrium/src/bundler/models/user_operation_receipt.rs @@ -1,4 +1,4 @@ -use alloy::primitives::Address; +use alloy::primitives::{b256, Address, B256}; use serde::{Deserialize, Serialize}; #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -19,10 +19,11 @@ pub struct UserOperationReceiptReceipt { pub effective_gas_price: String, } +// TODO replace with alloy's UserOperationReceipt #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct UserOperationReceipt { - pub user_op_hash: String, + pub user_op_hash: B256, pub entry_point: Address, pub sender: Address, pub nonce: String, @@ -38,7 +39,7 @@ pub struct UserOperationReceipt { impl UserOperationReceipt { pub fn mock() -> Self { UserOperationReceipt { - user_op_hash: "0x93c06f3f5909cc2b192713ed9bf93e3e1fde4b22fcd2466304fa404f9b80ff90".to_string(), + user_op_hash: b256!("93c06f3f5909cc2b192713ed9bf93e3e1fde4b22fcd2466304fa404f9b80ff90"), entry_point: "0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789" .parse() .unwrap(), diff --git a/crates/yttrium/src/chain.rs b/crates/yttrium/src/chain.rs index f6e7895..0d6371c 100644 --- a/crates/yttrium/src/chain.rs +++ b/crates/yttrium/src/chain.rs @@ -73,58 +73,46 @@ impl fmt::Display for ChainId { pub struct Chain { pub id: ChainId, pub entry_point_version: EntryPointVersion, - pub name: &'static str, } impl Chain { - pub fn new( - id: ChainId, - entry_point_version: EntryPointVersion, - name: &'static str, - ) -> Self { - Self { id, entry_point_version, name } + pub fn new(id: ChainId, entry_point_version: EntryPointVersion) -> Self { + Self { id, entry_point_version } } pub const ETHEREUM_MAINNET_V07: Self = Self { id: ChainId::ETHEREUM_MAINNET, entry_point_version: EntryPointVersion::V07, - name: "Ethereum Mainnet", }; pub const ETHEREUM_MAINNET_V06: Self = Self { id: ChainId::ETHEREUM_MAINNET, entry_point_version: EntryPointVersion::V06, - name: "Ethereum Mainnet", }; pub const ETHEREUM_SEPOLIA_V07: Self = Self { id: ChainId::ETHEREUM_SEPOLIA, entry_point_version: EntryPointVersion::V07, - name: "Ethereum Sepolia", }; pub const BASE_SEPOLIA_V07: Self = Self { id: ChainId::BASE_SEPOLIA, entry_point_version: EntryPointVersion::V07, - name: "Base Sepolia", }; pub const ETHEREUM_SEPOLIA_V06: Self = Self { id: ChainId::ETHEREUM_SEPOLIA, entry_point_version: EntryPointVersion::V06, - name: "Ethereum Sepolia", }; pub const LOCAL_ETHEREUM_SEPOLIA_V07: Self = Self { id: ChainId::LOCAL_FOUNDRY_ETHEREUM_SEPOLIA, entry_point_version: EntryPointVersion::V07, - name: "Local Ethereum Sepolia", }; pub const LOCAL_ETHEREUM_SEPOLIA_V06: Self = Self { id: ChainId::LOCAL_FOUNDRY_ETHEREUM_SEPOLIA, entry_point_version: EntryPointVersion::V06, - name: "Local Ethereum Sepolia", }; } @@ -143,16 +131,12 @@ impl Chain { impl From for Chain { fn from(chain_id: ChainId) -> Self { - Self { - id: chain_id, - entry_point_version: EntryPointVersion::V07, - name: "", - } + Self { id: chain_id, entry_point_version: EntryPointVersion::V07 } } } impl fmt::Display for Chain { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{} ({})", self.name, self.id) + write!(f, "{}", self.id) } } diff --git a/crates/yttrium/src/smart_accounts/safe.rs b/crates/yttrium/src/smart_accounts/safe.rs index 4f9259e..551975e 100644 --- a/crates/yttrium/src/smart_accounts/safe.rs +++ b/crates/yttrium/src/smart_accounts/safe.rs @@ -196,14 +196,20 @@ pub async fn get_account_address( } pub fn get_call_data(execution_calldata: Vec) -> Bytes { + get_call_data_with_try(execution_calldata, false) +} + +pub fn get_call_data_with_try( + execution_calldata: Vec, + exec_type: bool, +) -> Bytes { let batch = execution_calldata.len() != 1; - let revert_on_error = false; let selector = [0u8; 4]; let context = [0u8; 22]; let mode = DynSolValue::Tuple(vec![ DynSolValue::Uint(Uint::from(if batch { 0x01 } else { 0x00 }), 8), // DelegateCall is 0xFF - DynSolValue::Uint(Uint::from(revert_on_error as u8), 8), + DynSolValue::Uint(Uint::from(exec_type as u8), 8), // revertOnError in permissionless DynSolValue::Bytes(vec![0u8; 4]), DynSolValue::Bytes(selector.to_vec()), DynSolValue::Bytes(context.to_vec()), diff --git a/crates/yttrium/src/smart_accounts/simple_account/sender_address.rs b/crates/yttrium/src/smart_accounts/simple_account/sender_address.rs index df96dc3..a1c1f34 100644 --- a/crates/yttrium/src/smart_accounts/simple_account/sender_address.rs +++ b/crates/yttrium/src/smart_accounts/simple_account/sender_address.rs @@ -26,7 +26,7 @@ pub async fn get_sender_address_with_signer( let rpc_base_url = config.clone().endpoints.rpc.base_url; let chain_id = ChainId::new_eip155(chain_id); - let chain = crate::chain::Chain::new(chain_id, EntryPointVersion::V07, ""); + let chain = crate::chain::Chain::new(chain_id, EntryPointVersion::V07); let entry_point_config = chain.entry_point_config(); diff --git a/crates/yttrium/src/transaction/send.rs b/crates/yttrium/src/transaction/send.rs index c62e104..9d07a13 100644 --- a/crates/yttrium/src/transaction/send.rs +++ b/crates/yttrium/src/transaction/send.rs @@ -96,6 +96,7 @@ pub async fn send_transaction_with_private_key_signer( config, ) .await? + .user_op_hash } else { send_transaction_with_signer(transaction, config, chain_id, signer) .await? diff --git a/crates/yttrium/src/transaction/send/safe_test.rs b/crates/yttrium/src/transaction/send/safe_test.rs index 954c45a..ea357cf 100644 --- a/crates/yttrium/src/transaction/send/safe_test.rs +++ b/crates/yttrium/src/transaction/send/safe_test.rs @@ -2,19 +2,23 @@ use crate::{ bundler::{ client::BundlerClient, config::BundlerConfig, + models::user_operation_receipt::UserOperationReceipt, pimlico::{ client::BundlerClient as PimlicoBundlerClient, paymaster::client::PaymasterClient, }, }, + chain::ChainId, config::Config, + entry_point::EntryPointVersion, smart_accounts::{ account_address::AccountAddress, nonce::get_nonce, safe::{ - factory_data, get_account_address, get_call_data, Owners, - Safe7579Launchpad, DUMMY_SIGNATURE, SAFE_4337_MODULE_ADDRESS, - SAFE_ERC_7579_LAUNCHPAD_ADDRESS, SAFE_PROXY_FACTORY_ADDRESS, + factory_data, get_account_address, get_call_data, + get_call_data_with_try, Owners, Safe7579Launchpad, DUMMY_SIGNATURE, + SAFE_4337_MODULE_ADDRESS, SAFE_ERC_7579_LAUNCHPAD_ADDRESS, + SAFE_PROXY_FACTORY_ADDRESS, SEPOLIA_SAFE_ERC_7579_SINGLETON_ADDRESS, }, simple_account::factory::FactoryAddress, @@ -25,7 +29,7 @@ use crate::{ use alloy::{ dyn_abi::{DynSolValue, Eip712Domain}, network::Ethereum, - primitives::{aliases::U48, Address, Bytes, Uint, B256, U128, U256}, + primitives::{aliases::U48, Address, Bytes, Uint, U128, U160, U256}, providers::{Provider, ReqwestProvider}, signers::{k256::ecdsa::SigningKey, local::LocalSigner, SignerSync}, sol, @@ -87,28 +91,30 @@ pub async fn send_transaction( address: Option, authorization_list: Option>, config: Config, -) -> eyre::Result { - let bundler_base_url = config.endpoints.bundler.base_url; - let paymaster_base_url = config.endpoints.paymaster.base_url; +) -> eyre::Result { + let bundler_client = BundlerClient::new(BundlerConfig::new( + config.endpoints.bundler.base_url.clone(), + )); - let bundler_client = - BundlerClient::new(BundlerConfig::new(bundler_base_url.clone())); + let pimlico_client: PimlicoBundlerClient = PimlicoBundlerClient::new( + BundlerConfig::new(config.endpoints.bundler.base_url.clone()), + ); - let pimlico_client: PimlicoBundlerClient = - PimlicoBundlerClient::new(BundlerConfig::new(bundler_base_url.clone())); + let provider = ReqwestProvider::::new_http( + config.endpoints.rpc.base_url.parse()?, + ); - let chain = crate::chain::Chain::ETHEREUM_SEPOLIA_V07; + let chain_id = provider.get_chain_id().await?; + let chain = crate::chain::Chain::new( + ChainId::new_eip155(chain_id), + EntryPointVersion::V07, + ); let entry_point_config = chain.entry_point_config(); let chain_id = chain.id.eip155_chain_id(); let entry_point_address = entry_point_config.address(); - let rpc_url = config.endpoints.rpc.base_url; - - let rpc_url: reqwest::Url = rpc_url.parse()?; - let provider = ReqwestProvider::::new_http(rpc_url); - let safe_factory_address_primitives: Address = SAFE_PROXY_FACTORY_ADDRESS; let safe_factory_address = FactoryAddress::new(safe_factory_address_primitives); @@ -122,15 +128,27 @@ pub async fn send_transaction( let account_address = if let Some(address) = address { address } else { contract_address }; - let call_data = get_call_data(execution_calldata); - let deployed = provider.get_code_at(account_address.into()).await?.len() > 0; println!("Deployed: {}", deployed); // permissionless: signerToSafeSmartAccount -> encodeCallData - let call_data = if deployed { - call_data + let call_data = if deployed + && provider + .get_storage_at(account_address.into(), Uint::from(0)) + .await? + == U256::from(U160::from_be_bytes( + SEPOLIA_SAFE_ERC_7579_SINGLETON_ADDRESS.into_array(), + )) { + get_call_data(execution_calldata) } else { + // Note about using `try` mode for get_call_data & needing to check + // storage above. This is due to an issue in the Safe7579Launchpad + // contract where a revert will cause the Safe7579Launchpad::setupSafe + // to be reverted too. This leaves the account in a bricked state. To + // workaround, we use the `try` mode to ensure that the reverted + // execution does not revert the setupSafe call too. This unfortunately + // has the side-effect that the UserOp will be successful, which is + // misleading. Safe7579Launchpad::setupSafeCall { initData: Safe7579Launchpad::InitData { singleton: SEPOLIA_SAFE_ERC_7579_SINGLETON_ADDRESS, @@ -148,7 +166,7 @@ pub async fn send_transaction( .abi_encode() .into(), safe7579: SAFE_4337_MODULE_ADDRESS, - callData: call_data, + callData: get_call_data_with_try(execution_calldata, true), validators: vec![], }, } @@ -184,7 +202,7 @@ pub async fn send_transaction( if let Some(authorization_list) = authorization_list { let response = reqwest::Client::new() - .post(bundler_base_url.clone()) + .post(config.endpoints.paymaster.base_url.clone()) .json(&json!({ "jsonrpc": "2.0", "id": 1, @@ -205,7 +223,7 @@ pub async fn send_transaction( let user_op = { let paymaster_client = PaymasterClient::new(BundlerConfig::new( - paymaster_base_url.clone(), + config.endpoints.paymaster.base_url.clone(), )); let sponsor_user_op_result = paymaster_client @@ -311,13 +329,13 @@ pub async fn send_transaction( user_op .paymaster_data .clone() - .unwrap_or(Bytes::new()) + .unwrap_or_default() .to_vec(), ] .concat() .into() }) - .unwrap_or(Bytes::new()), + .unwrap_or_default(), validAfter: valid_after, validUntil: valid_until, entryPoint: entry_point_address.to_address(), @@ -367,10 +385,9 @@ pub async fn send_transaction( .wait_for_user_operation_receipt(user_operation_hash) .await?; - let tx_hash = receipt.receipt.transaction_hash; println!( "SAFE UserOperation included: https://sepolia.etherscan.io/tx/{}", - tx_hash + receipt.receipt.transaction_hash ); // Some extra calls to wait for/get the actual transaction. But these @@ -388,7 +405,7 @@ pub async fn send_transaction( // provider.get_transaction_receipt(tx_hash).await?; // println!("Transaction receipt: {:?}", transaction_receipt); - Ok(user_operation_hash) + Ok(receipt) } #[cfg(test)] @@ -398,7 +415,7 @@ mod tests { use alloy::{ consensus::{SignableTransaction, TxEip7702}, network::{EthereumWallet, TransactionBuilder, TxSignerSync}, - primitives::U64, + primitives::{U160, U64}, providers::{ext::AnvilApi, PendingTransactionConfig, ProviderBuilder}, rpc::types::TransactionRequest, }; @@ -463,7 +480,7 @@ mod tests { data: Bytes::new(), }]; - let transaction_hash = send_transaction( + let receipt = send_transaction( transaction, owner.clone(), None, @@ -471,8 +488,7 @@ mod tests { config.clone(), ) .await?; - - println!("Transaction sent: {}", transaction_hash); + assert!(receipt.success); let balance = provider.get_balance(destination.address()).await?; assert_eq!(balance, Uint::from(1)); @@ -483,10 +499,9 @@ mod tests { data: Bytes::new(), }]; - let transaction_hash = + let receipt = send_transaction(transaction, owner, None, None, config).await?; - - println!("Transaction sent: {}", transaction_hash); + assert!(receipt.success); let balance = provider.get_balance(destination.address()).await?; assert_eq!(balance, Uint::from(2)); @@ -530,6 +545,110 @@ mod tests { test_send_transaction(config, faucet).await.unwrap(); } + #[tokio::test] + async fn test_send_transaction_first_reverted_local() { + let config = Config::local(); + let faucet = anvil_faucet(config.clone()).await; + test_send_transaction_first_reverted(config, faucet).await; + } + + #[tokio::test] + #[ignore = "not useful currently, can do same test locally"] + #[cfg(feature = "test_pimlico_api")] + async fn test_send_transaction_first_reverted_pimlico() { + let config = Config::pimlico(); + let faucet = pimlico_faucet(); + test_send_transaction_first_reverted(config, faucet).await; + } + + async fn test_send_transaction_first_reverted( + config: Config, + faucet: LocalSigner, + ) { + let provider = ReqwestProvider::::new_http( + config.endpoints.rpc.base_url.parse().unwrap(), + ); + + let destination = LocalSigner::random(); + let balance = + provider.get_balance(destination.address()).await.unwrap(); + assert_eq!(balance, Uint::from(0)); + + let owner = LocalSigner::random(); + let sender_address = get_account_address( + provider.clone(), + Owners { owners: vec![owner.address()], threshold: 1 }, + ) + .await; + + let transaction = vec![Transaction { + to: destination.address(), + value: Uint::from(1), + data: Bytes::new(), + }]; + + let receipt = send_transaction( + transaction, + owner.clone(), + None, + None, + config.clone(), + ) + .await + .unwrap(); + // The UserOp is successful, but the transaction actually failed. See + // note above near `Safe7579Launchpad::setupSafe` + assert!(receipt.success); + assert!( + provider.get_code_at(sender_address.into()).await.unwrap().len() + > 0 + ); + assert_eq!( + provider + .get_storage_at(sender_address.into(), Uint::from(0)) + .await + .unwrap(), + U256::from(U160::from_be_bytes( + SEPOLIA_SAFE_ERC_7579_SINGLETON_ADDRESS.into_array() + )) + ); + + let balance = + provider.get_balance(destination.address()).await.unwrap(); + assert_eq!(balance, Uint::from(0)); + use_faucet( + provider.clone(), + faucet.clone(), + U256::from(1), + sender_address.into(), + ) + .await; + + let transaction = vec![Transaction { + to: destination.address(), + value: Uint::from(1), + data: Bytes::new(), + }]; + + let receipt = send_transaction(transaction, owner, None, None, config) + .await + .unwrap(); + assert!(receipt.success); + assert_eq!( + provider + .get_storage_at(sender_address.into(), Uint::from(0)) + .await + .unwrap(), + U256::from(U160::from_be_bytes( + SEPOLIA_SAFE_ERC_7579_SINGLETON_ADDRESS.into_array() + )) + ); + + let balance = + provider.get_balance(destination.address()).await.unwrap(); + assert_eq!(balance, Uint::from(1)); + } + #[tokio::test] async fn test_send_transaction_just_deploy() -> eyre::Result<()> { let config = Config::local(); @@ -561,7 +680,7 @@ mod tests { let transaction = vec![]; - let transaction_hash = send_transaction( + let receipt = send_transaction( transaction, owner.clone(), None, @@ -569,8 +688,7 @@ mod tests { config.clone(), ) .await?; - - println!("Transaction sent: {}", transaction_hash); + assert!(receipt.success); assert!(!provider .get_code_at(sender_address.into()) @@ -621,7 +739,7 @@ mod tests { }, ]; - let transaction_hash = send_transaction( + let receipt = send_transaction( transaction, owner.clone(), None, @@ -629,8 +747,7 @@ mod tests { config.clone(), ) .await?; - - println!("Transaction sent: {}", transaction_hash); + assert!(receipt.success); assert_eq!( provider.get_balance(destination1.address()).await?, @@ -698,7 +815,7 @@ mod tests { }]; let transaction = vec![]; - let transaction_hash = send_transaction( + let receipt = send_transaction( transaction, owner.clone(), Some(authority.address().into()), @@ -707,7 +824,7 @@ mod tests { config.clone(), ) .await?; - println!("Transaction sent: {}", transaction_hash); + assert!(receipt.success); println!("contract address: {}", contract_address); println!( @@ -725,7 +842,7 @@ mod tests { data: Bytes::new(), }]; - let transaction_hash = send_transaction( + let receipt = send_transaction( transaction, owner, Some(authority.address().into()), @@ -735,8 +852,7 @@ mod tests { config, ) .await?; - - println!("Transaction sent: {}", transaction_hash); + assert!(receipt.success); let balance = provider.get_balance(destination.address()).await?; assert_eq!(balance, Uint::from(1)); @@ -764,7 +880,7 @@ mod tests { .await; let transaction = vec![]; - let transaction_hash = send_transaction( + let receipt = send_transaction( transaction, owner.clone(), None, @@ -772,7 +888,7 @@ mod tests { config.clone(), ) .await?; - println!("Transaction sent: {}", transaction_hash); + assert!(receipt.success); let authority = LocalSigner::random(); provider.anvil_set_balance(authority.address(), U256::MAX).await?; @@ -871,7 +987,7 @@ mod tests { data: Bytes::new(), }]; - let transaction_hash = send_transaction( + let receipt = send_transaction( transaction, owner, Some(authority.address().into()), @@ -879,8 +995,7 @@ mod tests { config, ) .await?; - - println!("Transaction sent: {}", transaction_hash); + assert!(receipt.success); let balance = provider.get_balance(destination.address()).await?; assert_eq!(balance, Uint::from(2)); diff --git a/crates/yttrium/src/transaction/send/simple_account_test.rs b/crates/yttrium/src/transaction/send/simple_account_test.rs index 67c2e23..927bc1c 100644 --- a/crates/yttrium/src/transaction/send/simple_account_test.rs +++ b/crates/yttrium/src/transaction/send/simple_account_test.rs @@ -81,7 +81,7 @@ pub async fn send_transaction_with_signer( use crate::entry_point::EntryPointVersion; let chain_id = ChainId::new_eip155(chain_id); - let chain = crate::chain::Chain::new(chain_id, EntryPointVersion::V07, ""); + let chain = crate::chain::Chain::new(chain_id, EntryPointVersion::V07); let entry_point_config = chain.entry_point_config(); let entry_point_address = entry_point_config.address();