search

alloy-rs / alloy

Transports, Middleware, and Networks for the Alloy project
https://alloy.rs
Apache License 2.0
449 stars 148 forks source link

[Bug] TxEnvelope::Legacy::decode incorrect chain_id #897

Closed kpp closed 19 hours ago

kpp commented 3 weeks ago

Component

consensus, eips, genesis

What version of Alloy are you on?

commit 4157e2664fab0

Operating System

None

Describe the bug

Fails to encode/decode a Legacy TxEnvelope:

---- transaction::envelope::tests::test_encode_decode_legacy_parity_eip155 stdout ----
thread 'transaction::envelope::tests::test_encode_decode_legacy_parity_eip155' panicked at crates/consensus/src/transaction/envelope.rs:478:9:
assertion `left == right` failed
  left: Legacy(Signed { tx: TxLegacy { chain_id: Some(3), nonce: 100, gas_price: 3000000000, gas_limit: 50000, to: Call(0x0000000000000000000000000000000000000000), value: 10000000000000000000, input: 0x }, signature: Signature { inner: ecdsa::Signature<Secp256k1>(840CFC572845F5786E702984C2A582528CAD4B49B2A10B9DB1BE7FCA9005856525E7109CEB98168D95B09B18BBF6B685130E0562F233877D492B94EEE0C5B6D1), v: Eip155(42), r: 59728239767604526217550949535444980007647751110991992555989432467883920033125, s: 17143831728048845831134990513685258884275296176139135115431336905099564136145 }, hash: 0x7723239ba787d6d5c582b1cdfb2a931573ae84bacf31e586dea0ded664652f9f })
 right: Legacy(Signed { tx: TxLegacy { chain_id: Some(1), nonce: 100, gas_price: 3000000000, gas_limit: 50000, to: Call(0x0000000000000000000000000000000000000000), value: 10000000000000000000, input: 0x }, signature: Signature { inner: ecdsa::Signature<Secp256k1>(840CFC572845F5786E702984C2A582528CAD4B49B2A10B9DB1BE7FCA9005856525E7109CEB98168D95B09B18BBF6B685130E0562F233877D492B94EEE0C5B6D1), v: Eip155(42), r: 59728239767604526217550949535444980007647751110991992555989432467883920033125, s: 17143831728048845831134990513685258884275296176139135115431336905099564136145 }, hash: 0x7723239ba787d6d5c582b1cdfb2a931573ae84bacf31e586dea0ded664652f9f })

Test:

diff --git a/crates/consensus/src/transaction/envelope.rs b/crates/consensus/src/transaction/envelope.rs
index 93164122..7bc4329d 100644
--- a/crates/consensus/src/transaction/envelope.rs
+++ b/crates/consensus/src/transaction/envelope.rs
@@ -474,7 +474,7 @@ mod tests {
         let tx_envelope: TxEnvelope = tx_signed.into();
         let encoded = tx_envelope.encoded_2718();
         let decoded = TxEnvelope::decode_2718(&mut encoded.as_ref()).unwrap();
-        assert_eq!(encoded.len(), tx_envelope.encode_2718_len());
+        // assert_eq!(encoded.len(), tx_envelope.encode_2718_len());
         assert_eq!(decoded, tx_envelope);
     }

@@ -494,6 +494,21 @@ mod tests {
         test_encode_decode_roundtrip(tx, None);
     }

+    #[test]
+    fn test_encode_decode_legacy_parity_eip155() {
+        let tx = TxLegacy {
+            chain_id: Some(1),
+            nonce: 100,
+            gas_price: 3_000_000_000,
+            gas_limit: 50_000,
+            to: Address::default().into(),
+            value: U256::from(10e18),
+            input: Bytes::new(),
+        };
+        let signature = Signature::test_signature().with_parity(Parity::Eip155(42));
+        test_encode_decode_roundtrip(tx, Some(signature));
+    }
+
     #[test]
     fn test_encode_decode_eip1559_parity_eip155() {
         let tx = TxEip1559 {

Linked issue: https://github.com/alloy-rs/alloy/pull/893 Linked issue: https://github.com/alloy-rs/alloy/issues/539

prestwich commented 3 weeks ago

This is a bit of a GIGO situation, no? Passing a signature you know to be invalid results in invalid behavior. when the chain id of the signature and the legacy tx disagree, only the signature

we can protect against this better by storing parity only as a boolean instead of trying to capture eip155 status. but for now, I think the best solution is a debug_assert_eq until we prep that larger refactor

    fn into_signed(self, signature: Signature) -> Signed<Self> {
        debug_assert_eq!(signature.v().chain_id(), self.chain_id);