Merge #1068

1068: fix potential overflow in `max_bitcoin_for_price` r=delta1 a=delta1

In testing, ASB panicked in `max_bitcoin_for_price` when the Monero balance x Bitcoin price was enough to overflow `u64`.

This PR 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 also 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.

Co-authored-by: Byron Hambly <bhambly@blockstream.com>

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
 

swap/src/asb/event_loop.rs

@@ -326,11 +326,15 @@ where
             .ask()
             .context("Failed to compute asking price")?;
 
-        let max_bitcoin_for_monero = self
+        let xmr = self.monero_wallet.get_balance().await?;
-            .monero_wallet
+
-            .get_balance()
+        let max_bitcoin_for_monero = xmr.max_bitcoin_for_price(ask_price).ok_or_else(|| {
-            .await?
+            anyhow::anyhow!(
-            .max_bitcoin_for_price(ask_price);
+                "Bitcoin price ({}) x Monero ({}) calculation overflow",
+                ask_price,
+                xmr,
+            )
+        })?;
 
         if min_buy > max_bitcoin_for_monero {
             tracing::warn!(

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)
+// 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(30000000);
+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<bitcoin::Amount> {
         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
+        // divide first to reduce chance of multiplication overflow
-        // Satoshis
+        let xmr = piconero_minus_fee / PICONERO_OFFSET;
-        let piconero_calc = (piconero_minus_fee * ask_price.as_sat()) / 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<Self> {
@@ -375,16 +375,36 @@ mod tests {
     }
 
     #[test]
-    fn geting_max_bitcoin_to_trade() {
+    fn max_bitcoin_to_trade() {
-        let amount = Amount::parse_monero("10").unwrap();
+        // sanity check: if the asking price is 1 BTC / 1 XMR
-        let bitcoin_price_sats = bitcoin::Amount::from_sat(382_900);
+        // 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!(
+        let xmr = Amount::parse_monero("10").unwrap();
-            bitcoin::Amount::from_sat(3_828_988),
+        let ask = bitcoin::Amount::from_sat(382_900);
-            monero_max_from_bitcoin
+        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);
     }