Merge pull request #5439 from ethereum-optimism/fix/witness-file-parsing

op-chain-ops: prevent invalid decoding errors

op-chain-ops/crossdomain/witness.go

@@ -41,6 +41,40 @@ func NewSentMessageFromJSON(path string) ([]*SentMessage, error) {
 	return j, nil
 }
 
+// decodeWitnessCalldata abi decodes the calldata encoded in the input witness
+// file. It errors if the 4 byte selector is not specifically for `passMessageToL1`.
+// It also errors if the abi decoding fails.
+func decodeWitnessCalldata(msg []byte) ([]byte, error) {
+	abi, err := bindings.LegacyMessagePasserMetaData.GetAbi()
+	if err != nil {
+		panic("should always be able to get message passer abi")
+	}
+
+	if size := len(msg); size < 4 {
+		return nil, fmt.Errorf("message too short: %d", size)
+	}
+
+	method, err := abi.MethodById(msg[:4])
+	if err != nil {
+		return nil, err
+	}
+
+	if method.Sig != "passMessageToL1(bytes)" {
+		return nil, fmt.Errorf("unknown method: %s", method.Name)
+	}
+
+	out, err := method.Inputs.Unpack(msg[4:])
+	if err != nil {
+		return nil, err
+	}
+
+	cast, ok := out[0].([]byte)
+	if !ok {
+		panic("should always be able to cast type []byte")
+	}
+	return cast, nil
+}
+
 // ReadWitnessData will read messages and addresses from a raw l2geth state
 // dump file.
 func ReadWitnessData(path string) ([]*SentMessage, OVMETHAddresses, error) {
@@ -72,30 +106,18 @@ func ReadWitnessData(path string) ([]*SentMessage, OVMETHAddresses, error) {
 				msg = "0x" + msg
 			}
 
-			abi, err := bindings.LegacyMessagePasserMetaData.GetAbi()
-			if err != nil {
-				return nil, nil, fmt.Errorf("failed to get abi: %w", err)
-			}
-
 			msgB := hexutil.MustDecode(msg)
-			method, err := abi.MethodById(msgB[:4])
-			if err != nil {
-				return nil, nil, fmt.Errorf("failed to get method: %w", err)
-			}
 
-			out, err := method.Inputs.Unpack(msgB[4:])
+			// Skip any errors
+			calldata, err := decodeWitnessCalldata(msgB)
 			if err != nil {
-				return nil, nil, fmt.Errorf("failed to unpack: %w", err)
-			}
-
-			cast, ok := out[0].([]byte)
-			if !ok {
-				return nil, nil, fmt.Errorf("failed to cast to bytes")
+				log.Warn("cannot decode witness calldata", "err", err)
+				continue
 			}
 
 			witnesses = append(witnesses, &SentMessage{
 				Who: common.HexToAddress(splits[1]),
-				Msg: cast,
+				Msg: calldata,
 			})
 		case "ETH":
 			addresses[common.HexToAddress(splits[1])] = true

op-chain-ops/crossdomain/witness_test.go

 package crossdomain
 
 import (
+	"context"
+	"math/big"
 	"testing"
 
+	"github.com/ethereum-optimism/optimism/op-bindings/bindings"
+	"github.com/ethereum/go-ethereum/accounts/abi/bind"
+	"github.com/ethereum/go-ethereum/accounts/abi/bind/backends"
+	"github.com/ethereum/go-ethereum/common/hexutil"
+	"github.com/ethereum/go-ethereum/core"
+	"github.com/ethereum/go-ethereum/core/types"
+	"github.com/ethereum/go-ethereum/crypto"
+
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/stretchr/testify/require"
 )
@@ -41,3 +51,163 @@ func TestRead(t *testing.T) {
 		common.HexToAddress("0x6340d44c5174588B312F545eEC4a42f8a514eF50"): true,
 	}, addresses)
 }
+
+// TestDecodeWitnessCallData tests that the witness data is parsed correctly
+// from an input bytes slice.
+func TestDecodeWitnessCallData(t *testing.T) {
+	tests := []struct {
+		name string
+		err  bool
+		msg  []byte
+		want []byte
+	}{
+		{
+			name: "too-small",
+			err:  true,
+			msg:  common.FromHex("0x0000"),
+		},
+		{
+			name: "unknown-selector",
+			err:  true,
+			msg:  common.FromHex("0x00000000"),
+		},
+		{
+			name: "wrong-selector",
+			err:  true,
+			// 0x54fd4d50 is the selector for `version()`
+			msg: common.FromHex("0x54fd4d50"),
+		},
+		{
+			name: "invalid-calldata-only-selector",
+			err:  true,
+			// 0xcafa81dc is the selector for `passMessageToL1(bytes)`
+			msg: common.FromHex("0xcafa81dc"),
+		},
+		{
+			name: "invalid-calldata-invalid-bytes",
+			err:  true,
+			// 0xcafa81dc is the selector for passMessageToL1(bytes)
+			msg: common.FromHex("0xcafa81dc0000"),
+		},
+		{
+			name: "valid-calldata",
+			msg: common.FromHex(
+				"0xcafa81dc" +
+					"0000000000000000000000000000000000000000000000000000000000000020" +
+					"0000000000000000000000000000000000000000000000000000000000000002" +
+					"1234000000000000000000000000000000000000000000000000000000000000",
+			),
+			want: common.FromHex("0x1234"),
+		},
+	}
+
+	for _, tt := range tests {
+		test := tt
+		t.Run(test.name, func(t *testing.T) {
+			if test.err {
+				_, err := decodeWitnessCalldata(test.msg)
+				require.Error(t, err)
+			} else {
+				want, err := decodeWitnessCalldata(test.msg)
+				require.NoError(t, err)
+				require.Equal(t, test.want, want)
+			}
+		})
+	}
+}
+
+// TestMessagePasserSafety ensures that the LegacyMessagePasser contract reverts when it is called
+// with incorrect calldata. The function signature is correct but the calldata is not abi encoded
+// correctly. It is expected the solidity reverts when it cannot abi decode the calldata correctly.
+// Only a call to `passMessageToL1` with abi encoded `bytes` will result in the `successfulMessages`
+// mapping being updated.
+func TestMessagePasserSafety(t *testing.T) {
+	testKey, _ := crypto.HexToECDSA("b71c71a67e1177ad4e901695e1b4b9ee17ae16c6668d313eac2f96dbcda3f291")
+	testAddr := crypto.PubkeyToAddress(testKey.PublicKey)
+	opts, err := bind.NewKeyedTransactorWithChainID(testKey, big.NewInt(1337))
+	require.NoError(t, err)
+
+	backend := backends.NewSimulatedBackend(
+		core.GenesisAlloc{testAddr: {Balance: big.NewInt(10000000000000000)}},
+		30_000_000,
+	)
+	defer backend.Close()
+
+	// deploy the LegacyMessagePasser contract
+	addr, tx, contract, err := bindings.DeployLegacyMessagePasser(opts, backend)
+	require.NoError(t, err)
+
+	backend.Commit()
+	_, err = bind.WaitMined(context.Background(), backend, tx)
+	require.NoError(t, err)
+
+	// ensure that it deployed
+	code, err := backend.CodeAt(context.Background(), addr, nil)
+	require.NoError(t, err)
+	require.True(t, len(code) > 0)
+
+	// dummy message
+	msg := []byte{0x00, 0x01, 0x02, 0x03}
+
+	// call `passMessageToL1`
+	msgTx, err := contract.PassMessageToL1(opts, msg)
+	require.NoError(t, err)
+
+	// ensure that the receipt is successful
+	backend.Commit()
+	msgReceipt, err := bind.WaitMined(context.Background(), backend, msgTx)
+	require.NoError(t, err)
+	require.Equal(t, msgReceipt.Status, types.ReceiptStatusSuccessful)
+
+	// check for the data in the `successfulMessages` mapping
+	data := make([]byte, len(msg)+len(testAddr))
+	copy(data[:], msg)
+	copy(data[len(msg):], testAddr.Bytes())
+	digest := crypto.Keccak256Hash(data)
+	contains, err := contract.SentMessages(&bind.CallOpts{}, digest)
+	require.NoError(t, err)
+	require.True(t, contains)
+
+	// build a transaction with improperly formatted calldata
+	nonce, err := backend.NonceAt(context.Background(), testAddr, nil)
+	require.NoError(t, err)
+	// append msg without abi encoding it
+	selector := crypto.Keccak256([]byte("passMessageToL1(bytes)"))[0:4]
+	require.Equal(t, selector, hexutil.MustDecode("0xcafa81dc"))
+	calldata := append(selector, msg...)
+	faultyTransaction, err := opts.Signer(testAddr, types.NewTx(&types.DynamicFeeTx{
+		ChainID:   big.NewInt(1337),
+		Nonce:     nonce,
+		GasTipCap: msgTx.GasTipCap(),
+		GasFeeCap: msgTx.GasFeeCap(),
+		Gas:       msgTx.Gas() * 2,
+		To:        msgTx.To(),
+		Data:      calldata,
+	}))
+	require.NoError(t, err)
+	err = backend.SendTransaction(context.Background(), faultyTransaction)
+	require.NoError(t, err)
+
+	// the transaction should revert
+	backend.Commit()
+	badReceipt, err := bind.WaitMined(context.Background(), backend, faultyTransaction)
+	require.NoError(t, err)
+	require.Equal(t, badReceipt.Status, types.ReceiptStatusFailed)
+
+	// test the transaction calldata against the abi unpacking
+	abi, err := bindings.LegacyMessagePasserMetaData.GetAbi()
+	require.NoError(t, err)
+	method, err := abi.MethodById(selector)
+	require.NoError(t, err)
+	require.Equal(t, method.Name, "passMessageToL1")
+
+	// the faulty transaction has the correct 4 byte selector but doesn't
+	// have abi encoded bytes following it
+	require.Equal(t, faultyTransaction.Data()[:4], selector)
+	_, err = method.Inputs.Unpack(faultyTransaction.Data()[4:])
+	require.Error(t, err)
+
+	// the original transaction has the correct 4 byte selector and abi encoded bytes
+	_, err = method.Inputs.Unpack(msgTx.Data()[4:])
+	require.NoError(t, err)
+}