From 4d59099d48b92816fb0b2952b3ceabcfae8a215b Mon Sep 17 00:00:00 2001 From: Daniel Jordon Date: Thu, 9 Jan 2025 09:30:07 -0500 Subject: [PATCH] fix: reject empty pre-sign requests (#1190) * Reject basically empty bitcoin transaction pre-sign requests * Add more empty request checks --- signer/src/bitcoin/utxo.rs | 21 +- signer/src/bitcoin/validation.rs | 344 ++++++++++++++++++++++++------- signer/src/error.rs | 15 ++ 3 files changed, 288 insertions(+), 92 deletions(-) diff --git a/signer/src/bitcoin/utxo.rs b/signer/src/bitcoin/utxo.rs index 8b2bada01..4156abfb8 100644 --- a/signer/src/bitcoin/utxo.rs +++ b/signer/src/bitcoin/utxo.rs @@ -798,7 +798,7 @@ impl<'a> UnsignedTransaction<'a> { /// Construct an unsigned transaction. /// /// This function can fail if the output amounts are greater than the - /// input amounts. + /// input amounts or if the [`Requests`] object is empty. /// /// The returned BTC transaction has the following properties: /// 1. The amounts for each output has taken fees into consideration. @@ -822,7 +822,7 @@ impl<'a> UnsignedTransaction<'a> { /// Construct a transaction with stub witness data. /// /// This function can fail if the output amounts are greater than the - /// input amounts. + /// input amounts or if the [`Requests`] object is empty. /// /// The returned BTC transaction has the following properties: /// 1. The amounts for each output has taken fees into consideration. @@ -833,6 +833,9 @@ impl<'a> UnsignedTransaction<'a> { /// 5. All witness data is correctly set, except for the fake /// signatures from (4). pub fn new_stub(requests: Requests<'a>, state: &SignerBtcState) -> Result { + if requests.is_empty() { + return Err(Error::BitcoinNoRequests); + } // Construct a transaction base. This transaction's inputs have // witness data with dummy signatures so that our virtual size // estimates are accurate. Later we will update the fees. @@ -1806,10 +1809,9 @@ mod tests { assert_eq!(new_utxo.public_key, requests.signer_state.public_key); } - /// Transactions that do not service requests do not have the OP_RETURN - /// output. + /// You cannot create sweep transactions that do not service requests. #[test] - fn no_requests_no_op_return() { + fn no_requests_no_sweep() { let public_key = XOnlyPublicKey::from_str(X_ONLY_PUBLIC_KEY1).unwrap(); let signer_state = SignerBtcState { utxo: SignerUtxo { @@ -1823,14 +1825,9 @@ mod tests { magic_bytes: [0; 2], }; - // No requests so the first output should be the signers UTXO and - // that's it. No OP_RETURN output. let requests = Requests::new(Vec::new()); - let unsigned = UnsignedTransaction::new(requests, &signer_state).unwrap(); - assert_eq!(unsigned.tx.output.len(), 1); - - // There is only one output and it's a P2TR output - assert!(unsigned.tx.output[0].script_pubkey.is_p2tr()); + let sweep = UnsignedTransaction::new(requests, &signer_state); + assert!(sweep.is_err()); } /// We aggregate the bitmaps to form a single one at the end. Check diff --git a/signer/src/bitcoin/validation.rs b/signer/src/bitcoin/validation.rs index e299266b8..a82d577d3 100644 --- a/signer/src/bitcoin/validation.rs +++ b/signer/src/bitcoin/validation.rs @@ -98,15 +98,29 @@ pub fn is_unique(package: &[TxRequestIds]) -> bool { } impl BitcoinPreSignRequest { - /// Validate the current bitcoin transaction. - async fn pre_validation<'a, C>(&self, ctx: &C, cache: &ValidationCache<'a>) -> Result<(), Error> - where - C: Context + Send + Sync, - { + /// Check that the request object is valid + // TODO: Have the type system do these checks. Perhaps TxRequestIds + // should really be a wrapper around something like a (frozen) + // NonEmptySet> with the + // `request_package` field being a NonEmptySlice. + fn pre_validation(&self) -> Result<(), Error> { + let no_requests = self + .request_package + .iter() + .any(|x| x.deposits.is_empty() && x.withdrawals.is_empty()); + + if no_requests || self.request_package.is_empty() { + return Err(Error::PreSignContainsNoRequests); + } + if !is_unique(&self.request_package) { return Err(Error::DuplicateRequests); } - self.validate_max_mintable(ctx, cache).await?; + + if self.fee_rate <= 0.0 { + return Err(Error::PreSignInvalidFeeRate(self.fee_rate)); + } + Ok(()) } @@ -120,8 +134,8 @@ impl BitcoinPreSignRequest { { let mut cache = ValidationCache::default(); - // Fetch all deposit reports and votes for requests in &self.request_package { + // Fetch all deposit reports and votes for outpoint in &requests.deposits { let txid = outpoint.txid.into(); let output_index = outpoint.vout; @@ -210,9 +224,11 @@ impl BitcoinPreSignRequest { where C: Context + Send + Sync, { + // Let's do basic validation of the request object itself. + self.pre_validation()?; let cache = self.fetch_all_reports(&ctx.get_storage(), btc_ctx).await?; - self.pre_validation(ctx, &cache).await?; + self.validate_max_mintable(ctx, &cache).await?; let signer_utxo = ctx .get_storage() @@ -438,6 +454,13 @@ impl BitcoinTxValidationData { /// not a part of the signing set locking one or more deposits. In such /// a case, it will just sign for the deposits that it can. pub fn is_valid_tx(&self) -> bool { + // A transaction is invalid if it is not servicing any deposit or + // withdrawal requests. Doing so costs fees and the signers do not + // gain anything by permitting such a transaction. + if self.reports.deposits.is_empty() && self.reports.withdrawals.is_empty() { + return false; + } + let deposit_validation_results = self.reports.deposits.iter().all(|(_, report)| { matches!( report.validate( @@ -1190,85 +1213,246 @@ mod tests { } #[test_case( - vec![TxRequestIds { - deposits: vec![ - OutPoint { txid: Txid::from_byte_array([1; 32]), vout: 0 }, - OutPoint { txid: Txid::from_byte_array([1; 32]), vout: 1 } - ], - withdrawals: vec![ - QualifiedRequestId { - request_id: 0, - txid: StacksTxId::from([1; 32]), - block_hash: StacksBlockHash::from([1; 32]), - }, - QualifiedRequestId { - request_id: 0, - txid: StacksTxId::from([1; 32]), - block_hash: StacksBlockHash::from([2; 32]), - } - ]}], true; "unique-requests")] + BitcoinPreSignRequest { + request_package: vec![TxRequestIds { + deposits: vec![ + OutPoint { + txid: Txid::from_byte_array([1; 32]), + vout: 0, + }, + OutPoint { + txid: Txid::from_byte_array([1; 32]), + vout: 1, + }, + ], + withdrawals: vec![ + QualifiedRequestId { + request_id: 0, + txid: StacksTxId::from([1; 32]), + block_hash: StacksBlockHash::from([1; 32]), + }, + QualifiedRequestId { + request_id: 0, + txid: StacksTxId::from([1; 32]), + block_hash: StacksBlockHash::from([2; 32]), + }, + ], + }], + fee_rate: 1.0, + last_fees: None, + }, true; "unique-requests")] #[test_case( - vec![TxRequestIds { - deposits: vec![ - OutPoint { txid: Txid::from_byte_array([1; 32]), vout: 0 }, - OutPoint { txid: Txid::from_byte_array([1; 32]), vout: 0 } - ], - withdrawals: vec![ - QualifiedRequestId { - request_id: 0, - txid: StacksTxId::from([1; 32]), - block_hash: StacksBlockHash::from([1; 32]), - }, - QualifiedRequestId { - request_id: 0, - txid: StacksTxId::from([1; 32]), - block_hash: StacksBlockHash::from([2; 32]), - } - ]}], false; "duplicate-deposits-in-same-tx")] + BitcoinPreSignRequest { + request_package: vec![TxRequestIds { + deposits: vec![ + OutPoint { + txid: Txid::from_byte_array([1; 32]), + vout: 0, + }, + OutPoint { + txid: Txid::from_byte_array([1; 32]), + vout: 1, + }, + ], + withdrawals: vec![ + QualifiedRequestId { + request_id: 0, + txid: StacksTxId::from([1; 32]), + block_hash: StacksBlockHash::from([1; 32]), + }, + QualifiedRequestId { + request_id: 0, + txid: StacksTxId::from([1; 32]), + block_hash: StacksBlockHash::from([2; 32]), + }, + ], + }], + fee_rate: 0.0, + last_fees: None, + }, false; "unique-requests-zero-fee-rate")] #[test_case( - vec![TxRequestIds { - deposits: vec![ - OutPoint { txid: Txid::from_byte_array([1; 32]), vout: 0 }, - OutPoint { txid: Txid::from_byte_array([1; 32]), vout: 1 } - ], - withdrawals: vec![ - QualifiedRequestId { - request_id: 0, - txid: StacksTxId::from([1; 32]), - block_hash: StacksBlockHash::from([1; 32]), + BitcoinPreSignRequest { + request_package: vec![TxRequestIds { + deposits: vec![ + OutPoint { + txid: Txid::from_byte_array([1; 32]), + vout: 0, + }, + OutPoint { + txid: Txid::from_byte_array([1; 32]), + vout: 1, + }, + ], + withdrawals: vec![ + QualifiedRequestId { + request_id: 0, + txid: StacksTxId::from([1; 32]), + block_hash: StacksBlockHash::from([1; 32]), + }, + QualifiedRequestId { + request_id: 0, + txid: StacksTxId::from([1; 32]), + block_hash: StacksBlockHash::from([2; 32]), + }, + ], + }], + fee_rate: -1.0, + last_fees: None, + }, false; "unique-requests-negative-fee-rate")] + #[test_case( + BitcoinPreSignRequest { + request_package: vec![TxRequestIds { + deposits: vec![ + OutPoint { + txid: Txid::from_byte_array([1; 32]), + vout: 0, + }, + OutPoint { + txid: Txid::from_byte_array([1; 32]), + vout: 0, + }, + ], + withdrawals: vec![ + QualifiedRequestId { + request_id: 0, + txid: StacksTxId::from([1; 32]), + block_hash: StacksBlockHash::from([1; 32]), + }, + QualifiedRequestId { + request_id: 0, + txid: StacksTxId::from([1; 32]), + block_hash: StacksBlockHash::from([2; 32]), + }, + ], + }], + fee_rate: 1.0, + last_fees: None, + }, false; "duplicate-deposits-in-same-tx")] + #[test_case( + BitcoinPreSignRequest { + request_package: vec![TxRequestIds { + deposits: vec![ + OutPoint { + txid: Txid::from_byte_array([1; 32]), + vout: 0, + }, + OutPoint { + txid: Txid::from_byte_array([1; 32]), + vout: 1, + }, + ], + withdrawals: vec![ + QualifiedRequestId { + request_id: 0, + txid: StacksTxId::from([1; 32]), + block_hash: StacksBlockHash::from([1; 32]), + }, + QualifiedRequestId { + request_id: 0, + txid: StacksTxId::from([1; 32]), + block_hash: StacksBlockHash::from([1; 32]), + }, + ], + }], + fee_rate: 1.0, + last_fees: None, + }, false; "duplicate-withdrawals-in-same-tx")] + #[test_case( + BitcoinPreSignRequest { + request_package: vec![ + TxRequestIds { + deposits: vec![ + OutPoint { + txid: Txid::from_byte_array([1; 32]), + vout: 0, + }, + OutPoint { + txid: Txid::from_byte_array([1; 32]), + vout: 1, + }, + ], + withdrawals: vec![ + QualifiedRequestId { + request_id: 0, + txid: StacksTxId::from([1; 32]), + block_hash: StacksBlockHash::from([1; 32]), + }, + QualifiedRequestId { + request_id: 1, + txid: StacksTxId::from([1; 32]), + block_hash: StacksBlockHash::from([2; 32]), + }, + ], }, - QualifiedRequestId { - request_id: 0, - txid: StacksTxId::from([1; 32]), - block_hash: StacksBlockHash::from([1; 32]), - } - ]}], false; "duplicate-withdrawals-in-same-tx")] + TxRequestIds { + deposits: vec![OutPoint { + txid: Txid::from_byte_array([1; 32]), + vout: 1, + }], + withdrawals: vec![], + }, + ], + fee_rate: 1.0, + last_fees: None, + }, false; "duplicate-requests-in-different-txs")] + #[test_case( + BitcoinPreSignRequest { + request_package: Vec::new(), + fee_rate: 1.0, + last_fees: None, + }, false; "empty-package_requests")] #[test_case( - vec![TxRequestIds { - deposits: vec![ - OutPoint { txid: Txid::from_byte_array([1; 32]), vout: 0 }, - OutPoint { txid: Txid::from_byte_array([1; 32]), vout: 1 } + BitcoinPreSignRequest { + request_package: vec![ + TxRequestIds { + deposits: Vec::new(), + withdrawals: Vec::new(), + }, + TxRequestIds { + deposits: Vec::new(), + withdrawals: Vec::new(), + }, ], - withdrawals: vec![ - QualifiedRequestId { - request_id: 0, - txid: StacksTxId::from([1; 32]), - block_hash: StacksBlockHash::from([1; 32]), + fee_rate: 1.0, + last_fees: None, + }, false; "basically-empty-package_requests")] + #[test_case( + BitcoinPreSignRequest { + request_package: vec![ + TxRequestIds { + deposits: vec![ + OutPoint { + txid: Txid::from_byte_array([1; 32]), + vout: 0, + }, + OutPoint { + txid: Txid::from_byte_array([1; 32]), + vout: 1, + }, + ], + withdrawals: vec![ + QualifiedRequestId { + request_id: 0, + txid: StacksTxId::from([1; 32]), + block_hash: StacksBlockHash::from([1; 32]), + }, + QualifiedRequestId { + request_id: 0, + txid: StacksTxId::from([1; 32]), + block_hash: StacksBlockHash::from([2; 32]), + }, + ], + }, + TxRequestIds { + deposits: Vec::new(), + withdrawals: Vec::new(), }, - QualifiedRequestId { - request_id: 1, - txid: StacksTxId::from([1; 32]), - block_hash: StacksBlockHash::from([2; 32]), - } - ]}, - TxRequestIds { - deposits: vec![ - OutPoint { txid: Txid::from_byte_array([1; 32]), vout: 1 } ], - withdrawals: vec![] - }], false; "duplicate-requests-in-different-txs")] - fn test_is_unique(requests: Vec, result: bool) { - assert_eq!(is_unique(&requests), result); + fee_rate: 1.0, + last_fees: None, + }, false; "contains-empty-tx-requests")] + fn test_pre_validation(requests: BitcoinPreSignRequest, result: bool) { + assert_eq!(requests.pre_validation().is_ok(), result); } fn create_test_report(idx: u8, amount: u64) -> (DepositRequestReport, SignerVotes) { diff --git a/signer/src/error.rs b/signer/src/error.rs index 00bc888f3..4e227f553 100644 --- a/signer/src/error.rs +++ b/signer/src/error.rs @@ -530,6 +530,21 @@ pub enum Error { #[error("The request packages contain duplicate deposit or withdrawal entries.")] DuplicateRequests, + /// Indicates that the BitcoinPreSignRequest object does not contain + /// any deposit or withdrawal requests. + #[error("The BitcoinPreSignRequest object does not contain deposit or withdrawal requests")] + PreSignContainsNoRequests, + + /// Indicates that we tried to create an UnsignedTransaction object + /// without any deposit or withdrawal requests. + #[error("The UnsignedTransaction must contain deposit or withdrawal requests")] + BitcoinNoRequests, + + /// Indicates that the BitcoinPreSignRequest object contains a fee rate + /// that is less than or equal to zero. + #[error("The fee rate in the BitcoinPreSignRequest object is not greater than zero: {0}")] + PreSignInvalidFeeRate(f64), + /// Error when deposit requests would exceed sBTC supply cap #[error("Total deposit amount ({total_amount} sats) would exceed sBTC supply cap (current max mintable is {max_mintable} sats)")] ExceedsSbtcSupplyCap {