Bug when processing Event::Out in consume events
Closed this issue · 4 comments
Could make this as a pull request but probably simpler to post the solution here and let you guys add the fix when convenient. It doesn't look like a loss of funds issue but it definitely could eventually brick someone's account if their locked token accounts hit u64::max.
dex-v4/program/src/processor/consume_events.rs
Lines 322 to 337 in 637412d
Consume events function only increases token_free values, it never reduces the token_locked values. Essentially you're missing quote_token_locked.checked_sub and base_token_locked.checked_sub.
Compare with how these values are updated in the cancel orders
dex-v4/program/src/processor/cancel_order.rs
Lines 168 to 191 in 637412d
Hi @Henry-E! Thanks for reporting this. I agree with the fact that quote_token_locked isn't behaving as it should. We'll definitely fix this because this field might be useful down the line. However, I think that account bricking is a non-issue in this context. This is because we already store lifetime market volume as a u64, because we consider implicitly that a u64 is large enough. In this context, we still have quote_token_free << lifetime_market_volume. To give an idea for this, a volume of u64::MAX USDC would be around 18 trillion dollars.
Ok but it's still definitely doing incorrect maths!
We are in agreement, but since the field is non-critical and doesn't affect the business logic, and the unwrap case you outlined above is basically unreachable, this is a non-critical bug. We're definitely fixing it though, implementations composing with Serum might make use of this field.