Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reject orders using too much gas #2551

Merged
merged 4 commits into from
Mar 24, 2024
Merged

Reject orders using too much gas #2551

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b0cb12e
Reject orders using too much gas
fleupold Mar 20, 2024
8ac7533
adjust gas limit in test
fleupold Mar 20, 2024
ddbb3e9
e2e test for erc1271 gas consumption
fleupold Mar 24, 2024
f9a5792
Merge branch 'main' into max_gas_per_order
fleupold Mar 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions crates/e2e/tests/e2e/hooks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use {
order::{Hook, OrderCreation, OrderCreationAppData, OrderKind},
signature::{hashed_eip712_message, EcdsaSigningScheme, Signature},
},
reqwest::StatusCode,
secp256k1::SecretKey,
serde_json::json,
shared::ethrpc::Web3,
Expand All @@ -34,6 +35,65 @@ async fn local_node_partial_fills() {
run_test(partial_fills).await;
}

#[tokio::test]
#[ignore]
async fn local_node_gas_limit() {
run_test(gas_limit).await;
}

async fn gas_limit(web3: Web3) {
let mut onchain = OnchainComponents::deploy(web3).await;

let [solver] = onchain.make_solvers(to_wei(1)).await;
let [trader] = onchain.make_accounts(to_wei(1)).await;
let cow = onchain
.deploy_cow_weth_pool(to_wei(1_000_000), to_wei(1_000), to_wei(1_000))
.await;

// Fund trader accounts and approve relayer
cow.fund(trader.address(), to_wei(5)).await;
tx!(
trader.account(),
cow.approve(onchain.contracts().allowance, to_wei(5))
);

let services = Services::new(onchain.contracts()).await;
services.start_protocol(solver).await;

let order = OrderCreation {
sell_token: cow.address(),
sell_amount: to_wei(4),
buy_token: onchain.contracts().weth.address(),
buy_amount: to_wei(3),
valid_to: model::time::now_in_epoch_seconds() + 300,
kind: OrderKind::Sell,
app_data: OrderCreationAppData::Full {
full: json!({
"metadata": {
"hooks": {
"pre": [Hook {
target: trader.address(),
call_data: Default::default(),
gas_limit: 10_000_000,
}],
"post": [],
},
},
})
.to_string(),
},
..Default::default()
}
.sign(
EcdsaSigningScheme::Eip712,
&onchain.contracts().domain_separator,
SecretKeyRef::from(&SecretKey::from_slice(trader.private_key()).unwrap()),
);
let error = services.create_order(&order).await.unwrap_err();
assert_eq!(error.0, StatusCode::BAD_REQUEST);
assert!(error.1.contains("TooMuchGas"));
}

async fn allowance(web3: Web3) {
let mut onchain = OnchainComponents::deploy(web3).await;

Expand Down
1 change: 1 addition & 0 deletions crates/orderbook/openapi.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1187,6 +1187,7 @@ components:
ZeroAmount,
IncompatibleSigningScheme,
TooManyLimitOrders,
TooMuchGas,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mfw78 this introduces a new error type the watchtower needs to know about to not get stuck indexing these orders, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cowprotocol/cow-sdk#202

UnsupportedBuyTokenDestination,
UnsupportedSellTokenSource,
UnsupportedOrderType,
Expand Down
4 changes: 4 additions & 0 deletions crates/orderbook/src/api/post_order.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,10 @@ impl IntoWarpReply for ValidationErrorWrapper {
error("TooManyLimitOrders", "Too many limit orders"),
StatusCode::BAD_REQUEST,
),
ValidationError::TooMuchGas => with_status(
error("TooMuchGas", "Executing order requires too many gas units"),
StatusCode::BAD_REQUEST,
),

ValidationError::Other(err) => {
tracing::error!(?err, "ValidationErrorWrapper");
Expand Down
6 changes: 6 additions & 0 deletions crates/orderbook/src/arguments.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,10 @@ pub struct Arguments {
/// Set the maximum size in bytes of order app data.
#[clap(long, env, default_value = "8192")]
pub app_data_size_limit: usize,

/// The maximum gas amount a single order can use for getting settled.
#[clap(long, env, default_value = "8000000")]
pub max_gas_per_order: u64,
}

impl std::fmt::Display for Arguments {
Expand Down Expand Up @@ -173,6 +177,7 @@ impl std::fmt::Display for Arguments {
hooks_contract_address,
app_data_size_limit,
db_url,
max_gas_per_order,
} = self;

write!(f, "{}", shared)?;
Expand Down Expand Up @@ -237,6 +242,7 @@ impl std::fmt::Display for Arguments {
&hooks_contract_address.map(|a| format!("{a:?}")),
)?;
writeln!(f, "app_data_size_limit: {}", app_data_size_limit)?;
writeln!(f, "max_gas_per_order: {}", max_gas_per_order)?;

Ok(())
}
Expand Down
1 change: 1 addition & 0 deletions crates/orderbook/src/run.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,7 @@ pub async fn run(args: Arguments) {
Arc::new(CachedCodeFetcher::new(Arc::new(web3.clone()))),
app_data_validator.clone(),
args.shared.market_orders_deprecation_date,
args.max_gas_per_order,
)
.with_verified_quotes(args.price_estimation.trade_simulator.is_some()),
);
Expand Down
26 changes: 26 additions & 0 deletions crates/shared/src/order_validation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ pub enum ValidationError {
ZeroAmount,
IncompatibleSigningScheme,
TooManyLimitOrders,
TooMuchGas,
Other(anyhow::Error),
}

Expand Down Expand Up @@ -255,6 +256,7 @@ pub struct OrderValidator {
app_data_validator: crate::app_data::Validator,
request_verified_quotes: bool,
market_orders_deprecation_date: Option<chrono::DateTime<chrono::Utc>>,
max_gas_per_order: u64,
}

#[derive(Debug, Eq, PartialEq, Default)]
Expand Down Expand Up @@ -326,6 +328,7 @@ impl OrderValidator {
code_fetcher: Arc<dyn CodeFetching>,
app_data_validator: crate::app_data::Validator,
market_orders_deprecation_date: Option<chrono::DateTime<chrono::Utc>>,
max_gas_per_order: u64,
) -> Self {
Self {
native_token,
Expand All @@ -343,6 +346,7 @@ impl OrderValidator {
app_data_validator,
request_verified_quotes: false,
market_orders_deprecation_date,
max_gas_per_order,
}
}

Expand Down Expand Up @@ -728,6 +732,14 @@ impl OrderValidating for OrderValidator {
}
};

if quote.as_ref().is_some_and(|quote| {
// Quoted gas does not include additional gas for hooks
quote.data.fee_parameters.gas_amount as u64 + quote_parameters.additional_gas
> self.max_gas_per_order
}) {
return Err(ValidationError::TooMuchGas);
}

let order = Order {
metadata: OrderMetadata {
owner,
Expand Down Expand Up @@ -1061,6 +1073,7 @@ mod tests {
Arc::new(MockCodeFetching::new()),
Default::default(),
None,
u64::MAX,
);
let result = validator
.partial_validate(PreOrderData {
Expand Down Expand Up @@ -1208,6 +1221,7 @@ mod tests {
Arc::new(MockCodeFetching::new()),
Default::default(),
None,
u64::MAX,
);
let order = || PreOrderData {
valid_to: time::now_in_epoch_seconds()
Expand Down Expand Up @@ -1296,6 +1310,7 @@ mod tests {
Arc::new(MockCodeFetching::new()),
Default::default(),
None,
u64::MAX,
);

let creation = OrderCreation {
Expand Down Expand Up @@ -1500,6 +1515,7 @@ mod tests {
Arc::new(MockCodeFetching::new()),
Default::default(),
None,
u64::MAX,
);

let creation = OrderCreation {
Expand Down Expand Up @@ -1571,6 +1587,7 @@ mod tests {
Arc::new(MockCodeFetching::new()),
Default::default(),
None,
u64::MAX,
);

let creation = OrderCreation {
Expand Down Expand Up @@ -1627,6 +1644,7 @@ mod tests {
Arc::new(MockCodeFetching::new()),
Default::default(),
None,
u64::MAX,
);
let order = OrderCreation {
valid_to: time::now_in_epoch_seconds() + 2,
Expand Down Expand Up @@ -1685,6 +1703,7 @@ mod tests {
Arc::new(MockCodeFetching::new()),
Default::default(),
None,
u64::MAX,
);
let order = OrderCreation {
valid_to: time::now_in_epoch_seconds() + 2,
Expand Down Expand Up @@ -1742,6 +1761,7 @@ mod tests {
Arc::new(MockCodeFetching::new()),
Default::default(),
None,
u64::MAX,
);
let order = OrderCreation {
valid_to: time::now_in_epoch_seconds() + 2,
Expand Down Expand Up @@ -1794,6 +1814,7 @@ mod tests {
Arc::new(MockCodeFetching::new()),
Default::default(),
None,
u64::MAX,
);
let order = OrderCreation {
valid_to: time::now_in_epoch_seconds() + 2,
Expand Down Expand Up @@ -1848,6 +1869,7 @@ mod tests {
Arc::new(MockCodeFetching::new()),
Default::default(),
None,
u64::MAX,
);
let order = OrderCreation {
valid_to: time::now_in_epoch_seconds() + 2,
Expand Down Expand Up @@ -1906,6 +1928,7 @@ mod tests {
Arc::new(MockCodeFetching::new()),
Default::default(),
None,
u64::MAX,
);
let order = OrderCreation {
valid_to: time::now_in_epoch_seconds() + 2,
Expand Down Expand Up @@ -1958,6 +1981,7 @@ mod tests {
Arc::new(MockCodeFetching::new()),
Default::default(),
None,
u64::MAX,
);
let order = OrderCreation {
valid_to: time::now_in_epoch_seconds() + 2,
Expand Down Expand Up @@ -2014,6 +2038,7 @@ mod tests {
Arc::new(MockCodeFetching::new()),
Default::default(),
None,
u64::MAX,
);

let creation = OrderCreation {
Expand Down Expand Up @@ -2077,6 +2102,7 @@ mod tests {
Arc::new(MockCodeFetching::new()),
Default::default(),
None,
u64::MAX,
);

let order = OrderCreation {
Expand Down
Loading