From 4ada030af5f4957116e8b8a0ac435e0076a569bf Mon Sep 17 00:00:00 2001 From: "Max Kalashnikoff | maksy.eth" Date: Thu, 17 Oct 2024 21:51:46 +0200 Subject: [PATCH] fix(CoSigner): decoding the ABI calldata from Safe format, PCI expiration and revocation check. (#812) * fix(CoSigner): decoding the ABI calldata with packed and unpacked formats * fix: removing DynSolValue --- src/error.rs | 28 +++- src/handlers/sessions/cosign.rs | 24 ++- src/utils/permissions.rs | 22 +-- src/utils/sessions.rs | 281 ++++++++++++++------------------ 4 files changed, 180 insertions(+), 175 deletions(-) diff --git a/src/error.rs b/src/error.rs index 8657f4ce..1932b2af 100644 --- a/src/error.rs +++ b/src/error.rs @@ -194,9 +194,6 @@ pub enum RpcError { #[error("IRN client is not configured")] IrnNotConfigured, - #[error("Permission for PCI is not found: {0} {1}")] - PermissionNotFound(String, String), - #[error("Internal permissions get context error: {0}")] InternalGetSessionContextError(InternalGetSessionContextError), @@ -215,12 +212,21 @@ pub enum RpcError { #[error("Pkcs8 error: {0}")] Pkcs8Error(#[from] ethers::core::k256::pkcs8::Error), + #[error("Permission for PCI is not found: {0} {1}")] + PermissionNotFound(String, String), + #[error("Permission context was not updated yet: {0}")] PermissionContextNotUpdated(String), #[error("Permission is revoked: {0}")] RevokedPermission(String), + #[error("Permission is expired: {0}")] + PermissionExpired(String), + + #[error("Permissions set is empty")] + CoSignerEmptyPermissions, + #[error("Cosigner permission denied: {0}")] CosignerPermissionDenied(String), @@ -501,6 +507,14 @@ impl IntoResponse for RpcError { )), ) .into_response(), + Self::PermissionExpired(pci) => ( + StatusCode::BAD_REQUEST, + Json(new_error_response( + "pci".to_string(), + format!("Permission is expired: {}", pci), + )), + ) + .into_response(), Self::WrongBase64Format(e) => ( StatusCode::BAD_REQUEST, Json(new_error_response( @@ -525,6 +539,14 @@ impl IntoResponse for RpcError { )), ) .into_response(), + Self::CoSignerEmptyPermissions => ( + StatusCode::BAD_REQUEST, + Json(new_error_response( + "".to_string(), + "Permissions set is empty".to_string(), + )), + ) + .into_response(), Self::AbiDecodingError(e) => ( StatusCode::BAD_REQUEST, Json(new_error_response( diff --git a/src/handlers/sessions/cosign.rs b/src/handlers/sessions/cosign.rs index 884e3136..eba86d7c 100644 --- a/src/handlers/sessions/cosign.rs +++ b/src/handlers/sessions/cosign.rs @@ -147,10 +147,30 @@ async fn handler_internal( .add_irn_latency(irn_call_start, OperationType::Hget); let storage_permissions_item = serde_json::from_str::(&storage_permissions_item)?; - let call_data = request_payload.user_op.call_data.clone(); + // Check if the permission is revoked + if storage_permissions_item.revoked_at.is_some() { + return Err(RpcError::RevokedPermission(request_payload.pci.clone())); + } + + // Check if the permission is expired + if storage_permissions_item.expiry + < SystemTime::now() + .duration_since(SystemTime::UNIX_EPOCH) + .unwrap_or_default() + .as_secs() as usize + { + return Err(RpcError::PermissionExpired(request_payload.pci.clone())); + } + + let call_data = request_payload.user_op.call_data.clone(); // Extract the batch components - let execution_batch = extract_execution_batch_components(call_data.to_vec())?; + let execution_batch = extract_execution_batch_components(&call_data)?; + + // Check the permissions length + if storage_permissions_item.permissions.is_empty() { + return Err(RpcError::CoSignerEmptyPermissions); + } // Check permissions by types for permission in storage_permissions_item.permissions { diff --git a/src/utils/permissions.rs b/src/utils/permissions.rs index 25833a3e..58e4905e 100644 --- a/src/utils/permissions.rs +++ b/src/utils/permissions.rs @@ -2,13 +2,11 @@ use { crate::{ error::RpcError, utils::sessions::{ - extract_addresses_from_execution_batch, extract_values_from_execution_batch, + extract_addresses_from_execution_batch, extract_values_sum_from_execution_batch, + ExecutionTransaction, }, }, - ethers::{ - abi::Token, - types::{H160, U256}, - }, + alloy::primitives::{Address, U256}, serde::{Deserialize, Serialize}, serde_json::Value, strum_macros::{Display, EnumIter, EnumString}, @@ -27,7 +25,7 @@ pub enum PermissionType { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct ContractCallPermissionData { - pub address: H160, + pub address: Address, pub abi: Value, pub functions: Value, } @@ -43,10 +41,10 @@ pub struct NativeTokenTransferPermissionData { /// `contract-call` permission type check pub fn contract_call_permission_check( - execution_batch: Vec, + execution_batch: Vec, contract_call_permission_data: ContractCallPermissionData, ) -> Result<(), RpcError> { - let execution_addresses = extract_addresses_from_execution_batch(execution_batch.clone())?; + let execution_addresses = extract_addresses_from_execution_batch(execution_batch)?; let call_address = contract_call_permission_data.address; for address in execution_addresses { @@ -62,15 +60,11 @@ pub fn contract_call_permission_check( /// `native-token-transfer` permission type check pub fn native_token_transfer_permission_check( - execution_batch: Vec, + execution_batch: Vec, native_token_transfer_permission_data: NativeTokenTransferPermissionData, ) -> Result<(), RpcError> { - let execution_values = extract_values_from_execution_batch(execution_batch.clone())?; let allowance = native_token_transfer_permission_data.allowance; - // summ execution values from the execution batch and check if it is less than or equal to the allowance - let sum: U256 = execution_values - .iter() - .fold(U256::zero(), |acc, &x| acc + x); + let sum: U256 = extract_values_sum_from_execution_batch(execution_batch)?; if sum > allowance { error!( "Execution value is greater than the allowance. Execution Value: {:?}, Allowance: {:?}", diff --git a/src/utils/sessions.rs b/src/utils/sessions.rs index 3dd3e9da..9ffae72d 100644 --- a/src/utils/sessions.rs +++ b/src/utils/sessions.rs @@ -1,189 +1,158 @@ use { crate::error::RpcError, - ethers::{ - abi::{Abi, Token}, - types::{H160, U256}, + alloy::{ + primitives::{Address, Bytes, U256}, + sol, + sol_types::{SolCall, SolType}, }, + yttrium::smart_accounts::safe::Safe7579, }; +type BatchTransactionType = sol! { + (address, uint256, bytes)[] +}; + +#[derive(Clone)] +#[allow(dead_code)] +pub struct ExecutionTransaction { + address: Address, + value: U256, + call_data: Bytes, +} + // Extract the execution batch components from the calldata // from bundler's `execute` function ABI pub fn extract_execution_batch_components( - call_data_bytes: Vec, -) -> Result, RpcError> { - let execute_abi_json = r#" - [ - { - "type": "function", - "name": "execute", - "inputs": [ - { - "name": "execMode", - "type": "bytes32", - "internalType": "ExecMode" - }, - { - "name": "executionCalldata", - "type": "bytes", - "internalType": "bytes" - } - ], - "outputs": [], - "stateMutability": "payable" - } - ] - "#; - - let execute_abi: Abi = serde_json::from_str(execute_abi_json)?; - let execute_function = execute_abi.function("execute").map_err(|e| { - RpcError::AbiDecodingError(format!("Failed to parse execute function: {}", e)) + call_data_bytes: &[u8], +) -> Result, RpcError> { + // Decode the calldata into Safe7579::executeCall + let execute_call = Safe7579::executeCall::abi_decode(call_data_bytes, true).map_err(|e| { + RpcError::AbiDecodingError(format!("Failed to decode executeCall: {:?}", e)) })?; - // Verify the function selector - let function_selector = &call_data_bytes[0..4]; - let expected_selector = execute_function.short_signature(); - - if function_selector != expected_selector { - return Err(RpcError::AbiDecodingError( - "Function selector does not match `execute`".into(), - )); - } - - // Decode the calldata - let decoded_params = execute_function - .decode_input(&call_data_bytes[4..]) - .map_err(|e| RpcError::AbiDecodingError(format!("Failed to decode calldata: {}", e)))?; - - // Extract executionCalldata - let execution_calldata = match &decoded_params[1] { - Token::Bytes(bytes) => bytes, - _ => { + // Access the mode bytes directly + let mode_bytes = &execute_call.mode; + + // Manually parse the mode_bytes + let batch_flag = mode_bytes[0]; + let _exec_type = mode_bytes[1]; + let is_batch = batch_flag != 0; + + if is_batch { + let execution_calldata_bytes = &execute_call.executionCalldata; + let batch_transactions: Vec<(Address, U256, Bytes)> = + BatchTransactionType::abi_decode(execution_calldata_bytes, true).map_err(|e| { + RpcError::AbiDecodingError(format!( + "Failed to decode batch transactions ABI type: {:?}", + e + )) + })?; + + let execution_batch = batch_transactions + .into_iter() + .map(|(target, value, call_data)| ExecutionTransaction { + address: target, + value, + call_data, + }) + .collect(); + + Ok(execution_batch) + } else { + // Single transaction: executionCalldata is packed-encoded + let execution_calldata_bytes = &execute_call.executionCalldata; + if execution_calldata_bytes.len() < 20 + 32 { return Err(RpcError::AbiDecodingError( - "executionCalldata is not bytes".into(), - )) + "executionCalldata is too short for a single transaction".into(), + )); } - }; - // Define the executionCalldata ABI - let execution_calldata_abi_json = r#" - [ - { - "type": "function", - "name": "decodeExecutionCalldata", - "inputs": [ - { - "name": "executionBatch", - "type": "tuple[]", - "components": [ - { - "name": "target", - "type": "address" - }, - { - "name": "value", - "type": "uint256" - }, - { - "name": "callData", - "type": "bytes" - } - ] - } - ], - "outputs": [] - } - ] - "#; - - // Parse the executionCalldata ABI - let execution_calldata_abi: Abi = serde_json::from_str(execution_calldata_abi_json)?; - let decode_function = execution_calldata_abi - .function("decodeExecutionCalldata") - .map_err(|e| { - RpcError::AbiDecodingError(format!( - "Failed to parse decodeExecutionCalldata function: {}", - e - )) - })?; + // Manually parse the packed-encoded single transaction + // Address (20 bytes) + let address = Address::from_slice(&execution_calldata_bytes[0..20]); - // Decode executionCalldata - let tokens = decode_function - .decode_input(execution_calldata) - .map_err(|e| { - RpcError::AbiDecodingError(format!("Failed to decode executionCalldata: {}", e)) + // Uint256 value (32 bytes) + let value_bytes = &execution_calldata_bytes[20..52]; + let value_bytes_array: [u8; 32] = value_bytes.try_into().map_err(|_| { + RpcError::AbiDecodingError("Invalid value bytes length in execution calldata".into()) })?; - // Extract the execution batch - let execution_batch = match &tokens[0] { - Token::Array(arr) => arr, - _ => { - return Err(RpcError::AbiDecodingError( - "Expected an array for executionBatch".into(), - )) - } - }; + // Specify the const generic parameter <32> + let value = U256::from_be_bytes::<32>(value_bytes_array); - if execution_batch.is_empty() { - return Err(RpcError::AbiDecodingError("executionBatch is empty".into())); - } + // callData (remaining bytes) + let call_data = execution_calldata_bytes[52..].to_vec(); + + let transaction = ExecutionTransaction { + address, + value, + call_data: call_data.into(), + }; - Ok(execution_batch.clone()) + Ok(vec![transaction]) + } } /// Extract addresses from the bundler's execute calldata execution batch pub fn extract_addresses_from_execution_batch( - execution_batch: Vec, -) -> Result, RpcError> { + execution_batch: Vec, +) -> Result, RpcError> { let mut targets = Vec::with_capacity(execution_batch.len()); for tx in execution_batch { - let tx = match &tx { - Token::Tuple(tuple) => tuple, - _ => { - return Err(RpcError::AbiDecodingError( - "Expected a tuple for execution batch transaction".into(), - )) - } - }; - let target = match &tx[0] { - Token::Address(addr) => *addr, - _ => { - return Err(RpcError::AbiDecodingError( - "Expected address as a first field for target in the execution batch item" - .into(), - )) - } - }; - targets.push(target); + targets.push(tx.address); } - Ok(targets) } -/// Exract values from the bundler's execute calldata execution batch -pub fn extract_values_from_execution_batch( - execution_batch: Vec, -) -> Result, RpcError> { - let mut values = Vec::with_capacity(execution_batch.len()); +/// Exract sum of values from the bundler's execute calldata execution batch +pub fn extract_values_sum_from_execution_batch( + execution_batch: Vec, +) -> Result { + let mut values_vec = Vec::with_capacity(execution_batch.len()); for tx in execution_batch { - let tx = match &tx { - Token::Tuple(tuple) => tuple, - _ => { - return Err(RpcError::AbiDecodingError( - "Expected a tuple for execution batch transaction".into(), - )) - } - }; + values_vec.push(tx.value); + } + // summ execution values from the execution batch and check if it is less than or equal to the allowance + let sum: U256 = values_vec.iter().fold(U256::ZERO, |acc, &x| acc + x); + Ok(sum) +} - let value = match &tx[1] { - Token::Uint(value) => *value, - _ => { - return Err(RpcError::AbiDecodingError( - "Expected value as a second field for value in the execution batch item".into(), - )) - } - }; +#[cfg(test)] +mod tests { + use { + super::*, + alloy::primitives::address, + yttrium::{smart_accounts::safe::get_call_data, transaction::Transaction}, + }; - values.push(value); + #[test] + // Check for the packed calldata format for a single transaction + fn single_execution_call_data_value() { + let encoded_data = get_call_data(vec![Transaction { + to: address!("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), + value: U256::from(1010101), + data: Bytes::new(), + }]); + let decoded_data = extract_execution_batch_components(&encoded_data).unwrap(); + assert_eq!(decoded_data.len(), 1); + } + + #[test] + // Check for the regular calldata format for multiple transactions + fn multiple_execution_call_data_value() { + let encoded_data = get_call_data(vec![ + Transaction { + to: address!("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), + value: U256::from(1010101), + data: Bytes::new(), + }, + Transaction { + to: address!("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab"), + value: U256::from(2020202), + data: Bytes::new(), + }, + ]); + let decoded_data = extract_execution_batch_components(&encoded_data).unwrap(); + assert_eq!(decoded_data.len(), 2); } - Ok(values) }