From ccba4ee597e96624017c97e52285617089ce6a5d Mon Sep 17 00:00:00 2001 From: Christian Kamm Date: Sat, 13 Aug 2022 19:54:58 +0200 Subject: [PATCH] Cleanups and bugfixes from the audit (#163) * AccountExpand: cleanups from audit * Group instructions: cleanups from audit * AccountClose: cleanups from audit * TokenAddBank: Audit fixes, including significant bugs Must not take collected_fees and bump from the existing bank. * Instruction comment updates * GroupEdit: Add version/testing flag changes Ported from mc/vanity --- .../src/instructions/account_close.rs | 27 +++--- .../src/instructions/account_expand.rs | 10 +- .../mango-v4/src/instructions/group_close.rs | 4 +- .../mango-v4/src/instructions/group_create.rs | 2 +- .../mango-v4/src/instructions/group_edit.rs | 27 +++++- .../src/instructions/token_add_bank.rs | 14 +-- .../src/instructions/token_deposit.rs | 3 - .../mango-v4/src/instructions/token_edit.rs | 4 + .../src/instructions/token_register.rs | 2 - .../token_update_index_and_rate.rs | 7 ++ programs/mango-v4/src/lib.rs | 14 ++- programs/mango-v4/src/state/bank.rs | 30 ++++-- programs/mango-v4/src/state/mango_account.rs | 10 +- ts/client/src/client.ts | 13 ++- ts/client/src/mango_v4.ts | 92 +++++++++---------- ts/client/src/scripts/example1-admin.ts | 2 + ts/client/src/scripts/mb-example1-admin.ts | 2 +- 17 files changed, 148 insertions(+), 115 deletions(-) diff --git a/programs/mango-v4/src/instructions/account_close.rs b/programs/mango-v4/src/instructions/account_close.rs index da9b01b58..9f0795f0d 100644 --- a/programs/mango-v4/src/instructions/account_close.rs +++ b/programs/mango-v4/src/instructions/account_close.rs @@ -27,22 +27,19 @@ pub struct AccountClose<'info> { pub fn account_close(ctx: Context) -> Result<()> { let group = ctx.accounts.group.load()?; - { - let account = ctx.accounts.account.load_mut()?; + let account = ctx.accounts.account.load_mut()?; - // don't perform checks if group is just testing - if group.testing == 0 { - require!(!account.fixed.being_liquidated(), MangoError::SomeError); - require_eq!(account.fixed.delegate, Pubkey::default()); - for ele in account.token_iter() { - require_eq!(ele.is_active(), false); - } - for ele in account.serum3_iter() { - require_eq!(ele.is_active(), false); - } - for ele in account.perp_iter() { - require_eq!(ele.is_active(), false); - } + // don't perform checks if group is just testing + if !group.is_testing() { + require!(!account.fixed.being_liquidated(), MangoError::SomeError); + for ele in account.token_iter() { + require_eq!(ele.is_active(), false); + } + for ele in account.serum3_iter() { + require_eq!(ele.is_active(), false); + } + for ele in account.perp_iter() { + require_eq!(ele.is_active(), false); } } diff --git a/programs/mango-v4/src/instructions/account_expand.rs b/programs/mango-v4/src/instructions/account_expand.rs index 244ddca05..8f0397d12 100644 --- a/programs/mango-v4/src/instructions/account_expand.rs +++ b/programs/mango-v4/src/instructions/account_expand.rs @@ -1,6 +1,7 @@ use anchor_lang::prelude::*; use crate::state::*; +use crate::util::checked_math as cm; #[derive(Accounts)] pub struct AccountExpand<'info> { @@ -32,6 +33,7 @@ pub fn account_expand( let realloc_account = ctx.accounts.account.as_ref(); let old_space = realloc_account.data_len(); + let old_lamports = realloc_account.lamports(); require_gt!(new_space, old_space); @@ -44,13 +46,11 @@ pub fn account_expand( to: realloc_account.clone(), }, ), - new_rent_minimum - .checked_sub(realloc_account.lamports()) - .unwrap(), + cm!(new_rent_minimum - old_lamports), )?; - // realloc - realloc_account.realloc(new_space, true)?; + // realloc: it's safe to not re-zero-init since we never shrink accounts + realloc_account.realloc(new_space, false)?; // expand dynamic content, e.g. to grow token positions, we need to slide serum3orders further later, and so on.... let mut account = ctx.accounts.account.load_mut()?; diff --git a/programs/mango-v4/src/instructions/group_close.rs b/programs/mango-v4/src/instructions/group_close.rs index 0df176840..a97b347b4 100644 --- a/programs/mango-v4/src/instructions/group_close.rs +++ b/programs/mango-v4/src/instructions/group_close.rs @@ -26,9 +26,7 @@ pub struct GroupClose<'info> { } pub fn group_close(ctx: Context) -> Result<()> { - // TODO: checks - - // close insurance vault + // close insurance vault (must be empty) let group = ctx.accounts.group.load()?; let group_seeds = group_seeds!(group); let cpi_accounts = CloseAccount { diff --git a/programs/mango-v4/src/instructions/group_create.rs b/programs/mango-v4/src/instructions/group_create.rs index 981db35a8..40f37a3ce 100644 --- a/programs/mango-v4/src/instructions/group_create.rs +++ b/programs/mango-v4/src/instructions/group_create.rs @@ -46,12 +46,12 @@ pub fn group_create( ) -> Result<()> { let mut group = ctx.accounts.group.load_init()?; group.creator = ctx.accounts.creator.key(); + group.group_num = group_num; group.admin = ctx.accounts.creator.key(); group.fast_listing_admin = Pubkey::default(); group.insurance_vault = ctx.accounts.insurance_vault.key(); group.insurance_mint = ctx.accounts.insurance_mint.key(); group.bump = *ctx.bumps.get("group").ok_or(MangoError::SomeError)?; - group.group_num = group_num; group.testing = testing; group.version = version; Ok(()) diff --git a/programs/mango-v4/src/instructions/group_edit.rs b/programs/mango-v4/src/instructions/group_edit.rs index 8d8b9822d..425b46f4a 100644 --- a/programs/mango-v4/src/instructions/group_edit.rs +++ b/programs/mango-v4/src/instructions/group_edit.rs @@ -13,14 +13,31 @@ pub struct GroupEdit<'info> { } // use case - transfer group ownership to governance, where -// new_admin and new_fast_listing_admin are PDAs +// admin and fast_listing_admin are PDAs pub fn group_edit( ctx: Context, - new_admin: Pubkey, - new_fast_listing_admin: Pubkey, + admin_opt: Option, + fast_listing_admin_opt: Option, + testing_opt: Option, + version_opt: Option, ) -> Result<()> { let mut group = ctx.accounts.group.load_mut()?; - group.admin = new_admin; - group.fast_listing_admin = new_fast_listing_admin; + + if let Some(admin) = admin_opt { + group.admin = admin; + } + + if let Some(fast_listing_admin) = fast_listing_admin_opt { + group.fast_listing_admin = fast_listing_admin; + } + + if let Some(testing) = testing_opt { + group.testing = testing; + } + + if let Some(version) = version_opt { + group.version = version; + } + Ok(()) } diff --git a/programs/mango-v4/src/instructions/token_add_bank.rs b/programs/mango-v4/src/instructions/token_add_bank.rs index 719a9c554..87d51d952 100644 --- a/programs/mango-v4/src/instructions/token_add_bank.rs +++ b/programs/mango-v4/src/instructions/token_add_bank.rs @@ -1,6 +1,7 @@ use anchor_lang::prelude::*; use anchor_spl::token::{Mint, Token, TokenAccount}; +use crate::error::*; use crate::state::*; #[derive(Accounts)] @@ -44,10 +45,12 @@ pub struct TokenAddBank<'info> { #[account( mut, - seeds = [b"MintInfo".as_ref(), group.key().as_ref(), mint.key().as_ref()], - bump + constraint = mint_info.load()?.token_index == token_index, + has_one = group, + has_one = mint, )] pub mint_info: AccountLoader<'info, MintInfo>, + #[account(mut)] pub payer: Signer<'info>, @@ -56,8 +59,6 @@ pub struct TokenAddBank<'info> { pub rent: Sysvar<'info, Rent>, } -// TODO: should this be "configure_mint", we pass an explicit index, and allow -// overwriting config as long as the mint account stays the same? #[allow(clippy::too_many_arguments)] #[allow(unused_variables)] pub fn token_add_bank( @@ -65,11 +66,10 @@ pub fn token_add_bank( token_index: TokenIndex, bank_num: u32, ) -> Result<()> { - // TODO: Error if mint is already configured (technically, init of vault will fail) - let existing_bank = ctx.accounts.existing_bank.load()?; let mut bank = ctx.accounts.bank.load_init()?; - *bank = Bank::from_existing_bank(&existing_bank, ctx.accounts.vault.key(), bank_num); + let bump = *ctx.bumps.get("bank").ok_or(MangoError::SomeError)?; + *bank = Bank::from_existing_bank(&existing_bank, ctx.accounts.vault.key(), bank_num, bump); let mut mint_info = ctx.accounts.mint_info.load_mut()?; let free_slot = mint_info diff --git a/programs/mango-v4/src/instructions/token_deposit.rs b/programs/mango-v4/src/instructions/token_deposit.rs index 22afcfc88..28a8598fd 100644 --- a/programs/mango-v4/src/instructions/token_deposit.rs +++ b/programs/mango-v4/src/instructions/token_deposit.rs @@ -48,9 +48,6 @@ impl<'info> TokenDeposit<'info> { } } -// TODO: It may make sense to have the token_index passed in from the outside. -// That would save a lot of computation that needs to go into finding the -// right index for the mint. pub fn token_deposit(ctx: Context, amount: u64) -> Result<()> { require_msg!(amount > 0, "deposit amount must be positive"); diff --git a/programs/mango-v4/src/instructions/token_edit.rs b/programs/mango-v4/src/instructions/token_edit.rs index ab4100f00..6e907cfc1 100644 --- a/programs/mango-v4/src/instructions/token_edit.rs +++ b/programs/mango-v4/src/instructions/token_edit.rs @@ -7,6 +7,10 @@ use crate::accounts_zerocopy::LoadMutZeroCopyRef; use crate::state::*; +/// Changes a token's parameters. +/// +/// In addition to these accounts, all banks must be passed as remaining_accounts +/// in MintInfo order. #[derive(Accounts)] #[instruction(bank_num: u64)] pub struct TokenEdit<'info> { diff --git a/programs/mango-v4/src/instructions/token_register.rs b/programs/mango-v4/src/instructions/token_register.rs index 453d37f86..ad71ac055 100644 --- a/programs/mango-v4/src/instructions/token_register.rs +++ b/programs/mango-v4/src/instructions/token_register.rs @@ -73,8 +73,6 @@ pub struct InterestRateParams { pub adjustment_factor: f32, } -// TODO: should this be "configure_mint", we pass an explicit index, and allow -// overwriting config as long as the mint account stays the same? #[allow(clippy::too_many_arguments)] pub fn token_register( ctx: Context, diff --git a/programs/mango-v4/src/instructions/token_update_index_and_rate.rs b/programs/mango-v4/src/instructions/token_update_index_and_rate.rs index ea703f251..8867c237a 100644 --- a/programs/mango-v4/src/instructions/token_update_index_and_rate.rs +++ b/programs/mango-v4/src/instructions/token_update_index_and_rate.rs @@ -17,6 +17,13 @@ pub mod compute_budget { declare_id!("ComputeBudget111111111111111111111111111111"); } +/// Updates token interest and interest rates. +/// +/// In addition to these accounts, all banks must be passed as remaining_accounts +/// in MintInfo order. +/// +/// This instruction may only be used alongside other instructions of the same kind +/// or ComputeBudget instructions. #[derive(Accounts)] pub struct TokenUpdateIndexAndRate<'info> { pub group: AccountLoader<'info, Group>, // Required for group metadata parsing diff --git a/programs/mango-v4/src/lib.rs b/programs/mango-v4/src/lib.rs index be25edab5..2f7082231 100644 --- a/programs/mango-v4/src/lib.rs +++ b/programs/mango-v4/src/lib.rs @@ -41,10 +41,18 @@ pub mod mango_v4 { pub fn group_edit( ctx: Context, - new_admin: Pubkey, - new_fast_listing_admin: Pubkey, + admin_opt: Option, + fast_listing_admin_opt: Option, + testing_opt: Option, + version_opt: Option, ) -> Result<()> { - instructions::group_edit(ctx, new_admin, new_fast_listing_admin) + instructions::group_edit( + ctx, + admin_opt, + fast_listing_admin_opt, + testing_opt, + version_opt, + ) } pub fn group_close(ctx: Context) -> Result<()> { diff --git a/programs/mango-v4/src/state/bank.rs b/programs/mango-v4/src/state/bank.rs index cb8b9ddb1..9ca3f5ce7 100644 --- a/programs/mango-v4/src/state/bank.rs +++ b/programs/mango-v4/src/state/bank.rs @@ -109,20 +109,36 @@ const_assert_eq!( const_assert_eq!(size_of::() % 8, 0); impl Bank { - pub fn from_existing_bank(existing_bank: &Bank, vault: Pubkey, bank_num: u32) -> Self { + pub fn from_existing_bank( + existing_bank: &Bank, + vault: Pubkey, + bank_num: u32, + bump: u8, + ) -> Self { Self { + // values that must be reset/changed + vault, + indexed_deposits: I80F48::ZERO, + indexed_borrows: I80F48::ZERO, + collected_fees_native: I80F48::ZERO, + dust: I80F48::ZERO, + flash_loan_approved_amount: 0, + flash_loan_token_account_initial: u64::MAX, + bump, + bank_num, + + // values that can be copied + // these are listed explicitly, so someone must make the decision when a + // new field is added! name: existing_bank.name, group: existing_bank.group, mint: existing_bank.mint, - vault, oracle: existing_bank.oracle, oracle_config: existing_bank.oracle_config, deposit_index: existing_bank.deposit_index, borrow_index: existing_bank.borrow_index, cached_indexed_total_deposits: existing_bank.cached_indexed_total_deposits, cached_indexed_total_borrows: existing_bank.cached_indexed_total_borrows, - indexed_deposits: I80F48::ZERO, - indexed_borrows: I80F48::ZERO, index_last_updated: existing_bank.index_last_updated, bank_rate_last_updated: existing_bank.bank_rate_last_updated, avg_utilization: existing_bank.avg_utilization, @@ -132,7 +148,6 @@ impl Bank { util1: existing_bank.util1, rate1: existing_bank.rate1, max_rate: existing_bank.max_rate, - collected_fees_native: existing_bank.collected_fees_native, loan_origination_fee_rate: existing_bank.loan_origination_fee_rate, loan_fee_rate: existing_bank.loan_fee_rate, maint_asset_weight: existing_bank.maint_asset_weight, @@ -140,14 +155,9 @@ impl Bank { maint_liab_weight: existing_bank.maint_liab_weight, init_liab_weight: existing_bank.init_liab_weight, liquidation_fee: existing_bank.liquidation_fee, - dust: I80F48::ZERO, - flash_loan_approved_amount: 0, - flash_loan_token_account_initial: u64::MAX, token_index: existing_bank.token_index, - bump: existing_bank.bump, mint_decimals: existing_bank.mint_decimals, reserved: [0; 2560], - bank_num, } } diff --git a/programs/mango-v4/src/state/mango_account.rs b/programs/mango-v4/src/state/mango_account.rs index 5e0096c2a..859cdd641 100644 --- a/programs/mango-v4/src/state/mango_account.rs +++ b/programs/mango-v4/src/state/mango_account.rs @@ -956,14 +956,10 @@ impl< } } - // update header - let header_mut = self.header_mut(); - header_mut.token_count = new_token_count; - header_mut.serum3_count = new_serum3_count; - header_mut.perp_count = new_perp_count; - header_mut.perp_oo_count = new_perp_oo_count; + // update the already-parsed header + *self.header_mut() = new_header; - // write new lengths (uses header) + // write new lengths to the dynamic data (uses header) self.write_token_length(); self.write_serum3_length(); self.write_perp_length(); diff --git a/ts/client/src/client.ts b/ts/client/src/client.ts index d71a673ec..ecd4f46ea 100644 --- a/ts/client/src/client.ts +++ b/ts/client/src/client.ts @@ -94,11 +94,18 @@ export class MangoClient { public async groupEdit( group: Group, - newAdmin: PublicKey, - newFastListingAdmin: PublicKey, + admin: PublicKey | undefined, + fastListingAdmin: PublicKey | undefined, + testing: number | undefined, + version: number | undefined, ): Promise { return await this.program.methods - .groupEdit(newAdmin, newFastListingAdmin) + .groupEdit( + admin ?? null, + fastListingAdmin ?? null, + testing ?? null, + version ?? null, + ) .accounts({ group: group.publicKey, admin: (this.program.provider as AnchorProvider).wallet.publicKey, diff --git a/ts/client/src/mango_v4.ts b/ts/client/src/mango_v4.ts index d048e276a..c7cf653f5 100644 --- a/ts/client/src/mango_v4.ts +++ b/ts/client/src/mango_v4.ts @@ -110,12 +110,28 @@ export type MangoV4 = { ], "args": [ { - "name": "newAdmin", - "type": "publicKey" + "name": "adminOpt", + "type": { + "option": "publicKey" + } }, { - "name": "newFastListingAdmin", - "type": "publicKey" + "name": "fastListingAdminOpt", + "type": { + "option": "publicKey" + } + }, + { + "name": "testingOpt", + "type": { + "option": "u8" + } + }, + { + "name": "versionOpt", + "type": { + "option": "u8" + } } ] }, @@ -646,27 +662,7 @@ export type MangoV4 = { { "name": "mintInfo", "isMut": true, - "isSigner": false, - "pda": { - "seeds": [ - { - "kind": "const", - "type": "string", - "value": "MintInfo" - }, - { - "kind": "account", - "type": "publicKey", - "path": "group" - }, - { - "kind": "account", - "type": "publicKey", - "account": "Mint", - "path": "mint" - } - ] - } + "isSigner": false }, { "name": "payer", @@ -5150,12 +5146,28 @@ export const IDL: MangoV4 = { ], "args": [ { - "name": "newAdmin", - "type": "publicKey" + "name": "adminOpt", + "type": { + "option": "publicKey" + } }, { - "name": "newFastListingAdmin", - "type": "publicKey" + "name": "fastListingAdminOpt", + "type": { + "option": "publicKey" + } + }, + { + "name": "testingOpt", + "type": { + "option": "u8" + } + }, + { + "name": "versionOpt", + "type": { + "option": "u8" + } } ] }, @@ -5686,27 +5698,7 @@ export const IDL: MangoV4 = { { "name": "mintInfo", "isMut": true, - "isSigner": false, - "pda": { - "seeds": [ - { - "kind": "const", - "type": "string", - "value": "MintInfo" - }, - { - "kind": "account", - "type": "publicKey", - "path": "group" - }, - { - "kind": "account", - "type": "publicKey", - "account": "Mint", - "path": "mint" - } - ] - } + "isSigner": false }, { "name": "payer", diff --git a/ts/client/src/scripts/example1-admin.ts b/ts/client/src/scripts/example1-admin.ts index dea291ad7..99a629833 100644 --- a/ts/client/src/scripts/example1-admin.ts +++ b/ts/client/src/scripts/example1-admin.ts @@ -204,6 +204,8 @@ async function main() { group, group.admin, new PublicKey('Efhak3qj3MiyzgJr3cUUqXXz5wr3oYHt9sPzuqJf9eBN'), + undefined, + undefined, ); console.log(`sig https://explorer.solana.com/tx/${sig}?cluster=devnet`); console.log(`Registering MNGO...`); diff --git a/ts/client/src/scripts/mb-example1-admin.ts b/ts/client/src/scripts/mb-example1-admin.ts index 7f12a5b45..c22a6c69c 100644 --- a/ts/client/src/scripts/mb-example1-admin.ts +++ b/ts/client/src/scripts/mb-example1-admin.ts @@ -209,7 +209,7 @@ async function main() { ); console.log(`Registering MNGO...`); - await client.groupEdit(group, group.admin, group.admin); + await client.groupEdit(group, group.admin, group.admin, undefined, undefined); const mngoMainnetMint = new PublicKey(MAINNET_MINTS.get('MNGO')!); const mngoMainnetOracle = new PublicKey(MAINNET_ORACLES.get('MNGO')!); await client.tokenRegisterTrustless(