Skip to content

Commit cd6fedd

Browse files
ImplOfAnImplImplOfAnImpl
authored
Merge pull request #1936 from mintlayer/fill_order_input_no_sig
Remove destination from FillOrder v1 inputs. Require that such inputs are never signed.
2 parents 0bd963a + fdc487d commit cd6fedd

37 files changed

Lines changed: 971 additions & 554 deletions

File tree

‎Cargo.lock‎

Lines changed: 179 additions & 129 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,8 @@ zeroize = "1.5"
248248

249249
[workspace.dependencies.trezor-client]
250250
git = "https://github.com/mintlayer/mintlayer-trezor-firmware"
251-
rev = "07229ff4943537b81c18a612fe971792d2a37574" # Commit "Support Mintlayer input commitments V1"
251+
# The commit "Remove destination from MintlayerFillOrderV1; fail if the host asks to sign a FillOrder input"
252+
rev = "198346c2f731e7ff34be03b7a16818008eeeae0d"
252253
features = ["bitcoin", "mintlayer"]
253254

254255
[workspace.metadata.dist.dependencies.apt]

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -594,7 +594,7 @@ async fn calculate_tx_fee_and_collect_token_info<T: ApiServerStorageWrite>(
594594
}
595595
},
596596
TxInput::OrderAccountCommand(cmd) => match cmd {
597-
OrderAccountCommand::FillOrder(order_id, _, _)
597+
OrderAccountCommand::FillOrder(order_id, _)
598598
| OrderAccountCommand::FreezeOrder(order_id)
599599
| OrderAccountCommand::ConcludeOrder(order_id) => {
600600
let order = db_tx.get_order(*order_id).await?.expect("must exist");
@@ -829,7 +829,7 @@ async fn prefetch_orders<T: ApiServerStorageRead>(
829829
match input {
830830
TxInput::Utxo(_) | TxInput::Account(_) => {}
831831
TxInput::OrderAccountCommand(cmd) => match cmd {
832-
OrderAccountCommand::FillOrder(order_id, _, _)
832+
OrderAccountCommand::FillOrder(order_id, _)
833833
| OrderAccountCommand::FreezeOrder(order_id)
834834
| OrderAccountCommand::ConcludeOrder(order_id) => {
835835
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>(
12271227
}
12281228
},
12291229
TxInput::OrderAccountCommand(cmd) => match cmd {
1230-
OrderAccountCommand::FillOrder(order_id, fill_amount_in_ask_currency, _) => {
1230+
OrderAccountCommand::FillOrder(order_id, fill_amount_in_ask_currency) => {
12311231
let order = db_tx.get_order(*order_id).await?.expect("must exist");
12321232
let order =
12331233
order.fill(&chain_config, block_height, *fill_amount_in_ask_currency);

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

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -86,13 +86,9 @@ async fn create_fill_conclude_order(#[case] seed: Seed, #[case] version: OrdersV
8686
Destination::AnyoneCanSpend,
8787
),
8888
),
89-
OrdersVersion::V1 => {
90-
TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(
91-
order_id,
92-
Amount::from_atoms(1),
93-
Destination::AnyoneCanSpend,
94-
))
95-
}
89+
OrdersVersion::V1 => TxInput::OrderAccountCommand(
90+
OrderAccountCommand::FillOrder(order_id, Amount::from_atoms(1)),
91+
),
9692
};
9793
let tx2 = TransactionBuilder::new()
9894
.add_input(

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -323,13 +323,12 @@ pub fn tx_input_to_json(
323323
}
324324
},
325325
TxInput::OrderAccountCommand(cmd) => match cmd {
326-
OrderAccountCommand::FillOrder(id, fill, dest) => {
326+
OrderAccountCommand::FillOrder(id, fill) => {
327327
json!({
328328
"input_type": "OrderAccountCommand",
329329
"command": "FillOrder",
330330
"order_id": Address::new(chain_config, *id).expect("addressable").to_string(),
331331
"fill_atoms": json!({"atoms": fill.into_atoms().to_string()}),
332-
"destination": Address::new(chain_config, dest.clone()).expect("addressable").as_str(),
333332
})
334333
}
335334
OrderAccountCommand::ConcludeOrder(order_id) => {
@@ -410,13 +409,12 @@ pub fn tx_input_to_json(
410409
"order_id": Address::new(chain_config, *order_id).expect("addressable").to_string(),
411410
})
412411
}
413-
AccountCommand::FillOrder(order_id, fill, dest) => {
412+
AccountCommand::FillOrder(order_id, fill, _) => {
414413
json!({
415414
"input_type": "AccountCommand",
416415
"command": "FillOrder",
417416
"order_id": Address::new(chain_config, *order_id).expect("addressable").to_string(),
418417
"fill_atoms": json!({"atoms": fill.into_atoms().to_string()}),
419-
"destination": Address::new(chain_config, dest.clone()).expect("no error").as_str(),
420418
})
421419
}
422420
AccountCommand::ChangeTokenMetadataUri(token_id, metadata_uri) => {

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

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -329,15 +329,13 @@ impl ConstrainedValueAccumulator {
329329
O: OrdersAccountingOperations + OrdersAccountingView<Error = orders_accounting::Error>,
330330
{
331331
let result = match command {
332-
OrderAccountCommand::FillOrder(id, amount, _) => {
333-
Some(self.process_fill_order_command(
334-
chain_config,
335-
block_height,
336-
*id,
337-
*amount,
338-
orders_accounting_delta,
339-
)?)
340-
}
332+
OrderAccountCommand::FillOrder(id, amount) => Some(self.process_fill_order_command(
333+
chain_config,
334+
block_height,
335+
*id,
336+
*amount,
337+
orders_accounting_delta,
338+
)?),
341339
OrderAccountCommand::ConcludeOrder(order_id) => {
342340
Some(self.process_conclude_order_command(*order_id, orders_accounting_delta)?)
343341
}

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

Lines changed: 15 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,6 @@ fn fill_order_constraints(#[case] seed: Seed, #[case] version: OrdersVersion) {
309309
OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(
310310
order_id,
311311
(ask_amount + Amount::from_atoms(1)).unwrap(),
312-
Destination::AnyoneCanSpend,
313312
)),
314313
};
315314
let inputs = vec![
@@ -354,11 +353,9 @@ fn fill_order_constraints(#[case] seed: Seed, #[case] version: OrdersVersion) {
354353
AccountNonce::new(0),
355354
AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend),
356355
),
357-
OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(
358-
order_id,
359-
ask_amount,
360-
Destination::AnyoneCanSpend,
361-
)),
356+
OrdersVersion::V1 => {
357+
TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, ask_amount))
358+
}
362359
};
363360
let inputs = vec![
364361
TxInput::Utxo(UtxoOutPoint::new(
@@ -395,11 +392,9 @@ fn fill_order_constraints(#[case] seed: Seed, #[case] version: OrdersVersion) {
395392
AccountNonce::new(0),
396393
AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend),
397394
),
398-
OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(
399-
order_id,
400-
ask_amount,
401-
Destination::AnyoneCanSpend,
402-
)),
395+
OrdersVersion::V1 => {
396+
TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, ask_amount))
397+
}
403398
};
404399
let inputs = vec![
405400
TxInput::Utxo(UtxoOutPoint::new(
@@ -451,11 +446,9 @@ fn fill_order_constraints(#[case] seed: Seed, #[case] version: OrdersVersion) {
451446
AccountNonce::new(0),
452447
AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend),
453448
),
454-
OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(
455-
order_id,
456-
ask_amount,
457-
Destination::AnyoneCanSpend,
458-
)),
449+
OrdersVersion::V1 => {
450+
TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, ask_amount))
451+
}
459452
};
460453
let inputs = vec![
461454
TxInput::Utxo(UtxoOutPoint::new(
@@ -512,11 +505,9 @@ fn fill_order_constraints(#[case] seed: Seed, #[case] version: OrdersVersion) {
512505
AccountNonce::new(0),
513506
AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend),
514507
),
515-
OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(
516-
order_id,
517-
ask_amount,
518-
Destination::AnyoneCanSpend,
519-
)),
508+
OrdersVersion::V1 => {
509+
TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, ask_amount))
510+
}
520511
};
521512
let inputs = vec![
522513
TxInput::Utxo(UtxoOutPoint::new(
@@ -571,11 +562,9 @@ fn fill_order_constraints(#[case] seed: Seed, #[case] version: OrdersVersion) {
571562
AccountNonce::new(0),
572563
AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend),
573564
),
574-
OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(
575-
order_id,
576-
ask_amount,
577-
Destination::AnyoneCanSpend,
578-
)),
565+
OrdersVersion::V1 => {
566+
TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, ask_amount))
567+
}
579568
};
580569
let inputs = vec![
581570
TxInput::Utxo(UtxoOutPoint::new(

‎chainstate/src/detail/chainstateref/mod.rs‎

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,8 @@ use common::{
4141
},
4242
config::EpochIndex,
4343
tokens::{TokenAuxiliaryData, TokenId},
44-
AccountNonce, AccountType, Block, ChainConfig, GenBlock, GenBlockId, PoolId, Transaction,
45-
TxOutput, UtxoOutPoint,
44+
AccountNonce, AccountType, Block, ChainConfig, GenBlock, GenBlockId, OrderAccountCommand,
45+
PoolId, Transaction, TxInput, TxOutput, UtxoOutPoint,
4646
},
4747
primitives::{
4848
id::WithId, time::Time, Amount, BlockCount, BlockDistance, BlockHeight, Id, Idable,
@@ -779,14 +779,29 @@ impl<'a, S: BlockchainStorageRead, V: TransactionVerificationStrategy> Chainstat
779779

780780
#[log_error]
781781
fn check_duplicate_inputs(&self, block: &Block) -> Result<(), CheckBlockTransactionsError> {
782-
// check for duplicate inputs (see CVE-2018-17144)
783-
let mut block_inputs = BTreeSet::new();
782+
// Reject the block if it has duplicate inputs, with the exception of v1 FillOrder inputs,
783+
// which can't be unique (because they only contain the order id and the amount).
784+
// Note that this is a precaution "inspired" by the Bitcoin vulnerability CVE-2018-17144.
785+
// I.e. even if this check is removed, the corresponding erroneous conditions (like spending
786+
// the same UTXO twice or concluding an already concluded order) should be captured elsewhere.
787+
let mut block_unique_inputs = BTreeSet::new();
784788
for tx in block.transactions() {
785789
for input in tx.inputs() {
786-
ensure!(
787-
block_inputs.insert(input),
788-
CheckBlockTransactionsError::DuplicateInputInBlock(block.get_id())
789-
);
790+
let must_be_unique = match input {
791+
TxInput::Utxo(_) | TxInput::Account(_) | TxInput::AccountCommand(_, _) => true,
792+
TxInput::OrderAccountCommand(cmd) => match cmd {
793+
OrderAccountCommand::FillOrder(_, _) => false,
794+
| OrderAccountCommand::FreezeOrder(_)
795+
| OrderAccountCommand::ConcludeOrder(_) => true,
796+
},
797+
};
798+
799+
if must_be_unique {
800+
ensure!(
801+
block_unique_inputs.insert(input),
802+
CheckBlockTransactionsError::DuplicateInputInBlock(block.get_id())
803+
);
804+
}
790805
}
791806
}
792807
Ok(())

‎chainstate/src/rpc/types/account.rs‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,6 @@ pub enum RpcOrderAccountCommand {
145145
Fill {
146146
order_id: RpcAddress<OrderId>,
147147
fill_value: RpcAmountOut,
148-
destination: RpcAddress<Destination>,
149148
},
150149
Freeze {
151150
order_id: RpcAddress<OrderId>,
@@ -161,10 +160,9 @@ impl RpcOrderAccountCommand {
161160
OrderAccountCommand::ConcludeOrder(order_id) => RpcOrderAccountCommand::Conclude {
162161
order_id: RpcAddress::new(chain_config, *order_id)?,
163162
},
164-
OrderAccountCommand::FillOrder(id, fill, dest) => RpcOrderAccountCommand::Fill {
163+
OrderAccountCommand::FillOrder(id, fill) => RpcOrderAccountCommand::Fill {
165164
order_id: RpcAddress::new(chain_config, *id)?,
166165
fill_value: RpcAmountOut::from_amount(*fill, chain_config.coin_decimals()),
167-
destination: RpcAddress::new(chain_config, dest.clone())?,
168166
},
169167
OrderAccountCommand::FreezeOrder(id) => RpcOrderAccountCommand::Freeze {
170168
order_id: RpcAddress::new(chain_config, *id)?,

‎chainstate/test-framework/src/random_tx_maker.rs‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1021,13 +1021,9 @@ impl<'a> RandomTxMaker<'a> {
10211021
if !is_frozen_token(&filled_value, tokens_cache)
10221022
&& !is_frozen_order(&orders_cache, order_id)
10231023
{
1024-
let input =
1025-
TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(
1026-
order_id,
1027-
amount_to_spend,
1028-
key_manager
1029-
.new_destination(self.chainstate.get_chain_config(), rng),
1030-
));
1024+
let input = TxInput::OrderAccountCommand(
1025+
OrderAccountCommand::FillOrder(order_id, amount_to_spend),
1026+
);
10311027

10321028
let output = TxOutput::Transfer(
10331029
filled_value,
@@ -1260,10 +1256,6 @@ impl<'a> RandomTxMaker<'a> {
12601256
OrderAccountCommand::FillOrder(
12611257
order_id,
12621258
Amount::from_atoms(atoms),
1263-
key_manager.new_destination(
1264-
self.chainstate.get_chain_config(),
1265-
rng,
1266-
),
12671259
),
12681260
));
12691261

0 commit comments

Comments
 (0)