Skip to content

Commit

Permalink
fix: ensure mint amount is greater than the dust limit (#1178)
Browse files Browse the repository at this point in the history
* Handle the case where the deposit amount is below the dust limit in stacks validation
* Handle low mint amounts during deposit request filtering
* Make sure that we validate mint amount and dust amounts in bitcoin validation
  • Loading branch information
djordon authored Dec 23, 2024
1 parent 891677e commit e4c6b5f
Show file tree
Hide file tree
Showing 7 changed files with 303 additions and 51 deletions.
72 changes: 68 additions & 4 deletions signer/src/bitcoin/utxo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ use crate::storage::model::TxOutput;
use crate::storage::model::TxOutputType;
use crate::storage::model::TxPrevout;
use crate::storage::model::TxPrevoutType;
use crate::DEPOSIT_DUST_LIMIT;

/// The minimum incremental fee rate in sats per virtual byte for RBF
/// transactions.
Expand Down Expand Up @@ -172,11 +173,14 @@ impl SbtcRequests {
})
.map(RequestRef::Withdrawal);

// Filter deposit requests based on two constraints:
// 1. The user's max fee must be >= our minimum required fee for deposits
// (based on fixed deposit tx size)
// Filter deposit requests based on four constraints:
// 1. The user's max fee must be >= our minimum required fee for
// deposits (based on fixed deposit tx size)
// 2. The deposit amount must be less than the per-deposit limit
// 3. The total amount being minted must stay under the maximum allowed mintable amount
// 3. The total amount being minted must stay under the maximum
// allowed mintable amount
// 4. The mint amount is above the deposit dust limit in the smart
// contract.
let minimum_deposit_fee = self.compute_minimum_fee(SOLO_DEPOSIT_TX_VSIZE);
let max_mintable_cap = self.sbtc_limits.max_mintable_cap().to_sat();
let per_deposit_cap = self.sbtc_limits.per_deposit_cap().to_sat();
Expand All @@ -191,6 +195,9 @@ impl SbtcRequests {
} else {
false
};
if req.amount.saturating_sub(minimum_deposit_fee) < DEPOSIT_DUST_LIMIT {
return None;
}
if is_fee_valid && is_within_per_deposit_cap && is_within_max_mintable_cap {
amount_to_mint += req.amount;
Some(RequestRef::Deposit(req))
Expand Down Expand Up @@ -2849,6 +2856,63 @@ mod tests {
assert_eq!(total_amount, accepted_amount);
}

#[test_case(
create_deposit(
DEPOSIT_DUST_LIMIT + SOLO_DEPOSIT_TX_VSIZE as u64,
10_000,
0
),
true; "deposit amounts over the dust limit accepted")]
#[test_case(
create_deposit(
DEPOSIT_DUST_LIMIT + SOLO_DEPOSIT_TX_VSIZE as u64 - 1,
10_000,
0
),
false; "deposit amounts under the dust limit rejected")]
fn deposit_requests_respect_dust_limits(req: DepositRequest, is_included: bool) {
let outpoint = req.outpoint;
let public_key = XOnlyPublicKey::from_str(X_ONLY_PUBLIC_KEY1).unwrap();

// We use a fee rate of 1 to simplify the computation. The
// filtering done here uses a heuristic where we take the maximum
// fee that the user could pay, and subtract that amount from the
// deposit amount. The maximum fee that a user could pay is the
// SOLO_DEPOSIT_TX_VSIZE times the fee rate so with a fee rate of 1
// we should filter the request if the deposit amount is less than
// SOLO_DEPOSIT_TX_VSIZE + DEPOSIT_DUST_LIMIT.
let requests = SbtcRequests {
deposits: vec![create_deposit(2500000, 100000, 0), req],
withdrawals: vec![],
signer_state: SignerBtcState {
utxo: SignerUtxo {
outpoint: generate_outpoint(300_000, 0),
amount: 300_000_000,
public_key,
},
fee_rate: 1.0,
public_key,
last_fees: None,
magic_bytes: [0; 2],
},
num_signers: 11,
accept_threshold: 6,
sbtc_limits: SbtcLimits::default(),
};

// Let's construct the unsigned transaction and check to see if we
// include it in the deposit requests in the transaction.
let tx = requests.construct_transactions().unwrap().pop().unwrap();
let request_is_included = tx
.requests
.iter()
.filter_map(RequestRef::as_deposit)
.find(|req| req.outpoint == outpoint)
.is_some();

assert_eq!(request_is_included, is_included);
}

#[test]
fn test_construct_transactions_capped_by_number() {
// with 30 deposits and 30 withdrawals with 4 votes against each, we should generate 60 distinct transactions
Expand Down
44 changes: 42 additions & 2 deletions signer/src/bitcoin/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use crate::storage::model::BitcoinWithdrawalOutput;
use crate::storage::model::QualifiedRequestId;
use crate::storage::model::SignerVotes;
use crate::storage::DbRead;
use crate::DEPOSIT_DUST_LIMIT;
use crate::DEPOSIT_LOCKTIME_BLOCK_BUFFER;

use super::utxo::DepositRequest;
Expand Down Expand Up @@ -264,7 +265,7 @@ impl BitcoinPreSignRequest {
.deposit_reports
.get(&key)
// This should never happen because we have already validated that we have all the reports.
.ok_or(InputValidationResult::Unknown.into_error(btc_ctx))?;
.ok_or_else(|| InputValidationResult::Unknown.into_error(btc_ctx))?;
deposits.push((report.to_deposit_request(votes), report.clone()));
}

Expand All @@ -273,7 +274,7 @@ impl BitcoinPreSignRequest {
.withdrawal_reports
.get(id)
// This should never happen because we have already validated that we have all the reports.
.ok_or(WithdrawalValidationResult::Unknown.into_error(btc_ctx))?;
.ok_or_else(|| WithdrawalValidationResult::Unknown.into_error(btc_ctx))?;
withdrawals.push((report.to_withdrawal_request(votes), report.clone()));
}

Expand Down Expand Up @@ -508,6 +509,9 @@ impl SbtcReports {
pub enum InputValidationResult {
/// The deposit request passed validation
Ok,
/// The deposit request amount, less the fees, would be rejected from
/// the smart contract during the complete-deposit contract call.
MintAmountBelowDustLimit,
/// The deposit request amount exceeds the allowed per-deposit cap.
AmountTooHigh,
/// The assessed fee exceeds the max-fee in the deposit request.
Expand Down Expand Up @@ -730,6 +734,10 @@ impl DepositRequestReport {
return InputValidationResult::FeeTooHigh;
}

if self.amount.saturating_sub(assessed_fee.to_sat()) < DEPOSIT_DUST_LIMIT {
return InputValidationResult::MintAmountBelowDustLimit;
}

// Let's check whether we rejected this deposit.
match self.can_accept {
Some(true) => (),
Expand Down Expand Up @@ -1063,6 +1071,38 @@ mod tests {
status: InputValidationResult::FeeTooHigh,
chain_tip_height: 2,
} ; "one-sat-too-high-fee-amount")]
#[test_case(DepositReportErrorMapping {
report: DepositRequestReport {
status: DepositConfirmationStatus::Confirmed(0, BitcoinBlockHash::from([0; 32])),
can_sign: Some(true),
can_accept: Some(true),
amount: TX_FEE.to_sat() + DEPOSIT_DUST_LIMIT - 1,
max_fee: TX_FEE.to_sat(),
lock_time: LockTime::from_height(DEPOSIT_LOCKTIME_BLOCK_BUFFER + 3),
outpoint: OutPoint::null(),
deposit_script: ScriptBuf::new(),
reclaim_script: ScriptBuf::new(),
signers_public_key: *sbtc::UNSPENDABLE_TAPROOT_KEY,
},
status: InputValidationResult::MintAmountBelowDustLimit,
chain_tip_height: 2,
} ; "one-sat-under-dust-amount")]
#[test_case(DepositReportErrorMapping {
report: DepositRequestReport {
status: DepositConfirmationStatus::Confirmed(0, BitcoinBlockHash::from([0; 32])),
can_sign: Some(true),
can_accept: Some(true),
amount: TX_FEE.to_sat() + DEPOSIT_DUST_LIMIT,
max_fee: TX_FEE.to_sat(),
lock_time: LockTime::from_height(DEPOSIT_LOCKTIME_BLOCK_BUFFER + 3),
outpoint: OutPoint::null(),
deposit_script: ScriptBuf::new(),
reclaim_script: ScriptBuf::new(),
signers_public_key: *sbtc::UNSPENDABLE_TAPROOT_KEY,
},
status: InputValidationResult::Ok,
chain_tip_height: 2,
} ; "at-dust-amount")]
#[test_case(DepositReportErrorMapping {
report: DepositRequestReport {
status: DepositConfirmationStatus::Confirmed(0, BitcoinBlockHash::from([0; 32])),
Expand Down
5 changes: 5 additions & 0 deletions signer/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@ pub const MAX_REORG_BLOCK_COUNT: i64 = 10;
/// per block.
pub const MAX_TX_PER_BITCOIN_BLOCK: i64 = 25;

/// This is the dust limit for deposits in the sBTC smart contracts.
/// Deposit amounts that is less than this amount will be rejected by the
/// smart contract.
pub const DEPOSIT_DUST_LIMIT: u64 = 546;

/// These are all build info variables. Many of them are set in build.rs.
/// The name of the binary that is being run,
Expand Down
22 changes: 12 additions & 10 deletions signer/src/stacks/contracts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ use crate::storage::model::BitcoinBlockRef;
use crate::storage::model::BitcoinTxId;
use crate::storage::model::ToLittleEndianOrder as _;
use crate::storage::DbRead;
use crate::DEPOSIT_DUST_LIMIT;

use super::api::StacksInteract;

Expand Down Expand Up @@ -347,7 +348,7 @@ impl AsContractCall for CompleteDepositV1 {
/// as an input.
/// 5. That the recipients in the transaction matches that of the
/// deposit request.
/// 6. That the amount to mint does not exceed the deposit amount.
/// 6. That the amount to mint is above the dust amount.
/// 7. That the fee matches the expected assessed fee for the outpoint.
/// 8. That the fee is less than the specified max-fee.
/// 9. That the first input into the sweep transaction is the signers'
Expand Down Expand Up @@ -387,7 +388,7 @@ impl CompleteDepositV1 {
/// of swept deposit requests.
/// 5. That the recipients in the transaction matches that of the
/// deposit request.
/// 6. That the amount to mint does not exceed the deposit amount.
/// 6. That the amount to mint is above the dust amount.
/// 7. That the fee matches the expected assessed fee for the outpoint.
/// 8. That the fee is less than the specified max-fee.
///
Expand Down Expand Up @@ -418,10 +419,10 @@ impl CompleteDepositV1 {
if &self.recipient != deposit_request.recipient.deref() {
return Err(DepositErrorMsg::RecipientMismatch.into_error(req_ctx, self));
}
// 6. Check that the amount to mint does not exceed the deposit
// 6. Check that the amount to mint is above the dust amount
// amount.
if self.amount > deposit_request.amount {
return Err(DepositErrorMsg::InvalidMintAmount.into_error(req_ctx, self));
if self.amount < DEPOSIT_DUST_LIMIT {
return Err(DepositErrorMsg::AmountBelowDustLimit.into_error(req_ctx, self));
}
// 7. That the fee matches the expected assessed fee for the outpoint.
if fee.to_sat() + self.amount != deposit_request.amount {
Expand Down Expand Up @@ -547,19 +548,20 @@ impl std::error::Error for DepositValidationError {
/// transactions.
#[derive(Debug, thiserror::Error, PartialEq, Eq)]
pub enum DepositErrorMsg {
/// The smart contract has a dust limit which is used to rejects
/// contract calls if the mint amount is below that limit. We check for
/// that condition here before minting.
#[error("the amount to mint is below the dust limit in the smart contract")]
AmountBelowDustLimit,
/// The smart contract deployer is fixed, so this should always match.
#[error("The deployer in the transaction does not match the expected deployer")]
#[error("the deployer in the transaction does not match the expected deployer")]
DeployerMismatch,
/// The fee paid to the bitcoin miners exceeded the max fee.
#[error("fee paid to the bitcoin miners exceeded the max fee")]
FeeTooHigh,
/// The supplied fee does not match what is expected.
#[error("the supplied fee does not match what is expected")]
IncorrectFee,
/// The amount to mint must not exceed the amount in the deposit
/// request.
#[error("amount to mint exceeded the amount in the deposit request")]
InvalidMintAmount,
/// The deposit outpoint is missing from the indicated sweep
/// transaction.
#[error("deposit outpoint is missing from the indicated sweep transaction")]
Expand Down
2 changes: 1 addition & 1 deletion signer/src/testing/block_observer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ impl TestHarness {
vout: Vec::new(),
block_hash: response
.block_hash
.unwrap_or(bitcoin::BlockHash::all_zeros()),
.unwrap_or_else(bitcoin::BlockHash::all_zeros),
confirmations: 0,
block_time: 0,
};
Expand Down
Loading

0 comments on commit e4c6b5f

Please sign in to comment.