diff --git a/solana/bridge/agent/src/main.rs b/solana/bridge/agent/src/main.rs index b34c3759..6fe7eabe 100644 --- a/solana/bridge/agent/src/main.rs +++ b/solana/bridge/agent/src/main.rs @@ -72,6 +72,7 @@ use service::{ SubmitVaaRequest, SubmitVaaResponse, }; +use sha3::Digest; use solitaire::{ processors::seeded::Seeded, AccountState, @@ -238,6 +239,11 @@ fn pack_sig_verification_txs<'a>( } let vaa_body = serialize_vaa(vaa); + let body_hash: [u8; 32] = { + let mut h = sha3::Keccak256::default(); + h.write(vaa_body.as_slice())?; + h.finalize().into() + }; let mut verify_txs: Vec = Vec::new(); for (_tx_index, chunk) in signature_items.chunks(6).enumerate() { @@ -257,7 +263,7 @@ fn pack_sig_verification_txs<'a>( secp_payload.write_u16::((data_offset + 85 * i + 65) as u16)?; secp_payload.write_u8(0)?; secp_payload.write_u16::(message_offset as u16)?; - secp_payload.write_u16::(vaa_body.len() as u16)?; + secp_payload.write_u16::(body_hash.len() as u16)?; secp_payload.write_u8(0)?; signature_status[s.index as usize] = i as i8; } @@ -269,7 +275,7 @@ fn pack_sig_verification_txs<'a>( } // Write body - secp_payload.write(&vaa_body)?; + secp_payload.write(&body_hash)?; let secp_ix = Instruction { program_id: solana_sdk::secp256k1_program::id(), @@ -281,7 +287,6 @@ fn pack_sig_verification_txs<'a>( let payload = VerifySignaturesData { signers: signature_status, - hash: body_hash, initial_creation: false, }; @@ -289,6 +294,7 @@ fn pack_sig_verification_txs<'a>( *bridge, sender_keypair.pubkey(), vaa.guardian_set_index, + body_hash, payload, ) { Ok(v) => v, diff --git a/solana/bridge/program/src/api/verify_signature.rs b/solana/bridge/program/src/api/verify_signature.rs index 4fec6f7d..dd64576f 100644 --- a/solana/bridge/program/src/api/verify_signature.rs +++ b/solana/bridge/program/src/api/verify_signature.rs @@ -16,13 +16,11 @@ use crate::{ MAX_LEN_GUARDIAN_KEYS, }; use byteorder::ByteOrder; -use sha3::Digest; use solana_program::program_error::ProgramError; use solitaire::{ processors::seeded::Seeded, CreationLamports::Exempt, }; -use std::io::Write; #[derive(FromAccounts)] pub struct VerifySignatures<'b> { @@ -58,8 +56,6 @@ impl From<[u8; 32]> for SignatureSetDerivationData { #[derive(Default, BorshSerialize, BorshDeserialize)] pub struct VerifySignaturesData { - /// Guardian set of the signatures - pub hash: [u8; 32], /// instruction indices of signers (-1 for missing) pub signers: [i8; MAX_LEN_GUARDIAN_KEYS], } @@ -173,22 +169,18 @@ pub fn verify_signatures( return Err(ProgramError::InvalidArgument.into()); } + // Data must be a hash + if secp_ixs[0].msg_size != 32 { + return Err(ProgramError::InvalidArgument.into()); + } + // Extract message which is encoded in Solana Secp256k1 instruction data. let message = &secp_ix.data [secp_ixs[0].msg_offset as usize..(secp_ixs[0].msg_offset + secp_ixs[0].msg_size) as usize]; // Hash the message part, which contains the serialized VAA body. - let msg_hash: [u8; 32] = { - let mut h = sha3::Keccak256::default(); - if let Err(e) = h.write(message) { - return Err(e.into()); - }; - h.finalize().into() - }; - - if msg_hash != data.hash { - return Err(InvalidHash.into()); - } + let mut msg_hash: [u8; 32] = [0u8; 32]; + msg_hash.copy_from_slice(message); // Confirm at this point that the derivation succeeds, we didn't have a signature set with the // correct hash until this point. @@ -198,7 +190,7 @@ pub fn verify_signatures( if !accs.signature_set.is_initialized() { accs.signature_set.signatures = vec![[0u8; 65]; 19]; accs.signature_set.guardian_set_index = accs.guardian_set.index; - accs.signature_set.hash = data.hash; + accs.signature_set.hash = msg_hash; accs.signature_set .create(&msg_hash.into(), ctx, accs.payer.key, Exempt)?; } else { @@ -207,7 +199,7 @@ pub fn verify_signatures( return Err(GuardianSetMismatch.into()); } - if accs.signature_set.hash != data.hash { + if accs.signature_set.hash != msg_hash { return Err(InvalidHash.into()); } } diff --git a/solana/bridge/program/src/instructions.rs b/solana/bridge/program/src/instructions.rs index ed6b2077..d838c295 100644 --- a/solana/bridge/program/src/instructions.rs +++ b/solana/bridge/program/src/instructions.rs @@ -137,6 +137,7 @@ pub fn verify_signatures( program_id: Pubkey, payer: Pubkey, guardian_set_index: u32, + hash: [u8; 32], data: VerifySignaturesData, ) -> solitaire::Result { let guardian_set = GuardianSet::<'_, { AccountState::Uninitialized }>::key( @@ -147,7 +148,7 @@ pub fn verify_signatures( ); let signature_set = SignatureSet::<'_, { AccountState::Uninitialized }>::key( - &SignatureSetDerivationData { hash: data.hash }, + &SignatureSetDerivationData { hash: hash }, &program_id, ); diff --git a/solana/bridge/program/tests/common.rs b/solana/bridge/program/tests/common.rs index 0dd3bad3..180dae6f 100644 --- a/solana/bridge/program/tests/common.rs +++ b/solana/bridge/program/tests/common.rs @@ -193,7 +193,7 @@ mod helpers { nonce: u32, guardian_set_index: u32, emitter_chain: u16, - ) -> (PostVAAData, Vec, [u8; 32]) { + ) -> (PostVAAData, [u8; 32], [u8; 32]) { let mut vaa = PostVAAData { version: 0, guardian_set_index, @@ -226,12 +226,18 @@ mod helpers { // Hash this body, which is expected to be the same as the hash currently stored in the // signature account, binding that set of signatures to this VAA. - let body_hash: [u8; 32] = { + let body: [u8; 32] = { let mut h = sha3::Keccak256::default(); h.write(body.as_slice()).unwrap(); h.finalize().into() }; + let body_hash: [u8; 32] = { + let mut h = sha3::Keccak256::default(); + h.write(&body).unwrap(); + h.finalize().into() + }; + (vaa, body, body_hash) } @@ -318,8 +324,7 @@ mod helpers { client: &RpcClient, program: &Pubkey, payer: &Keypair, - body: Vec, - body_hash: [u8; 32], + body: [u8; 32], secret_keys: &[SecretKey], guardian_set_version: u32, ) -> Result<(), ClientError> { @@ -339,8 +344,8 @@ mod helpers { *program, payer.pubkey(), guardian_set_version, + body, VerifySignaturesData { - hash: body_hash, signers, }, ) diff --git a/solana/bridge/program/tests/integration.rs b/solana/bridge/program/tests/integration.rs index 82f94c78..f980df02 100644 --- a/solana/bridge/program/tests/integration.rs +++ b/solana/bridge/program/tests/integration.rs @@ -217,14 +217,13 @@ fn test_bridge_messages(context: &mut Context) { // Emulate Guardian behaviour, verifying the data and publishing signatures/VAA. let (vaa, body, body_hash) = common::generate_vaa(&emitter, message.clone(), nonce, 0, 1); - common::verify_signatures(client, program, payer, body, body_hash, &context.secret, 0) - .unwrap(); + common::verify_signatures(client, program, payer, body, &context.secret, 0).unwrap(); common::post_vaa(client, program, payer, vaa).unwrap(); common::sync(client, payer); // Derive where we expect created accounts to be. let signature_set = SignatureSet::<'_, { AccountState::Uninitialized }>::key( - &SignatureSetDerivationData { hash: body_hash }, + &SignatureSetDerivationData { hash: body}, &program, ); @@ -246,7 +245,7 @@ fn test_bridge_messages(context: &mut Context) { ); // Verify on chain Signatures - assert_eq!(signatures.hash, body_hash); + assert_eq!(signatures.hash, body); assert_eq!(signatures.guardian_set_index, 0); for (signature, secret_key) in signatures.signatures.iter().zip(context.secret.iter()) { @@ -284,13 +283,13 @@ fn test_bridge_messages(context: &mut Context) { // Emulate Guardian behaviour, verifying the data and publishing signatures/VAA. let (vaa, body, body_hash) = common::generate_vaa(&emitter, message.clone(), nonce, 0, 1); - common::verify_signatures(client, program, payer, body, body_hash, &context.secret, 0).unwrap(); + common::verify_signatures(client, program, payer, body, &context.secret, 0).unwrap(); common::post_vaa(client, program, payer, vaa).unwrap(); common::sync(client, payer); // Derive where we expect created accounts to be. let signature_set = SignatureSet::<'_, { AccountState::Uninitialized }>::key( - &SignatureSetDerivationData { hash: body_hash }, + &SignatureSetDerivationData { hash: body }, &program, ); @@ -312,7 +311,7 @@ fn test_bridge_messages(context: &mut Context) { ); // Verify on chain Signatures - assert_eq!(signatures.hash, body_hash); + assert_eq!(signatures.hash, body); assert_eq!(signatures.guardian_set_index, 0); for (signature, secret_key) in signatures.signatures.iter().zip(context.secret.iter()) { @@ -375,13 +374,13 @@ fn test_persistent_bridge_messages(context: &mut Context) { // Emulate Guardian behaviour, verifying the data and publishing signatures/VAA. let (vaa, body, body_hash) = common::generate_vaa(&emitter, message.clone(), nonce, 0, 1); - common::verify_signatures(client, program, payer, body, body_hash, &context.secret, 0).unwrap(); + common::verify_signatures(client, program, payer, body, &context.secret, 0).unwrap(); common::post_vaa(client, program, payer, vaa).unwrap(); common::sync(client, payer); // Derive where we expect created accounts to be. let signature_set = SignatureSet::<'_, { AccountState::Uninitialized }>::key( - &SignatureSetDerivationData { hash: body_hash }, + &SignatureSetDerivationData { hash: body }, &program, ); @@ -403,7 +402,7 @@ fn test_persistent_bridge_messages(context: &mut Context) { ); // Verify on chain Signatures - assert_eq!(signatures.hash, body_hash); + assert_eq!(signatures.hash, body); assert_eq!(signatures.guardian_set_index, 0); for (signature, secret_key) in signatures.signatures.iter().zip(context.secret.iter()) { @@ -541,7 +540,7 @@ fn test_guardian_set_change(context: &mut Context) { .unwrap(); let (vaa, body, body_hash) = common::generate_vaa(&emitter, message.clone(), nonce, 0, 1); - common::verify_signatures(client, program, payer, body, body_hash, &context.secret, 0).unwrap(); + common::verify_signatures(client, program, payer, body, &context.secret, 0).unwrap(); common::post_vaa(client, program, payer, vaa).unwrap(); common::upgrade_guardian_set( client, @@ -598,13 +597,13 @@ fn test_guardian_set_change(context: &mut Context) { // Emulate Guardian behaviour, verifying the data and publishing signatures/VAA. let sequence = context.seq.next(emitter.pubkey().to_bytes()); let (vaa, body, body_hash) = common::generate_vaa(&emitter, message.clone(), nonce, 1, 1); - common::verify_signatures(client, program, payer, body, body_hash, &context.secret, 1).unwrap(); + common::verify_signatures(client, program, payer, body, &context.secret, 1).unwrap(); common::post_vaa(client, program, payer, vaa).unwrap(); common::sync(client, payer); // Derive where we expect created accounts to be. let signature_set = SignatureSet::<'_, { AccountState::Uninitialized }>::key( - &SignatureSetDerivationData { hash: body_hash }, + &SignatureSetDerivationData { hash: body }, &program, ); @@ -626,7 +625,7 @@ fn test_guardian_set_change(context: &mut Context) { ); // Verify on chain Signatures - assert_eq!(signatures.hash, body_hash); + assert_eq!(signatures.hash, body); assert_eq!(signatures.guardian_set_index, 1); for (signature, secret_key) in signatures.signatures.iter().zip(context.secret.iter()) { @@ -715,7 +714,7 @@ fn test_set_fees(context: &mut Context) { .unwrap(); let (vaa, body, body_hash) = common::generate_vaa(&emitter, message.clone(), nonce, 1, 1); - common::verify_signatures(client, program, payer, body, body_hash, &context.secret, 1).unwrap(); + common::verify_signatures(client, program, payer, body, &context.secret, 1).unwrap(); common::post_vaa(client, program, payer, vaa).unwrap(); common::set_fees( client, @@ -776,7 +775,7 @@ fn test_set_fees(context: &mut Context) { .unwrap(); let (vaa, body, body_hash) = common::generate_vaa(&emitter, message.clone(), nonce, 1, 1); - common::verify_signatures(client, program, payer, body, body_hash, &context.secret, 1).unwrap(); + common::verify_signatures(client, program, payer, body, &context.secret, 1).unwrap(); common::post_vaa(client, program, payer, vaa).unwrap(); common::sync(client, payer); @@ -788,7 +787,7 @@ fn test_set_fees(context: &mut Context) { // Derive where we expect created accounts to be. let signature_set = SignatureSet::<'_, { AccountState::Uninitialized }>::key( - &SignatureSetDerivationData { hash: body_hash }, + &SignatureSetDerivationData { hash: body }, &program, ); @@ -810,7 +809,7 @@ fn test_set_fees(context: &mut Context) { ); // Verify on chain Signatures - assert_eq!(signatures.hash, body_hash); + assert_eq!(signatures.hash, body); assert_eq!(signatures.guardian_set_index, 1); for (signature, secret_key) in signatures.signatures.iter().zip(context.secret.iter()) { @@ -857,7 +856,7 @@ fn test_set_fees_fails(context: &mut Context) { .unwrap(); let (vaa, body, body_hash) = common::generate_vaa(&emitter, message.clone(), nonce, 1, 1); - common::verify_signatures(client, program, payer, body, body_hash, &context.secret, 1).unwrap(); + common::verify_signatures(client, program, payer, body, &context.secret, 1).unwrap(); common::post_vaa(client, program, payer, vaa).unwrap(); assert!(common::set_fees( client, @@ -899,7 +898,7 @@ fn test_free_fees(context: &mut Context) { .unwrap(); let (vaa, body, body_hash) = common::generate_vaa(&emitter, message.clone(), nonce, 1, 1); - common::verify_signatures(client, program, payer, body, body_hash, &context.secret, 1).unwrap(); + common::verify_signatures(client, program, payer, body, &context.secret, 1).unwrap(); common::post_vaa(client, program, payer, vaa).unwrap(); common::set_fees( client, @@ -938,7 +937,7 @@ fn test_free_fees(context: &mut Context) { .unwrap(); let (vaa, body, body_hash) = common::generate_vaa(&emitter, message.clone(), nonce, 1, 1); - common::verify_signatures(client, program, payer, body, body_hash, &context.secret, 1).unwrap(); + common::verify_signatures(client, program, payer, body, &context.secret, 1).unwrap(); common::post_vaa(client, program, payer, vaa).unwrap(); common::sync(client, payer); @@ -950,7 +949,7 @@ fn test_free_fees(context: &mut Context) { // Derive where we expect created accounts to be. let signature_set = SignatureSet::<'_, { AccountState::Uninitialized }>::key( - &SignatureSetDerivationData { hash: body_hash }, + &SignatureSetDerivationData { hash: body }, &program, ); @@ -972,7 +971,7 @@ fn test_free_fees(context: &mut Context) { ); // Verify on chain Signatures - assert_eq!(signatures.hash, body_hash); + assert_eq!(signatures.hash, body); assert_eq!(signatures.guardian_set_index, 1); for (signature, secret_key) in signatures.signatures.iter().zip(context.secret.iter()) { @@ -1022,7 +1021,7 @@ fn test_transfer_fees(context: &mut Context) { .unwrap(); let (vaa, body, body_hash) = common::generate_vaa(&emitter, message.clone(), nonce, 1, 1); - common::verify_signatures(client, program, payer, body, body_hash, &context.secret, 1).unwrap(); + common::verify_signatures(client, program, payer, body, &context.secret, 1).unwrap(); common::post_vaa(client, program, payer, vaa).unwrap(); common::transfer_fees( client, @@ -1071,7 +1070,7 @@ fn test_transfer_fees_fails(context: &mut Context) { .unwrap(); let (vaa, body, body_hash) = common::generate_vaa(&emitter, message.clone(), nonce, 1, 1); - common::verify_signatures(client, program, payer, body, body_hash, &context.secret, 1).unwrap(); + common::verify_signatures(client, program, payer, body, &context.secret, 1).unwrap(); common::post_vaa(client, program, payer, vaa).unwrap(); assert!(common::transfer_fees( @@ -1118,7 +1117,7 @@ fn test_transfer_too_much(context: &mut Context) { .unwrap(); let (vaa, body, body_hash) = common::generate_vaa(&emitter, message.clone(), nonce, 1, 1); - common::verify_signatures(client, program, payer, body, body_hash, &context.secret, 1).unwrap(); + common::verify_signatures(client, program, payer, body, &context.secret, 1).unwrap(); common::post_vaa(client, program, payer, vaa).unwrap(); // Should fail to transfer. @@ -1157,12 +1156,12 @@ fn test_foreign_bridge_messages(context: &mut Context) { &program, ); - common::verify_signatures(client, program, payer, body, body_hash, &context.secret, 0).unwrap(); + common::verify_signatures(client, program, payer, body, &context.secret, 0).unwrap(); common::post_vaa(client, program, payer, vaa).unwrap(); common::sync(client, payer); let signature_set = SignatureSet::<'_, { AccountState::Uninitialized }>::key( - &SignatureSetDerivationData { hash: body_hash }, + &SignatureSetDerivationData { hash: body }, &program, ); @@ -1183,7 +1182,7 @@ fn test_foreign_bridge_messages(context: &mut Context) { ); // Verify on chain Signatures - assert_eq!(signatures.hash, body_hash); + assert_eq!(signatures.hash, body); assert_eq!(signatures.guardian_set_index, 0); for (signature, secret_key) in signatures.signatures.iter().zip(context.secret.iter()) { @@ -1236,7 +1235,7 @@ fn test_transfer_total_fails(context: &mut Context) { .unwrap(); let (vaa, body, body_hash) = common::generate_vaa(&emitter, message.clone(), nonce, 1, 1); - common::verify_signatures(client, program, payer, body, body_hash, &context.secret, 1).unwrap(); + common::verify_signatures(client, program, payer, body, &context.secret, 1).unwrap(); common::post_vaa(client, program, payer, vaa).unwrap(); // Transferring total fees should fail, to prevent the account being de-allocated. @@ -1289,7 +1288,7 @@ fn test_upgrade_contract(context: &mut Context) { .unwrap(); let (vaa, body, body_hash) = common::generate_vaa(&emitter, message.clone(), nonce, 0, 1); - common::verify_signatures(client, program, payer, body, body_hash, &context.secret, 0).unwrap(); + common::verify_signatures(client, program, payer, body, &context.secret, 0).unwrap(); common::post_vaa(client, program, payer, vaa).unwrap(); common::upgrade_contract( client,