Audit Fixes #1 (#1293)

* address runtime panics

* default pubkey not allowed as admin

* add checks for ownership

* fix typo

* standardize data.len check

* linting

* update transfer conditions

* lint

---------

Co-authored-by: Vladimir <[email protected]>

Author: augustbleeds

contracts/programs/data-feeds-cache/src/error.rs

@@ -72,4 +72,7 @@ pub enum DataCacheError {
 
     #[msg("failed legacy write")]
     FailedLegacyWrite,
+
+    #[msg("Invalid proposed owner")]
+    InvalidProposedOwner,
 }

contracts/programs/data-feeds-cache/src/lib.rs

@@ -71,6 +71,8 @@ pub mod data_feeds_cache {
     }
 
     pub fn set_feed_admin(ctx: Context<SetFeedAdmin>, admin: Pubkey, is_admin: bool) -> Result<()> {
+        require_keys_neq!(admin, Pubkey::default(), DataCacheError::InvalidAddress);
+
         let mut state = ctx.accounts.state.load_mut()?;
         let mut changed = false;
 
@@ -101,8 +103,14 @@ pub mod data_feeds_cache {
         proposed_owner: Pubkey,
     ) -> Result<()> {
         let state = &mut ctx.accounts.state.load_mut()?;
-        state.proposed_owner = proposed_owner;
+        require!(
+            proposed_owner != Pubkey::default()
+                && proposed_owner != state.owner
+                && proposed_owner != state.proposed_owner,
+            DataCacheError::InvalidProposedOwner
+        );
 
+        state.proposed_owner = proposed_owner;
         emit!(OwnershipTransfer {
             current_owner: state.owner,
             proposed_owner
@@ -689,6 +697,12 @@ pub mod data_feeds_cache {
             delete_permission_accounts.append(&mut temp_candidates_deletion)
         }
 
+        require_eq!(
+            delete_permission_accounts.len(),
+            delete_permission_account_infos.len(),
+            DataCacheError::ArrayLengthMismatch
+        );
+
         for (i, permission_account) in delete_permission_accounts.iter().enumerate() {
             let curr_permission_account_info = &delete_permission_account_infos[i];
 
@@ -1047,6 +1061,12 @@ pub mod data_feeds_cache {
         ctx: Context<'_, '_, 'info, 'info, QueryValues<'info>>,
         data_ids: Vec<[u8; 16]>,
     ) -> Result<Vec<DecimalReport>> {
+        require_eq!(
+            data_ids.len(),
+            ctx.remaining_accounts.len(),
+            DataCacheError::ArrayLengthMismatch
+        );
+
         let mut reports = Vec::new();
 
         for (i, data_id) in data_ids.iter().enumerate() {

contracts/programs/keystone-forwarder/src/context.rs

@@ -1,7 +1,10 @@
 use crate::common::ANCHOR_DISCRIMINATOR;
 use crate::error::AuthError;
 use crate::state::{ExecutionState, ForwarderState, OraclesConfig};
-use crate::utils::{extract_config_id, extract_raw_report, extract_transmission_id, get_config_id};
+use crate::utils::{
+    extract_config_id, extract_raw_report, extract_transmission_id, get_config_id, report_size_ok,
+};
+use crate::ForwarderError;
 use anchor_lang::prelude::*;
 
 #[derive(Accounts)]
@@ -99,6 +102,7 @@ pub struct Report<'info> {
 
     #[account(
         mut,
+        constraint = report_size_ok(&data) @ ForwarderError::InvalidReport,
         seeds = [b"config", state.key().as_ref(), &extract_config_id(extract_raw_report(&data))],
         bump
     )]
@@ -114,6 +118,7 @@ pub struct Report<'info> {
     // it is dependent on the state.key(), a predetermined bump, workflow execution id, config_id, report_id
     #[account(
         init_if_needed,
+        constraint = report_size_ok(&data) @ ForwarderError::InvalidReport,
         payer = transmitter,
         space = ANCHOR_DISCRIMINATOR + ExecutionState::INIT_SPACE,
         seeds = [

contracts/programs/keystone-forwarder/src/error.rs

@@ -22,6 +22,9 @@ pub enum AuthError {
 
 #[error_code]
 pub enum ForwarderError {
+    #[msg("Invalid proposed owner")]
+    InvalidProposedOwner,
+
     #[msg("Signers exceed max limit")]
     ExcessSigners,
 

contracts/programs/keystone-forwarder/src/lib.rs

@@ -25,6 +25,8 @@ declare_id!("whV7Q5pi17hPPyaPksToDw1nMx6Lh8qmNWKFaLRQ4wz");
 pub mod keystone_forwarder {
     use anchor_lang::solana_program::{instruction::Instruction, program::invoke_signed};
 
+    use crate::utils::report_size_ok;
+
     use super::*;
 
     pub fn initialize(ctx: Context<Initialize>) -> Result<()> {
@@ -52,11 +54,17 @@ pub mod keystone_forwarder {
         proposed_owner: Pubkey,
     ) -> Result<()> {
         let state = &mut ctx.accounts.state;
-        let state_current_owner = state.owner;
+        require!(
+            proposed_owner != Pubkey::default()
+                && proposed_owner != state.owner
+                && proposed_owner != state.proposed_owner,
+            ForwarderError::InvalidProposedOwner
+        );
+
         state.proposed_owner = proposed_owner;
 
         emit!(OwnershipTransfer {
-            current_owner: state_current_owner,
+            current_owner: state.owner,
             proposed_owner: state.proposed_owner
         });
 
@@ -114,10 +122,9 @@ pub mod keystone_forwarder {
         ctx: Context<'_, '_, '_, 'info, Report<'info>>,
         data: Vec<u8>,
     ) -> Result<()> {
-        let num_signatures = data[0] as usize;
-        let min_data_size = 1 + num_signatures * SIGNATURE_LEN + REPORT_CONTEXT_LEN;
+        require!(report_size_ok(&data), ForwarderError::InvalidReport);
 
-        require_gt!(data.len(), min_data_size, ForwarderError::InvalidReport);
+        let num_signatures = data[0] as usize;
 
         // get config
         let oracles_config = ctx.accounts.oracles_config.load()?;

contracts/programs/keystone-forwarder/src/utils.rs

@@ -1,12 +1,21 @@
 use anchor_lang::prelude::Pubkey;
 use anchor_lang::solana_program::hash;
 
-use crate::common::{REPORT_CONTEXT_LEN, SIGNATURE_LEN};
+use crate::common::{METADATA_LENGTH, REPORT_CONTEXT_LEN, SIGNATURE_LEN};
 
 pub fn get_config_id(don_id: u32, config_version: u32) -> u64 {
     ((don_id as u64) << 32) | (config_version as u64)
 }
 
+pub fn report_size_ok(data: &[u8]) -> bool {
+    if !data.is_empty() {
+        let num_signatures = data[0] as usize;
+        data.len() > 1 + num_signatures * SIGNATURE_LEN + METADATA_LENGTH + REPORT_CONTEXT_LEN
+    } else {
+        false
+    }
+}
+
 // data =  len_signatures (1) | signatures (N*65) | raw_report (M) | report_context (96)
 pub fn extract_raw_report(data: &[u8]) -> &[u8] {
     let num_signatures = data[0] as usize;