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.
This commit is contained in:
Byron Hambly 2022-07-28 14:37:07 +02:00
parent d5bb1e9c08
commit 39e34a608b
No known key found for this signature in database
GPG Key ID: DE8F6EA20A661697
3 changed files with 48 additions and 22 deletions

View File

@ -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

View File

@ -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!(

View File

@ -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<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
// 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<Self> {
@ -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);
}