From 199a1e4232acf26910d39740ebfbce38912f0bb3 Mon Sep 17 00:00:00 2001
From: Evan Forbes <42654277+evan-forbes@users.noreply.github.com>
Date: Sun, 21 Nov 2021 03:35:20 -0600
Subject: [PATCH] protobuf decode tx patch (#583)

* fix protobuf bug

* clean up

* mispelling

* add more comments to a weird test

* Update types/tx.go

Co-authored-by: John Adler <adlerjohn@users.noreply.github.com>

* better comment w/ todo and use prexisting hash constant

Co-authored-by: John Adler <adlerjohn@users.noreply.github.com>
---
 types/tx.go      | 12 +++++++++++-
 types/tx_test.go | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/types/tx.go b/types/tx.go
index a43623d1cb..98cd17bdbf 100644
--- a/types/tx.go
+++ b/types/tx.go
@@ -165,7 +165,10 @@ func ComputeProtoSizeForTxs(txs []Tx) int64 {
 // transaction wrapper, if this an be done, then it returns true. A child
 // transaction is a normal transaction that has been derived from a different
 // parent transaction. The returned hash is that of the parent transaction,
-// which allows us to remove the parent transaction from the mempool
+// which allows us to remove the parent transaction from the mempool. NOTE:
+// protobuf sometimes does not throw an error if the transaction passed is not
+// a tmproto.ChildTx, since the schema for PayForMessage is kept in the app, we
+// cannot perform further checks without creating an import cycle.
 func DecodeChildTx(tx Tx) (hash []byte, unwrapped Tx, has bool) {
 	// attempt to unmarshal into a a child transaction
 	var childTx tmproto.ChildTx
@@ -173,6 +176,13 @@ func DecodeChildTx(tx Tx) (hash []byte, unwrapped Tx, has bool) {
 	if err != nil {
 		return nil, nil, false
 	}
+	// this check will fail to catch unwanted types should those unmarshalled
+	// types happen to have a hash sized slice of bytes in the same field number
+	// as ParentTxHash. TODO(evan): either fix this, or better yet use a different
+	// mechanism
+	if len(childTx.ParentTxHash) != tmhash.Size {
+		return nil, nil, false
+	}
 	return childTx.ParentTxHash, childTx.Tx, true
 }
 
diff --git a/types/tx_test.go b/types/tx_test.go
index 0ee308ceda..dde2eee2d4 100644
--- a/types/tx_test.go
+++ b/types/tx_test.go
@@ -2,6 +2,7 @@ package types
 
 import (
 	"bytes"
+	"crypto/sha256"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -149,3 +150,47 @@ func assertBadProof(t *testing.T, root []byte, bad []byte, good TxProof) {
 		}
 	}
 }
+
+func TestDecodeChildTx(t *testing.T) {
+	// perform a simple test for being unable to decode a non
+	// child transaction
+	tx := Tx{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0}
+	_, _, ok := DecodeChildTx(tx)
+	require.False(t, ok)
+
+	// create a proto message that used to be decoded when it shouldn't have
+	randomBlock := MakeBlock(
+		1,
+		[]Tx{tx},
+		nil,
+		nil,
+		[]Message{
+			{
+				NamespaceID: []byte{1, 2, 3, 4, 5, 6, 7, 8},
+				Data:        []byte{1, 2, 3, 4, 5, 6, 7, 8, 9},
+			},
+		},
+		&Commit{},
+	)
+
+	protoB, err := randomBlock.ToProto()
+	require.NoError(t, err)
+
+	rawBlock, err := protoB.Marshal()
+	require.NoError(t, err)
+
+	// due to protobuf not actually requiring type compatibility
+	// we need to make sure that there is some check
+	_, _, ok = DecodeChildTx(rawBlock)
+	require.False(t, ok)
+
+	pHash := sha256.Sum256(rawBlock)
+	childTx, err := WrapChildTx(pHash[:], rawBlock)
+	require.NoError(t, err)
+
+	// finally, ensure that the unwrapped bytes are identical to the input
+	unwrappedHash, unwrapped, ok := DecodeChildTx(childTx)
+	require.True(t, ok)
+	assert.Equal(t, 32, len(unwrappedHash))
+	require.Equal(t, rawBlock, []byte(unwrapped))
+}