Miscellaneous fixes and cleanup (#24)

Fixes a number of small things including:

* Potential for self invocation of CopyGossipContactInfo to overwrite
real values
* Reading data offsets in CopyGossipContactInfo rather than hardcoding
assumed offsets
* Potential epoch overflow for cast epoch
* Comments around how specific structs and instructions are intended to
work
* Shifting epoch_start_timestamp to avoid overwriting existing bytes and
avoid trimming data type

keepers/validator-keeper/src/gossip.rs

@@ -128,7 +128,8 @@ impl GossipEntry {
                 validator_history_account: self.validator_history_account,
                 vote_account: self.vote_account,
                 instructions: solana_program::sysvar::instructions::id(),
-                signer: self.signer,
+                config: self.config,
+                oracle_authority: self.signer,
             }
             .to_account_metas(None),
             data: validator_history::instruction::CopyGossipContactInfo {}.data(),

programs/validator-history/idl/validator_history.json

@@ -318,7 +318,12 @@
           "isSigner": false
         },
         {
-          "name": "signer",
+          "name": "config",
+          "isMut": false,
+          "isSigner": false
+        },
+        {
+          "name": "oracleAuthority",
           "isMut": true,
           "isSigner": true
         }
@@ -649,20 +654,29 @@
             "name": "totalBlocks",
             "type": "u32"
           },
-          {
-            "name": "epochStartTimestamp",
-            "type": "u32"
-          },
           {
             "name": "epoch",
             "type": "u16"
           },
+          {
+            "name": "padding0",
+            "type": {
+              "array": [
+                "u8",
+                2
+              ]
+            }
+          },
+          {
+            "name": "epochStartTimestamp",
+            "type": "u64"
+          },
           {
             "name": "padding",
             "type": {
               "array": [
                 "u8",
-                246
+                240
               ]
             }
           }
@@ -935,6 +949,11 @@
       "code": 6010,
       "name": "SlotHistoryOutOfDate",
       "msg": "Slot history sysvar is not containing expected slots"
+    },
+    {
+      "code": 6011,
+      "name": "EpochTooLarge",
+      "msg": "Epoch larger than 65535, cannot be stored"
     }
   ]
 }

programs/validator-history/src/errors.rs

@@ -26,4 +26,6 @@ pub enum ValidatorHistoryError {
     ArithmeticError,
     #[msg("Slot history sysvar is not containing expected slots")]
     SlotHistoryOutOfDate,
+    #[msg("Epoch larger than 65535, cannot be stored")]
+    EpochTooLarge,
 }

programs/validator-history/src/instructions/backfill_total_blocks.rs

@@ -22,7 +22,7 @@ pub fn handle_backfill_total_blocks(
 ) -> Result<()> {
     let mut cluster_history_account = ctx.accounts.cluster_history_account.load_mut()?;
 
-    let epoch = cast_epoch(epoch);
+    let epoch = cast_epoch(epoch)?;
 
     // Need to backfill in ascending order when initially filling in the account otherwise entries will be out of order
     if !cluster_history_account.history.is_empty()

programs/validator-history/src/instructions/copy_cluster_info.rs

@@ -32,9 +32,10 @@ pub fn handle_copy_cluster_info(ctx: Context<CopyClusterInfo>) -> Result<()> {
 
     let clock = Clock::get()?;
 
-    let epoch = cast_epoch(clock.epoch);
+    let epoch = cast_epoch(clock.epoch)?;
 
     let epoch_start_timestamp = cast_epoch_start_timestamp(clock.epoch_start_timestamp);
+
     // Sets the slot history for the previous epoch, since the current epoch is not yet complete.
     if epoch > 0 {
         cluster_history_account

programs/validator-history/src/instructions/copy_gossip_contact_info.rs

@@ -2,12 +2,34 @@ use anchor_lang::{
     prelude::*,
     solana_program::{self, clock::Clock, pubkey::Pubkey, sysvar},
 };
+use bytemuck::{from_bytes, Pod, Zeroable};
 
 use crate::{
-    crds_value::CrdsData, errors::ValidatorHistoryError, state::ValidatorHistory, utils::cast_epoch,
+    crds_value::CrdsData, errors::ValidatorHistoryError, state::ValidatorHistory,
+    utils::cast_epoch, Config,
 };
 use validator_history_vote_state::VoteStateVersions;
 
+// Structs and constants copied from solana_sdk::ed25519_instruction. Copied in order to make fields public. Compilation issues hit when importing solana_sdk
+#[derive(Default, Debug, Copy, Clone, Zeroable, Pod, Eq, PartialEq)]
+#[repr(C)]
+pub struct Ed25519SignatureOffsets {
+    pub signature_offset: u16, // offset to ed25519 signature of 64 bytes
+    pub signature_instruction_index: u16, // instruction index to find signature
+    pub public_key_offset: u16, // offset to public key of 32 bytes
+    pub public_key_instruction_index: u16, // instruction index to find public key
+    pub message_data_offset: u16, // offset to start of message data
+    pub message_data_size: u16, // size of message data
+    pub message_instruction_index: u16, // index of instruction data to get message data
+}
+
+pub const PUBKEY_SERIALIZED_SIZE: usize = 32;
+pub const SIGNATURE_SERIALIZED_SIZE: usize = 64;
+pub const SIGNATURE_OFFSETS_SERIALIZED_SIZE: usize = 14;
+// bytemuck requires structures to be aligned
+pub const SIGNATURE_OFFSETS_START: usize = 2;
+pub const DATA_START: usize = SIGNATURE_OFFSETS_SERIALIZED_SIZE + SIGNATURE_OFFSETS_START;
+
 #[derive(Accounts)]
 pub struct CopyGossipContactInfo<'info> {
     #[account(
@@ -22,15 +44,21 @@ pub struct CopyGossipContactInfo<'info> {
     /// CHECK: Safe because it's a sysvar account
     #[account(address = sysvar::instructions::ID)]
     pub instructions: UncheckedAccount<'info>,
+    #[account(
+        seeds = [Config::SEED],
+        bump = config.bump,
+        has_one = oracle_authority
+    )]
+    pub config: Account<'info, Config>,
     #[account(mut)]
-    pub signer: Signer<'info>,
+    pub oracle_authority: Signer<'info>,
 }
 
 pub fn handle_copy_gossip_contact_info(ctx: Context<CopyGossipContactInfo>) -> Result<()> {
     let mut validator_history_account = ctx.accounts.validator_history_account.load_mut()?;
     let instructions = ctx.accounts.instructions.to_account_info();
     let clock = Clock::get()?;
-    let epoch = cast_epoch(clock.epoch);
+    let epoch = cast_epoch(clock.epoch)?;
 
     let verify_instruction = sysvar::instructions::get_instruction_relative(-1, &instructions)?;
 
@@ -39,9 +67,30 @@ pub fn handle_copy_gossip_contact_info(ctx: Context<CopyGossipContactInfo>) -> R
         return Err(ValidatorHistoryError::NotSigVerified.into());
     }
 
-    let message_signer = Pubkey::try_from(&verify_instruction.data[16..48])
-        .map_err(|_| ValidatorHistoryError::GossipDataInvalid)?;
-    let message_data = &verify_instruction.data[112..];
+    let ed25519_offsets = from_bytes::<Ed25519SignatureOffsets>(
+        &verify_instruction.data
+            [SIGNATURE_OFFSETS_START..SIGNATURE_OFFSETS_START + SIGNATURE_OFFSETS_SERIALIZED_SIZE],
+    );
+
+    // Check offsets and indices are correct so an attacker cannot submit invalid data
+    if ed25519_offsets.signature_instruction_index != ed25519_offsets.public_key_instruction_index
+        || ed25519_offsets.signature_instruction_index != ed25519_offsets.message_instruction_index
+        || ed25519_offsets.public_key_offset
+            != (SIGNATURE_OFFSETS_START + SIGNATURE_OFFSETS_SERIALIZED_SIZE) as u16
+        || ed25519_offsets.signature_offset
+            != ed25519_offsets.public_key_offset + PUBKEY_SERIALIZED_SIZE as u16
+        || ed25519_offsets.message_data_offset
+            != ed25519_offsets.signature_offset + SIGNATURE_SERIALIZED_SIZE as u16
+    {
+        return Err(ValidatorHistoryError::GossipDataInvalid.into());
+    }
+
+    let message_signer = Pubkey::try_from(
+        &verify_instruction.data[ed25519_offsets.public_key_offset as usize
+            ..ed25519_offsets.public_key_offset as usize + PUBKEY_SERIALIZED_SIZE],
+    )
+    .map_err(|_| ValidatorHistoryError::GossipDataInvalid)?;
+    let message_data = &verify_instruction.data[ed25519_offsets.message_data_offset as usize..];
 
     let crds_data: CrdsData =
         bincode::deserialize(message_data).map_err(|_| ValidatorHistoryError::GossipDataInvalid)?;

programs/validator-history/src/instructions/copy_tip_distribution_account.rs

@@ -57,7 +57,7 @@ pub fn handle_copy_tip_distribution_account(
     if epoch > Clock::get()?.epoch {
         return Err(ValidatorHistoryError::EpochOutOfRange.into());
     }
-    let epoch = cast_epoch(epoch);
+    let epoch = cast_epoch(epoch)?;
     let mut validator_history_account = ctx.accounts.validator_history_account.load_mut()?;
 
     let mut tda_data: &[u8] = &ctx.accounts.tip_distribution_account.try_borrow_data()?;

programs/validator-history/src/instructions/copy_vote_account.rs

@@ -25,7 +25,7 @@ pub struct CopyVoteAccount<'info> {
 pub fn handle_copy_vote_account(ctx: Context<CopyVoteAccount>) -> Result<()> {
     let mut validator_history_account = ctx.accounts.validator_history_account.load_mut()?;
     let clock = Clock::get()?;
-    let epoch = cast_epoch(clock.epoch);
+    let epoch = cast_epoch(clock.epoch)?;
 
     let commission = VoteStateVersions::deserialize_commission(&ctx.accounts.vote_account)?;
     validator_history_account.set_commission_and_slot(epoch, commission, clock.slot)?;

programs/validator-history/src/instructions/update_stake_history.rs

@@ -29,20 +29,23 @@ pub struct UpdateStakeHistory<'info> {
     pub oracle_authority: Signer<'info>,
 }
 
+// NOTE: If using this instruction to backfill a new validator history account, you must ensure that epochs are added in ascending order.
+// This is because new entries cannot be inserted for an epoch that is lower than the last entry's if missed.
 pub fn handle_update_stake_history(
     ctx: Context<UpdateStakeHistory>,
     epoch: u64,
     lamports: u64,
     rank: u32,
     is_superminority: bool,
 ) -> Result<()> {
-    let mut validator_history_account = ctx.accounts.validator_history_account.load_mut()?;
+    let mut validator_history_account: std::cell::RefMut<'_, ValidatorHistory> =
+        ctx.accounts.validator_history_account.load_mut()?;
 
     // Cannot set stake for future epochs
     if epoch > Clock::get()?.epoch {
         return Err(ValidatorHistoryError::EpochOutOfRange.into());
     }
-    let epoch = cast_epoch(epoch);
+    let epoch = cast_epoch(epoch)?;
 
     validator_history_account.set_stake(epoch, lamports, rank, is_superminority)?;
 

programs/validator-history/src/state.rs

@@ -61,11 +61,13 @@ pub struct ValidatorHistoryEntry {
     pub rank: u32,
     // Most recent updated slot for epoch credits and commission
     pub vote_account_last_update_slot: u64,
-    // MEV earned, stored as 1/100th SOL. mev_earned = 100 means 1 SOL earned
+    // MEV earned, stored as 1/100th SOL. mev_earned = 100 means 1.00 SOL earned
     pub mev_earned: u32,
     pub padding1: [u8; 84],
 }
 
+// Default values for fields in `ValidatorHistoryEntry` are the type's max value.
+// It's important to ensure that the max value is not a valid value for the field, so we can check if the field has been set.
 impl Default for ValidatorHistoryEntry {
     fn default() -> Self {
         Self {
@@ -378,7 +380,7 @@ impl ValidatorHistory {
         let epoch_credits_map: HashMap<u16, u32> =
             HashMap::from_iter(epoch_credits.iter().map(|(epoch, cur, prev)| {
                 (
-                    cast_epoch(*epoch),
+                    cast_epoch(*epoch).unwrap(), // all epochs in list will be valid if current epoch is valid
                     (cur.checked_sub(*prev)
                         .ok_or(ValidatorHistoryError::InvalidEpochCredits)
                         .unwrap() as u32),
@@ -654,18 +656,20 @@ pub struct ClusterHistory {
 #[zero_copy]
 pub struct ClusterHistoryEntry {
     pub total_blocks: u32,
-    pub epoch_start_timestamp: u32,
     pub epoch: u16,
-    pub padding: [u8; 246],
+    pub padding0: [u8; 2],
+    pub epoch_start_timestamp: u64,
+    pub padding: [u8; 240],
 }
 
 impl Default for ClusterHistoryEntry {
     fn default() -> Self {
         Self {
             total_blocks: u32::MAX,
             epoch: u16::MAX,
-            epoch_start_timestamp: u32::MAX,
-            padding: [u8::MAX; 246],
+            padding0: [u8::MAX; 2],
+            epoch_start_timestamp: u64::MAX,
+            padding: [u8::MAX; 240],
         }
     }
 }
@@ -798,7 +802,7 @@ impl ClusterHistory {
     pub fn set_epoch_start_timestamp(
         &mut self,
         epoch: u16,
-        epoch_start_timestamp: u32,
+        epoch_start_timestamp: u64,
     ) -> Result<()> {
         if let Some(entry) = self.history.last_mut() {
             if entry.epoch == epoch {

programs/validator-history/src/utils.rs

@@ -1,13 +1,21 @@
 use anchor_lang::{
-    prelude::{AccountInfo, Pubkey},
+    prelude::{AccountInfo, Pubkey, Result},
+    require,
     solana_program::native_token::lamports_to_sol,
 };
 
-pub fn cast_epoch(epoch: u64) -> u16 {
-    (epoch % u16::MAX as u64).try_into().unwrap()
+use crate::errors::ValidatorHistoryError;
+
+pub fn cast_epoch(epoch: u64) -> Result<u16> {
+    require!(
+        epoch < (u16::MAX as u64),
+        ValidatorHistoryError::EpochTooLarge
+    );
+    let epoch_u16: u16 = (epoch % u16::MAX as u64).try_into().unwrap();
+    Ok(epoch_u16)
 }
 
-pub fn cast_epoch_start_timestamp(start_timestamp: i64) -> u32 {
+pub fn cast_epoch_start_timestamp(start_timestamp: i64) -> u64 {
     start_timestamp.try_into().unwrap()
 }
 

tests/tests/test_cluster_history.rs

@@ -113,10 +113,10 @@ async fn test_start_epoch_timestamp() {
 
     assert_eq!(account.history.arr[0].epoch, 0);
     assert_eq!(account.history.arr[1].epoch, 1);
-    assert_ne!(account.history.arr[0].epoch_start_timestamp, u32::MAX);
-    assert_ne!(account.history.arr[1].epoch_start_timestamp, u32::MAX);
+    assert_ne!(account.history.arr[0].epoch_start_timestamp, u64::MAX);
+    assert_ne!(account.history.arr[1].epoch_start_timestamp, u64::MAX);
     assert_eq!(
         account.history.arr[0].epoch_start_timestamp,
-        account.history.arr[1].epoch_start_timestamp - (dif_slots * MS_PER_SLOT) as u32
+        account.history.arr[1].epoch_start_timestamp - dif_slots * MS_PER_SLOT
     );
 }