blockworks-foundation
/
mango-v4
mirror of https://github.com/blockworks-foundation/mango-v4.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

HealthRegion: Whitelist allowed instruction types (#508) 

This fixes a security issue where bankruptcy related instructions could
be called inside a health region. Now health regions are limited to
compute optimization like when placing multiple orders in one
transaction.

This limitation also makes it impossible to abuse health regions for
flash loans. Use the FlashLoan instructions for that purpose.
This commit is contained in:
Christian Kamm 2023-03-20 14:02:35 +01:00 committed by GitHub
parent 658a220095
commit b22a1e7f57
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 179 additions and 76 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
programs/mango-v4/src/error.rs
View File

@ -95,6 +95,8 @@ pub enum MangoError {
NoLiquidatablePerpBasePosition,
#[msg("perp order id not found on the orderbook")]
PerpOrderIdNotFound,
#[msg("HealthRegions allow only specific instructions between Begin and End")]
HealthRegionBadInnerInstruction,
}
impl MangoError {

52
programs/mango-v4/src/instructions/health_region.rs
View File

@ -10,11 +10,33 @@ use fixed::types::I80F48;
pub fn health_region_begin<'key, 'accounts, 'remaining, 'info>(
ctx: Context<'key, 'accounts, 'remaining, 'info, HealthRegionBegin<'info>>,
) -> Result<()> {
// Check if the other instructions in the transactions are compatible
// The instructions that may be called inside a HealthRegion
let allowed_inner_ix = [
crate::instruction::PerpCancelAllOrders::discriminator(),
crate::instruction::PerpCancelAllOrdersBySide::discriminator(),
crate::instruction::PerpCancelOrder::discriminator(),
crate::instruction::PerpCancelOrderByClientOrderId::discriminator(),
crate::instruction::PerpPlaceOrder::discriminator(),
crate::instruction::PerpPlaceOrderPegged::discriminator(),
crate::instruction::Serum3CancelAllOrders::discriminator(),
crate::instruction::Serum3CancelOrder::discriminator(),
crate::instruction::Serum3PlaceOrder::discriminator(),
crate::instruction::Serum3SettleFunds::discriminator(),
crate::instruction::Serum3SettleFundsV2::discriminator(),
];
// Check if the other instructions in the transaction are compatible
{
let ixs = ctx.accounts.instructions.as_ref();
let current_index = tx_instructions::load_current_index_checked(ixs)? as usize;
// Forbid HealthRegionBegin to be called from CPI (it does not have to be the first instruction)
let current_ix = tx_instructions::load_instruction_at_checked(current_index, ixs)?;
require_msg!(
current_ix.program_id == *ctx.program_id,
"HealthRegionBegin must be a top-level instruction"
);
// There must be a matching HealthRegionEnd instruction
let mut index = current_index + 1;
let mut found_end = false;
@ -26,18 +48,24 @@ pub fn health_region_begin<'key, 'accounts, 'remaining, 'info>(
};
index += 1;
if ix.program_id != crate::id() {
continue;
}
if ix.data[0..8] != crate::instruction::HealthRegionEnd::discriminator() {
continue;
}
require_keys_eq!(
ix.program_id,
crate::id(),
MangoError::HealthRegionBadInnerInstruction
);
// check that it's for the same account
require_keys_eq!(ix.accounts[0].pubkey, ctx.accounts.account.key());
found_end = true;
index += 1;
let discriminator: [u8; 8] = ix.data[0..8].try_into().unwrap();
if discriminator == crate::instruction::HealthRegionEnd::discriminator() {
// check that it's for the same account
require_keys_eq!(ix.accounts[0].pubkey, ctx.accounts.account.key());
found_end = true;
break;
} else {
require!(
allowed_inner_ix.contains(&discriminator),
MangoError::HealthRegionBadInnerInstruction
);
}
}
require_msg!(
found_end,

201
programs/mango-v4/tests/cases/test_health_region.rs
View File

@ -1,9 +1,10 @@
use super::*;
use mango_v4::accounts_ix::{Serum3OrderType, Serum3SelfTradeBehavior, Serum3Side};
#[tokio::test]
async fn test_health_wrap() -> Result<(), TransportError> {
let mut test_builder = TestContextBuilder::new();
test_builder.test().set_compute_max_units(100000);
test_builder.test().set_compute_max_units(135000);
let context = test_builder.start_default().await;
let solana = &context.solana.clone();
@ -11,7 +12,6 @@ async fn test_health_wrap() -> Result<(), TransportError> {
let owner = context.users[0].key;
let payer = context.users[1].key;
let mints = &context.mints[0..2];
let payer_mint_accounts = &context.users[1].token_accounts;
//
// SETUP: Create a group, account, register a token (mint0)
@ -25,73 +25,102 @@ async fn test_health_wrap() -> Result<(), TransportError> {
}
.create(solana)
.await;
let quote_token = &tokens[0];
let base_token = &tokens[1];
// SETUP: Create an account with deposits, so the second account can borrow more than it has
create_funded_account(&solana, group, owner, 0, &context.users[1], mints, 2000, 0).await;
// SETUP: Make a second account
let account = send_tx(
//
// SETUP: Create serum market
//
let serum_market_cookie = context
.serum
.list_spot_market(&base_token.mint, &quote_token.mint)
.await;
let serum_market = send_tx(
solana,
AccountCreateInstruction {
account_num: 1,
token_count: 8,
serum3_count: 0,
perp_count: 0,
perp_oo_count: 0,
Serum3RegisterMarketInstruction {
group,
owner,
admin,
serum_program: context.serum.program_id,
serum_market_external: serum_market_cookie.market,
market_index: 0,
base_bank: base_token.bank,
quote_bank: quote_token.bank,
payer,
},
)
.await
.unwrap()
.account;
.serum_market;
// SETUP: Create an account with deposits, so the second account can borrow more than it has
create_funded_account(
&solana,
group,
owner,
0,
&context.users[1],
mints,
200000000,
0,
)
.await;
// SETUP: Make a second account
let account = create_funded_account(
&solana,
group,
owner,
1,
&context.users[1],
&mints[0..=1],
100,
0,
)
.await;
// SETUP: deposit something, so only one new token position needs to be created
// simply because the test code can't deal with two affected banks right now
send_tx(
solana,
TokenDepositInstruction {
amount: 1,
reduce_only: false,
Serum3CreateOpenOrdersInstruction {
account,
serum_market,
owner,
token_account: payer_mint_accounts[0],
token_authority: payer.clone(),
bank_index: 0,
payer,
},
)
.await
.unwrap();
let send_test_tx = |repay_amount| {
let tokens = tokens.clone();
let send_test_tx = |limit_price, order_size, cancel| {
async move {
let mut tx = ClientTransaction::new(solana);
tx.add_instruction(HealthRegionBeginInstruction { account })
.await;
tx.add_instruction(TokenWithdrawInstruction {
amount: 1000, // more than the 1 token that's on the account
allow_borrow: true,
tx.add_instruction(Serum3PlaceOrderInstruction {
side: Serum3Side::Ask,
limit_price: (limit_price * 100.0 / 10.0) as u64, // in quote_lot (10) per base lot (100)
max_base_qty: (order_size as u64) / 100, // in base lot (100)
max_native_quote_qty_including_fees: (limit_price * (order_size as f64)) as u64,
self_trade_behavior: Serum3SelfTradeBehavior::AbortTransaction,
order_type: Serum3OrderType::Limit,
client_order_id: 42,
limit: 10,
account,
owner,
token_account: payer_mint_accounts[0],
bank_index: 0,
})
.await;
tx.add_instruction(TokenDepositInstruction {
amount: repay_amount,
reduce_only: false,
account,
owner,
token_account: payer_mint_accounts[1],
token_authority: payer.clone(),
bank_index: 0,
serum_market,
})
.await;
if cancel {
tx.add_instruction(Serum3CancelAllOrdersInstruction {
limit: 10,
account,
owner,
serum_market,
})
.await;
}
tx.add_instruction(HealthRegionEndInstruction {
account,
affected_bank: Some(tokens[1].bank),
affected_bank: None,
})
.await;
tx.send().await
@ -99,10 +128,10 @@ async fn test_health_wrap() -> Result<(), TransportError> {
};
//
// TEST: Borrow a lot of token0 without collateral, but repay too little
// TEST: Placing a giant order fails
//
{
send_test_tx(1000).await.unwrap_err();
send_test_tx(1.0, 100000, false).await.unwrap_err();
let logs = solana.program_log();
// reaches the End instruction
assert!(logs
@ -112,35 +141,79 @@ async fn test_health_wrap() -> Result<(), TransportError> {
assert!(logs
.iter()
.any(|line| line.contains("Error Code: HealthMustBePositiveOrIncrease")));
// health computed only once
assert_eq!(
logs.iter()
.filter(|line| line.contains("post_init_health"))
.count(),
1
);
}
//
// TEST: Borrow a lot of token0 without collateral, and repay in token1 in the same tx
// TEST: If we cancel the order again before the HealthRegionEnd, it can go through
//
{
let start_payer_mint0 = solana.token_account_balance(payer_mint_accounts[0]).await;
let start_payer_mint1 = solana.token_account_balance(payer_mint_accounts[1]).await;
send_test_tx(1.0, 100000, true).await.unwrap();
let logs = solana.program_log();
// health computed only once
assert_eq!(
logs.iter()
.filter(|line| line.contains("post_init_health"))
.count(),
1
);
}
send_test_tx(3000).await.unwrap();
//
// TEST: Try using withdraw in a health region
//
{
let mut tx = ClientTransaction::new(solana);
tx.add_instruction(HealthRegionBeginInstruction { account })
.await;
tx.add_instruction(TokenWithdrawInstruction {
amount: 1,
allow_borrow: true,
account,
owner,
token_account: context.users[1].token_accounts[0],
bank_index: 0,
})
.await;
tx.add_instruction(HealthRegionEndInstruction {
account,
affected_bank: None,
})
.await;
tx.send().await.unwrap_err();
}
assert_eq!(
solana.token_account_balance(payer_mint_accounts[0]).await - start_payer_mint0,
1000
//
// TEST: Try using a different program in a health region
//
{
let mut tx = ClientTransaction::new(solana);
tx.add_instruction(HealthRegionBeginInstruction { account })
.await;
tx.add_instruction_direct(
spl_token::instruction::transfer(
&spl_token::ID,
&context.users[1].token_accounts[0],
&context.users[0].token_accounts[0],
&owner.pubkey(),
&[&owner.pubkey()],
1,
)
.unwrap(),
);
assert_eq!(
start_payer_mint1 - solana.token_account_balance(payer_mint_accounts[1]).await,
3000
);
assert_eq!(
account_position(solana, account, tokens[0].bank).await,
-999
);
assert_eq!(
account_position(solana, account, tokens[1].bank).await,
3000
);
let account_data: MangoAccount = solana.get_account(account).await;
assert_eq!(account_data.in_health_region, 0);
tx.add_instruction(HealthRegionEndInstruction {
account,
affected_bank: None,
})
.await;
tx.add_signer(owner);
tx.send().await.unwrap_err();
}
Ok(())