Double hashing in Solana

Change-Id: I86769ae07cb28b38e00025b2f0bcf01919262c98

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<Transaction> = 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::<LittleEndian>((data_offset + 85 * i + 65) as u16)?;
             secp_payload.write_u8(0)?;
             secp_payload.write_u16::<LittleEndian>(message_offset as u16)?;
-            secp_payload.write_u16::<LittleEndian>(vaa_body.len() as u16)?;
+            secp_payload.write_u16::<LittleEndian>(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,

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 msg_hash: [u8; 32] = [0u8; 32];
-        let mut h = sha3::Keccak256::default();
+    msg_hash.copy_from_slice(message);
-        if let Err(e) = h.write(message) {
-            return Err(e.into());
-        };
-        h.finalize().into()
-    };
-
-    if msg_hash != data.hash {
-        return Err(InvalidHash.into());
-    }
 
     // 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());
         }
     }

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<Instruction> {
     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,
     );
 

solana/bridge/program/tests/common.rs

@@ -193,7 +193,7 @@ mod helpers {
         nonce: u32,
         guardian_set_index: u32,
         emitter_chain: u16,
-    ) -> (PostVAAData, Vec<u8>, [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<u8>,
+        body: [u8; 32],
-        body_hash: [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,
                         },
                     )

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)
+        common::verify_signatures(client, program, payer, body, &context.secret, 0).unwrap();
-            .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,