Skip to content

Commit

Permalink
Reject orders with unknown preimages (#2360)
Browse files Browse the repository at this point in the history
# 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
  • Loading branch information
fleupold authored Feb 6, 2024
1 parent f14f127 commit e49b325
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 32 deletions.
9 changes: 3 additions & 6 deletions crates/e2e/tests/e2e/app_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Expand Down
17 changes: 0 additions & 17 deletions crates/e2e/tests/e2e/app_data_signer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down
7 changes: 5 additions & 2 deletions crates/e2e/tests/e2e/order_cancellation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ use {
e2e::{setup::*, tx},
ethcontract::prelude::U256,
model::{
app_data::AppDataHash,
order::{
CancellationPayload,
OrderCancellation,
OrderCancellations,
OrderCreation,
OrderCreationAppData,
OrderStatus,
OrderUid,
SignedOrderCancellations,
Expand All @@ -18,6 +18,7 @@ use {
},
number::nonzero::U256 as NonZeroU256,
secp256k1::SecretKey,
serde_json::json,
shared::ethrpc::Web3,
web3::signing::SecretKeyRef,
};
Expand Down Expand Up @@ -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 {
Expand Down
5 changes: 2 additions & 3 deletions crates/e2e/tests/e2e/smart_contract_orders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand Down Expand Up @@ -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,
Expand Down
45 changes: 41 additions & 4 deletions crates/shared/src/order_validation.rs
Original file line number Diff line number Diff line change
@@ -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::{
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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()
};

Expand Down

0 comments on commit e49b325

Please sign in to comment.