Fix partial fill of global bid (#315)

* Fix partial fill of global bid

* Comment

* fmt

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.
+- 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.

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(),
                 )?;
             }
 

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<RestingOrder> = 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;