Remove destination from FillOrder v1 inputs. Require that such inputs are never signed.

Cargo.toml

@@ -248,7 +248,8 @@ zeroize = "1.5"
 
 [workspace.dependencies.trezor-client]
 git = "https://github.com/mintlayer/mintlayer-trezor-firmware"
-rev = "07229ff4943537b81c18a612fe971792d2a37574" # Commit "Support Mintlayer input commitments V1"
+# The commit "Remove destination from MintlayerFillOrderV1; fail if the host asks to sign a FillOrder input"
+rev = "8bde1135d9ea313cc04a6f1755893acd237e5515"
 features = ["bitcoin", "mintlayer"]
 
 [workspace.metadata.dist.dependencies.apt]

api-server/scanner-lib/src/blockchain_state/mod.rs

@@ -594,7 +594,7 @@ async fn calculate_tx_fee_and_collect_token_info<T: ApiServerStorageWrite>(
                     }
                 },
                 TxInput::OrderAccountCommand(cmd) => match cmd {
-                    OrderAccountCommand::FillOrder(order_id, _, _)
+                    OrderAccountCommand::FillOrder(order_id, _)
                     | OrderAccountCommand::FreezeOrder(order_id)
                     | OrderAccountCommand::ConcludeOrder(order_id) => {
                         let order = db_tx.get_order(*order_id).await?.expect("must exist");
@@ -829,7 +829,7 @@ async fn prefetch_orders<T: ApiServerStorageRead>(
         match input {
             TxInput::Utxo(_) | TxInput::Account(_) => {}
             TxInput::OrderAccountCommand(cmd) => match cmd {
-                OrderAccountCommand::FillOrder(order_id, _, _)
+                OrderAccountCommand::FillOrder(order_id, _)
                 | OrderAccountCommand::FreezeOrder(order_id)
                 | OrderAccountCommand::ConcludeOrder(order_id) => {
                     let order = db_tx.get_order(*order_id).await?.expect("must be present ");
@@ -1227,7 +1227,7 @@ async fn update_tables_from_transaction_inputs<T: ApiServerStorageWrite>(
                 }
             },
             TxInput::OrderAccountCommand(cmd) => match cmd {
-                OrderAccountCommand::FillOrder(order_id, fill_amount_in_ask_currency, _) => {
+                OrderAccountCommand::FillOrder(order_id, fill_amount_in_ask_currency) => {
                     let order = db_tx.get_order(*order_id).await?.expect("must exist");
                     let order =
                         order.fill(&chain_config, block_height, *fill_amount_in_ask_currency);

api-server/stack-test-suite/tests/v2/orders.rs

@@ -86,13 +86,9 @@ async fn create_fill_conclude_order(#[case] seed: Seed, #[case] version: OrdersV
                             Destination::AnyoneCanSpend,
                         ),
                     ),
-                    OrdersVersion::V1 => {
-                        TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(
-                            order_id,
-                            Amount::from_atoms(1),
-                            Destination::AnyoneCanSpend,
-                        ))
-                    }
+                    OrdersVersion::V1 => TxInput::OrderAccountCommand(
+                        OrderAccountCommand::FillOrder(order_id, Amount::from_atoms(1)),
+                    ),
                 };
                 let tx2 = TransactionBuilder::new()
                     .add_input(

api-server/web-server/src/api/json_helpers.rs

@@ -323,13 +323,12 @@ pub fn tx_input_to_json(
             }
         },
         TxInput::OrderAccountCommand(cmd) => match cmd {
-            OrderAccountCommand::FillOrder(id, fill, dest) => {
+            OrderAccountCommand::FillOrder(id, fill) => {
                 json!({
                     "input_type": "OrderAccountCommand",
                     "command": "FillOrder",
                     "order_id": Address::new(chain_config, *id).expect("addressable").to_string(),
                     "fill_atoms": json!({"atoms": fill.into_atoms().to_string()}),
-                    "destination": Address::new(chain_config, dest.clone()).expect("addressable").as_str(),
                 })
             }
             OrderAccountCommand::ConcludeOrder(order_id) => {
@@ -410,13 +409,12 @@ pub fn tx_input_to_json(
                     "order_id": Address::new(chain_config, *order_id).expect("addressable").to_string(),
                 })
             }
-            AccountCommand::FillOrder(order_id, fill, dest) => {
+            AccountCommand::FillOrder(order_id, fill, _) => {
                 json!({
                     "input_type": "AccountCommand",
                     "command": "FillOrder",
                     "order_id": Address::new(chain_config, *order_id).expect("addressable").to_string(),
                     "fill_atoms": json!({"atoms": fill.into_atoms().to_string()}),
-                    "destination": Address::new(chain_config, dest.clone()).expect("no error").as_str(),
                 })
             }
             AccountCommand::ChangeTokenMetadataUri(token_id, metadata_uri) => {

chainstate/constraints-value-accumulator/src/constraints_accumulator.rs

@@ -329,15 +329,13 @@ impl ConstrainedValueAccumulator {
         O: OrdersAccountingOperations + OrdersAccountingView<Error = orders_accounting::Error>,
     {
         let result = match command {
-            OrderAccountCommand::FillOrder(id, amount, _) => {
-                Some(self.process_fill_order_command(
-                    chain_config,
-                    block_height,
-                    *id,
-                    *amount,
-                    orders_accounting_delta,
-                )?)
-            }
+            OrderAccountCommand::FillOrder(id, amount) => Some(self.process_fill_order_command(
+                chain_config,
+                block_height,
+                *id,
+                *amount,
+                orders_accounting_delta,
+            )?),
             OrderAccountCommand::ConcludeOrder(order_id) => {
                 Some(self.process_conclude_order_command(*order_id, orders_accounting_delta)?)
             }

chainstate/constraints-value-accumulator/src/tests/orders_constraints.rs

@@ -309,7 +309,6 @@ fn fill_order_constraints(#[case] seed: Seed, #[case] version: OrdersVersion) {
             OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(
                 order_id,
                 (ask_amount + Amount::from_atoms(1)).unwrap(),
-                Destination::AnyoneCanSpend,
             )),
         };
         let inputs = vec![
@@ -354,11 +353,9 @@ fn fill_order_constraints(#[case] seed: Seed, #[case] version: OrdersVersion) {
                 AccountNonce::new(0),
                 AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend),
             ),
-            OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(
-                order_id,
-                ask_amount,
-                Destination::AnyoneCanSpend,
-            )),
+            OrdersVersion::V1 => {
+                TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, ask_amount))
+            }
         };
         let inputs = vec![
             TxInput::Utxo(UtxoOutPoint::new(
@@ -395,11 +392,9 @@ fn fill_order_constraints(#[case] seed: Seed, #[case] version: OrdersVersion) {
                 AccountNonce::new(0),
                 AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend),
             ),
-            OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(
-                order_id,
-                ask_amount,
-                Destination::AnyoneCanSpend,
-            )),
+            OrdersVersion::V1 => {
+                TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, ask_amount))
+            }
         };
         let inputs = vec![
             TxInput::Utxo(UtxoOutPoint::new(
@@ -451,11 +446,9 @@ fn fill_order_constraints(#[case] seed: Seed, #[case] version: OrdersVersion) {
                 AccountNonce::new(0),
                 AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend),
             ),
-            OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(
-                order_id,
-                ask_amount,
-                Destination::AnyoneCanSpend,
-            )),
+            OrdersVersion::V1 => {
+                TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, ask_amount))
+            }
         };
         let inputs = vec![
             TxInput::Utxo(UtxoOutPoint::new(
@@ -512,11 +505,9 @@ fn fill_order_constraints(#[case] seed: Seed, #[case] version: OrdersVersion) {
                 AccountNonce::new(0),
                 AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend),
             ),
-            OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(
-                order_id,
-                ask_amount,
-                Destination::AnyoneCanSpend,
-            )),
+            OrdersVersion::V1 => {
+                TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, ask_amount))
+            }
         };
         let inputs = vec![
             TxInput::Utxo(UtxoOutPoint::new(
@@ -571,11 +562,9 @@ fn fill_order_constraints(#[case] seed: Seed, #[case] version: OrdersVersion) {
             AccountNonce::new(0),
             AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend),
         ),
-        OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(
-            order_id,
-            ask_amount,
-            Destination::AnyoneCanSpend,
-        )),
+        OrdersVersion::V1 => {
+            TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, ask_amount))
+        }
     };
     let inputs = vec![
         TxInput::Utxo(UtxoOutPoint::new(

chainstate/src/detail/chainstateref/mod.rs

@@ -41,8 +41,8 @@ use common::{
         },
         config::EpochIndex,
         tokens::{TokenAuxiliaryData, TokenId},
-        AccountNonce, AccountType, Block, ChainConfig, GenBlock, GenBlockId, PoolId, Transaction,
-        TxOutput, UtxoOutPoint,
+        AccountNonce, AccountType, Block, ChainConfig, GenBlock, GenBlockId, OrderAccountCommand,
+        PoolId, Transaction, TxInput, TxOutput, UtxoOutPoint,
     },
     primitives::{
         id::WithId, time::Time, Amount, BlockCount, BlockDistance, BlockHeight, Id, Idable,
@@ -779,14 +779,29 @@ impl<'a, S: BlockchainStorageRead, V: TransactionVerificationStrategy> Chainstat
 
     #[log_error]
     fn check_duplicate_inputs(&self, block: &Block) -> Result<(), CheckBlockTransactionsError> {
-        // check for duplicate inputs (see CVE-2018-17144)
-        let mut block_inputs = BTreeSet::new();
+        // Reject the block if it has duplicate inputs, with the exception of v1 FillOrder inputs,
+        // which can't be unique (because they only contain the order id and the amount).
+        // Note that this is a precaution "inspired" by the Bitcoin vulnerability CVE-2018-17144.
+        // I.e. even if this check is removed, the corresponding erroneous conditions (like spending
+        // the same UTXO twice or concluding an already concluded order) should be captured elsewhere.
+        let mut block_unique_inputs = BTreeSet::new();
         for tx in block.transactions() {
             for input in tx.inputs() {
-                ensure!(
-                    block_inputs.insert(input),
-                    CheckBlockTransactionsError::DuplicateInputInBlock(block.get_id())
-                );
+                let must_be_unique = match input {
+                    TxInput::Utxo(_) | TxInput::Account(_) | TxInput::AccountCommand(_, _) => true,
+                    TxInput::OrderAccountCommand(cmd) => match cmd {
+                        OrderAccountCommand::FillOrder(_, _) => false,
+                        | OrderAccountCommand::FreezeOrder(_)
+                        | OrderAccountCommand::ConcludeOrder(_) => true,
+                    },
+                };
+
+                if must_be_unique {
+                    ensure!(
+                        block_unique_inputs.insert(input),
+                        CheckBlockTransactionsError::DuplicateInputInBlock(block.get_id())
+                    );
+                }
             }
         }
         Ok(())

chainstate/src/rpc/types/account.rs

@@ -145,7 +145,6 @@ pub enum RpcOrderAccountCommand {
     Fill {
         order_id: RpcAddress<OrderId>,
         fill_value: RpcAmountOut,
-        destination: RpcAddress<Destination>,
     },
     Freeze {
         order_id: RpcAddress<OrderId>,
@@ -161,10 +160,9 @@ impl RpcOrderAccountCommand {
             OrderAccountCommand::ConcludeOrder(order_id) => RpcOrderAccountCommand::Conclude {
                 order_id: RpcAddress::new(chain_config, *order_id)?,
             },
-            OrderAccountCommand::FillOrder(id, fill, dest) => RpcOrderAccountCommand::Fill {
+            OrderAccountCommand::FillOrder(id, fill) => RpcOrderAccountCommand::Fill {
                 order_id: RpcAddress::new(chain_config, *id)?,
                 fill_value: RpcAmountOut::from_amount(*fill, chain_config.coin_decimals()),
-                destination: RpcAddress::new(chain_config, dest.clone())?,
             },
             OrderAccountCommand::FreezeOrder(id) => RpcOrderAccountCommand::Freeze {
                 order_id: RpcAddress::new(chain_config, *id)?,

chainstate/test-framework/src/random_tx_maker.rs

@@ -1021,13 +1021,9 @@ impl<'a> RandomTxMaker<'a> {
                         if !is_frozen_token(&filled_value, tokens_cache)
                             && !is_frozen_order(&orders_cache, order_id)
                         {
-                            let input =
-                                TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(
-                                    order_id,
-                                    amount_to_spend,
-                                    key_manager
-                                        .new_destination(self.chainstate.get_chain_config(), rng),
-                                ));
+                            let input = TxInput::OrderAccountCommand(
+                                OrderAccountCommand::FillOrder(order_id, amount_to_spend),
+                            );
 
                             let output = TxOutput::Transfer(
                                 filled_value,
@@ -1260,10 +1256,6 @@ impl<'a> RandomTxMaker<'a> {
                                     OrderAccountCommand::FillOrder(
                                         order_id,
                                         Amount::from_atoms(atoms),
-                                        key_manager.new_destination(
-                                            self.chainstate.get_chain_config(),
-                                            rng,
-                                        ),
                                     ),
                                 ));
 

chainstate/test-framework/src/signature_destination_getter.rs

@@ -184,10 +184,12 @@ impl<'a> SignatureDestinationGetter<'a> {
                                 ))?;
                             Ok(order_data.conclude_key().clone())
                         }
-                        AccountCommand::FillOrder(_, _, d) => Ok(d.clone()),
+                        // Signatures on FillOrder v0 inputs are not checked.
+                        AccountCommand::FillOrder(_, _, _) => Ok(Destination::AnyoneCanSpend),
                     },
                     TxInput::OrderAccountCommand(command) => match command {
-                        OrderAccountCommand::FillOrder(_, _, d) => Ok(d.clone()),
+                        // FillOrder v1 inputs must not be signed.
+                        OrderAccountCommand::FillOrder(_, _) => Ok(Destination::AnyoneCanSpend),
                         OrderAccountCommand::FreezeOrder(order_id)
                         | OrderAccountCommand::ConcludeOrder(order_id) => {
                             let order_data = orders_view

chainstate/test-suite/src/tests/input_commitments.rs

@@ -853,11 +853,9 @@ fn make_fill_order_input(
             nonce,
             AccountCommand::FillOrder(*order_id, fill_amount, Destination::AnyoneCanSpend),
         ),
-        OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(
-            *order_id,
-            fill_amount,
-            Destination::AnyoneCanSpend,
-        )),
+        OrdersVersion::V1 => {
+            TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(*order_id, fill_amount))
+        }
     }
 }