shw - Panic when decoding a malformed deposit transaction JSON string

[github-actions]

shw

medium

Panic when decoding a malformed deposit transaction JSON string

Summary

When decoding a deposit transaction JSON string without the "gas" field, a panic/runtime error is triggered due to a nil pointer dereference.

Vulnerability Detail

The op-geth/core/types/transaction_marshalling.go file defines how transactions are encoded and decoded from JSON format. In the UnmarshalJSON() function, from L283 to L315, a new logic is added for the deposit transaction type, DepositTxType. The bug happens at L293, where the dec.Gas field is dereferenced without checking it against nil first. As a result, if the provided deposit transaction JSON string does not have the "gas" field, the nil pointer dereference will trigger a runtime error and crash the program.

For a PoC: Add the following test case to op-geth/core/types/transaction_test.go and run cd op-geth && go test -run TestDecodeJSON -v ./core/types

func TestDecodeJSON(t *testing.T) {
    // the "gas" field does not exist
    var data = []byte("{\"type\":\"0x7e\",\"nonce\":null,\"gasPrice\":null,\"maxPriorityFeePerGas\":null,\"maxFeePerGas\":null,\"value\":\"0x1\",\"input\":\"0x616263646566\",\"v\":null,\"r\":null,\"s\":null,\"to\":null,\"sourceHash\":\"0x0000000000000000000000000000000000000000000000000000000000000000\",\"from\":\"0x0000000000000000000000000000000000000001\",\"isSystemTx\":false,\"hash\":\"0xa4341f3db4363b7ca269a8538bd027b2f8784f84454ca917668642d5f6dffdf9\"}")
    var parsedTx = &Transaction{}
    _ = json.Unmarshal(data, &parsedTx) // will panic here
}

Impact

A possible exploit scenario is targeting an ethclient compiled based on op-geth. For example, according to op-geth/ethclient/ethclient.go, the BlockByHash() API makes a RPC eth_getBlockByHash request to an RPC endpoint and expect to receive a json.RawMessage data that represents the queried block. In the getBlock() function, the JSON raw message is then decoded into a rpcBlock, containing a list of rpcTransactions with type of Transaction. Therefore, a malicious RPC endpoint can construct a deposit transaction without the "gas" field in a block and return the block to the ethclient. The ethclient will fail to decode the received block and crash due to this bug.

Marking this issue as medium severity according to previous similar audit findings from Sigma Prime.

Code Snippet

Please refer to op-geth/core/types/transaction_marshalling.go#L293, op-geth/ethclient/ethclient.go#L77-L79 and op-geth/ethclient/ethclient.go#L118-L126.
https://github.com/ethereum-optimism/op-geth/blob/985086bf2a5c61e76a8ce7c74ac029660751e260/core/types/transaction_marshalling.go#L293

Tool used

Manual Review

Recommendation

Check whether dec.Gas is nil before dereferencing it:

if dec.Gas == nil {
    return errors.New("missing required field 'gas' in transaction")
}

[rcstanciu]

Comment from Optimism

---

Description: Panic in transaction unmarshalling code

Reason: This is a legit bug in our transaction unmarshalling code.

Action: Check for nil gas before unmarshalling