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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions crates/contracts/artifacts/GasHog.json
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"abi":[{"inputs":[{"internalType":"contract ERC20","name":"token","type":"address"},{"internalType":"address","name":"spender","type":"address"},{"internalType":"uint256","name":"amount","type":"uint256"}],"name":"approve","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"bytes32","name":"order","type":"bytes32"},{"internalType":"bytes","name":"signature","type":"bytes"}],"name":"isValidSignature","outputs":[{"internalType":"bytes4","name":"","type":"bytes4"}],"stateMutability":"view","type":"function"}],"bytecode":"0x608060405234801561001057600080fd5b50610318806100206000396000f3fe608060405234801561001057600080fd5b50600436106100365760003560e01c80631626ba7e1461003b578063e1f21c6714610083575b600080fd5b61004e6100493660046101d0565b610098565b6040517fffffffff00000000000000000000000000000000000000000000000000000000909116815260200160405180910390f35b610096610091366004610271565b610143565b005b6000805a905060006100ac848601866102b2565b90507fce7d7369855be79904099402d83db6d6ab8840dcd5c086e062cd1ca0c8111dfc5b815a6100dc90856102cb565b101561010b576040805160208101839052016040516020818303038152906040528051906020012090506100d0565b86810361011757600080fd5b507f1626ba7e000000000000000000000000000000000000000000000000000000009695505050505050565b6040517f095ea7b300000000000000000000000000000000000000000000000000000000815273ffffffffffffffffffffffffffffffffffffffff83811660048301526024820183905284169063095ea7b390604401600060405180830381600087803b1580156101b357600080fd5b505af11580156101c7573d6000803e3d6000fd5b50505050505050565b6000806000604084860312156101e557600080fd5b83359250602084013567ffffffffffffffff8082111561020457600080fd5b818601915086601f83011261021857600080fd5b81358181111561022757600080fd5b87602082850101111561023957600080fd5b6020830194508093505050509250925092565b73ffffffffffffffffffffffffffffffffffffffff8116811461026e57600080fd5b50565b60008060006060848603121561028657600080fd5b83356102918161024c565b925060208401356102a18161024c565b929592945050506040919091013590565b6000602082840312156102c457600080fd5b5035919050565b81810381811115610305577f4e487b7100000000000000000000000000000000000000000000000000000000600052601160045260246000fd5b9291505056fea164736f6c6343000811000a","deployedBytecode":"0x608060405234801561001057600080fd5b50600436106100365760003560e01c80631626ba7e1461003b578063e1f21c6714610083575b600080fd5b61004e6100493660046101d0565b610098565b6040517fffffffff00000000000000000000000000000000000000000000000000000000909116815260200160405180910390f35b610096610091366004610271565b610143565b005b6000805a905060006100ac848601866102b2565b90507fce7d7369855be79904099402d83db6d6ab8840dcd5c086e062cd1ca0c8111dfc5b815a6100dc90856102cb565b101561010b576040805160208101839052016040516020818303038152906040528051906020012090506100d0565b86810361011757600080fd5b507f1626ba7e000000000000000000000000000000000000000000000000000000009695505050505050565b6040517f095ea7b300000000000000000000000000000000000000000000000000000000815273ffffffffffffffffffffffffffffffffffffffff83811660048301526024820183905284169063095ea7b390604401600060405180830381600087803b1580156101b357600080fd5b505af11580156101c7573d6000803e3d6000fd5b50505050505050565b6000806000604084860312156101e557600080fd5b83359250602084013567ffffffffffffffff8082111561020457600080fd5b818601915086601f83011261021857600080fd5b81358181111561022757600080fd5b87602082850101111561023957600080fd5b6020830194508093505050509250925092565b73ffffffffffffffffffffffffffffffffffffffff8116811461026e57600080fd5b50565b60008060006060848603121561028657600080fd5b83356102918161024c565b925060208401356102a18161024c565b929592945050506040919091013590565b6000602082840312156102c457600080fd5b5035919050565b81810381811115610305577f4e487b7100000000000000000000000000000000000000000000000000000000600052601160045260246000fd5b9291505056fea164736f6c6343000811000a","devdoc":{"methods":{}},"userdoc":{"methods":{}}}
3 changes: 3 additions & 0 deletions crates/contracts/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -691,6 +691,9 @@ fn main() {

// Test Contract for incrementing arbitrary counters.
generate_contract("Counter");

// Test Contract for using up a specified amount of gas.
generate_contract("GasHog");
}

fn generate_contract(name: &str) {
Expand Down
6 changes: 3 additions & 3 deletions crates/contracts/solidity/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ CONTRACTS := \
Trader.sol
ARTIFACTS := $(patsubst %.sol,$(ARTIFACTDIR)/%.json,$(CONTRACTS))

TEST_CONTRACTS := Counter.sol
TEST_CONTRACTS := Counter.sol GasHog.sol
TEST_ARTIFACTS := $(patsubst %.sol,$(ARTIFACTDIR)/%.json,$(TEST_CONTRACTS))

.PHONY: artifacts
Expand Down Expand Up @@ -58,11 +58,11 @@ $(TARGETDIR)/%.abi: %.sol
$(SOLC) \
$(SOLFLAGS) -o /target $<

$(TARGETDIR)/%.abi: test/%.sol
$(TARGETDIR)/%.abi: tests/%.sol
@mkdir -p $(TARGETDIR)
@echo solc $(SOLFLAGS) -o /target $(notdir $<)
@$(DOCKER) run -it --rm \
-v "$(abspath .)/test:/contracts" -w "/contracts" \
-v "$(abspath .)/tests:/contracts" -w "/contracts" \
-v "$(abspath $(TARGETDIR)):/target" \
$(SOLC) \
$(SOLFLAGS) -o /target $(notdir $<)
27 changes: 27 additions & 0 deletions crates/contracts/solidity/tests/GasHog.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.17;

interface ERC20 {
function approve(address spender, uint amount) external;
}

/// @title Helper contract to simulate gas intensive ERC1271 signatures
contract GasHog {
function isValidSignature(bytes32 order, bytes calldata signature) public view returns (bytes4) {
uint start = gasleft();
uint target = abi.decode(signature, (uint));
bytes32 hash = keccak256("go");
while (start - gasleft() < target) {
hash = keccak256(abi.encode(hash));
}
// Assert the impossible so that the compiler doesn't optimise the loop away
require(hash != order);

// ERC1271 Magic Value
return 0x1626ba7e;
}

function approve(ERC20 token, address spender, uint amount) external {
token.approve(spender, amount);
}
}
1 change: 1 addition & 0 deletions crates/contracts/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ pub mod support {
pub mod test {
include_contracts! {
Counter;
GasHog;
}
}

Expand Down
43 changes: 30 additions & 13 deletions crates/e2e/src/setup/services.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ impl ServicesBuilder {
}
}

#[derive(Default)]
pub struct ExtraServiceArgs {
pub api: Vec<String>,
pub autopilot: Vec<String>,
}

/// Wrapper over offchain services.
/// Exposes various utility methods for tests.
pub struct Services<'a> {
Expand Down Expand Up @@ -103,11 +109,6 @@ impl<'a> Services<'a> {
"--baseline-sources=None".to_string(),
"--network-block-interval=1s".to_string(),
"--solver-competition-auth=super_secret_key".to_string(),
format!(
"--custom-univ2-baseline-sources={:?}|{:?}",
self.contracts.uniswap_v2_router.address(),
self.contracts.default_pool_code(),
),
format!(
"--settlement-contract-address={:?}",
self.contracts.gp_settlement.address()
Expand Down Expand Up @@ -168,6 +169,11 @@ impl<'a> Services<'a> {

/// Starts a basic version of the protocol with a single baseline solver.
pub async fn start_protocol(&self, solver: TestAccount) {
self.start_protocol_with_args(Default::default(), solver)
.await;
}

pub async fn start_protocol_with_args(&self, args: ExtraServiceArgs, solver: TestAccount) {
let solver_endpoint =
colocation::start_baseline_solver(self.contracts.weth.address()).await;
colocation::start_driver(
Expand All @@ -180,15 +186,26 @@ impl<'a> Services<'a> {
);
self.start_autopilot(
None,
vec![
"--drivers=test_solver|http://localhost:11088/test_solver".to_string(),
"--price-estimation-drivers=test_quoter|http://localhost:11088/test_solver"
.to_string(),
],
[
vec![
"--drivers=test_solver|http://localhost:11088/test_solver".to_string(),
"--price-estimation-drivers=test_quoter|http://localhost:11088/test_solver"
.to_string(),
],
args.autopilot,
]
.concat(),
);
self.start_api(vec![
"--price-estimation-drivers=test_quoter|http://localhost:11088/test_solver".to_string(),
])
self.start_api(
[
vec![
"--price-estimation-drivers=test_quoter|http://localhost:11088/test_solver"
.to_string(),
],
args.api,
]
.concat(),
)
.await;
}

Expand Down
60 changes: 60 additions & 0 deletions crates/e2e/tests/e2e/hooks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use {
order::{OrderCreation, OrderCreationAppData, OrderKind},
signature::{hashed_eip712_message, EcdsaSigningScheme, Signature},
},
reqwest::StatusCode,
secp256k1::SecretKey,
serde_json::json,
shared::ethrpc::Web3,
Expand All @@ -35,6 +36,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
64 changes: 63 additions & 1 deletion crates/e2e/tests/e2e/smart_contract_orders.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
use {
e2e::setup::{safe::Safe, *},
e2e::{
setup::{safe::Safe, *},
tx,
},
ethcontract::{Bytes, H160, U256},
model::{
order::{OrderCreation, OrderCreationAppData, OrderKind, OrderStatus, OrderUid},
signature::Signature,
},
reqwest::StatusCode,
shared::ethrpc::Web3,
};

Expand All @@ -14,6 +18,12 @@ async fn local_node_smart_contract_orders() {
run_test(smart_contract_orders).await;
}

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

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

Expand Down Expand Up @@ -149,3 +159,55 @@ async fn smart_contract_orders(web3: Web3) {
.expect("Couldn't fetch native token balance");
assert_eq!(balance, U256::from(7_975_363_406_512_003_608_u128));
}

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

let [solver] = onchain.make_solvers(to_wei(1)).await;
let trader = contracts::test::GasHog::builder(&web3)
.deploy()
.await
.unwrap();

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!(
solver.account(),
trader.approve(cow.address(), onchain.contracts().allowance, to_wei(10))
);

let services = Services::new(onchain.contracts()).await;
services
.start_protocol_with_args(
ExtraServiceArgs {
api: vec!["--max-gas-per-order=1000000".to_string()],
..Default::default()
},
solver,
)
.await;

// Use 1M gas units during signature verification
let mut signature = [0; 32];
U256::exp10(6).to_big_endian(&mut signature);

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,
signature: Signature::Eip1271(signature.to_vec()),
from: Some(trader.address()),
..Default::default()
};

let error = services.create_order(&order).await.unwrap_err();
assert_eq!(error.0, StatusCode::BAD_REQUEST);
assert!(error.1.contains("TooMuchGas"));
}
1 change: 1 addition & 0 deletions crates/orderbook/openapi.yml
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.

UnsupportedBuyTokenDestination,
UnsupportedSellTokenSource,
UnsupportedOrderType,
Expand Down
4 changes: 4 additions & 0 deletions crates/orderbook/src/api/post_order.rs
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,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
4 changes: 2 additions & 2 deletions crates/shared/src/order_quoting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ impl QuoteParameters {
}
}

fn additional_cost(&self) -> u64 {
pub fn additional_cost(&self) -> u64 {
self.signing_scheme
.additional_gas_amount()
.saturating_add(self.additional_gas)
Expand Down Expand Up @@ -279,7 +279,7 @@ impl QuoteSearchParameters {
}

/// Returns additional gas costs incurred by the quote.
fn additional_cost(&self) -> u64 {
pub fn additional_cost(&self) -> u64 {
self.signing_scheme
.additional_gas_amount()
.saturating_add(self.additional_gas)
Expand Down
Loading
Loading