From 39e34a608b93e3960e43b49c96f58318ea0e59e0 Mon Sep 17 00:00:00 2001 From: Byron Hambly Date: Thu, 28 Jul 2022 14:37:07 +0200 Subject: [PATCH] fix: fix potential overflow in `max_bitcoin_for_price` In testing, ASB panicked in `max_bitcoin_for_price` when the Monero balance x Bitcoin price was enough to overflow `u64`. This commit changes the function to do the piconero offset division first, and then to use `checked_mul` to return None if the calculation would overflow. This required changing the function return signature to an `Option`. Additional tests for the function were also added. MONERO_FEE was changed from 0.000030 to 0.000016, which is still double the current median transaction fee listed at https://www.monero.how/monero-transaction-fees as of 2022-07-28. --- CHANGELOG.md | 2 ++ swap/src/asb/event_loop.rs | 14 ++++++---- swap/src/monero.rs | 54 ++++++++++++++++++++++++++------------ 3 files changed, 48 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2bce431..7a5f8cec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Update from monero v17.2.0 to monero v17.3.0 - Always write logs as JSON to files - Change to UTC time for log messages, due to a bug causing no logging at all to be printed (linux/macos), and an [unsoundness issue](https://docs.rs/tracing-subscriber/latest/tracing_subscriber/fmt/time/struct.LocalTime.html) with local time in [the time crate](https://github.com/time-rs/time/issues/293#issuecomment-748151025) +- Fix potential integer overflow in ASB when calculating maximum Bitcoin amount for Monero balance +- Reduce Monero locking transaction fee amount from 0.000030 to 0.000016 XMR, which is still double the current median fee as reported at [monero.how](https://www.monero.how/monero-transaction-fees) ### Added diff --git a/swap/src/asb/event_loop.rs b/swap/src/asb/event_loop.rs index 1a06d95f..b819682d 100644 --- a/swap/src/asb/event_loop.rs +++ b/swap/src/asb/event_loop.rs @@ -326,11 +326,15 @@ where .ask() .context("Failed to compute asking price")?; - let max_bitcoin_for_monero = self - .monero_wallet - .get_balance() - .await? - .max_bitcoin_for_price(ask_price); + let xmr = self.monero_wallet.get_balance().await?; + + let max_bitcoin_for_monero = xmr.max_bitcoin_for_price(ask_price).ok_or_else(|| { + anyhow::anyhow!( + "Bitcoin price ({}) x Monero ({}) calculation overflow", + ask_price, + xmr, + ) + })?; if min_buy > max_bitcoin_for_monero { tracing::warn!( diff --git a/swap/src/monero.rs b/swap/src/monero.rs index bf3045ce..6d595cf0 100644 --- a/swap/src/monero.rs +++ b/swap/src/monero.rs @@ -81,8 +81,8 @@ pub struct PublicViewKey(PublicKey); #[derive(Debug, Copy, Clone, Deserialize, Serialize, PartialEq, PartialOrd)] pub struct Amount(u64); -// Median tx fees on Monero as found here: https://www.monero.how/monero-transaction-fees, XMR 0.000_015 * 2 (to be on the safe side) -pub const MONERO_FEE: Amount = Amount::from_piconero(30000000); +// Median tx fees on Monero as found here: https://www.monero.how/monero-transaction-fees, XMR 0.000_008 * 2 (to be on the safe side) +pub const MONERO_FEE: Amount = Amount::from_piconero(16_000_000); impl Amount { pub const ZERO: Self = Self(0); @@ -99,18 +99,18 @@ impl Amount { self.0 } - pub fn max_bitcoin_for_price(&self, ask_price: bitcoin::Amount) -> bitcoin::Amount { + pub fn max_bitcoin_for_price(&self, ask_price: bitcoin::Amount) -> Option { let piconero_minus_fee = self.as_piconero().saturating_sub(MONERO_FEE.as_piconero()); if piconero_minus_fee == 0 { - return bitcoin::Amount::ZERO; + return Some(bitcoin::Amount::ZERO); } - // There needs to be an offset for difference in zeroes beetween Piconeros and - // Satoshis - let piconero_calc = (piconero_minus_fee * ask_price.as_sat()) / PICONERO_OFFSET; + // divide first to reduce chance of multiplication overflow + let xmr = piconero_minus_fee / PICONERO_OFFSET; + let satoshi = xmr.checked_mul(ask_price.as_sat())?; - bitcoin::Amount::from_sat(piconero_calc) + Some(bitcoin::Amount::from_sat(satoshi)) } pub fn from_monero(amount: f64) -> Result { @@ -375,16 +375,36 @@ mod tests { } #[test] - fn geting_max_bitcoin_to_trade() { - let amount = Amount::parse_monero("10").unwrap(); - let bitcoin_price_sats = bitcoin::Amount::from_sat(382_900); + fn max_bitcoin_to_trade() { + // sanity check: if the asking price is 1 BTC / 1 XMR + // and we have 1 XMR + fee + // then max BTC we can buy is 1 + let xmr = Amount::parse_monero("1.0").unwrap() + MONERO_FEE; + let ask = bitcoin::Amount::from_btc(1.0).unwrap(); + let btc = xmr.max_bitcoin_for_price(ask).unwrap(); - let monero_max_from_bitcoin = amount.max_bitcoin_for_price(bitcoin_price_sats); + assert_eq!(ask, btc); - assert_eq!( - bitcoin::Amount::from_sat(3_828_988), - monero_max_from_bitcoin - ); + let xmr = Amount::parse_monero("10").unwrap(); + let ask = bitcoin::Amount::from_sat(382_900); + let btc = xmr.max_bitcoin_for_price(ask).unwrap(); + + assert_eq!(bitcoin::Amount::from_sat(3_446_100), btc); + } + + #[test] + fn max_bitcoin_to_trade_overflow() { + let xmr = Amount::from_monero(30.0).unwrap(); + let ask = bitcoin::Amount::from_sat(728_688); + let btc = xmr.max_bitcoin_for_price(ask).unwrap(); + + assert_eq!(bitcoin::Amount::from_sat(21_131_952), btc); + + let xmr = Amount::from_piconero(u64::MAX); + let ask = bitcoin::Amount::from_sat(u64::MAX); + let btc = xmr.max_bitcoin_for_price(ask); + + assert!(btc.is_none()); } #[test] @@ -393,7 +413,7 @@ mod tests { let amount = Amount::parse_monero(monero).unwrap(); let bitcoin_price_sats = bitcoin::Amount::from_sat(382_900); - let monero_max_from_bitcoin = amount.max_bitcoin_for_price(bitcoin_price_sats); + let monero_max_from_bitcoin = amount.max_bitcoin_for_price(bitcoin_price_sats).unwrap(); assert_eq!(bitcoin::Amount::ZERO, monero_max_from_bitcoin); }