From 78a1b81c5da367fddcb1bf27f7cf9d9b3001fa20 Mon Sep 17 00:00:00 2001 From: Martin Magnus Date: Fri, 6 Dec 2024 17:44:38 +0100 Subject: [PATCH] Verify quotes from zero address (#3150) # Description @nlordell's recent PR stack enabled the use case where a real trader (wallet is connected in the frontend) does not have the required balances by faking balances. But with the new ability to fake balances for many tokens we can even go one step further. # Changes If we know how to fake balances for the given sell token AND no wallet is connected (i.e. `verification.from.is_zero()`) we simply generate a random address and use that as the trader with fake balances. Using a non-zero address is important because many token transfer functions don't like sending tokens to or from the zero address. This should prevent the annoying UX where you sometime see a really good (unverified) price when your wallet is not connected and end up seeing worse (but verified) prices after you connect your wallet. ## How to test added an e2e test which produces a verified quote with `from: H160::zero()` --- crates/e2e/tests/e2e/quote_verification.rs | 18 +++ .../src/price_estimation/trade_verifier.rs | 110 ++++++++++-------- 2 files changed, 78 insertions(+), 50 deletions(-) diff --git a/crates/e2e/tests/e2e/quote_verification.rs b/crates/e2e/tests/e2e/quote_verification.rs index 8af977edcf..149afbfc7d 100644 --- a/crates/e2e/tests/e2e/quote_verification.rs +++ b/crates/e2e/tests/e2e/quote_verification.rs @@ -409,4 +409,22 @@ async fn verified_quote_with_simulated_balance(web3: Web3) { .await .unwrap(); assert!(response.verified); + + // with balance overrides we can even verify quotes for the 0 address + // which is used when no wallet is connected in the frontend + let response = services + .submit_quote(&OrderQuoteRequest { + from: H160::zero(), + sell_token: weth.address(), + buy_token: token.address(), + side: OrderQuoteSide::Sell { + sell_amount: SellAmount::BeforeFee { + value: to_wei(1).try_into().unwrap(), + }, + }, + ..Default::default() + }) + .await + .unwrap(); + assert!(response.verified); } diff --git a/crates/shared/src/price_estimation/trade_verifier.rs b/crates/shared/src/price_estimation/trade_verifier.rs index 8d871f4656..496cecdd4f 100644 --- a/crates/shared/src/price_estimation/trade_verifier.rs +++ b/crates/shared/src/price_estimation/trade_verifier.rs @@ -25,7 +25,6 @@ use { }, ethcontract::{tokens::Tokenize, Bytes, H160, U256}, ethrpc::{block_stream::CurrentBlockWatcher, extensions::StateOverride, Web3}, - maplit::hashmap, model::{ order::{OrderData, OrderKind, BUY_ETH_ADDRESS}, signature::{Signature, SigningScheme}, @@ -102,17 +101,20 @@ impl TradeVerifier { async fn verify_inner( &self, query: &PriceQuery, - verification: &Verification, + mut verification: Verification, trade: &TradeKind, out_amount: &U256, ) -> Result { - if verification.from.is_zero() { - // Don't waste time on common simulations which will always fail. - return Err(anyhow::anyhow!("trader is zero address").into()); - } - let start = std::time::Instant::now(); + // this may change the `verification` parameter (to make more + // quotes verifiable) so we do it as the first thing to ensure + // that all the following code uses the updated value + let overrides = self + .prepare_state_overrides(&mut verification, query, trade) + .await + .map_err(Error::SimulationFailed)?; + // Use `tx_origin` if response indicates that a special address is needed for // the simulation to pass. Otherwise just use the solver address. let solver = trade.tx_origin().unwrap_or(trade.solver()); @@ -132,7 +134,7 @@ impl TradeVerifier { let settlement = encode_settlement( query, - verification, + &verification, trade, &tokens, &clearing_prices, @@ -141,7 +143,7 @@ impl TradeVerifier { &self.domain_separator, )?; - let settlement = add_balance_queries(settlement, query, verification, &solver); + let settlement = add_balance_queries(settlement, query, &verification, &solver); let settlement = self .settlement @@ -191,11 +193,6 @@ impl TradeVerifier { ..Default::default() }; - let overrides = self - .prepare_state_overrides(verification, query, trade) - .await - .map_err(Error::SimulationFailed)?; - let output = self .simulator .simulate(call, overrides, Some(block.number)) @@ -287,17 +284,60 @@ impl TradeVerifier { /// trade. async fn prepare_state_overrides( &self, - verification: &Verification, + verification: &mut Verification, query: &PriceQuery, trade: &TradeKind, ) -> Result> { + let mut overrides = HashMap::with_capacity(6); + + // Provide mocked balances if possible to the spardose to allow it to + // give some balances to the trader in order to verify trades even for + // owners without balances. Note that we use a separate account for + // funding to not interfere with the settlement process. This allows the + // simulation to conditionally transfer the balance only when it is + // safe to mock the trade pre-conditions on behalf of the user and to + // not alter solver balances which may be used during settlement. We use + // a similar strategy for determining whether or not to set approvals on + // behalf of the trader. + if let Some(solver_balance_override) = self + .balance_overrides + .state_override(BalanceOverrideRequest { + token: query.sell_token, + holder: Self::SPARDOSE, + amount: match query.kind { + OrderKind::Sell => query.in_amount.get(), + OrderKind::Buy => trade.out_amount( + &query.buy_token, + &query.sell_token, + &query.in_amount.get(), + &query.kind, + )?, + }, + }) + .await + { + tracing::trace!(?solver_balance_override, "solver balance override enabled"); + overrides.insert(query.sell_token, solver_balance_override); + + if verification.from.is_zero() { + verification.from = H160::random(); + tracing::debug!( + trader = ?verification.from, + "use random trader address with fake balances" + ); + } + } else if verification.from.is_zero() { + anyhow::bail!("trader is zero address and balances can not be faked"); + } + // Set up mocked trader. - let mut overrides = hashmap! { - verification.from => StateOverride { + overrides.insert( + verification.from, + StateOverride { code: Some(deployed_bytecode!(Trader)), ..Default::default() }, - }; + ); // If the trader is a smart contract we also need to store its implementation // to proxy into it during the simulation. @@ -328,36 +368,6 @@ impl TradeVerifier { }, ); - // Provide mocked balances if possible to the spardose to allow it to - // give some balances to the trader in order to verify trades even for - // owners without balances. Note that we use a separate account for - // funding to not interfere with the settlement process. This allows the - // simulation to conditionally transfer the balance only when it is - // safe to mock the trade pre-conditions on behalf of the user and to - // not alter solver balances which may be used during settlement. We use - // a similar strategy for determining whether or not to set approvals on - // behalf of the trader. - if let Some(solver_balance_override) = self - .balance_overrides - .state_override(BalanceOverrideRequest { - token: query.sell_token, - holder: Self::SPARDOSE, - amount: match query.kind { - OrderKind::Sell => query.in_amount.get(), - OrderKind::Buy => trade.out_amount( - &query.buy_token, - &query.sell_token, - &query.in_amount.get(), - &query.kind, - )?, - }, - }) - .await - { - tracing::trace!(?solver_balance_override, "solver balance override enabled"); - overrides.insert(query.sell_token, solver_balance_override); - } - // Set up mocked solver. let mut solver_override = StateOverride { code: Some(deployed_bytecode!(Solver)), @@ -408,7 +418,7 @@ impl TradeVerifying for TradeVerifier { ) .context("failed to compute trade out amount")?; match self - .verify_inner(query, verification, &trade, &out_amount) + .verify_inner(query, verification.clone(), &trade, &out_amount) .await { Ok(verified) => Ok(verified), @@ -790,7 +800,7 @@ enum Error { #[cfg(test)] mod tests { - use {super::*, std::str::FromStr}; + use {super::*, maplit::hashmap, std::str::FromStr}; #[test] fn discards_inaccurate_quotes() {