Merge pull request #16 from Squads-Protocol/audit/certora-remediations

Audit/certora remediations

Author: 0xRigel-squads

programs/squads_smart_account_program/src/instructions/smart_account_create.rs

@@ -126,7 +126,6 @@ impl<'info> CreateSmartAccount<'info> {
                 ),
                 creation_fee,
             )?;
-            msg!("Creation fee: {}", creation_fee / LAMPORTS_PER_SOL);
         }
 
         // Increment the smart account index.

programs/squads_smart_account_program/src/state/policies/implementations/spending_limit_policy.rs

@@ -124,7 +124,7 @@ impl PolicyPayloadConversionTrait for SpendingLimitPolicyCreationPayload {
         Ok(SpendingLimitPolicy {
             spending_limit: SpendingLimitV2 {
                 mint: self.mint,
-                time_constraints: self.time_constraints,
+                time_constraints: modified_time_constraints,
                 quantity_constraints: self.quantity_constraints,
                 usage: usage_state,
             },
@@ -333,7 +333,10 @@ impl SpendingLimitPolicy {
                 );
 
                 // Check that the source account is not the same as the destination account
-                require!(source_account_info.key() != destination_account_info.key(), SmartAccountError::InvalidAccount);
+                require!(
+                    source_account_info.key() != destination_account_info.key(),
+                    SmartAccountError::InvalidAccount
+                );
 
                 // Check the system program
                 require!(

programs/squads_smart_account_program/src/state/policies/utils/account_tracking.rs

@@ -1,12 +1,14 @@
 use anchor_lang::{prelude::*, Ids};
 use anchor_spl::token_interface::{TokenAccount, TokenInterface};
 
-use crate::{errors::SmartAccountError, state::policies::utils::spending_limit_v2::SpendingLimitV2};
+use crate::{
+    errors::SmartAccountError, state::policies::utils::spending_limit_v2::SpendingLimitV2,
+};
 
 pub struct TrackedTokenAccount<'info> {
     pub account: &'info AccountInfo<'info>,
     pub balance: u64,
-    pub delegate: Option<Pubkey>,
+    pub delegate: Option<(Pubkey, u64)>,
     pub authority: Pubkey,
 }
 
@@ -43,10 +45,8 @@ pub fn check_pre_balances<'info>(
     // owned by the executing account
     let token_program_ids = TokenInterface::ids();
     for account in accounts {
-
         // Only track accounts owned by a token program and that are writable
         if token_program_ids.contains(&account.owner) && account.is_writable {
-
             // This may fail for accounts that are not token accounts, so skip if it does
             let Ok(token_account) = InterfaceAccount::<TokenAccount>::try_from(account) else {
                 continue;
@@ -55,7 +55,7 @@ pub fn check_pre_balances<'info>(
             if token_account.owner == executing_account {
                 let balance = token_account.amount;
                 let delegate = if let Some(delegate_key) = Option::from(token_account.delegate) {
-                    Some(delegate_key)
+                    Some((delegate_key, token_account.delegated_amount))
                 } else {
                     None
                 };
@@ -91,6 +91,7 @@ impl<'info> Balances<'info> {
         let current_lamports = self.executing_account.account.lamports();
 
         // Check the SOL spending limit
+        // Note: Assumes spending limits have been deduplicated, and no two spending limits can have the same mint
         if let Some(spending_limit) = spending_limits.iter_mut().find(|spending_limit| {
             spending_limit.mint() == Pubkey::default()
                 && spending_limit.is_active(current_timestamp).is_ok()
@@ -165,12 +166,24 @@ impl<'info> Balances<'info> {
                 );
             }
 
-            // Ensure the delegate and authority have not changed
-            let post_delegate = Option::from(post_token_account.delegate);
-            require!(
-                post_delegate == tracked_token_account.delegate,
-                SmartAccountError::ProgramInteractionIllegalTokenAccountModification
-            );
+            // Ensure the delegate, and authority have not changed. Delegated
+            // amount may decrease
+            let post_delegate: Option<(Pubkey, u64)> =
+                if let Some(delegate_key) = Option::from(post_token_account.delegate) {
+                    Some((delegate_key, post_token_account.delegated_amount))
+                } else {
+                    None
+                };
+            match (post_delegate, tracked_token_account.delegate) {
+                (Some(post_delegate), Some(tracked_delegate)) => {
+                    require_eq!(post_delegate.0, tracked_delegate.0);
+                    require_gte!(post_delegate.1, tracked_delegate.1);
+                }
+                (None, None) => {}
+                _ => {
+                    return Err(SmartAccountError::ProgramInteractionIllegalTokenAccountModification.into());
+                }
+            };
             require_eq!(
                 post_token_account.owner,
                 tracked_token_account.authority,

programs/squads_smart_account_program/src/state/settings.rs

@@ -495,7 +495,16 @@ impl Settings {
                     PolicyCreationPayload::ProgramInteraction(creation_payload) => {
                         PolicyState::ProgramInteraction(creation_payload.to_policy_state()?)
                     }
-                    PolicyCreationPayload::SpendingLimit(creation_payload) => {
+                    PolicyCreationPayload::SpendingLimit(mut creation_payload) => {
+                        // If accumulate unused is true, and the policy has a
+                        // start date in the past, set it to the current
+                        // timestamp to avoid unintended accumulated usage
+                        let current_timestamp = Clock::get()?.unix_timestamp;
+                        if creation_payload.time_constraints.accumulate_unused
+                            && creation_payload.time_constraints.start < current_timestamp
+                        {
+                            creation_payload.time_constraints.start = current_timestamp;
+                        }
                         PolicyState::SpendingLimit(creation_payload.to_policy_state()?)
                     }
                     PolicyCreationPayload::SettingsChange(creation_payload) => {
@@ -589,18 +598,26 @@ impl Settings {
                     .as_ref()
                     .ok_or(SmartAccountError::MissingAccount)?;
 
-                let new_policy_state = match policy_update_payload.clone() {
-                    PolicyCreationPayload::InternalFundTransfer(creation_payload) => {
-                        PolicyState::InternalFundTransfer(creation_payload.to_policy_state()?)
-                    }
-                    PolicyCreationPayload::ProgramInteraction(creation_payload) => {
-                        PolicyState::ProgramInteraction(creation_payload.to_policy_state()?)
-                    }
-                    PolicyCreationPayload::SpendingLimit(creation_payload) => {
-                        PolicyState::SpendingLimit(creation_payload.to_policy_state()?)
-                    }
-                    PolicyCreationPayload::SettingsChange(creation_payload) => {
-                        PolicyState::SettingsChange(creation_payload.to_policy_state()?)
+                // Only accept updates to the same policy type
+                let new_policy_state = match (&policy.policy_state, policy_update_payload.clone()) {
+                    (
+                        PolicyState::InternalFundTransfer(_),
+                        PolicyCreationPayload::InternalFundTransfer(creation_payload),
+                    ) => PolicyState::InternalFundTransfer(creation_payload.to_policy_state()?),
+                    (
+                        PolicyState::ProgramInteraction(_),
+                        PolicyCreationPayload::ProgramInteraction(creation_payload),
+                    ) => PolicyState::ProgramInteraction(creation_payload.to_policy_state()?),
+                    (
+                        PolicyState::SpendingLimit(_),
+                        PolicyCreationPayload::SpendingLimit(creation_payload),
+                    ) => PolicyState::SpendingLimit(creation_payload.to_policy_state()?),
+                    (
+                        PolicyState::SettingsChange(_),
+                        PolicyCreationPayload::SettingsChange(creation_payload),
+                    ) => PolicyState::SettingsChange(creation_payload.to_policy_state()?),
+                    (_, _) => {
+                        return err!(SmartAccountError::InvalidPolicyPayload);
                     }
                 };
 

programs/squads_smart_account_program/src/utils/context_validation.rs

@@ -7,7 +7,11 @@ pub fn validate_synchronous_consensus(
     remaining_accounts: &[AccountInfo],
 ) -> Result<()> {
     // Settings must not be time locked
-    require_eq!(consensus_account.time_lock(), 0, SmartAccountError::TimeLockNotZero);
+    require_eq!(
+        consensus_account.time_lock(),
+        0,
+        SmartAccountError::TimeLockNotZero
+    );
 
     // Get signers from remaining accounts using threshold
     let required_signer_count = consensus_account.threshold() as usize;
@@ -54,7 +58,7 @@ pub fn validate_synchronous_consensus(
 
     // Check if we have all required permissions (Initiate | Vote | Execute = 7)
     require!(
-        aggregated_permissions.mask == 7,
+        aggregated_permissions.mask == Permissions::all().mask,
         SmartAccountError::InsufficientAggregatePermissions
     );