From 3699d01bb61c003e9b07fcffbcb24c6a232878eb Mon Sep 17 00:00:00 2001 From: Britt Cyr Date: Mon, 9 Dec 2024 11:48:34 -0500 Subject: [PATCH] Fix partial fill of global bid (#315) * Fix partial fill of global bid * Comment * fmt --- audits/updates.md | 9 +-- programs/manifest/src/state/market.rs | 22 ++++-- programs/manifest/tests/cases/global.rs | 101 ++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 11 deletions(-) diff --git a/audits/updates.md b/audits/updates.md index c0bf8d104..842e17f0a 100644 --- a/audits/updates.md +++ b/audits/updates.md @@ -1,7 +1,6 @@ ## Updates since audit: -Global with token 22 with transfer fees could result in a loss of funds from incorrect accounting when moving from the global account to the market. Fixed in [https://github.com/CKS-Systems/manifest/pull/209](https://github.com/CKS-Systems/manifest/pull/209 "https://github.com/CKS-Systems/manifest/pull/209"). Was discovered and patched before any globals were created with transfer fees, so we know there was no cleanup to do. - -Rounding on cancel. This issue was discovered in the audit. And it was fixed in [https://github.com/CKS-Systems/manifest/pull/140](https://github.com/CKS-Systems/manifest/pull/140 "https://github.com/CKS-Systems/manifest/pull/140"), however during merge of the formal verification rules, it was reintroduced [https://github.com/CKS-Systems/manifest/pull/294](https://github.com/CKS-Systems/manifest/pull/294 "https://github.com/CKS-Systems/manifest/pull/294"). The daily formal verification did not catch them because those rules were incorrectly disabled. This was fixed in [https://github.com/CKS-Systems/manifest/pull/310](https://github.com/CKS-Systems/manifest/pull/310 "https://github.com/CKS-Systems/manifest/pull/310"). The scope of this issue is that if a bid required rounding for quote, then on cancel, there would be one quote atom less received by their claimedSeat. The rule has since been enabled and passing. - -Global bringing incorrect amount to the market. Typo with which amount traded to bring over to the market resulted in more tokens brought over on a fill and not accounted for on the market. While it could have been less, because of prices it never was. Fixed in [https://github.com/CKS-Systems/manifest/pull/311](https://github.com/CKS-Systems/manifest/pull/311 "https://github.com/CKS-Systems/manifest/pull/311"). The impact was limited to users who had a global order filled, and at the time of the fix, was just dev accounts. To detect any similar issues in the future, the balance-checker.ts script was added. \ No newline at end of file +- Global with token 22 with transfer fees could result in a loss of funds from incorrect accounting when moving from the global account to the market. Fixed in [https://github.com/CKS-Systems/manifest/pull/209](https://github.com/CKS-Systems/manifest/pull/209 "https://github.com/CKS-Systems/manifest/pull/209"). Was discovered and patched before any globals were created with transfer fees, so we know there was no cleanup to do. +- Rounding on cancel. This issue was discovered in the audit. And it was fixed in [https://github.com/CKS-Systems/manifest/pull/140](https://github.com/CKS-Systems/manifest/pull/140 "https://github.com/CKS-Systems/manifest/pull/140"), however during merge of the formal verification rules, it was reintroduced [https://github.com/CKS-Systems/manifest/pull/294](https://github.com/CKS-Systems/manifest/pull/294 "https://github.com/CKS-Systems/manifest/pull/294"). The daily formal verification did not catch them because those rules were incorrectly disabled. This was fixed in [https://github.com/CKS-Systems/manifest/pull/310](https://github.com/CKS-Systems/manifest/pull/310 "https://github.com/CKS-Systems/manifest/pull/310"). The scope of this issue is that if a bid required rounding for quote, then on cancel, there would be one quote atom less received by their claimedSeat. The rule has since been enabled and passing. +- Global bringing incorrect amount to the market. Typo with which amount traded to bring over to the market resulted in more tokens brought over on a fill and not accounted for on the market. While it could have been less, because of prices it never was. Fixed in [https://github.com/CKS-Systems/manifest/pull/311](https://github.com/CKS-Systems/manifest/pull/311 "https://github.com/CKS-Systems/manifest/pull/311"). The impact was limited to users who had a global order filled, and at the time of the fix, was just dev accounts. To detect any similar issues in the future, the balance-checker.ts script was added. +- Partial fill on global bid loses 1 atom. Fixed in [https://github.com/CKS-Systems/manifest/pull/315](https://github.com/CKS-Systems/manifest/pull/315). Max loss was 1 atom of quote on a partial fill of a global bid. Scope was < 100 USDC atoms total. \ No newline at end of file diff --git a/programs/manifest/src/state/market.rs b/programs/manifest/src/state/market.rs index 5adf0c644..b71a55d3d 100644 --- a/programs/manifest/src/state/market.rs +++ b/programs/manifest/src/state/market.rs @@ -1035,8 +1035,13 @@ impl< // Note that we do not have to do this on the other direction // because the amount of atoms that a maker needs to support an ask // is exact. The rounding is always on quote. - if !is_bid { - // These are only used when is_bid, included up here for borrow checker reasons. + // + // Do not credit the bonus atom on global orders. This is because + // only the number of atoms required for the trade were brought + // over. The extra one that is no longer needed for taker rounding + // is not brought over, so dont credit the maker for it. + if !is_bid && !is_global { + // These are only used when is_bid. let previous_maker_quote_atoms_allocated: QuoteAtoms = matched_price.checked_quote_for_base(maker_order.get_num_base_atoms(), true)?; let new_maker_quote_atoms_allocated: QuoteAtoms = matched_price @@ -1046,16 +1051,21 @@ impl< .checked_sub(base_atoms_traded)?, true, )?; + let bonus_atom_or_zero: QuoteAtoms = previous_maker_quote_atoms_allocated + .checked_sub(new_maker_quote_atoms_allocated)? + .checked_sub(quote_atoms_traded)?; + + // The bonus atom isnt actually traded, it is recouped to the + // maker though from the tokens that they had been using to back + // the order since it is no longer needed. So we do not need to + // update the fill logs or amounts. update_balance( fixed, dynamic, maker_trader_index, is_bid, true, - (previous_maker_quote_atoms_allocated - .checked_sub(new_maker_quote_atoms_allocated)? - .checked_sub(quote_atoms_traded)?) - .as_u64(), + bonus_atom_or_zero.as_u64(), )?; } diff --git a/programs/manifest/tests/cases/global.rs b/programs/manifest/tests/cases/global.rs index 2ab875ee9..7dc339e46 100644 --- a/programs/manifest/tests/cases/global.rs +++ b/programs/manifest/tests/cases/global.rs @@ -321,6 +321,107 @@ async fn global_match_order() -> anyhow::Result<()> { Ok(()) } +#[tokio::test] +async fn global_match_order_quote_no_bonus() -> anyhow::Result<()> { + let mut test_fixture: TestFixture = TestFixture::new().await; + test_fixture.claim_seat().await?; + + test_fixture + .claim_seat_for_keypair(&test_fixture.second_keypair.insecure_clone()) + .await?; + test_fixture + .global_add_trader_for_keypair(&test_fixture.second_keypair.insecure_clone()) + .await?; + test_fixture + .global_deposit_for_keypair(&test_fixture.second_keypair.insecure_clone(), 1_000_000) + .await?; + + test_fixture + .batch_update_with_global_for_keypair( + None, + vec![], + vec![PlaceOrderParams::new( + 100, + 11, + -1, + true, + OrderType::Global, + NO_EXPIRATION_LAST_VALID_SLOT, + )], + &test_fixture.second_keypair.insecure_clone(), + ) + .await?; + + test_fixture.deposit(Token::SOL, 1_000_000).await?; + + test_fixture + .batch_update_with_global_for_keypair( + None, + vec![], + vec![PlaceOrderParams::new( + 1, + 9, + -1, + false, + OrderType::Limit, + NO_EXPIRATION_LAST_VALID_SLOT, + )], + &test_fixture.payer_keypair().insecure_clone(), + ) + .await?; + + test_fixture.market_fixture.reload().await; + let orders: Vec = test_fixture.market_fixture.get_resting_orders().await; + assert_eq!(orders.len(), 1, "Order still on orderbook"); + + // Global buys 1 base for 1.1 quote --> rounded to 1 quote + // Local sells 1 base for 1.1 quote --> rounded to 1 quote + + // Match will leave global: 1 quote, 1 base + // match will leave local: 1 quote, 1_000_000 - 1 base + + assert_eq!( + test_fixture + .market_fixture + .get_base_balance_atoms(&test_fixture.second_keypair.pubkey()) + .await, + 1 + ); + assert_eq!( + test_fixture + .market_fixture + .get_quote_balance_atoms(&test_fixture.second_keypair.pubkey()) + .await, + 0 + ); + assert_eq!( + test_fixture + .market_fixture + .get_base_balance_atoms(&test_fixture.payer()) + .await, + 1_000_000 - 1 + ); + assert_eq!( + test_fixture + .market_fixture + .get_quote_balance_atoms(&test_fixture.payer()) + .await, + 1 + ); + + test_fixture.global_fixture.reload().await; + assert_eq!( + test_fixture + .global_fixture + .global + .get_balance_atoms(&test_fixture.second_keypair.insecure_clone().pubkey()) + .as_u64(), + 1_000_000 - 1 + ); + + Ok(()) +} + #[tokio::test] async fn global_deposit_withdraw_22() -> anyhow::Result<()> { let test_fixture: TestFixture = TestFixture::new().await;