[solana] Refactoring transfer vesting

[djb15]

No description provided.

[soteria-bc]

Note

commit 8e26bac

After the refactoring, it seems the issue is not fully resolved.

Currently, in transfer_vesting, the checks for new_stake_account_metadata, stake_account_metadata, and delegate_stake_account_metadata are only performed when the corresponding vesting_balance.stake_account_metadata or new_vesting_balance.stake_account_metadata exists.

However, when vesting_balance.stake_account_metadata or new_vesting_balance.stake_account_metadata does not exist, there is no requirement that the corresponding account must be None. Instead, any StakeAccountMetadata account can be passed in, which leads to the following issue:

self.vesting_balance.stake_account_metadata != Pubkey::default() && self.new_vesting_balance.stake_account_metadata == Pubkey::default(): In this case, no restrictions will be applied to new_stake_account_metadata, allowing it to be any StakeAccountMetadata account. A malicious user could set it to stake_account_metadata. Since in account list, the new_stake_account_metadata comes after stake_account_metadata, any changes to stake_account_metadata will be ineffective.

/* solana/programs/staking/src/contexts/transfer_vesting.rs */
016 | pub struct TransferVesting<'info> {
081 |     #[account(mut)]
082 |     pub delegate_stake_account_checkpoints: Option<AccountLoader<'info, CheckpointData>>,
083 |     #[account(mut)]
084 |     pub delegate_stake_account_metadata: Option<Box<Account<'info, StakeAccountMetadata>>>,
085 |     #[account(mut)]
086 |     pub stake_account_metadata: Option<Box<Account<'info, StakeAccountMetadata>>>,
         // @audit: when `new_stake_account_metadata == stake_account_metadata`, all changes made on stake_account_metadata will be overwritten by new_stake_account_metadata
087 |     #[account(mut)]
088 |     pub new_stake_account_metadata: Option<Box<Account<'info, StakeAccountMetadata>>>,
093 | }

108 | pub fn transfer_vesting(
109 |     &mut self,
110 |     new_vest_bump: u8,
111 |     new_vesting_balance_bump: u8,
112 | ) -> Result<TransferVestingEvents> {
148 |     if self.vesting_balance.stake_account_metadata != Pubkey::default() {
149 |         if let (
150 |             Some(stake_account_metadata),
151 |         ) = (
152 |             &mut self.stake_account_metadata,
153 |         ) {
250 |             let new_recorded_vesting_balance = stake_account_metadata
251 |                 .recorded_vesting_balance
252 |                 .checked_sub(self.vest.amount)
253 |                 .ok_or(VestingError::Underflow)?;
254 | 
                  // @audit: changes made on stake_account_metadata.recorded_vesting_balance will be overwritten by `new_stake_account_metadata`
255 |             let recorded_vesting_balance_changed = stake_account_metadata
256 |                 .update_recorded_vesting_balance(new_recorded_vesting_balance);
265 |     }
266 | 
267 |     if self.new_vesting_balance.stake_account_metadata != Pubkey::default() {
272 |         if let (
273 |             Some(new_stake_account_metadata),
274 |         ) = (
275 |             &mut self.new_stake_account_metadata,
276 |         ) {
                  // @audit: check on `new_stake_account_metadata` only applies when `new_vesting_balance.stake_account_metadata != Pubkey::default()``
278 |             require!(
279 |                 new_stake_account_metadata.owner == self.new_vesting_balance.vester,
280 |                 VestingError::InvalidStakeAccountOwner
281 |             );
311 |     }

Do you think it's a good idea to enforce that the corresponding option account must be None when no write or read operation is needed for it.

[djb15]

@soteria-bc Nice catch! Yeah I'd like this instruction to become much more exhaustive. I'm refactoring this PR at the moment, will be updating soon

[soteria-bc]

@soteria-bc Nice catch! Yeah I'd like this instruction to become much more exhaustive. I'm refactoring this PR at the moment, will be updating soon

Thanks! This exhaustive check looks great! It does simplify things a lot.

[mdulin2]

@soteria-bc I'm super impressed with your review of the fix. Thanks for taking it so seriously!

[mdulin2]

This has been a nightmare to handle. I'm hopeful we got the bugs out now. If not the contest will find them.

In my evaluation, I can't see anything wrong with it though. The derivation for the account PDAs is always verified, the optional accounts are handled better, the counting looks correct and the only case where duplicate accounts can be passed in is explicitly handled.