fix(TypedTransaction::decode): do not panic on bad rlp input #2694
Conversation
emostov
force-pushed
the
avoid-panic-on-transaction-deserialization
branch
from
0095e6f
to
1975cfd
Compare
emostov
force-pushed
the
avoid-panic-on-transaction-deserialization
branch
from
1975cfd
to
7b60416
Compare
emostov
changed the title
| @@ -417,7 +417,7 @@ impl TypedTransaction { | |||
| impl Decodable for TypedTransaction { | |||
| fn decode(rlp: &rlp::Rlp) -> Result<Self, rlp::DecoderError> { | |||
| let tx_type: Option<U64> = match rlp.is_data() { | |||
| true => Some(rlp.data()?.into()), | |||
| true => Some(U64::decode(rlp)?), | |||
There was a problem hiding this comment.
hmm, unclear what this even did before.
this entire function looks a bit weird.
this should not rlp decode here I believe, because this is the type id of the envelope?
unclear what rlp.data()?.into() even did, but I think this here should just use the single byte.
idk what is_data is
There was a problem hiding this comment.
rlp.data() returns Result<&[u8]>, and apparently Parity Uint types implement From<&[u8]> with a function that can panic on bad input ^^
There was a problem hiding this comment.
I refactored a bit to explicitly check the first byte as a the tx type - see e7f8369.
It is a bit strange that the initial code was trying to coerce into a U64, when EIP-2718 specifies a single u8 as the tx type
There was a problem hiding this comment.
I also noticed decode_signed has similar logic and saw that you made a similar refactor to it in pr 1733. I am not familiar with the code base, but wondering if there are other places that may make the same mistake
|
|
||
| #[test] | ||
| fn malformed_transaction_does_not_panic() { | ||
| let tx = hex::decode("b9011e02f9011a0180850ba43b7400850df8475864830493e0947a250d5630b4cf539739df2c5dacb4c659f2488d88016345785d8a0000b8e4b6f9de95000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000800000000000000000000000005bd929b84c3dac16515c6b6c95f70fdae9c05c0c00000000000000000000000000000000000000000000000000000000650267140000000000000000000000000000000000000000000000000000000000000002000000000000000000000000c02aaa39b223fe8d0a0e5c4f27ead9083c756cc2000000000000000000000000dac17f958d2ee523a2206206994597c13d831ec7c0808080").unwrap(); |
There was a problem hiding this comment.
This was given to me - apparently it was spit out from go-ethereum. But I also have encountered the same issue while fuzzing inputs to this function. Regardless of the exact payload, I don't think think we want it to panic. If you run the test on the commit prior (9ad0740) to the fix being introduced, it will panic and the test will fail
|
Pushed a fix that should get CI to pass (or at least get farther along) |
|
Looks like CI is good other then just a flaky example |
Motivation
I encountered an edge case where
TypedTransaction::decodepanicked from a malformed transaction.Solution
Explicitly check the leading byte of the tx for the transaction type.
PR Checklist