Prevent orders with excessive valid to from being created

crates/e2e/tests/e2e/services.rs

@@ -176,6 +176,7 @@ impl OrderbookServices {
             HashSet::default(),
             HashSet::default(),
             Duration::from_secs(120),
+            Duration::MAX,
             fee_calculator.clone(),
             bad_token_detector.clone(),
             balance_fetcher,

crates/model/src/signature.rs

@@ -8,13 +8,20 @@ use web3::{
     types::Recovery,
 };
 
-#[derive(Eq, PartialEq, Clone, Copy, Debug, Serialize, Hash)]
+#[derive(Eq, PartialEq, Clone, Copy, Debug, Deserialize, Serialize, Hash)]
 #[serde(rename_all = "lowercase")]
 pub enum SigningScheme {
     Eip712,
     EthSign,
     PreSign,
 }
+
+impl Default for SigningScheme {
+    fn default() -> Self {
+        SigningScheme::Eip712
+    }
+}
+
 #[derive(Eq, PartialEq, Clone, Copy, Debug, Deserialize, Serialize, Hash)]
 #[serde(rename_all = "lowercase")]
 #[serde(tag = "signingScheme", content = "signature")]
@@ -26,7 +33,7 @@ pub enum Signature {
 
 impl Default for Signature {
     fn default() -> Self {
-        Self::default_with(SigningScheme::Eip712)
+        Self::default_with(SigningScheme::default())
     }
 }
 

crates/orderbook/openapi.yml

@@ -610,6 +610,9 @@ components:
         buyTokenBalance:
           $ref: "#/components/schemas/BuyTokenDestination"
           default: "erc20"
+        signingScheme:
+          $ref: "#/components/schemas/SigningScheme"
+          default: "eip712"
       required:
         - sellToken
         - buyToken
@@ -831,6 +834,7 @@ components:
               InsufficientAllowance,
               InsufficientBalance,
               InsufficientValidTo,
+              ExcessiveValidTo,
               InvalidSignature,
               TransferEthToContract,
               TransferSimulationFailed,

crates/orderbook/src/api/order_validation.rs

@@ -59,6 +59,7 @@ pub trait OrderValidating: Send + Sync {
 pub enum PartialValidationError {
     Forbidden,
     InsufficientValidTo,
+    ExcessiveValidTo,
     TransferEthToContract,
     InvalidNativeSellToken,
     SameBuyAndSellToken,
@@ -97,6 +98,10 @@ impl IntoWarpReply for PartialValidationError {
                 ),
                 StatusCode::BAD_REQUEST,
             ),
+            Self::ExcessiveValidTo => with_status(
+                super::error("ExcessiveValidTo", "validTo is too far into the future"),
+                StatusCode::BAD_REQUEST,
+            ),
             Self::TransferEthToContract => with_status(
                 super::error(
                     "TransferEthToContract",
@@ -216,13 +221,14 @@ pub struct OrderValidator {
     banned_users: HashSet<H160>,
     liquidity_order_owners: HashSet<H160>,
     min_order_validity_period: Duration,
+    max_order_validity_period: Duration,
     /// For Full-Validation: performed time of order placement
     fee_validator: Arc<dyn MinFeeCalculating>,
     bad_token_detector: Arc<dyn BadTokenDetecting>,
     balance_fetcher: Arc<dyn BalanceFetching>,
 }
 
-#[derive(Default, Debug, PartialEq)]
+#[derive(Debug, PartialEq, Default)]
 pub struct PreOrderData {
     pub owner: H160,
     pub sell_token: H160,
@@ -232,6 +238,7 @@ pub struct PreOrderData {
     pub partially_fillable: bool,
     pub buy_token_balance: BuyTokenDestination,
     pub sell_token_balance: SellTokenSource,
+    pub signing_scheme: SigningScheme,
     pub is_liquidity_order: bool,
 }
 
@@ -259,6 +266,7 @@ impl PreOrderData {
             partially_fillable: order.partially_fillable,
             buy_token_balance: order.buy_token_balance,
             sell_token_balance: order.sell_token_balance,
+            signing_scheme: order.signature.scheme(),
             is_liquidity_order,
         }
     }
@@ -272,6 +280,7 @@ impl OrderValidator {
         banned_users: HashSet<H160>,
         liquidity_order_owners: HashSet<H160>,
         min_order_validity_period: Duration,
+        max_order_validity_period: Duration,
         fee_validator: Arc<dyn MinFeeCalculating>,
         bad_token_detector: Arc<dyn BadTokenDetecting>,
         balance_fetcher: Arc<dyn BalanceFetching>,
@@ -282,6 +291,7 @@ impl OrderValidator {
             banned_users,
             liquidity_order_owners,
             min_order_validity_period,
+            max_order_validity_period,
             fee_validator,
             bad_token_detector,
             balance_fetcher,
@@ -311,11 +321,18 @@ impl OrderValidating for OrderValidator {
                 order.sell_token_balance,
             ));
         }
-        if order.valid_to
-            < shared::time::now_in_epoch_seconds() + self.min_order_validity_period.as_secs() as u32
-        {
+
+        let now = shared::time::now_in_epoch_seconds();
+        if order.valid_to < now + self.min_order_validity_period.as_secs() as u32 {
             return Err(PartialValidationError::InsufficientValidTo);
         }
+        if order.valid_to > now.saturating_add(self.max_order_validity_period.as_secs() as u32)
+            && !order.is_liquidity_order
+            && order.signing_scheme != SigningScheme::PreSign
+        {
+            return Err(PartialValidationError::ExcessiveValidTo);
+        }
+
         if has_same_buy_and_sell_token(&order, &self.native_token) {
             return Err(PartialValidationError::SameBuyAndSellToken);
         }
@@ -564,6 +581,7 @@ mod tests {
         let mut code_fetcher = Box::new(MockCodeFetching::new());
         let native_token = dummy_contract!(WETH9, [0xef; 20]);
         let min_order_validity_period = Duration::from_secs(1);
+        let max_order_validity_period = Duration::from_secs(100);
         let banned_users = hashset![H160::from_low_u64_be(1)];
         let legit_valid_to =
             shared::time::now_in_epoch_seconds() + min_order_validity_period.as_secs() as u32 + 2;
@@ -577,6 +595,7 @@ mod tests {
             banned_users,
             hashset!(),
             min_order_validity_period,
+            max_order_validity_period,
             Arc::new(MockMinFeeCalculating::new()),
             Arc::new(MockBadTokenDetecting::new()),
             Arc::new(MockBalanceFetching::new()),
@@ -630,6 +649,15 @@ mod tests {
                 .await,
             Err(PartialValidationError::InsufficientValidTo)
         ));
+        assert!(matches!(
+            validator
+                .partial_validate(PreOrderData {
+                    valid_to: legit_valid_to + max_order_validity_period.as_secs() as u32 + 1,
+                    ..Default::default()
+                })
+                .await,
+            Err(PartialValidationError::ExcessiveValidTo)
+        ));
         assert!(matches!(
             validator
                 .partial_validate(PreOrderData {
@@ -674,6 +702,7 @@ mod tests {
             hashset!(),
             hashset!(),
             Duration::from_secs(1),
+            Duration::from_secs(100),
             Arc::new(MockMinFeeCalculating::new()),
             Arc::new(MockBadTokenDetecting::new()),
             Arc::new(MockBalanceFetching::new()),
@@ -695,12 +724,14 @@ mod tests {
     async fn pre_validate_ok() {
         let liquidity_order_owner = H160::from_low_u64_be(0x42);
         let min_order_validity_period = Duration::from_secs(1);
+        let max_order_validity_period = Duration::from_secs(100);
         let validator = OrderValidator::new(
             Box::new(MockCodeFetching::new()),
             dummy_contract!(WETH9, [0xef; 20]),
             hashset!(),
             hashset!(liquidity_order_owner),
             min_order_validity_period,
+            max_order_validity_period,
             Arc::new(MockMinFeeCalculating::new()),
             Arc::new(MockBadTokenDetecting::new()),
             Arc::new(MockBalanceFetching::new()),
@@ -715,11 +746,20 @@ mod tests {
         };
 
         assert!(validator.partial_validate(order()).await.is_ok());
+        assert!(validator
+            .partial_validate(PreOrderData {
+                valid_to: u32::MAX,
+                signing_scheme: SigningScheme::PreSign,
+                ..order()
+            })
+            .await
+            .is_ok());
         assert!(validator
             .partial_validate(PreOrderData {
                 partially_fillable: true,
                 is_liquidity_order: true,
                 owner: liquidity_order_owner,
+                valid_to: u32::MAX,
                 ..order()
             })
             .await
@@ -746,6 +786,7 @@ mod tests {
             hashset!(),
             hashset!(),
             Duration::from_secs(1),
+            Duration::from_secs(100),
             Arc::new(fee_calculator),
             Arc::new(bad_token_detector),
             Arc::new(balance_fetcher),
@@ -785,6 +826,7 @@ mod tests {
             hashset!(),
             hashset!(),
             Duration::from_secs(1),
+            Duration::from_secs(100),
             Arc::new(fee_calculator),
             Arc::new(bad_token_detector),
             Arc::new(balance_fetcher),
@@ -824,6 +866,7 @@ mod tests {
             hashset!(),
             hashset!(),
             Duration::from_secs(1),
+            Duration::from_secs(100),
             Arc::new(fee_calculator),
             Arc::new(bad_token_detector),
             Arc::new(balance_fetcher),
@@ -867,6 +910,7 @@ mod tests {
             hashset!(),
             hashset!(),
             Duration::from_secs(1),
+            Duration::from_secs(100),
             Arc::new(fee_calculator),
             Arc::new(bad_token_detector),
             Arc::new(balance_fetcher),
@@ -908,6 +952,7 @@ mod tests {
             hashset!(),
             hashset!(),
             Duration::from_secs(1),
+            Duration::from_secs(100),
             Arc::new(fee_calculator),
             Arc::new(bad_token_detector),
             Arc::new(balance_fetcher),
@@ -947,6 +992,7 @@ mod tests {
             hashset!(),
             hashset!(),
             Duration::from_secs(1),
+            Duration::from_secs(100),
             Arc::new(fee_calculator),
             Arc::new(bad_token_detector),
             Arc::new(balance_fetcher),
@@ -987,6 +1033,7 @@ mod tests {
             hashset!(),
             hashset!(),
             Duration::from_secs(1),
+            Duration::from_secs(100),
             Arc::new(fee_calculator),
             Arc::new(bad_token_detector),
             Arc::new(balance_fetcher),
@@ -1028,6 +1075,7 @@ mod tests {
                     hashset!(),
                     hashset!(),
                     Duration::from_secs(1),
+                    Duration::MAX,
                     Arc::new(fee_calculator),
                     Arc::new(bad_token_detector),
                     Arc::new(balance_fetcher),

crates/orderbook/src/api/post_quote.rs

@@ -13,6 +13,7 @@ use futures::try_join;
 use model::{
     app_id::AppId,
     order::{BuyTokenDestination, OrderKind, SellTokenSource},
+    signature::SigningScheme,
     u256_decimal,
 };
 use serde::{Deserialize, Serialize};
@@ -38,6 +39,8 @@ pub struct OrderQuoteRequest {
     #[serde(default)]
     buy_token_balance: BuyTokenDestination,
     #[serde(default)]
+    signing_scheme: SigningScheme,
+    #[serde(default)]
     price_quality: PriceQuality,
 }
 
@@ -53,6 +56,7 @@ impl From<&OrderQuoteRequest> for PreOrderData {
             partially_fillable: quote_request.partially_fillable,
             buy_token_balance: quote_request.buy_token_balance,
             sell_token_balance: quote_request.sell_token_balance,
+            signing_scheme: quote_request.signing_scheme,
             is_liquidity_order: quote_request.partially_fillable,
         }
     }
@@ -451,6 +455,7 @@ mod tests {
                 "appData": "0x9090909090909090909090909090909090909090909090909090909090909090",
                 "partiallyFillable": false,
                 "buyTokenBalance": "internal",
+                "signingScheme": "presign",
                 "priceQuality": "optimal"
             }))
             .unwrap(),
@@ -467,6 +472,7 @@ mod tests {
                 partially_fillable: false,
                 sell_token_balance: SellTokenSource::Erc20,
                 buy_token_balance: BuyTokenDestination::Internal,
+                signing_scheme: SigningScheme::PreSign,
                 price_quality: PriceQuality::Optimal
             }
         );
@@ -500,8 +506,8 @@ mod tests {
                 app_data: AppId([0x90; 32]),
                 partially_fillable: false,
                 sell_token_balance: SellTokenSource::External,
-                buy_token_balance: BuyTokenDestination::Erc20,
-                price_quality: PriceQuality::Fast
+                price_quality: PriceQuality::Fast,
+                ..Default::default()
             }
         );
     }
@@ -532,9 +538,7 @@ mod tests {
                 valid_to: 0x12345678,
                 app_data: AppId([0x90; 32]),
                 partially_fillable: false,
-                sell_token_balance: SellTokenSource::Erc20,
-                buy_token_balance: BuyTokenDestination::Erc20,
-                price_quality: Default::default()
+                ..Default::default()
             }
         );
     }

crates/orderbook/src/main.rs

@@ -98,6 +98,16 @@ struct Arguments {
     )]
     min_order_validity_period: Duration,
 
+    /// The maximum amount of time in seconds an order can be valid for. Defaults to 3 hours. This
+    /// restriction does not apply to liquidity owner orders or presign orders.
+    #[clap(
+        long,
+        env,
+        default_value = "10800",
+        parse(try_from_str = shared::arguments::duration_from_seconds),
+    )]
+    max_order_validity_period: Duration,
+
     /// Don't use the trace_callMany api that only some nodes support to check whether a token
     /// should be denied.
     /// Note that if a node does not support the api we still use the less accurate call api.
@@ -690,6 +700,7 @@ async fn main() {
         args.banned_users.iter().copied().collect(),
         args.liquidity_order_owners.iter().copied().collect(),
         args.min_order_validity_period,
+        args.max_order_validity_period,
         fee_calculator.clone(),
         bad_token_detector.clone(),
         balance_fetcher,