Verify quote with pre interactions (#3160)

# Description
Not too long ago #3081
changed the simulation logic to not do any set up steps on behalf of the
user if the quote provided some pre-interactions. That was done to avoid
reverts when a pre-interactions actually set up everything for the trade
(e.g. get balances by unstaking).
However, this caused issues with verifying quotes with `permit`
pre-interactions. Because the quote provided the `permit`
pre-interaction the simulation logic also did not try to fake balances
although it could have.

# Changes
Now we again always call the code to setup approvals and such BUT we do
it as the very last `pre-interaction`. That way this operation becomes a
no-op if the user already sets up everything with their pre-interactions
OR does the usual logic and reverts with helpful error messages if the
pre-conditions can't be met.
Since this function may now be a no-op I renamed it to
`ensureSwapPreconditions()`.
Additionally this moves code that was previously run before the
settlement inside the settlement which increases the measured gas cost.
To work around that I also introduced a wrapper function in `Solver.sol`
that simply calls the `Trader` function and discounts the gas costs.
Note that we can't simply do the necessary steps in the `Solver` because
we need to be inside the context of the `Trader` to be able to set
approvals and such.

That way we can verify quotes which do no, partial or the entire swap
setup in their pre-interactions while keeping good error messages and
accurate gas estimates.
Kudos to @nlordell for the idea.

## How to test
I added an e2e test that fails without the PR.

crates/contracts/solidity/Solver.sol

@@ -17,11 +17,6 @@ contract Solver {
     using Caller for *;
     using Math for *;
 
-    struct Mock {
-        bool enabled;
-        address spardose;
-    }
-
     uint256 private _simulationOverhead;
     uint256[] private _queriedBalances;
 
@@ -32,49 +27,23 @@ contract Solver {
     ///
     /// @param settlementContract - address of the settlement contract because
     /// it does not have a stable address in tests.
-    /// @param trader - address of the order owner doing the trade
-    /// @param sellToken - address of the token being sold
-    /// @param sellAmount - amount being sold
-    /// @param nativeToken - ERC20 version of the chain's token
     /// @param tokens - list of tokens used in the trade
     /// @param receiver - address receiving the bought tokens
     /// @param settlementCall - the calldata of the `settle()` call
-    /// @param mock - mocking configuration for the simulation; this controls
-    ///             whether things like ETH wrapping, setting allowance and
-    ///             pre-funding should be done on behalf of the user to support
-    ///             quote verification for users who aren't ready to swap.
     ///
     /// @return gasUsed - gas used for the `settle()` call
     /// @return queriedBalances - list of balances stored during the simulation
     function swap(
         ISettlement settlementContract,
-        address payable trader,
-        address sellToken,
-        uint256 sellAmount,
-        address nativeToken,
         address[] calldata tokens,
         address payable receiver,
-        bytes calldata settlementCall,
-        Mock memory mock
+        bytes calldata settlementCall
     ) external returns (
         uint256 gasUsed,
         uint256[] memory queriedBalances
     ) {
         require(msg.sender == address(this), "only simulation logic is allowed to call 'swap' function");
 
-        if (mock.enabled) {
-            // Prepare the trade in the context of the trader so we are allowed
-            // to set approvals and things like that.
-            Trader(trader)
-                .prepareSwap(
-                    settlementContract,
-                    sellToken,
-                    sellAmount,
-                    nativeToken,
-                    mock.spardose
-                );
-        }
-
         // Warm the storage for sending ETH to smart contract addresses.
         // We allow this call to revert becaues it was either unnecessary in the first place
         // or failing to send `ETH` to the `receiver` will cause a revert in the settlement
@@ -135,4 +104,28 @@ contract Solver {
         address(settlementContract).doCall(settlementCall);
         gasUsed = gasStart - gasleft() - _simulationOverhead;
     }
+
+    /// @dev Simple wrapper around `Trader.ensureTradePreconditions()` that
+    ///      discounts the gas used to prepare the swap (setting up approvals
+    ///      and balances) from the total gas cost since that would normally
+    ///      not happen during the settlement.
+    function ensureTradePreconditions(
+        Trader trader,
+        ISettlement settlementContract,
+        address sellToken,
+        uint256 sellAmount,
+        address nativeToken,
+        address spardose
+    ) external {
+        uint256 gasStart = gasleft();
+        trader.ensureTradePreconditions(
+            settlementContract,
+            sellToken,
+            sellAmount,
+            nativeToken,
+            spardose
+        );
+        // Account for costs of gas used outside of metered section.
+        _simulationOverhead += gasStart - gasleft() + 4460;
+    }
 }

crates/contracts/solidity/Trader.sol

@@ -70,7 +70,7 @@ contract Trader {
     /// @param sellAmount - expected amount to be sold according to the quote
     /// @param nativeToken - ERC20 version of the chain's native token
     /// @param spardose - piggy bank for requesting additional funds
-    function prepareSwap(
+    function ensureTradePreconditions(
         ISettlement settlementContract,
         address sellToken,
         uint256 sellAmount,
@@ -104,8 +104,13 @@ contract Trader {
             // We first reset the allowance to 0 since some ERC20 tokens (e.g. USDT)
             // require that due to this attack:
             // https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
-            IERC20(sellToken).safeApprove(address(settlementContract.vaultRelayer()), 0);
-            IERC20(sellToken).safeApprove(address(settlementContract.vaultRelayer()), type(uint256).max);
+            // We catch reverts because we'll later assert the correct approval got set anyway.
+            try IERC20(sellToken).approve(address(settlementContract.vaultRelayer()), 0) {}
+            catch {}
+            try IERC20(sellToken).approve(address(settlementContract.vaultRelayer()), type(uint256).max) {}
+            catch {}
+            uint256 allowance = IERC20(sellToken).allowance(address(this), address(settlementContract.vaultRelayer()));
+            require(allowance >= sellAmount, "trader did not give the required approvals");
         }
 
         // Ensure that the user has sufficient sell token balance. If not, request some

crates/e2e/tests/e2e/quote_verification.rs

@@ -9,6 +9,7 @@ use {
         quote::{OrderQuoteRequest, OrderQuoteSide, SellAmount},
     },
     number::nonzero::U256 as NonZeroU256,
+    serde_json::json,
     shared::{
         price_estimation::{
             trade_verifier::{
@@ -435,4 +436,38 @@ async fn verified_quote_with_simulated_balance(web3: Web3) {
         .await
         .unwrap();
     assert!(response.verified);
+
+    // Previously quote verification did not set up the trade correctly
+    // if the user provided pre-interactions. This works now.
+    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(),
+                },
+            },
+            app_data: model::order::OrderCreationAppData::Full {
+                full: json!({
+                    "metadata": {
+                        "hooks": {
+                            "pre": [
+                                {
+                                    "target": "0x0000000000000000000000000000000000000000",
+                                    "callData": "0x",
+                                    "gasLimit": "0"
+                                }
+                            ]
+                        }
+                    }
+                })
+                .to_string(),
+            },
+            ..Default::default()
+        })
+        .await
+        .unwrap();
+    assert!(response.verified);
 }

crates/shared/src/price_estimation/trade_verifier.rs

@@ -143,6 +143,7 @@ impl TradeVerifier {
             out_amount,
             self.native_token,
             &self.domain_separator,
+            self.settlement.address(),
         )?;
 
         let settlement = add_balance_queries(settlement, query, &verification, &solver);
@@ -158,28 +159,13 @@ impl TradeVerifier {
             )
             .tx;
 
-        let sell_amount = match query.kind {
-            OrderKind::Sell => query.in_amount.get(),
-            OrderKind::Buy => *out_amount,
-        };
-
-        // Only enable additional mocking (approvals, native token wrapping,
-        // balance overrides) if the user did not provide pre-interactions. If
-        // the user did provide pre-interactions, it's reasonable to assume that
-        // they will set up all the necessary details of the trade.
-        let mock_enabled = verification.pre_interactions.is_empty();
         let simulation = solver
             .methods()
             .swap(
                 self.settlement.address(),
-                verification.from,
-                query.sell_token,
-                sell_amount,
-                self.native_token,
                 tokens.clone(),
                 verification.receiver,
                 Bytes(settlement.data.unwrap().0),
-                (mock_enabled, Self::SPARDOSE),
             )
             .tx;
 
@@ -471,6 +457,7 @@ fn encode_settlement(
     out_amount: &U256,
     native_token: H160,
     domain_separator: &DomainSeparator,
+    settlement: H160,
 ) -> Result<EncodedSettlement> {
     let mut trade_interactions = encode_interactions(&trade.interactions());
     if query.buy_token == BUY_ETH_ADDRESS {
@@ -483,7 +470,13 @@ fn encode_settlement(
             OrderKind::Buy => query.in_amount.get(),
         };
         let weth = dummy_contract!(WETH9, native_token);
-        let calldata = weth.methods().withdraw(buy_amount).tx.data.unwrap().0;
+        let calldata = weth
+            .methods()
+            .withdraw(buy_amount)
+            .tx
+            .data
+            .expect("data gets populated by function call above")
+            .0;
         trade_interactions.push((native_token, 0.into(), Bytes(calldata)));
         tracing::trace!("adding unwrap interaction for paying out ETH");
     }
@@ -498,11 +491,42 @@ fn encode_settlement(
         )?);
     }
 
-    let pre_interactions = [
-        verification.pre_interactions.clone(),
-        trade.pre_interactions(),
-    ]
-    .concat();
+    // Execute interaction to set up trade right before transfering funds.
+    // This interaction does nothing if the user-provided pre-interactions
+    // already set everything up (e.g. approvals, balances). That way we can
+    // correctly verify quotes with or without these user pre-interactions
+    // with helpful error messages.
+    let trade_setup_interaction = {
+        let sell_amount = match query.kind {
+            OrderKind::Sell => query.in_amount.get(),
+            OrderKind::Buy => *out_amount,
+        };
+        let solver = dummy_contract!(Solver, trade.solver());
+        let setup_call = solver
+            .ensure_trade_preconditions(
+                verification.from,
+                settlement,
+                query.sell_token,
+                sell_amount,
+                native_token,
+                TradeVerifier::SPARDOSE,
+            )
+            .tx
+            .data
+            .expect("data gets populated by function call above")
+            .0;
+        Interaction {
+            target: solver.address(),
+            value: 0.into(),
+            data: setup_call,
+        }
+    };
+
+    let user_interactions = verification.pre_interactions.iter().cloned();
+    let pre_interactions: Vec<_> = user_interactions
+        .chain(trade.pre_interactions())
+        .chain([trade_setup_interaction])
+        .collect();
 
     Ok(EncodedSettlement {
         tokens: tokens.to_vec(),
@@ -668,9 +692,14 @@ fn add_balance_queries(
         // track how much `sell_token` the `from` address actually spent
         OrderKind::Buy => (query.sell_token, verification.from),
     };
-    let query_balance = solver.methods().store_balance(token, owner, true);
-    let query_balance = Bytes(query_balance.tx.data.unwrap().0);
-    let interaction = (solver.address(), 0.into(), query_balance);
+    let query_balance_call = solver
+        .methods()
+        .store_balance(token, owner, true)
+        .tx
+        .data
+        .expect("data gets populated by function call above")
+        .0;
+    let interaction = (solver.address(), 0.into(), Bytes(query_balance_call));
     // query balance query at the end of pre-interactions
     settlement.interactions[0].push(interaction.clone());
     // query balance right after we payed out all `buy_token`