fix(codec): pad zero integers with zeros not sign extension

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<T, B: ByteOrder, const ALIGN: usize>(value: &T, expected_fill: u8)
+            where
+                T: Encoder<B, ALIGN, true, true> + core::fmt::Debug,
+            {
+                let mut buf = BytesMut::new();
+                value.encode(&mut buf, 0).unwrap();
+
+                let value_width = size_of::<T>();
+                let padding = if is_big_endian::<B>() {
+                    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();
+        <u64 as Encoder<BigEndian, 32, true, true>>::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!(
+            <u64 as Encoder<BigEndian, 32, true, true>>::decode(&encoded, 0).unwrap(),
+            0
+        );
+    }
 }

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::<sol_data::Uint<8>>(&0u8));
+    assert_eq!(topic(&0u16), expected::<sol_data::Uint<16>>(&0u16));
+    assert_eq!(topic(&0u32), expected::<sol_data::Uint<32>>(&0u32));
+    assert_eq!(topic(&0u64), expected::<sol_data::Uint<64>>(&0u64));
+    assert_eq!(topic(&0i16), expected::<sol_data::Int<16>>(&0i16));
+    assert_eq!(topic(&0i32), expected::<sol_data::Int<32>>(&0i32));
+    assert_eq!(topic(&0i64), expected::<sol_data::Int<64>>(&0i64));
+    assert_eq!(topic(&U256::ZERO), expected::<sol_data::Uint<256>>(&U256::ZERO));
+    assert_eq!(topic(&I256::ZERO), expected::<sol_data::Int<256>>(&I256::ZERO));
+    assert_eq!(topic(&false), expected::<sol_data::Bool>(&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::<sol_data::Int<32>>(&-1i32));
+    assert_eq!(preimage(&(-1i32)), [0xffu8; 32]);
+
+    assert_eq!(topic(&i64::MIN), expected::<sol_data::Int<64>>(&i64::MIN));
+    assert_eq!(topic(&i64::MAX), expected::<sol_data::Int<64>>(&i64::MAX));
+    assert_eq!(topic(&u64::MAX), expected::<sol_data::Uint<64>>(&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::<sol_data::Array<sol_data::Uint<32>>>(&values)
+    );
+
+    let fixed = [0u64, 7];
+    assert_eq!(
+        topic(&fixed),
+        expected::<sol_data::FixedArray<sol_data::Uint<64>, 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();