Skip to content

Commit 8f73fa3

Browse files
author
Edward (Mike's bot)
committed
fix(order-book): widen money-math products to u128 to avoid spurious u64 overflow
place_order and cancel_order both do price * quantity in u64. u64 * u64 overflows at ~1.8e19 base units \u2014 perfectly reachable once you scale by token decimals (an 18-decimal quote mint hits it at modest mid-cap prices and quantities). u64::checked_mul would then refuse a legitimate order with NumericalOverflow even though the final lock amount fits a u64 balance fine. Promote both operands to u128 before the multiply, then narrow back to u64 with try_into. Same pattern the gross_quote-fee-quote chain was already using \u2014 now used consistently for: - bid lock (price * quantity) - per-fill gross_quote (fill_price * fill_quantity) - per-fill locked_for_this_fill rebate base (price * fill_quantity) - cancel_order Bid refund (order.price * remaining) Also: replace order.filled_quantity = quantity.saturating_sub(taker_remaining) with checked_sub. saturating_* on the matching engine's bookkeeping is a silent-clamp hazard; if taker_remaining > quantity that's a real bug and the program should abort, not write a misleading filled_quantity. Also: add require!(fee_quote <= gross_quote) after the fee calculation as a defence-in-depth invariant. fee_basis_points is bounded at init, so the require is unreachable in normal operation \u2014 but a stale bound assumption would otherwise let a misconfigured market overdraw the maker's net payout silently.
1 parent 7c3f9c8 commit 8f73fa3

2 files changed

Lines changed: 53 additions & 15 deletions

File tree

defi/order-book/anchor/programs/order-book/src/instructions/cancel_order.rs

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,16 @@ pub fn handle_cancel_order(context: Context<CancelOrder>) -> Result<()> {
2727
let market_user = &mut context.accounts.market_user;
2828
match order.side {
2929
OrderSide::Bid => {
30-
let quote_amount = order
31-
.price
32-
.checked_mul(remaining)
33-
.ok_or(ErrorCode::NumericalOverflow)?;
30+
// u128 intermediate: the lock was originally taken on a
31+
// u64 quote balance, so price * remaining must fit u64
32+
// — but the multiplication itself can transiently exceed
33+
// u64. Mirror the same pattern as place_order: widen,
34+
// multiply, narrow.
35+
let quote_amount: u64 = (order.price as u128)
36+
.checked_mul(remaining as u128)
37+
.ok_or(ErrorCode::NumericalOverflow)?
38+
.try_into()
39+
.map_err(|_| error!(ErrorCode::NumericalOverflow))?;
3440
market_user.unsettled_quote = market_user
3541
.unsettled_quote
3642
.checked_add(quote_amount)

defi/order-book/anchor/programs/order-book/src/instructions/place_order.rs

Lines changed: 43 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -50,15 +50,25 @@ pub fn handle_place_order<'info>(
5050
// (price * quantity); asks lock base (quantity). This always happens —
5151
// matching consumes from the locked pot (already in the vault), and any
5252
// unmatched remainder rests as a maker order with its lock still in place.
53+
//
54+
// The bid lock multiplies two u64s. A plain `u64::checked_mul` would
55+
// refuse anything that overflows u64 (~1.8e19) — which is a perfectly
56+
// legal lock once you scale by token decimals (e.g. 18-decimal quote
57+
// mint * mid-cap price * mid-cap quantity). Promote to u128 for the
58+
// multiplication, then narrow back to u64 with try_into so the failure
59+
// mode is "can't fit the on-chain transfer" not "silently rejected at
60+
// the math step".
5361
let (source_account, mint_account_info, decimals, transfer_amount, destination_vault) =
5462
match side {
5563
OrderSide::Bid => (
5664
context.accounts.user_quote_account.to_account_info(),
5765
context.accounts.quote_mint.to_account_info(),
5866
context.accounts.quote_mint.decimals,
59-
price
60-
.checked_mul(quantity)
61-
.ok_or(ErrorCode::NumericalOverflow)?,
67+
(price as u128)
68+
.checked_mul(quantity as u128)
69+
.ok_or(ErrorCode::NumericalOverflow)?
70+
.try_into()
71+
.map_err(|_| error!(ErrorCode::NumericalOverflow))?,
6272
context.accounts.quote_vault.to_account_info(),
6373
),
6474
OrderSide::Ask => (
@@ -173,10 +183,15 @@ pub fn handle_place_order<'info>(
173183
// lists. Real CLOBs (Openbook v2, Phoenix) use a similar
174184
// deduct-from-gross pattern for simplicity; the fee can be thought
175185
// of as the maker pricing their ask a fraction higher to cover it.
176-
let gross_quote: u64 = fill
177-
.fill_price
178-
.checked_mul(fill.fill_quantity)
179-
.ok_or(ErrorCode::NumericalOverflow)?;
186+
// fill_price * fill_quantity in u128 to avoid u64-overflow on big
187+
// fills (high-decimal mints scale this product fast). Narrow back
188+
// to u64 with try_into so the transfer/balance step gets a real
189+
// u64 and the failure mode is explicit.
190+
let gross_quote: u64 = (fill.fill_price as u128)
191+
.checked_mul(fill.fill_quantity as u128)
192+
.ok_or(ErrorCode::NumericalOverflow)?
193+
.try_into()
194+
.map_err(|_| error!(ErrorCode::NumericalOverflow))?;
180195

181196
let fee_quote: u64 = (gross_quote as u128)
182197
.checked_mul(market.fee_basis_points as u128)
@@ -186,6 +201,12 @@ pub fn handle_place_order<'info>(
186201
.try_into()
187202
.map_err(|_| error!(ErrorCode::NumericalOverflow))?;
188203

204+
// Defensive invariant: fees are a fraction of gross, never more.
205+
// `fee_basis_points <= 10_000` is enforced at market init, so this
206+
// should be unreachable — but a stale assumption here would let a
207+
// misconfigured market overdraw the maker's net payout. Cheap check.
208+
require!(fee_quote <= gross_quote, ErrorCode::NumericalOverflow);
209+
189210
match side {
190211
// Taker Bid, resting Ask. Taker pays quote, gets base.
191212
OrderSide::Bid => {
@@ -203,9 +224,14 @@ pub fn handle_place_order<'info>(
203224

204225
// Price improvement: taker locked (price * quantity) but
205226
// only needs (fill_price * fill_quantity) for this fill.
206-
let locked_for_this_fill: u64 = price
207-
.checked_mul(fill.fill_quantity)
208-
.ok_or(ErrorCode::NumericalOverflow)?;
227+
// u128 intermediate for the same reason as the bid lock
228+
// and gross_quote above — the original lock is already
229+
// bounded to u64, so this product narrows back cleanly.
230+
let locked_for_this_fill: u64 = (price as u128)
231+
.checked_mul(fill.fill_quantity as u128)
232+
.ok_or(ErrorCode::NumericalOverflow)?
233+
.try_into()
234+
.map_err(|_| error!(ErrorCode::NumericalOverflow))?;
209235
let rebate: u64 = locked_for_this_fill
210236
.checked_sub(gross_quote)
211237
.ok_or(ErrorCode::NumericalOverflow)?;
@@ -352,7 +378,13 @@ pub fn handle_place_order<'info>(
352378
order.side = side;
353379
order.price = price;
354380
order.original_quantity = quantity;
355-
order.filled_quantity = quantity.saturating_sub(taker_remaining);
381+
// checked_sub, not saturating_sub: a silent clamp on filled_quantity
382+
// would be a real correctness bug (the matching engine should never
383+
// hand us a remainder larger than the original). saturating_* belongs
384+
// on cosmetic/UX paths, not on a field the matching engine relies on.
385+
order.filled_quantity = quantity
386+
.checked_sub(taker_remaining)
387+
.ok_or(ErrorCode::NumericalOverflow)?;
356388
order.timestamp = timestamp;
357389
order.bump = context.bumps.order;
358390

0 commit comments

Comments
 (0)