From 4a39e90b0a9cb32b450c46fe91cec3331eec6300 Mon Sep 17 00:00:00 2001 From: Jordy Romuald <87231934+JordyRo1@users.noreply.github.com> Date: Thu, 18 Jul 2024 15:45:18 +0200 Subject: [PATCH] Audit: Static variable size for hash parameters (#56) * fix: static variables size * fix: remove size from hash input * fix: changing hash configuration * fix: size definition for messageBody * fix: remove unecessary prints * fix: padding correction for message body * test: example test * fix: validators hashes --- .../isms/multisig/validator_announce.cairo | 10 +- .../src/contracts/libs/checkpoint_lib.cairo | 17 +- contracts/src/contracts/libs/message.cairo | 145 ++++++++++++++---- contracts/src/tests/setup.cairo | 24 +-- .../src/tests/test_validator_announce.cairo | 4 +- contracts/src/utils/keccak256.cairo | 32 ++++ 6 files changed, 168 insertions(+), 64 deletions(-) diff --git a/contracts/src/contracts/isms/multisig/validator_announce.cairo b/contracts/src/contracts/isms/multisig/validator_announce.cairo index a26d066..21ee809 100644 --- a/contracts/src/contracts/isms/multisig/validator_announce.cairo +++ b/contracts/src/contracts/isms/multisig/validator_announce.cairo @@ -236,14 +236,8 @@ pub mod validator_announce { fn domain_hash(self: @ContractState) -> u256 { let mailbox_address: felt252 = self.mailboxclient.mailbox().try_into().unwrap(); let mut input: Array = array![ - ByteData { - value: self.mailboxclient.get_local_domain().into(), - size: u64_word_size(self.mailboxclient.get_local_domain().into()).into() - }, - ByteData { - value: mailbox_address.try_into().unwrap(), - size: u256_word_size(mailbox_address.try_into().unwrap()).into() - }, + ByteData { value: self.mailboxclient.get_local_domain().into(), size: 4 }, + ByteData { value: mailbox_address.try_into().unwrap(), size: 32 }, ByteData { value: HYPERLANE_ANNOUNCEMENT.into(), size: 22 } ]; reverse_endianness(compute_keccak(input.span())) diff --git a/contracts/src/contracts/libs/checkpoint_lib.cairo b/contracts/src/contracts/libs/checkpoint_lib.cairo index 588c357..7632eba 100644 --- a/contracts/src/contracts/libs/checkpoint_lib.cairo +++ b/contracts/src/contracts/libs/checkpoint_lib.cairo @@ -44,14 +44,8 @@ pub mod checkpoint_lib { let domain_hash = CheckpointLib::domain_hash(_origin, _origin_merkle_tree_hook); let mut input: Array = array![ ByteData { value: domain_hash.into(), size: HASH_SIZE }, - ByteData { - value: _checkpoint_root.into(), - size: u256_word_size(_checkpoint_root.into()).into() - }, - ByteData { - value: _checkpoint_index.into(), - size: u64_word_size(_checkpoint_index.into()).into() - }, + ByteData { value: _checkpoint_root.into(), size: 32 }, + ByteData { value: _checkpoint_index.into(), size: 4 }, ByteData { value: _message_id.into(), size: HASH_SIZE }, ]; to_eth_signature(reverse_endianness(compute_keccak(input.span()))) @@ -69,11 +63,8 @@ pub mod checkpoint_lib { /// u256 - The domain hash. fn domain_hash(_origin: u32, _origin_merkle_tree_hook: u256) -> u256 { let mut input: Array = array![ - ByteData { value: _origin.into(), size: u64_word_size(_origin.into()).into() }, - ByteData { - value: _origin_merkle_tree_hook.into(), - size: u256_word_size(_origin_merkle_tree_hook).into() - }, + ByteData { value: _origin.into(), size: 4 }, + ByteData { value: _origin_merkle_tree_hook.into(), size: 32 }, ByteData { value: HYPERLANE.into(), size: 9 } ]; reverse_endianness(compute_keccak(input.span())) diff --git a/contracts/src/contracts/libs/message.cairo b/contracts/src/contracts/libs/message.cairo index 9dc129f..3f49917 100644 --- a/contracts/src/contracts/libs/message.cairo +++ b/contracts/src/contracts/libs/message.cairo @@ -1,6 +1,8 @@ use alexandria_bytes::{Bytes, BytesTrait, BytesStore}; +use alexandria_math::BitShift; use hyperlane_starknet::utils::keccak256::{ - reverse_endianness, compute_keccak, ByteData, u256_word_size, u64_word_size, ADDRESS_SIZE + reverse_endianness, compute_keccak, ByteData, u256_word_size, u64_word_size, ADDRESS_SIZE, + u128_mask, }; use starknet::{ContractAddress, contract_address_const}; @@ -52,41 +54,126 @@ pub impl MessageImpl of MessageTrait { let recipient: felt252 = _message.recipient.into(); let mut input: Array = array![ - ByteData { - value: _message.version.into(), size: u64_word_size(_message.version.into()).into() - }, - ByteData { - value: _message.nonce.into(), size: u64_word_size(_message.nonce.into()).into() - }, - ByteData { - value: _message.origin.into(), size: u64_word_size(_message.origin.into()).into() - }, - ByteData { value: sender.into(), size: ADDRESS_SIZE }, - ByteData { - value: _message.destination.into(), - size: u64_word_size(_message.destination.into()).into() - }, - ByteData { value: recipient.into(), size: ADDRESS_SIZE }, - ByteData { - value: _message.body.size().into(), - size: u64_word_size(_message.body.size().into()).into() - }, + ByteData { value: _message.version.into(), size: 1 }, + ByteData { value: _message.nonce.into(), size: 4 }, + ByteData { value: _message.origin.into(), size: 4 }, + ByteData { value: sender.into(), size: 32 }, + ByteData { value: _message.destination.into(), size: 4 }, + ByteData { value: recipient.into(), size: 32 }, ]; + let message_data = _message.clone().body.data(); + let finalized_input = MessageImpl::append_span_u128_to_byte_data( + input, message_data.span(), _message.clone().body.size() + ); + (reverse_endianness(compute_keccak(finalized_input)), _message) + } - let mut message_data = _message.clone().body.data(); + fn append_span_u128_to_byte_data( + mut _input: Array, _to_append: Span, size: u32 + ) -> Span { + let mut cur_idx = 0; + let range = size / 16; loop { - match message_data.pop_front() { - Option::Some(data) => { - input + if (cur_idx == range) { + if (size % 16 == 0) { + break; + } else { + let remaining_size = size - cur_idx * 16; + let mask = u128_mask(remaining_size.try_into().unwrap()); + _input .append( ByteData { - value: data.into(), size: u256_word_size(data.into()).into() + value: (BitShift::shr( + *_to_append.at(cur_idx), ((16 - remaining_size) * 8).into() + ) + & mask) + .into(), + size: remaining_size } ); - }, - Option::None(_) => { break (); } - }; + break; + } + } + _input.append(ByteData { value: (*_to_append.at(cur_idx)).into(), size: 16 }); + cur_idx += 1; }; - (reverse_endianness(compute_keccak(input.span())), _message) + _input.span() + } +} + + +#[cfg(test)] +mod tests { + use super::{MessageImpl, ByteData}; + + #[test] + fn test_append_u128_to_byte_array() { + let input: Array = array![ + 0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, + 0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb, + 0xcc000000000000000000000000000000 + ]; + let output_array = MessageImpl::append_span_u128_to_byte_data(array![], input.span(), 33); + assert_eq!( + output_array, + array![ + ByteData { value: 0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, size: 16 }, + ByteData { value: 0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb, size: 16 }, + ByteData { value: 0xcc, size: 1 } + ] + .span() + ); + + let input: Array = array![ + 0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, + 0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb, + 0xcccccccccccccccccccccccccccccccc + ]; + let output_array = MessageImpl::append_span_u128_to_byte_data(array![], input.span(), 48); + assert_eq!( + output_array, + array![ + ByteData { value: 0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, size: 16 }, + ByteData { value: 0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb, size: 16 }, + ByteData { value: 0xcccccccccccccccccccccccccccccccc, size: 16 } + ] + .span() + ); + + let input: Array = array![ + 0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, + 0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb, + 0xcccccccccccccccccccccccccccccc00 + ]; + let output_array = MessageImpl::append_span_u128_to_byte_data(array![], input.span(), 47); + assert_eq!( + output_array, + array![ + ByteData { value: 0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, size: 16 }, + ByteData { value: 0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb, size: 16 }, + ByteData { value: 0xcccccccccccccccccccccccccccccc, size: 15 } + ] + .span() + ); + + let input: Array = array![ + 0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, + 0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb, + 0xcccccccccccccccccccccccccc000000 + ]; + let output_array = MessageImpl::append_span_u128_to_byte_data(array![], input.span(), 45); + assert_eq!( + output_array, + array![ + ByteData { value: 0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, size: 16 }, + ByteData { value: 0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb, size: 16 }, + ByteData { value: 0xcccccccccccccccccccccccccc, size: 13 } + ] + .span() + ); + + let input: Array = array![0x12345678000000000000000000000000]; + let output_array = MessageImpl::append_span_u128_to_byte_data(array![], input.span(), 4); + assert_eq!(output_array, array![ByteData { value: 0x12345678, size: 4 }].span()); } } diff --git a/contracts/src/tests/setup.cairo b/contracts/src/tests/setup.cairo index 6071444..54bd6ad 100644 --- a/contracts/src/tests/setup.cairo +++ b/contracts/src/tests/setup.cairo @@ -406,13 +406,13 @@ pub fn build_fake_messageid_metadata( // Configuration from the main cairo repo: https://github.com/starkware-libs/cairo/blob/main/corelib/src/test/secp256k1_test.cairo pub fn get_message_and_signature() -> (u256, Array, Array) { - let msg_hash = 0x91745536C898FFA3831357EDCE68E1584287129C8E3AE9A065AC5B31AA355E8C; + let msg_hash = 0xD2B30308834E6E76F50891F0B45E742BE4F4A163BAF697B465DD14C968777AD0; let validators_array: Array = array![ - 0xcc802e4ba45ee188d1955bba8d3ac4d64efe561e.try_into().unwrap(), - 0x3f20deb9b7de97fe4d1c07cdd189eafece816489.try_into().unwrap(), - 0x58adaf0255f4626f75cdeb5f75f0c5a309847132.try_into().unwrap(), - 0x4e30030da04bf8f52a4ff5cf0fc2ef7bd7d4a840.try_into().unwrap(), - 0x3c521ab0c2ade5410483d95e40a7a1f956b87fa9.try_into().unwrap(), + 0xfa3ed5df8369fb40e75978937607b6f0c0e04fb8.try_into().unwrap(), + 0x2f7008b31a614685a1d24a684827cf05f15dc17a.try_into().unwrap(), + 0x108e9c0c2a24c02d70b224ee9dd97136cdc1e072.try_into().unwrap(), + 0x7851fee3ec28606dfb18041396876524e8fa6256.try_into().unwrap(), + 0xaef2f8fa001982939416fd49e6e533e9fba65a1b.try_into().unwrap(), ]; let signatures = array![ EthSignature { @@ -509,13 +509,13 @@ pub fn build_fake_merkle_metadata( // Configuration from the main cairo repo: https://github.com/starkware-libs/cairo/blob/main/corelib/src/test/secp256k1_test.cairo pub fn get_merkle_message_and_signature() -> (u256, Array, Array) { - let msg_hash = 0x59771D2D2620ABA94DF0A5E7EE7B1FE0D437D7710D43847097A471A4101AE2D4; + let msg_hash = 0xC9E505E3C9DDD36638A10238CAD43278311CDF09894EF7EFBE6AFBF510507907; let validators_array: Array = array![ - 0xf4a1ea6c9380f182730df38c8f2347ac95a15839.try_into().unwrap(), - 0xe8d8f27c89192757cd4be34d5cf5eda849b85929.try_into().unwrap(), - 0x18836f7e081a0a971178413c428230dc3b5d62c5.try_into().unwrap(), - 0x77a0172ee7de58a66da42fee91c0cb92775cf4e1.try_into().unwrap(), - 0x7058f79d7b18f6ee3808139a0b9ccf9fd8883574.try_into().unwrap(), + 0x2614ea33eaf750585f8ae3b59a45b2c800b952ed.try_into().unwrap(), + 0xc69bb3f1bedb35543f1c28c0ff4f6428622068d3.try_into().unwrap(), + 0x7f2c0c840dd8855e0625f1e1ede157d4e09bc9c4.try_into().unwrap(), + 0x6d937801434bbf68eb109d3ab2b103a64afcbac2.try_into().unwrap(), + 0xfe5fb670978ee5c686db8a7180509b5682f9797d.try_into().unwrap(), ]; let signatures = array![ EthSignature { diff --git a/contracts/src/tests/test_validator_announce.cairo b/contracts/src/tests/test_validator_announce.cairo index 9b54c09..d4da4ab 100644 --- a/contracts/src/tests/test_validator_announce.cairo +++ b/contracts/src/tests/test_validator_announce.cairo @@ -15,7 +15,7 @@ pub const TEST_STARKNET_DOMAIN: u32 = 23448593; #[test] fn test_announce() { let (validator_announce, mut spy) = setup_validator_announce(); - let validator_address: EthAddress = 0xe6076407ca06f2b0a0ec716db2b5361beccdcfa8 + let validator_address: EthAddress = 0xf85362bdff5a3561481819b8c9010770384aaecf .try_into() .unwrap(); let mut _storage_location: Array = array![ @@ -98,7 +98,7 @@ fn test_announce_fails_if_wrong_signer() { #[should_panic(expected: ('Announce already occured',))] fn test_announce_fails_if_replay() { let (validator_announce, _) = setup_validator_announce(); - let validator_address: EthAddress = 0xe6076407ca06f2b0a0ec716db2b5361beccdcfa8 + let validator_address: EthAddress = 0xf85362bdff5a3561481819b8c9010770384aaecf .try_into() .unwrap(); let mut storage_location: Array = array![ diff --git a/contracts/src/utils/keccak256.cairo b/contracts/src/utils/keccak256.cairo index 67f8356..fc007f3 100644 --- a/contracts/src/utils/keccak256.cairo +++ b/contracts/src/utils/keccak256.cairo @@ -240,6 +240,38 @@ pub fn one_shift_left_bytes_u256(n_bytes: u8) -> u256 { } } +/// Shifts equivalent u128 mask for a given number of bytes +/// dev : panics if u128 overflow +/// +/// # Arguments +/// +/// * `n_bytes` - The number of bytes shift +/// +/// # Returns +/// +/// u256 representing the associated mask +pub fn u128_mask(n_bytes: u8) -> u128 { + match n_bytes { + 0 => 0, + 1 => 0xFF, + 2 => 0xFFFF, + 3 => 0xFFFFFF, + 4 => 0xFFFFFFFF, + 5 => 0xFFFFFFFFFF, + 6 => 0xFFFFFFFFFFFF, + 7 => 0xFFFFFFFFFFFFFF, + 8 => 0xFFFFFFFFFFFFFFFF, + 9 => 0xFFFFFFFFFFFFFFFFFF, + 10 => 0xFFFFFFFFFFFFFFFFFFFF, + 11 => 0xFFFFFFFFFFFFFFFFFFFFFF, + 12 => 0xFFFFFFFFFFFFFFFFFFFFFFFF, + 13 => 0xFFFFFFFFFFFFFFFFFFFFFFFFFF, + 14 => 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFF, + 15 => 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, + 16 => 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, + _ => core::panic_with_felt252('n_bytes too big'), + } +} /// Givens a span of ByteData, returns a concatenated string (ByteArray) of the input ///