diff --git a/Cargo.lock b/Cargo.lock index efd858b9cec..afa6411fcfd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5448,6 +5448,7 @@ dependencies = [ "solana-program-test", "solana-sdk", "solend-sdk", + "spl-associated-token-account 1.1.3", "spl-token 3.5.0", "static_assertions", "switchboard-on-demand", diff --git a/token-lending/program/Cargo.toml b/token-lending/program/Cargo.toml index ae8a379dd44..535d35b1d77 100644 --- a/token-lending/program/Cargo.toml +++ b/token-lending/program/Cargo.toml @@ -15,11 +15,10 @@ test-bpf = [] [dependencies] bytemuck = "1.5.1" -# pyth-sdk-solana = "0.8.0" -# pyth-solana-receiver-sdk = "0.3.0" +oracles = { path = "../oracles" } solana-program = "=1.16.20" solend-sdk = { path = "../sdk" } -oracles = { path = "../oracles" } +spl-associated-token-account = "1.0.5" # compatible with spl-token 3.3.0 spl-token = { version = "3.3.0", features = ["no-entrypoint"] } static_assertions = "1.1.0" diff --git a/token-lending/program/src/processor/liquidity_mining/claim_user_reward.rs b/token-lending/program/src/processor/liquidity_mining/claim_user_reward.rs index d53d6b7cdc0..e8ff96b97d0 100644 --- a/token-lending/program/src/processor/liquidity_mining/claim_user_reward.rs +++ b/token-lending/program/src/processor/liquidity_mining/claim_user_reward.rs @@ -19,8 +19,9 @@ use solana_program::{ pubkey::Pubkey, sysvar::Sysvar, }; -use solend_sdk::state::{Obligation, PositionKind}; +use solend_sdk::state::{HasRewardEnded, Obligation, PositionKind}; use solend_sdk::{error::LendingError, instruction::reward_vault_authority_seeds}; +use spl_associated_token_account::get_associated_token_address_with_program_id; use super::{ check_and_unpack_pool_reward_accounts, unpack_token_account, Bumps, @@ -29,6 +30,8 @@ use super::{ /// Use [Self::from_unchecked_iter] to validate the accounts. struct ClaimUserReward<'a, 'info> { + /// ✅ is_signer + perhaps_payer_info: Option<&'a AccountInfo<'info>>, /// ✅ belongs to this program /// ✅ unpacks /// ✅ matches `lending_market_info` @@ -37,7 +40,7 @@ struct ClaimUserReward<'a, 'info> { /// ✅ belongs to the token program /// ✅ is writable /// ✅ matches `reward_mint_info` - /// ✅ owned by the obligation owner + /// ✅ is obligation owner's ATA for the reward mint obligation_owner_token_account_info: &'a AccountInfo<'info>, /// ✅ belongs to this program /// ✅ unpacks @@ -88,11 +91,22 @@ pub(crate) fn process( )?; let reserve_key = accounts.reserve.key(); + // AUDIT: + // > ClaimUserReward doesn’t check if the Obligation is stale. + // > This can cause problems for borrow rewards, because the obligation's liability_shares will + // > be stale. + if matches!(position_kind, PositionKind::Borrow) + && accounts.obligation.last_update.is_stale(clock.slot)? + { + msg!("obligation is stale and must be refreshed in the current slot"); + return Err(LendingError::ObligationStale.into()); + } + // 1. let pool_reward_manager = accounts.reserve.pool_reward_manager_mut(position_kind); - if let Some(user_reward_manager) = accounts + if let Some((_, user_reward_manager)) = accounts .obligation .user_reward_managers .find_mut(reserve_key, position_kind) @@ -106,12 +120,23 @@ pub(crate) fn process( // 2. - let total_reward_amount = user_reward_manager.claim_rewards( + let (has_ended, total_reward_amount) = user_reward_manager.claim_rewards( pool_reward_manager, *accounts.reward_token_vault_info.key, clock, )?; + // AUDIT: + // > ClaimUserReward on Suilend can only be called permissionlessly if the reward period is + // > fully elapsed. + let payer_matches_obligation_owner = accounts + .perhaps_payer_info + .map_or(false, |payer| payer.key == &accounts.obligation.owner); + if !matches!(has_ended, HasRewardEnded::Yes) && !payer_matches_obligation_owner { + msg!("User reward manager has not ended, but payer does not match obligation owner"); + return Err(LendingError::InvalidSigner.into()); + } + // 3. if total_reward_amount > 0 { @@ -204,6 +229,7 @@ impl<'a, 'info> ClaimUserReward<'a, 'info> { let reward_token_vault_info = next_account_info(iter)?; let lending_market_info = next_account_info(iter)?; let token_program_info = next_account_info(iter)?; + let perhaps_payer_info = next_account_info(iter).ok(); let (_, reserve) = check_and_unpack_pool_reward_accounts( program_id, @@ -218,6 +244,11 @@ impl<'a, 'info> ClaimUserReward<'a, 'info> { }, )?; + if perhaps_payer_info.map(|a| !a.is_signer).unwrap_or(false) { + msg!("Payer account must be a signer"); + return Err(LendingError::InvalidSigner.into()); + } + if obligation_info.owner != program_id { msg!("Obligation provided is not owned by the lending program"); return Err(LendingError::InvalidAccountOwner.into()); @@ -230,6 +261,22 @@ impl<'a, 'info> ClaimUserReward<'a, 'info> { return Err(LendingError::InvalidAccountInput.into()); } + // AUDIT: + // > In ClaimUserReward, because this is a permissionless instruction, we recommend + // > validating that obligation_owner_token_account_info is an associated token account + // > (ATA), rather than only a token account owned by the obligation owner. + // > Allowing arbitrary token accounts would require indexing each one, adding unnecessary + // > complexity and risk. + let expected_ata = get_associated_token_address_with_program_id( + &obligation.owner, + reward_mint_info.key, + token_program_info.key, + ); + if expected_ata != *obligation_owner_token_account_info.key { + msg!("Token account for collecting rewards must be ATA"); + return Err(LendingError::InvalidAccountInput.into()); + } + if obligation_owner_token_account_info.owner != token_program_info.key { msg!("Obligation owner token account provided must be owned by the token program"); return Err(LendingError::InvalidTokenOwner.into()); @@ -237,12 +284,6 @@ impl<'a, 'info> ClaimUserReward<'a, 'info> { let obligation_owner_token_account = unpack_token_account(&obligation_owner_token_account_info.data.borrow())?; - if obligation_owner_token_account.owner != obligation.owner { - msg!( - "Obligation owner token account owner does not match the obligation owner provided" - ); - return Err(LendingError::InvalidAccountInput.into()); - } if obligation_owner_token_account.mint != *reward_mint_info.key { msg!("Obligation owner token account mint does not match the reward mint provided"); return Err(LendingError::InvalidAccountInput.into()); @@ -283,6 +324,7 @@ impl<'a, 'info> ClaimUserReward<'a, 'info> { } Ok(Self { + perhaps_payer_info, obligation_info, obligation_owner_token_account_info, _reserve_info: reserve_info, diff --git a/token-lending/program/src/processor/liquidity_mining/upgrade_reserve.rs b/token-lending/program/src/processor/liquidity_mining/upgrade_reserve.rs index 35e97e5e773..60d5508dc81 100644 --- a/token-lending/program/src/processor/liquidity_mining/upgrade_reserve.rs +++ b/token-lending/program/src/processor/liquidity_mining/upgrade_reserve.rs @@ -11,7 +11,7 @@ use solana_program::{ sysvar::Sysvar, }; use solend_sdk::state::discriminator::AccountDiscriminator; -use solend_sdk::state::RESERVE_LEN_V2_0_2; +use solend_sdk::state::{PROGRAM_VERSION_2_0_2, RESERVE_LEN_V2_0_2}; use solend_sdk::{error::LendingError, state::Reserve}; struct UpgradeReserveAccounts<'a, 'info> { @@ -121,6 +121,20 @@ impl<'a, 'info> UpgradeReserveAccounts<'a, 'info> { return Err(LendingError::InvalidAccountInput.into()); } + // AUDIT: + // > UpgradeReserve should verify that a Reserve was previously stored in the account before + // > performing the upgrade. + // > This doesn’t appear to be exploitable—it would just allow an attacker to create + // > a valid-looking Reserve filled with zeros—but it’s worth addressing. + // > Anybody can create an account owned by solend with size == RESERVE_LEN_V2_0_2 by + // > calling system_instruction::create_account, so you can't only rely on checking the + // > account length && ownership. + // > Asserting that the first byte is equal to the expected old version would fix it. + if reserve_info.data.borrow()[0] != PROGRAM_VERSION_2_0_2 { + msg!("Reserve provided must be a v2.0.2 reserve"); + return Err(LendingError::InvalidAccountInput.into()); + } + if system_program.key != &solana_program::system_program::id() { msg!("System program provided must be the system program"); return Err(LendingError::InvalidAccountInput.into()); diff --git a/token-lending/program/tests/claim_pool_reward.rs b/token-lending/program/tests/claim_pool_reward.rs index 34acd62cd87..092786ca8d8 100644 --- a/token-lending/program/tests/claim_pool_reward.rs +++ b/token-lending/program/tests/claim_pool_reward.rs @@ -116,11 +116,32 @@ async fn test_(position_kind: PositionKind) { .await; // user must have a token account to deposit rewards into ahead of time - user.create_token_account(&reward.mint, &mut test).await; + user.create_associated_token_account(&reward.mint, &mut test) + .await; let balance_checker = BalanceChecker::start(&mut test, &[&TokenAccount(reward.vault.pubkey()), &user]).await; + let err = lending_market + .claim_pool_reward( + &mut test, + &obligation, + &usdc_reserve, + &user, + &reward, + position_kind, + None, + ) + .await + .expect_err("Cannot claim reward before it ends unless owner"); + + match err.unwrap() { + TransactionError::InstructionError(_, InstructionError::Custom(err_code)) => { + assert_eq!(err_code, LendingError::InvalidSigner as u32); + } + _ => panic!("Expected LendingError::InvalidSigner, got: {:?}", err), + }; + lending_market .claim_pool_reward( &mut test, @@ -129,6 +150,7 @@ async fn test_(position_kind: PositionKind) { &user, &reward, position_kind, + Some(&user), ) .await .expect("Should claim reward"); @@ -233,6 +255,7 @@ async fn test_(position_kind: PositionKind) { &user, &reward, position_kind, + None, ) .await .expect("Should claim reward"); @@ -334,6 +357,7 @@ async fn test_cannot_claim_into_wrong_destination() { &lending_market_owner, // ! wrong &reward, PositionKind::Deposit, + None, ) .await .expect_err("Cannot steal user reward"); @@ -430,7 +454,8 @@ async fn test_migrate_obligation() { .await; // user must have a token account to deposit rewards into ahead of time - user.create_token_account(&reward.mint, &mut test).await; + user.create_associated_token_account(&reward.mint, &mut test) + .await; let balance_checker = BalanceChecker::start(&mut test, &[&user]).await; @@ -447,6 +472,7 @@ async fn test_migrate_obligation() { &user, &reward, PositionKind::Deposit, + None, ) .await .expect("Should claim reward"); @@ -494,6 +520,7 @@ async fn test_migrate_obligation() { &user, &reward, PositionKind::Deposit, + None, ) .await .expect("Should claim reward"); diff --git a/token-lending/program/tests/helpers/solend_program_test.rs b/token-lending/program/tests/helpers/solend_program_test.rs index 097a40ede00..b2a649835cf 100644 --- a/token-lending/program/tests/helpers/solend_program_test.rs +++ b/token-lending/program/tests/helpers/solend_program_test.rs @@ -77,7 +77,7 @@ mod cu_budgets { pub(super) const ADD_POOL_REWARD: u32 = 80_017; pub(super) const EDIT_POOL_REWARD: u32 = 80_018; pub(super) const CLOSE_POOL_REWARD: u32 = 80_019; - pub(super) const CLAIM_POOL_REWARD: u32 = 80_020; + pub(super) const CLAIM_POOL_REWARD: u32 = 200_020; } /// This is at most how many bytes can an obligation grow. @@ -446,6 +446,29 @@ impl SolendProgramTest { keypair.pubkey() } + pub async fn create_associated_token_account( + &mut self, + owner: &Pubkey, + mint: &Pubkey, + ) -> Pubkey { + let instructions = [ + spl_associated_token_account::instruction::create_associated_token_account( + &self.context.payer.pubkey(), + owner, + mint, + &spl_token::id(), + ), + ]; + + self.process_transaction(&instructions, None).await.unwrap(); + + spl_associated_token_account::get_associated_token_address_with_program_id( + owner, + mint, + &spl_token::id(), + ) + } + pub async fn mint_to(&mut self, mint: &Pubkey, dst: &Pubkey, amount: u64) { assert!(self.mints.contains_key(mint)); @@ -836,6 +859,30 @@ impl User { } } + pub async fn create_associated_token_account( + &mut self, + mint: &Pubkey, + test: &mut SolendProgramTest, + ) -> Info { + match self + .token_accounts + .iter() + .find(|ta| ta.account.mint == *mint) + { + None => { + let pubkey = test + .create_associated_token_account(&self.keypair.pubkey(), mint) + .await; + let account = test.load_account::(pubkey).await; + + self.token_accounts.push(account.clone()); + + account + } + Some(t) => t.clone(), + } + } + pub async fn transfer( &self, mint: &Pubkey, @@ -1066,6 +1113,7 @@ impl Info { obligation_owner: &User, reward: &LiqMiningReward, position_kind: PositionKind, + signer: Option<&User>, ) -> Result<(), BanksClientError> { let (reward_authority_pda, reward_authority_bump) = find_reward_vault_authority( &solend_program::id(), @@ -1073,23 +1121,40 @@ impl Info { &reward.vault.pubkey(), ); - let instructions = [ + let mut instructions = if matches!(position_kind, PositionKind::Borrow) { + self.build_refresh_instructions(test, obligation, None) + .await + } else { + vec![] + }; + + instructions.extend_from_slice(&[ ComputeBudgetInstruction::set_compute_unit_limit(cu_budgets::CLAIM_POOL_REWARD), claim_pool_reward( solend_program::id(), reward_authority_bump, position_kind, obligation.pubkey, - obligation_owner.get_account(&reward.mint).unwrap(), + spl_associated_token_account::get_associated_token_address_with_program_id( + &obligation_owner.keypair.pubkey(), + &reward.mint, + &spl_token::id(), + ), reserve.pubkey, reward.mint, reward_authority_pda, reward.vault.pubkey(), self.pubkey, + signer.map(|s| s.keypair.pubkey()), ), - ]; + ]); - test.process_transaction(&instructions, None).await + if let Some(signer) = signer { + test.process_transaction(&instructions, Some(&[&signer.keypair])) + .await + } else { + test.process_transaction(&instructions, None).await + } } pub async fn donate_to_reserve( diff --git a/token-lending/sdk/src/instruction.rs b/token-lending/sdk/src/instruction.rs index 9b203aa5975..396c9dbb421 100644 --- a/token-lending/sdk/src/instruction.rs +++ b/token-lending/sdk/src/instruction.rs @@ -2284,19 +2284,26 @@ pub fn claim_pool_reward( reward_vault_authority: Pubkey, reward_vault: Pubkey, lending_market: Pubkey, + payer: Option, ) -> Instruction { + let mut accounts = vec![ + AccountMeta::new(obligation, false), + AccountMeta::new(obligation_owner_token_account_for_reward, false), + AccountMeta::new(reserve, false), + AccountMeta::new_readonly(reward_mint, false), + AccountMeta::new_readonly(reward_vault_authority, false), + AccountMeta::new(reward_vault, false), + AccountMeta::new_readonly(lending_market, false), + AccountMeta::new_readonly(spl_token::id(), false), + ]; + + if let Some(payer) = payer { + accounts.push(AccountMeta::new(payer, true)); + } + Instruction { program_id, - accounts: vec![ - AccountMeta::new(obligation, false), - AccountMeta::new(obligation_owner_token_account_for_reward, false), - AccountMeta::new(reserve, false), - AccountMeta::new_readonly(reward_mint, false), - AccountMeta::new_readonly(reward_vault_authority, false), - AccountMeta::new(reward_vault, false), - AccountMeta::new_readonly(lending_market, false), - AccountMeta::new_readonly(spl_token::id(), false), - ], + accounts, data: LendingInstruction::ClaimReward { reward_authority_bump, position_kind, diff --git a/token-lending/sdk/src/state/liquidity_mining.rs b/token-lending/sdk/src/state/liquidity_mining.rs index 19b97d454f1..439439a3b16 100644 --- a/token-lending/sdk/src/state/liquidity_mining.rs +++ b/token-lending/sdk/src/state/liquidity_mining.rs @@ -100,7 +100,7 @@ mod tests { clock.unix_timestamp = reward_period as i64 / 2; - let not_cancelled_claimed_slnd = { + let (_, not_cancelled_claimed_slnd) = { let mut pool_reward_manager = pool_reward_manager.clone(); let mut user_reward_manager = user_reward_manager.clone(); @@ -109,7 +109,7 @@ mod tests { .expect("It claims rewards") }; - let edited_claimed_slnd = { + let (_, edited_claimed_slnd) = { let mut pool_reward_manager = pool_reward_manager.clone(); let mut user_reward_manager = user_reward_manager.clone(); @@ -124,7 +124,7 @@ mod tests { .expect("It claims rewards") }; - let canceled_claimed_slnd = { + let (_, canceled_claimed_slnd) = { let pool_reward_index = 0; pool_reward_manager .cancel_pool_reward(pool_reward_index, &clock) @@ -237,11 +237,11 @@ mod tests { clock.unix_timestamp += rng.gen_range(0..MIN_REWARD_PERIOD_SECS) as i64; for user_reward_manager in &mut user_reward_managers { - let claimed_foo = user_reward_manager + let (_, claimed_foo) = user_reward_manager .claim_rewards(&mut pool_reward_manager, foo_vault, &clock) .expect("It claims foo rewards"); - let claimed_bar = user_reward_manager + let (_, claimed_bar) = user_reward_manager .claim_rewards(&mut pool_reward_manager, bar_vault, &clock) .expect("It claims bar rewards"); @@ -277,11 +277,11 @@ mod tests { let neither_has_ended = !has_foo_ended && !has_bar_ended; for user_reward_manager in &mut user_reward_managers { - let claimed_foo = user_reward_manager + let (_, claimed_foo) = user_reward_manager .claim_rewards(&mut pool_reward_manager, foo_vault, &clock) .expect("It claims foo rewards"); - let claimed_bar = user_reward_manager + let (_, claimed_bar) = user_reward_manager .claim_rewards(&mut pool_reward_manager, bar_vault, &clock) .expect("It claims bar rewards"); @@ -310,12 +310,12 @@ mod tests { // check that no more rewards can be claimed for user_reward_manager in &mut user_reward_managers { - let claimed_foo = user_reward_manager + let (_, claimed_foo) = user_reward_manager .claim_rewards(&mut pool_reward_manager, foo_vault, &clock) .expect("It claims foo rewards"); prop_assert_eq!(claimed_foo, 0); - let claimed_bar = user_reward_manager + let (_, claimed_bar) = user_reward_manager .claim_rewards(&mut pool_reward_manager, bar_vault, &clock) .expect("It claims bar rewards"); prop_assert_eq!(claimed_bar, 0); @@ -420,7 +420,7 @@ mod suilend_tests { // 1/4 of the reward time passes clock.unix_timestamp = 5 * SECONDS_IN_A_DAY as i64; - let claimed_slnd = user_reward_manager_1 + let (_, claimed_slnd) = user_reward_manager_1 .claim_rewards(&mut pool_reward_manager, slnd_vault, &clock) .expect("It claims rewards"); assert_eq!(claimed_slnd, 25 * 1_000_000); @@ -440,12 +440,12 @@ mod suilend_tests { // 1/2 of the reward time passes clock.unix_timestamp = 10 * SECONDS_IN_A_DAY as i64; - let claimed_slnd = user_reward_manager_1 + let (_, claimed_slnd) = user_reward_manager_1 .claim_rewards(&mut pool_reward_manager, slnd_vault, &clock) .expect("It claims rewards"); assert_eq!(claimed_slnd, 5 * 1_000_000); - let claimed_slnd = user_reward_manager_2 + let (_, claimed_slnd) = user_reward_manager_2 .claim_rewards(&mut pool_reward_manager, slnd_vault, &clock) .expect("It claims rewards"); assert_eq!(claimed_slnd, 20 * 1_000_000); @@ -461,12 +461,12 @@ mod suilend_tests { // the reward is finished clock.unix_timestamp = 20 * SECONDS_IN_A_DAY as i64; - let claimed_slnd = user_reward_manager_1 + let (_, claimed_slnd) = user_reward_manager_1 .claim_rewards(&mut pool_reward_manager, slnd_vault, &clock) .expect("It claims rewards"); assert_eq!(claimed_slnd, 25 * 1_000_000); - let claimed_slnd = user_reward_manager_2 + let (_, claimed_slnd) = user_reward_manager_2 .claim_rewards(&mut pool_reward_manager, slnd_vault, &clock) .expect("It claims rewards"); assert_eq!(claimed_slnd, 25 * 1_000_000); @@ -538,22 +538,22 @@ mod suilend_tests { { clock.unix_timestamp = 30 * SECONDS_IN_A_DAY as i64; - let claimed_slnd = user_reward_manager_1 + let (_, claimed_slnd) = user_reward_manager_1 .claim_rewards(&mut pool_reward_manager, slnd_vault1, &clock) .expect("It claims rewards"); assert_eq!(claimed_slnd, 87_500_000); - let claimed_slnd = user_reward_manager_1 + let (_, claimed_slnd) = user_reward_manager_1 .claim_rewards(&mut pool_reward_manager, slnd_vault2, &clock) .expect("It claims rewards"); assert_eq!(claimed_slnd, 75 * 1_000_000); - let claimed_slnd = user_reward_manager_2 + let (_, claimed_slnd) = user_reward_manager_2 .claim_rewards(&mut pool_reward_manager, slnd_vault1, &clock) .expect("It claims rewards"); assert_eq!(claimed_slnd, 12_500_000); - let claimed_slnd = user_reward_manager_2 + let (_, claimed_slnd) = user_reward_manager_2 .claim_rewards(&mut pool_reward_manager, slnd_vault2, &clock) .expect("It claims rewards"); assert_eq!(claimed_slnd, 25 * 1_000_000); @@ -595,7 +595,7 @@ mod suilend_tests { user_reward_manager_1.set_share(&mut pool_reward_manager, 1); clock.unix_timestamp = 20 * SECONDS_IN_A_DAY as i64; - let claimed_slnd = user_reward_manager_1 + let (_, claimed_slnd) = user_reward_manager_1 .claim_rewards(&mut pool_reward_manager, slnd_vault, &clock) .expect("It claims rewards"); // 50 usdc is unallocated since there was zero share from 0-10 seconds @@ -643,13 +643,13 @@ mod suilend_tests { { clock.unix_timestamp = 20 * SECONDS_IN_A_DAY as i64; - let claimed_slnd = user_reward_manager_1 + let (_, claimed_slnd) = user_reward_manager_1 .claim_rewards(&mut pool_reward_manager, slnd_vault, &clock) .expect("It claims rewards"); assert_eq!(claimed_slnd, 75 * 1_000_000); user_reward_manager_2.set_share(&mut pool_reward_manager, 1); - let claimed_slnd = user_reward_manager_2 + let (_, claimed_slnd) = user_reward_manager_2 .claim_rewards(&mut pool_reward_manager, slnd_vault, &clock) .expect("It claims rewards"); assert_eq!(claimed_slnd, 25 * 1_000_000); @@ -732,7 +732,7 @@ mod suilend_tests { { clock.unix_timestamp = 15 * SECONDS_IN_A_DAY as i64; - let claimed_slnd = user_reward_manager_1 + let (_, claimed_slnd) = user_reward_manager_1 .claim_rewards(&mut pool_reward_manager, slnd_vault, &clock) .expect("It claims rewards"); assert_eq!(claimed_slnd, 50 * 1_000_000); @@ -797,7 +797,7 @@ mod suilend_tests { assert_eq!(unallocated_rewards, -50 * 1_000_000 + 1); // approx clock.unix_timestamp = 15 * SECONDS_IN_A_DAY as i64; - let claim_slnd = user_reward_manager_1 + let (_, claim_slnd) = user_reward_manager_1 .claim_rewards(&mut pool_reward_manager, slnd_vault1, &clock) .expect("It claims rewards"); assert_eq!(claim_slnd, 50 * 1_000_000); @@ -819,7 +819,7 @@ mod suilend_tests { { clock.unix_timestamp = 30 * SECONDS_IN_A_DAY as i64; - let claimed_slnd = user_reward_manager_2 + let (_, claimed_slnd) = user_reward_manager_2 .claim_rewards(&mut pool_reward_manager, slnd_vault2, &clock) .expect("It claims rewards"); assert_eq!(claimed_slnd, 50 * 1_000_000); diff --git a/token-lending/sdk/src/state/liquidity_mining/user_reward_manager.rs b/token-lending/sdk/src/state/liquidity_mining/user_reward_manager.rs index 516d277cf45..914362290b6 100644 --- a/token-lending/sdk/src/state/liquidity_mining/user_reward_manager.rs +++ b/token-lending/sdk/src/state/liquidity_mining/user_reward_manager.rs @@ -7,7 +7,7 @@ use crate::{ math::{Decimal, TryAdd, TryMul, TrySub}, state::{ pack_decimal, unpack_decimal, PoolRewardEntry, PoolRewardId, PoolRewardManager, - PositionKind, MAX_REWARDS, + PositionKind, MAX_OBLIGATION_RESERVES, MAX_REWARDS, }, }; use arrayref::{array_mut_ref, array_ref, array_refs, mut_array_refs}; @@ -97,11 +97,14 @@ impl UserRewardManagers { &mut self, reserve: Pubkey, position_kind: PositionKind, - ) -> Option<&mut UserRewardManager> { - self.0.iter_mut().find(|user_reward_manager| { - user_reward_manager.reserve == reserve - && user_reward_manager.position_kind == position_kind - }) + ) -> Option<(usize, &mut UserRewardManager)> { + self.0 + .iter_mut() + .enumerate() + .find(|(_, user_reward_manager)| { + user_reward_manager.reserve == reserve + && user_reward_manager.position_kind == position_kind + }) } /// Updates the [UserRewardManager] for the given reserve. @@ -123,7 +126,7 @@ impl UserRewardManagers { new_share: u64, clock: &Clock, ) -> Result<(), ProgramError> { - let user_reward_manager = if let Some(user_reward_manager) = + let (index, user_reward_manager) = if let Some((index, user_reward_manager)) = self.find_mut(reserve, position_kind) { user_reward_manager.update( @@ -131,21 +134,48 @@ impl UserRewardManagers { clock, CreatingNewUserRewardManager::No, )?; - user_reward_manager + + (index, user_reward_manager) + } else if self.len() >= MAX_OBLIGATION_RESERVES { + // AUDIT: + // > Right now the max number of UserRewardManagers is 10 and the max number of rewards + // > is 30, but that's not really enforced because you are using debug_asserts which are + // > ignored in release mode (see MAX_REWARDS and MAX_OBLIGATION_REWARDS). + // > It's only enforced by checking the final packed length is less than + // > Obligation::MAX_LEN, so for example you can have an Obligation with 134 + // > UserRewardManagers, each tracking 1 reward. + msg!("User rewards full, claim rewards to make space."); + return Err(LendingError::ObligationReserveLimit.into()); } else { let mut new_user_reward_manager = UserRewardManager::new(reserve, position_kind, clock); new_user_reward_manager.populate(pool_reward_manager, clock)?; self.0.push(new_user_reward_manager); - // SAFETY: we just pushed a new item to the vector so ok to unwrap - self.0.last_mut().unwrap() + + // SAFETY: we just pushed a new item to the vector + (self.0.len() - 1, self.0.last_mut().unwrap()) }; user_reward_manager.set_share(pool_reward_manager, new_share); + // AUDIT: + // > We believe you should remove UserRewardManager entries when all the earned rewards are + // > claimed and the share is set to 0 (ie there is no corresponding Position in the + // > obligation) + if new_share == 0 && user_reward_manager.rewards.is_empty() { + self.0.swap_remove(index); + } + Ok(()) } } +/// Whether the reward was removed from the user manager. +#[allow(missing_docs)] +pub enum HasRewardEnded { + No, + Yes, +} + impl UserRewardManager { /// Claims all rewards that the user has earned. /// Returns how many tokens should be transferred to the user. @@ -158,7 +188,7 @@ impl UserRewardManager { pool_reward_manager: &mut PoolRewardManager, vault: Pubkey, clock: &Clock, - ) -> Result { + ) -> Result<(HasRewardEnded, u64), ProgramError> { self.update(pool_reward_manager, clock, CreatingNewUserRewardManager::No)?; let (pool_reward_index, pool_reward) = pool_reward_manager @@ -185,22 +215,36 @@ impl UserRewardManager { // User is not tracking this reward, nothing to claim. // Let's be graceful and make this a no-op. // Prevents failures when multiple parties crank rewards. - return Ok(0); + return Ok((HasRewardEnded::Yes, 0)); }; let to_claim = user_reward.withdraw_earned_rewards()?; - if pool_reward.has_ended(clock) && user_reward.earned_rewards.try_floor_u64()? == 0 { + if (self.share == 0 || pool_reward.has_ended(clock)) + && user_reward.earned_rewards.try_floor_u64()? == 0 + { // This reward won't be used anymore as it ended and the user // claimed all there was to claim. // We can clean up this user reward. + + // AUDIT: + // > UserRewards tracked inside UserRewardManager.rewards can only be removed when the + // > pool_reward period has ended and the earned_reward has been claimed. + // > So this means that users are still forced to wait for reward expiration even when + // > they haven't any share. + // > I think you should also make it possible to cleanup the UserRewardManager.rewards + // > when the UserRewardManager.share is set to 0 and + // > UserRewardManager.rewards[i].earned_rewards.floor() == 0 + // We're fine with swap remove bcs `user_reward_index` is meaningless. // SAFETY: We got the index from enumeration, so must exist. self.rewards.swap_remove(user_reward_index); pool_reward.num_user_reward_managers -= 1; - } - Ok(to_claim) + Ok((HasRewardEnded::Yes, to_claim)) + } else { + Ok((HasRewardEnded::No, to_claim)) + } } } @@ -277,6 +321,9 @@ impl UserRewardManager { /// Should be updated before any interaction with rewards. /// + /// We expect the user share to be 0 if they are creating a new user manager. + /// The share is updated later. + /// /// # Assumption /// Invoker has checked that this [PoolRewardManager] matches the /// [UserRewardManager]. @@ -289,11 +336,12 @@ impl UserRewardManager { pool_reward_manager.update(clock)?; let curr_unix_timestamp_secs = clock.unix_timestamp as u64; - - if matches!( + let is_creating_new_reward_manager = matches!( creating_new_reward_manager, - CreatingNewUserRewardManager::No - ) && curr_unix_timestamp_secs == self.last_update_time_secs + CreatingNewUserRewardManager::Yes + ); + + if !is_creating_new_reward_manager && curr_unix_timestamp_secs == self.last_update_time_secs { return Ok(()); } @@ -313,7 +361,8 @@ impl UserRewardManager { .find(|(_, r)| r.pool_reward_index == pool_reward_index); let end_time_secs = pool_reward.start_time_secs + pool_reward.duration_secs as u64; - let has_ended_for_user = self.last_update_time_secs >= end_time_secs; + let has_ended_for_user = (!is_creating_new_reward_manager && self.share == 0) + || self.last_update_time_secs >= end_time_secs; match maybe_user_reward { Some((user_reward_index, user_reward)) @@ -346,6 +395,9 @@ impl UserRewardManager { None if pool_reward.start_time_secs > curr_unix_timestamp_secs => { // reward period has not started yet } + None if self.share == 0 && !is_creating_new_reward_manager => { + // user has no share, nothing to accrue + } None => { // user did not yet start accruing rewards @@ -359,10 +411,7 @@ impl UserRewardManager { .cumulative_rewards_per_share .try_mul(Decimal::from(self.share))? } else { - debug_assert!(matches!( - creating_new_reward_manager, - CreatingNewUserRewardManager::Yes - )); + debug_assert!(is_creating_new_reward_manager); Decimal::zero() }, };