Refactor Signature Validation (#1873)

This PR is a follow-up to #1871

Specifically, that PR added error handling for the `eth_call` simulation
such that reverts are properly classified as
`SignatureValidationError::Invalid`. The `Signatures.sol` contract was
_supposed to_ handle all reverts from signature verifiers, so that error
classification shouldn't have been needed.

**However**, it turns out I made some assumptions about the Solidity
`try ... catch` semantics that turned out to be invalid and cause the
`Signatures.sol` to still revert in some cases. In particular,
`Signatures::validate` will revert if:
- `signer` is not a contract
- `signer.isValidSignature` returns less than 32 bytes of data

With this in mind, it doesn't really make sense to try and handle revert
errors in both the Solidity code and the Rust code, so just handle all
revert errors in Rust in the
`signature_validation::simulation::Validator` code and get rid of the
`try ... catch` in the Solidity code.

I also refactored the `From<ethcontract::ExecutionError>` implementation
into the existing `From<web3::Error>` (which was no longer being used,
and IMO made more sense to be there).

Furthermore, #1842 removed
some `signature_validator::{arguments, web3}` submodules whose files
were accidentally left behind, so I removed them here as well.

### Test Plan

CI - the existing E2E tests for signature validation continue to pass,
and added a new one to check that errors are correctly detected.

crates/contracts/solidity/Signatures.sol

@@ -13,9 +13,6 @@ contract Signatures {
         IVaultRelayer vaultRelayer;
     }
 
-    /// @dev Signature validation result.
-    enum Result { OK, INVALID, REVERT }
-
     /// @dev Validates an ERC-1271 signature.
     ///
     /// @param contracts - On-chain contract addresses required for the
@@ -26,16 +23,14 @@ contract Signatures {
     /// @param interactions - A list of pre-interactions required for setting
     /// up signature requirements.
     ///
-    /// @return result - The signature validation result.
     /// @return gasUsed - The gas units used for verifying the signature.
     function validate(
         Contracts memory contracts,
-        address signer,
+        IERC1271 signer,
         bytes32 order,
         bytes calldata signature,
         Interaction[] calldata interactions
     ) external returns (
-        Result result,
         uint256 gasUsed
     ) {
         // Execute the interactions within the current context. This ensures
@@ -44,16 +39,10 @@ contract Signatures {
         executeInteractions(contracts, interactions);
 
         gasUsed = gasleft();
-        try IERC1271(signer).isValidSignature(order, signature) returns (bytes4 magicValue) {
-            gasUsed = gasUsed - gasleft();
-            result = magicValue == ERC1271_MAGICVALUE
-                ? Result.OK
-                : Result.INVALID;
-        }
-        catch {
-            gasUsed = gasUsed - gasleft();
-            result = Result.REVERT;
-        }
+        bytes4 magicValue = signer.isValidSignature(order, signature);
+        gasUsed = gasUsed - gasleft();
+
+        require(magicValue == ERC1271_MAGICVALUE, "didn't say the magic word");
     }
 
     /// @dev Execute a set of interactions. This code is ported from the CoW

crates/e2e/tests/e2e/smart_contract_orders.rs

@@ -65,7 +65,6 @@ async fn smart_contract_orders(web3: Web3) {
     services.start_autopilot(vec![]);
     services.start_api(vec![]).await;
 
-    // Place Orders
     let order_template = OrderCreation {
         kind: OrderKind::Sell,
         sell_token: token.address(),
@@ -76,20 +75,39 @@ async fn smart_contract_orders(web3: Web3) {
         valid_to: model::time::now_in_epoch_seconds() + 300,
         ..Default::default()
     };
+    let signature1271 = gnosis_safe_eip1271_signature(
+        SecretKeyRef::from(&SecretKey::from_slice(trader.private_key()).unwrap()),
+        &safe,
+        H256(hashed_eip712_message(
+            &onchain.contracts().domain_separator,
+            &order_template.data().hash_struct(),
+        )),
+    )
+    .await;
+
+    // Check that we can't place invalid orders.
+    let orders = [
+        OrderCreation {
+            from: Some(safe.address()),
+            signature: Signature::Eip1271(b"invalid signature".to_vec()),
+            ..order_template.clone()
+        },
+        OrderCreation {
+            from: Some(H160(*b"invalid address\0\0\0\0\0")),
+            signature: Signature::Eip1271(signature1271.clone()),
+            ..order_template.clone()
+        },
+    ];
+    for order in &orders {
+        let (_, err) = dbg!(services.create_order(order).await.unwrap_err());
+        assert!(err.contains("InvalidEip1271Signature"));
+    }
+
+    // Place orders
     let orders = [
         OrderCreation {
             from: Some(safe.address()),
-            signature: Signature::Eip1271(
-                gnosis_safe_eip1271_signature(
-                    SecretKeyRef::from(&SecretKey::from_slice(trader.private_key()).unwrap()),
-                    &safe,
-                    H256(hashed_eip712_message(
-                        &onchain.contracts().domain_separator,
-                        &order_template.data().hash_struct(),
-                    )),
-                )
-                .await,
-            ),
+            signature: Signature::Eip1271(signature1271),
             ..order_template.clone()
         },
         OrderCreation {

crates/shared/src/signature_validator.rs

@@ -1,6 +1,5 @@
 use {
-    crate::ethcontract_error::EthcontractErrorType,
-    ethcontract::{errors::ExecutionError, Bytes},
+    ethcontract::Bytes,
     ethrpc::Web3,
     hex_literal::hex,
     model::interaction::InteractionData,
@@ -74,12 +73,3 @@ pub fn validator(contracts: Contracts, web3: Web3) -> Arc<dyn SignatureValidatin
         contracts.vault_relayer,
     ))
 }
-
-impl From<ExecutionError> for SignatureValidationError {
-    fn from(err: ExecutionError) -> Self {
-        match EthcontractErrorType::classify(&err) {
-            EthcontractErrorType::Contract => Self::Invalid,
-            _ => Self::Other(err.into()),
-        }
-    }
-}

crates/shared/src/signature_validator/simulation.rs

@@ -5,6 +5,7 @@
 
 use {
     super::{SignatureCheck, SignatureValidating, SignatureValidationError},
+    crate::ethcontract_error::EthcontractErrorType,
     anyhow::{Context, Result},
     ethcontract::{common::abi::Token, errors::ExecutionError, tokens::Tokenize, Bytes},
     ethrpc::Web3,
@@ -30,7 +31,7 @@ impl Validator {
     async fn simulate(
         &self,
         check: &SignatureCheck,
-    ) -> Result<Verification, SignatureValidationError> {
+    ) -> Result<Simulation, SignatureValidationError> {
         // We simulate the signature verification from the Settlement contract's
         // context. This allows us to check:
         // 1. How the pre-interactions would behave as part of the settlement
@@ -59,22 +60,11 @@ impl Validator {
             tx.data.unwrap(),
         );
 
-        let output = self
-            .web3
-            .eth()
-            .call(call, None)
-            .await
-            .map_err(ExecutionError::from)?;
+        let output = self.web3.eth().call(call, None).await?;
         let simulation = Simulation::decode(&output.0)?;
 
-        tracing::trace!(?check, ?simulation, "simulated signatures");
-        match simulation.result {
-            SimulationResult::Ok => Ok(simulation.verification),
-            result => {
-                tracing::debug!(?result, "invalid signature");
-                Err(SignatureValidationError::Invalid)
-            }
-        }
+        tracing::trace!(?check, ?simulation, "simulated signature");
+        Ok(simulation)
     }
 }
 
@@ -106,51 +96,39 @@ impl SignatureValidating for Validator {
 
 #[derive(Debug)]
 struct Simulation {
-    result: SimulationResult,
-    verification: Verification,
-}
-
-#[derive(Debug)]
-struct Verification {
     gas_used: U256,
 }
 
-#[derive(Debug)]
-enum SimulationResult {
-    Ok,
-    Invalid,
-    Revert,
-}
-
 impl Simulation {
     fn decode(output: &[u8]) -> Result<Self> {
         let function = contracts::support::Signatures::raw_contract()
             .abi
             .function("validate")
             .unwrap();
         let tokens = function.decode_output(output).context("decode")?;
-        let (result, gas_used) = Tokenize::from_token(Token::Tuple(tokens))?;
+        let (gas_used,) = Tokenize::from_token(Token::Tuple(tokens))?;
 
-        Ok(Self {
-            result: SimulationResult::decode(result)?,
-            verification: Verification { gas_used },
-        })
+        Ok(Self { gas_used })
     }
 }
 
-impl SimulationResult {
-    fn decode(value: u8) -> Result<Self> {
-        match value {
-            0 => Ok(Self::Ok),
-            1 => Ok(Self::Invalid),
-            2 => Ok(Self::Revert),
-            _ => anyhow::bail!("invalid simulation result variant {value}"),
+impl From<web3::Error> for SignatureValidationError {
+    fn from(err: web3::Error) -> Self {
+        // TODO: This is needed to parse Hardhat revert errors, which
+        // `ethcontract` does not support currently.
+        if matches!(
+            &err,
+            web3::Error::Rpc(err)
+                if err.message.contains("VM Exception") ||
+                    err.message.contains("Transaction reverted")
+        ) {
+            return Self::Invalid;
         }
-    }
-}
 
-impl From<web3::error::Error> for SignatureValidationError {
-    fn from(err: web3::error::Error) -> Self {
-        Self::Other(err.into())
+        let err = ExecutionError::from(err);
+        match EthcontractErrorType::classify(&err) {
+            EthcontractErrorType::Contract => Self::Invalid,
+            _ => Self::Other(err.into()),
+        }
     }
 }