#38828 [BC-Low] Decode RLP of Legacy Transaction Allows Tailing Bytes
Submitted on Jan 14th 2025 at 19:33:55 UTC by @CertiK for Attackathon | Ethereum Protocol
Report ID: #38828
Report Type: Blockchain/DLT
Report severity: Low
Target: https://github.com/ledgerwatch/erigon
Impacts:
(Specifications) A bug in specifications with no direct impact on client implementations
Description
Brief/Intro
RLP serialization is widely used in the Ethereum clients, which provides a standard format for the data transfer between nodes.
In Erigon (https://github.com/erigontech/erigon/ ), the decode RLP of legacy tx misses check the tailing bytes, which allows extra tailing bytes during the RLP decoding. This could possibly be a consensus issue as other Ethereum clients reject such legacy transactions.
Vulnerability Details
Affected Codebase: https://github.com/erigontech/erigon/tree/v2.61.0
The function DecodeRLPTransaction() is intended to decode the RLP encoding of transactions:
https://github.com/erigontech/erigon/blob/v2.61.0/core/types/transaction.go#L116
func DecodeRLPTransaction(s *rlp.Stream, blobTxnsAreWrappedWithBlobs bool) (Transaction, error) {
kind, _, err := s.Kind()
if err != nil {
return nil, err
}
if rlp.List == kind {
tx := &LegacyTx{}
if err = tx.DecodeRLP(s); err != nil {
return nil, err
}
return tx, nil
}
if rlp.String != kind {
return nil, fmt.Errorf("not an RLP encoded transaction. If this is a canonical encoded transaction, use UnmarshalTransactionFromBinary instead. Got %v for kind, expected String", kind)
}
// Decode the EIP-2718 typed TX envelope.
var b []byte
if b, err = s.Bytes(); err != nil {
return nil, err
}
if len(b) == 0 {
return nil, rlp.EOL
}
return UnmarshalTransactionFromBinary(b, blobTxnsAreWrappedWithBlobs)
}For non-legacy transactions, it invokes UnmarshalTransactionFromBinary()that handles the tailing bytes with the check s.Remaining() != 0:
https://github.com/erigontech/erigon/blob/v2.61.0/core/types/transaction.go#L178C1-L211C2
func UnmarshalTransactionFromBinary(data []byte, blobTxnsAreWrappedWithBlobs bool) (Transaction, error) {
if len(data) <= 1 {
return nil, fmt.Errorf("short input: %v", len(data))
}
s := rlp.NewStream(bytes.NewReader(data[1:]), uint64(len(data)-1))
var t Transaction
switch data[0] {
case AccessListTxType:
t = &AccessListTx{}
case DynamicFeeTxType:
t = &DynamicFeeTransaction{}
case BlobTxType:
if blobTxnsAreWrappedWithBlobs {
t = &BlobTxWrapper{}
} else {
t = &BlobTx{}
}
case SetCodeTxType:
t = &SetCodeTransaction{}
default:
if data[0] >= 0x80 {
// Tx is type legacy which is RLP encoded
return DecodeTransaction(data)
}
return nil, ErrTxTypeNotSupported
}
if err := t.DecodeRLP(s); err != nil {
return nil, err
}
if s.Remaining() != 0 {
return nil, fmt.Errorf("trailing bytes after rlp encoded transaction")
}
return t, nil
}However, if it’s the legacy transaction, it calls tx.DecodeRLP(s) to directly decode the legacy transaction, in which the tailing bytes is unchecked. This oversight would allow tailing bytes to sneak into the legacy transaction when calling DecodeRLPTransaction() to decode the legacy transaction.
https://github.com/erigontech/erigon/blob/v2.61.0/core/types/legacy_tx.go#L291C1-L340C2
func (tx *LegacyTx) DecodeRLP(s *rlp.Stream) error {
_, err := s.List()
if err != nil {
return fmt.Errorf("legacy tx must be a list: %w", err)
}
if tx.Nonce, err = s.Uint(); err != nil {
return fmt.Errorf("read Nonce: %w", err)
}
var b []byte
if b, err = s.Uint256Bytes(); err != nil {
return fmt.Errorf("read GasPrice: %w", err)
}
tx.GasPrice = new(uint256.Int).SetBytes(b)
if tx.Gas, err = s.Uint(); err != nil {
return fmt.Errorf("read Gas: %w", err)
}
if b, err = s.Bytes(); err != nil {
return fmt.Errorf("read To: %w", err)
}
if len(b) > 0 && len(b) != 20 {
return fmt.Errorf("wrong size for To: %d", len(b))
}
if len(b) > 0 {
tx.To = &libcommon.Address{}
copy((*tx.To)[:], b)
}
if b, err = s.Uint256Bytes(); err != nil {
return fmt.Errorf("read Value: %w", err)
}
tx.Value = new(uint256.Int).SetBytes(b)
if tx.Data, err = s.Bytes(); err != nil {
return fmt.Errorf("read Data: %w", err)
}
if b, err = s.Uint256Bytes(); err != nil {
return fmt.Errorf("read V: %w", err)
}
tx.V.SetBytes(b)
if b, err = s.Uint256Bytes(); err != nil {
return fmt.Errorf("read R: %w", err)
}
tx.R.SetBytes(b)
if b, err = s.Uint256Bytes(); err != nil {
return fmt.Errorf("read S: %w", err)
}
tx.S.SetBytes(b)
if err = s.ListEnd(); err != nil {
return fmt.Errorf("close tx struct: %w", err)
}
return nil
}Impact Details
The function DecodeRLPTransaction() has been widely utilized in the Erigon codebase to decode the transactions from block, block body and user’s submitted raw transaction. Since tailing bytes of a transaction is disallowed in other Ethereum clients, it could introduce consensus issues with this flawed decoding function.
References
https://github.com/erigontech/erigon/tree/v2.61.0
Proof of Concept
Proof of Concept
For simplicity, we provide the following unit test to compare two transaction decoding functions, DecodeRLPTransaction() vs UnmarshalTransactionFromBinary().
The encoded legacy transaction is []byte{248, 99, 128, 2, 1, 148, 9, 94, 123, 174, 166, 166, 199, 196, 194, 223, 235, 151, 126, 250, 195, 38, 175, 85, 45, 135, 128, 134, 97, 98, 99, 100, 101, 102, 37, 160, 142, 183, 96, 239, 234, 152, 124, 9, 98, 209, 245, 242, 175, 209, 122, 221, 51, 54, 23, 237, 233, 142, 83, 7, 50, 17, 119, 99, 15, 34, 57, 125, 160, 21, 141, 44, 195, 195, 220, 247, 64, 91, 79, 6, 37, 93, 226, 96, 69, 227, 240, 8, 168, 169, 112, 118, 124, 14, 250, 73, 19, 190, 7, 104, 57, 1} that contains the last taling bytes 0x01.
Run the following unit test:
func TestDecodeRLPTransactionTailing(t *testing.T) {
input := []byte{248, 99, 128, 2, 1, 148, 9, 94, 123, 174, 166, 166, 199, 196, 194, 223, 235, 151, 126, 250, 195, 38, 175, 85, 45, 135, 128, 134, 97, 98, 99, 100, 101, 102, 37, 160, 142, 183, 96, 239, 234, 152, 124, 9, 98, 209, 245, 242, 175, 209, 122, 221, 51, 54, 23, 237, 233, 142, 83, 7, 50, 17, 119, 99, 15, 34, 57, 125, 160, 21, 141, 44, 195, 195, 220, 247, 64, 91, 79, 6, 37, 93, 226, 96, 69, 227, 240, 8, 168, 169, 112, 118, 124, 14, 250, 73, 19, 190, 7, 104, 57, 1}
tx, err := UnmarshalTransactionFromBinary(input, false)
if err != nil {
fmt.Printf("Unmarshal Tx with UnmarshalTransactionFromBinary() fails: %+v\n", err)
} else {
fmt.Printf("Unmarshal Tx with UnmarshalTransactionFromBinary() succeeds: %+v\n", tx.Hash())
}
data := input
s := rlp.NewStream(bytes.NewReader(data), uint64(len(data)))
legacyTx, err := DecodeRLPTransaction(s, false)
if err != nil {
fmt.Printf("Decode RLP of Tx DecodeRLPTransaction() fails: %+v\n", err)
} else {
fmt.Printf("Decode RLP of Tx DecodeRLPTransaction() succeeds: %+v\n", legacyTx.Hash())
}
}The test results show that the decoding of this legacy transaction with tailing bytes succeeds with
DecodeRLPTransaction()but fails withUnmarshalTransactionFromBinary().
=== RUN TestDecodeRLPTransactionTailing
Unmarshal Tx with UnmarshalTransactionFromBinary() fails: trailing bytes after rlp encoded transaction
Decode RLP of Tx DecodeRLPTransaction() succeeds: 0x74487f78cbc2fd511acd00d2d5ec65d9a19fb8eaad38c33b4e5737d780c7cc60
--- PASS: TestDecodeRLPTransactionTailing (0.00s)
PASS
Process finished with the exit code 0Was this helpful?