diff --git a/crates/codec/src/primitive.rs b/crates/codec/src/primitive.rs index 07b6b33b9..70f48029c 100644 --- a/crates/codec/src/primitive.rs +++ b/crates/codec/src/primitive.rs @@ -124,9 +124,10 @@ macro_rules! impl_int { B::$write_method(&mut buf[start..end], *self); - // Fill the rest of the buffer with 0x00 or 0xFF depending on the sign of the - // integer - let fill_val = if *self > 0 { 0x00 } else { 0xFF }; + // Sign-extend negative values; everything else, zero included, pads with zeros. + // `< 0` is always false for the unsigned instantiations of this macro. + #[allow(unused_comparisons)] + let fill_val = if *self < 0 { 0xFF } else { 0x00 }; for i in offset..start { buf[i] = fill_val; @@ -783,4 +784,88 @@ mod tests { assert_multi_value_u64_vector!(LittleEndian); assert_multi_value_u64_vector!(BigEndian); } + + /// Encodes `value` at both alignments and byte orders and checks every padding byte. + /// + /// Padding is sign extension, so it is `0xFF` only for negative values. Zero used to take + /// the negative branch and pad with `0xFF`, producing a word no other ABI implementation + /// accepts - and the round-trip through this codec hid it, because decoding ignores the + /// padding entirely. + macro_rules! assert_padding { + ($typ:ty, $value:expr, $expected_fill:expr) => {{ + fn check(value: &T, expected_fill: u8) + where + T: Encoder + core::fmt::Debug, + { + let mut buf = BytesMut::new(); + value.encode(&mut buf, 0).unwrap(); + + let value_width = size_of::(); + let padding = if is_big_endian::() { + 0..buf.len() - value_width + } else { + value_width..buf.len() + }; + + assert!( + buf[padding.clone()].iter().all(|byte| *byte == expected_fill), + "{value:?}: padding {padding:?} should be all 0x{expected_fill:02x}, got {}", + hex_words(&buf) + ); + } + + let value: $typ = $value; + check::<$typ, BigEndian, 32>(&value, $expected_fill); + check::<$typ, LittleEndian, 4>(&value, $expected_fill); + }}; + } + + fn hex_words(bytes: &[u8]) -> alloc::string::String { + use alloc::string::String; + bytes.iter().fold(String::new(), |mut acc, byte| { + acc.push_str(&alloc::format!("{byte:02x}")); + acc + }) + } + + /// Zero is not negative, so its padding must be zeros in every width and byte order. + #[test] + fn test_zero_pads_with_zeros_not_sign_extension() { + assert_padding!(u16, 0, 0x00); + assert_padding!(u32, 0, 0x00); + assert_padding!(u64, 0, 0x00); + assert_padding!(i16, 0, 0x00); + assert_padding!(i32, 0, 0x00); + assert_padding!(i64, 0, 0x00); + } + + /// The neighbours of zero keep the behaviour the fix must not change. + #[test] + fn test_padding_follows_sign_around_zero() { + assert_padding!(u16, 1, 0x00); + assert_padding!(u32, 1, 0x00); + assert_padding!(u64, u64::MAX, 0x00); + assert_padding!(i16, 1, 0x00); + assert_padding!(i16, -1, 0xFF); + assert_padding!(i32, -1, 0xFF); + assert_padding!(i32, i32::MIN, 0xFF); + assert_padding!(i64, -1, 0xFF); + assert_padding!(i64, i64::MAX, 0x00); + } + + /// The whole point of the padding: a zero must survive a round-trip through the codec and + /// still be the canonical zero word on the wire. + #[test] + fn test_zero_encodes_to_the_canonical_word() { + let mut buf = BytesMut::new(); + >::encode(&0, &mut buf, 0).unwrap(); + assert_eq!(buf.len(), 32); + assert_eq!(hex_words(&buf), "0".repeat(64)); + + let encoded = buf.freeze(); + assert_eq!( + >::decode(&encoded, 0).unwrap(), + 0 + ); + } } diff --git a/crates/codec/tests/topic.rs b/crates/codec/tests/topic.rs index 7ca7dec6e..d0581a012 100644 --- a/crates/codec/tests/topic.rs +++ b/crates/codec/tests/topic.rs @@ -67,6 +67,61 @@ fn value_types_occupy_the_topic_word_directly() { ); } +/// Zero is the value the padding rule is easiest to get wrong on: it is not negative, so it must +/// be zero-padded, but a sign test written as `> 0` sends it down the sign-extension branch and +/// produces `0xff..ff0000`. Round-tripping through this codec cannot catch that - the decoder +/// ignores the padding - so every width is checked against `alloy-sol-types` instead. +#[test] +fn zero_is_zero_padded_in_every_integer_width() { + assert_eq!(topic(&0u8), expected::>(&0u8)); + assert_eq!(topic(&0u16), expected::>(&0u16)); + assert_eq!(topic(&0u32), expected::>(&0u32)); + assert_eq!(topic(&0u64), expected::>(&0u64)); + assert_eq!(topic(&0i16), expected::>(&0i16)); + assert_eq!(topic(&0i32), expected::>(&0i32)); + assert_eq!(topic(&0i64), expected::>(&0i64)); + assert_eq!(topic(&U256::ZERO), expected::>(&U256::ZERO)); + assert_eq!(topic(&I256::ZERO), expected::>(&I256::ZERO)); + assert_eq!(topic(&false), expected::(&false)); + + assert_eq!(preimage(&0u64), [0u8; 32]); +} + +/// The sign rule itself, on both sides of zero, so a fix for the zero case cannot quietly drop +/// sign extension for negatives. +#[test] +fn padding_follows_the_sign_of_the_value() { + assert_eq!(topic(&(-1i32)), expected::>(&-1i32)); + assert_eq!(preimage(&(-1i32)), [0xffu8; 32]); + + assert_eq!(topic(&i64::MIN), expected::>(&i64::MIN)); + assert_eq!(topic(&i64::MAX), expected::>(&i64::MAX)); + assert_eq!(topic(&u64::MAX), expected::>(&u64::MAX)); +} + +/// Zero inside a container: the members are concatenated in place, so a wrongly padded member +/// changes the hash rather than one visible word. +#[test] +fn zero_members_keep_the_container_topic_correct() { + let values = vec![0u32, 1, u32::MAX]; + assert_eq!( + topic(&values), + expected::>>(&values) + ); + + let fixed = [0u64, 7]; + assert_eq!( + topic(&fixed), + expected::, 2>>(&fixed) + ); + + let mixed = (0u32, "x".to_string()); + assert_eq!( + topic(&mixed), + expected::<(sol_data::Uint<32>, sol_data::String)>(&(0u32, "x".to_string())) + ); +} + #[test] fn indexed_string_hashes_its_raw_contents() { let value = "hello".to_string();