From e49b325ed9dd41d1c0f2a9a631405aa4a8fcb563 Mon Sep 17 00:00:00 2001 From: Felix Leupold Date: Tue, 6 Feb 2024 10:40:11 +0100 Subject: [PATCH] Reject orders with unknown preimages (#2360) # Description Now that we have identified the remainder of legit use cases which don't provide us a valid pre image for their app data (we will assume an empty app data for those), we can now start rejecting any new integrations/orders that are trying to place orders with dummy or unknown `appData`. This is important in order to have a reasonable assurance that we are aware in case a user places an order with important metadata (e.g. pre/post interactions) and can't later on be surprised by claims that we ignored part of the intent. # Changes - [x] Return an error if app data pre-image is unknown ## How to test Adjusted the existing e2e test ## Related Issues I could swear there was a cleanup issue somewhere but I cannot find it --- crates/e2e/tests/e2e/app_data.rs | 9 ++-- crates/e2e/tests/e2e/app_data_signer.rs | 17 ------- crates/e2e/tests/e2e/order_cancellation.rs | 7 ++- crates/e2e/tests/e2e/smart_contract_orders.rs | 5 +-- crates/shared/src/order_validation.rs | 45 +++++++++++++++++-- 5 files changed, 51 insertions(+), 32 deletions(-) diff --git a/crates/e2e/tests/e2e/app_data.rs b/crates/e2e/tests/e2e/app_data.rs index f2ba729a1f..e8e8515d50 100644 --- a/crates/e2e/tests/e2e/app_data.rs +++ b/crates/e2e/tests/e2e/app_data.rs @@ -60,21 +60,18 @@ async fn app_data(web3: Web3) { let services = Services::new(onchain.contracts()).await; services.start_protocol(solver).await; - // Temporarily custom hashes are still accepted. + // Unknown hashes are not accepted. let order0 = create_order(OrderCreationAppData::Hash { hash: AppDataHash([1; 32]), }); - let uid = services.create_order(&order0).await.unwrap(); - let order0_ = services.get_order(&uid).await.unwrap(); - assert_eq!(order0_.data.app_data, AppDataHash([1; 32])); - assert_eq!(order0_.metadata.full_app_data, None); - let err = services .get_app_data(AppDataHash([1; 32])) .await .unwrap_err(); assert_eq!(err.0, StatusCode::NOT_FOUND); + assert!(services.create_order(&order0).await.is_err()); + // hash matches let app_data = "{}"; let app_data_hash = AppDataHash(app_data_hash::hash_full_app_data(app_data.as_bytes())); diff --git a/crates/e2e/tests/e2e/app_data_signer.rs b/crates/e2e/tests/e2e/app_data_signer.rs index eb88015864..7983eac22e 100644 --- a/crates/e2e/tests/e2e/app_data_signer.rs +++ b/crates/e2e/tests/e2e/app_data_signer.rs @@ -5,11 +5,9 @@ use { }, ethcontract::{prelude::U256, H160}, model::{ - app_data::AppDataHash, order::{OrderCreation, OrderCreationAppData, OrderKind}, signature::EcdsaSigningScheme, }, - reqwest::StatusCode, secp256k1::SecretKey, shared::ethrpc::Web3, web3::signing::SecretKeyRef, @@ -68,21 +66,6 @@ async fn order_creation_checks_metadata_signer(web3: Web3) { let services = Services::new(onchain.contracts()).await; services.start_protocol(solver).await; - // Accepted: custom hashes that aren't found in the DB. - let custom_hash_app_data = AppDataHash([1; 32]); - let order0 = sign( - create_order(OrderCreationAppData::Hash { - hash: custom_hash_app_data, - }), - &trader, - ); - assert!(matches!( - services.get_app_data(custom_hash_app_data).await, - Err((StatusCode::NOT_FOUND, ..)) - )); - let uid = services.create_order(&order0).await.unwrap(); - assert!(matches!(services.get_order(&uid).await, Ok(..))); - // Rejected: app data with different signer. let full_app_data = full_app_data_with_signer(adversary.address()); let order1 = sign(create_order(full_app_data), &trader); diff --git a/crates/e2e/tests/e2e/order_cancellation.rs b/crates/e2e/tests/e2e/order_cancellation.rs index ce48ee67af..1bffc41b69 100644 --- a/crates/e2e/tests/e2e/order_cancellation.rs +++ b/crates/e2e/tests/e2e/order_cancellation.rs @@ -3,12 +3,12 @@ use { e2e::{setup::*, tx}, ethcontract::prelude::U256, model::{ - app_data::AppDataHash, order::{ CancellationPayload, OrderCancellation, OrderCancellations, OrderCreation, + OrderCreationAppData, OrderStatus, OrderUid, SignedOrderCancellations, @@ -18,6 +18,7 @@ use { }, number::nonzero::U256 as NonZeroU256, secp256k1::SecretKey, + serde_json::json, shared::ethrpc::Web3, web3::signing::SecretKeyRef, }; @@ -82,7 +83,9 @@ async fn order_cancellation(web3: Web3) { value: NonZeroU256::try_from(to_wei(1)).unwrap(), }, }, - app_data: AppDataHash([salt; 32]).into(), + app_data: OrderCreationAppData::Full { + full: json!({"salt": salt}).to_string(), + }, ..Default::default() }; async move { diff --git a/crates/e2e/tests/e2e/smart_contract_orders.rs b/crates/e2e/tests/e2e/smart_contract_orders.rs index 5a8d769777..30d000d192 100644 --- a/crates/e2e/tests/e2e/smart_contract_orders.rs +++ b/crates/e2e/tests/e2e/smart_contract_orders.rs @@ -2,7 +2,6 @@ use { e2e::setup::{safe::Safe, *}, ethcontract::{Bytes, H160, U256}, model::{ - app_data::AppDataHash, order::{OrderCreation, OrderCreationAppData, OrderKind, OrderStatus, OrderUid}, signature::Signature, }, @@ -73,8 +72,8 @@ async fn smart_contract_orders(web3: Web3) { ..order_template.clone() }, OrderCreation { - app_data: OrderCreationAppData::Hash { - hash: AppDataHash([1; 32]), + app_data: OrderCreationAppData::Full { + full: "{\"salt\": \"second\"}".to_string(), }, from: Some(safe.address()), signature: Signature::PreSign, diff --git a/crates/shared/src/order_validation.rs b/crates/shared/src/order_validation.rs index 7d0ccfc23b..7f70efdbb2 100644 --- a/crates/shared/src/order_validation.rs +++ b/crates/shared/src/order_validation.rs @@ -1,7 +1,7 @@ use { crate::{ account_balances::{self, BalanceFetching, TransferSimulationError}, - app_data::{ProtocolAppData, ValidatedAppData}, + app_data::ValidatedAppData, bad_token::{BadTokenDetecting, TokenQuality}, code_fetching::CodeFetching, order_quoting::{ @@ -483,8 +483,10 @@ impl OrderValidating for OrderValidator { let protocol = if let Some(full) = full_app_data_override { validate(full)?.protocol } else { - tracing::warn!(hash = hex::encode(hash.0), "Unknown appData pre-image"); - ProtocolAppData::default() + return Err(AppDataValidationError::Invalid(anyhow!( + "Unknown pre-image for app data hash {:?}", + hash, + ))); }; ValidatedAppData { @@ -1263,6 +1265,9 @@ mod tests { sell_amount: U256::from(1), fee_amount: U256::from(1), signature: Signature::Eip712(EcdsaSignature::non_zero()), + app_data: OrderCreationAppData::Full { + full: "{}".to_string(), + }, ..Default::default() }; validator @@ -1388,7 +1393,9 @@ mod tests { let creation_ = OrderCreation { fee_amount: U256::zero(), partially_fillable: true, - app_data: OrderCreationAppData::default(), + app_data: OrderCreationAppData::Full { + full: "{}".to_string(), + }, ..creation }; let (order, quote) = validator @@ -1451,6 +1458,9 @@ mod tests { buy_amount: U256::from(1), sell_amount: U256::from(1), signature: Signature::Eip712(EcdsaSignature::non_zero()), + app_data: OrderCreationAppData::Full { + full: "{}".to_string(), + }, ..Default::default() }; let res = validator @@ -1507,6 +1517,9 @@ mod tests { sell_amount: U256::from(0), fee_amount: U256::from(1), signature: Signature::Eip712(EcdsaSignature::non_zero()), + app_data: OrderCreationAppData::Full { + full: "{}".to_string(), + }, ..Default::default() }; let result = validator @@ -1563,6 +1576,9 @@ mod tests { fee_amount: U256::from(1), kind: OrderKind::Sell, signature: Signature::Eip712(EcdsaSignature::non_zero()), + app_data: OrderCreationAppData::Full { + full: "{}".to_string(), + }, ..Default::default() }; let (order, quote) = validator @@ -1617,6 +1633,9 @@ mod tests { fee_amount: U256::from(1), from: Some(Default::default()), signature: Signature::Eip712(EcdsaSignature::non_zero()), + app_data: OrderCreationAppData::Full { + full: "{}".to_string(), + }, ..Default::default() }; let result = validator @@ -1665,6 +1684,9 @@ mod tests { sell_amount: U256::from(1), fee_amount: U256::from(1), signature: Signature::Eip712(EcdsaSignature::non_zero()), + app_data: OrderCreationAppData::Full { + full: "{}".to_string(), + }, ..Default::default() }; let result = validator @@ -1716,6 +1738,9 @@ mod tests { sell_amount: U256::from(1), fee_amount: U256::from(1), signature: Signature::Eip712(EcdsaSignature::non_zero()), + app_data: OrderCreationAppData::Full { + full: "{}".to_string(), + }, ..Default::default() }; let result = validator @@ -1771,6 +1796,9 @@ mod tests { sell_amount: U256::MAX, fee_amount: U256::from(1), signature: Signature::Eip712(EcdsaSignature::non_zero()), + app_data: OrderCreationAppData::Full { + full: "{}".to_string(), + }, ..Default::default() }; let result = validator @@ -1820,6 +1848,9 @@ mod tests { sell_amount: U256::from(1), fee_amount: U256::from(1), signature: Signature::Eip712(EcdsaSignature::non_zero()), + app_data: OrderCreationAppData::Full { + full: "{}".to_string(), + }, ..Default::default() }; let result = validator @@ -1875,6 +1906,9 @@ mod tests { fee_amount: U256::from(1), from: Some(H160([1; 20])), signature: Signature::Eip1271(vec![1, 2, 3]), + app_data: OrderCreationAppData::Full { + full: "{}".to_string(), + }, ..Default::default() }; let domain = DomainSeparator::default(); @@ -1933,6 +1967,9 @@ mod tests { buy_token: H160::from_low_u64_be(2), buy_amount: 1.into(), fee_amount: 1.into(), + app_data: OrderCreationAppData::Full { + full: "{}".to_string(), + }, ..Default::default() };