Properly validate whether tranactions are standard

CHANGELOG.md

@@ -51,6 +51,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 * [#168](https://github.com/babylonlabs-io/babylon/pull/168) Remove devdoc from
   Makefile and remove unnecessary gin replace.
 
+### State Machine Breaking
+
+* [#185](https://github.com/babylonlabs-io/babylon/pull/185) Check that
+unbonding / slashing transactions are standard
+
 ## v0.12.0
 
 ### State Machine Breaking

btcstaking/staking.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 
 	sdkmath "cosmossdk.io/math"
+	asig "github.com/babylonlabs-io/babylon/crypto/schnorr-adaptor-signature"
 	"github.com/btcsuite/btcd/blockchain"
 	"github.com/btcsuite/btcd/btcec/v2"
 	"github.com/btcsuite/btcd/btcec/v2/schnorr"
@@ -14,8 +15,15 @@ import (
 	"github.com/btcsuite/btcd/mempool"
 	"github.com/btcsuite/btcd/txscript"
 	"github.com/btcsuite/btcd/wire"
+)
 
-	asig "github.com/babylonlabs-io/babylon/crypto/schnorr-adaptor-signature"
+const (
+	// MaxTxVersion is the maximum transaction version allowed in Babylon system.
+	// Changing that constant will require upgrade in the future, if we ever need
+	// to support v3 transactions.
+	MaxTxVersion = 2
+
+	MaxStandardTxWeight = 400000
 )
 
 // buildSlashingTxFromOutpoint builds a valid slashing transaction by creating a new Bitcoin transaction that slashes a portion
@@ -215,6 +223,89 @@ func IsSimpleTransfer(tx *wire.MsgTx) error {
 	return nil
 }
 
+// CheckPreSignedTxSanity perfomorms basic checks on a pre-signed transaction:
+// - the transaction is not nil.
+// - the transaction obeys basic BTC rules.
+// - the transaction has exactly numInputs inputs.
+// - the transaction has exactly numOutputs outputs.
+// - the transaction lock time is 0.
+// - the transaction version is between 1 and maxTxVersion.
+// - each input has a sequence number equal to MaxTxInSequenceNum.
+// - each input has an empty signature script.
+// - each input has an empty witness.
+func CheckPreSignedTxSanity(
+	tx *wire.MsgTx,
+	numInputs, numOutputs uint32,
+	maxTxVersion int32,
+) error {
+	if tx == nil {
+		return fmt.Errorf("tx must not be nil")
+	}
+
+	transaction := btcutil.NewTx(tx)
+
+	if err := blockchain.CheckTransactionSanity(transaction); err != nil {
+		return fmt.Errorf("btc transaction do not obey BTC rules: %w", err)
+	}
+
+	if len(tx.TxIn) != int(numInputs) {
+		return fmt.Errorf("tx must have exactly %d inputs", numInputs)
+	}
+
+	if len(tx.TxOut) != int(numOutputs) {
+		return fmt.Errorf("tx must have exactly %d outputs", numOutputs)
+	}
+
+	// this requirement makes every pre-signed tx final
+	if tx.LockTime != 0 {
+		return fmt.Errorf("pre-signed tx must not have locktime")
+	}
+
+	if tx.Version > maxTxVersion || tx.Version < 1 {
+		return fmt.Errorf("tx version must be between 1 and %d", maxTxVersion)
+	}
+
+	txWeight := blockchain.GetTransactionWeight(transaction)
+
+	// Check that the transaction weight does not exceed the maximum standard tx weight
+	// alternative would be to require len(in.Witness) == 0 for all inptus.
+	if txWeight > MaxStandardTxWeight {
+		return fmt.Errorf("tx weight must not exceed %d", MaxStandardTxWeight)
+	}
+
+	for _, in := range tx.TxIn {
+		if in.Sequence != wire.MaxTxInSequenceNum {
+			return fmt.Errorf("pre-signed tx must not be replaceable")
+		}
+
+		// We require this to be 0, as all babylon pre-signed transactions use
+		// witness
+		if len(in.SignatureScript) != 0 {
+			return fmt.Errorf("pre-signed tx must not have signature script")
+		}
+	}
+
+	return nil
+}
+
+func CheckPreSignedUnbondingTxSanity(tx *wire.MsgTx) error {
+	return CheckPreSignedTxSanity(
+		tx,
+		1,
+		1,
+		MaxTxVersion,
+	)
+}
+
+func CheckPreSignedSlashingTxSanity(tx *wire.MsgTx) error {
+	return CheckPreSignedTxSanity(
+		tx,
+		1,
+		2,
+		MaxTxVersion,
+	)
+}
+
 // validateSlashingTx performs basic checks on a slashing transaction:
 // - the slashing transaction is not nil.
 // - the slashing transaction has exactly one input.
@@ -235,29 +326,9 @@ func validateSlashingTx(
 	slashingChangeLockTime uint16,
 	net *chaincfg.Params,
 ) error {
-	// Verify that the slashing transaction is not nil.
-	if slashingTx == nil {
-		return fmt.Errorf("slashing transaction must not be nil")
-	}
 
-	// Verify that the slashing transaction has exactly one input.
-	if len(slashingTx.TxIn) != 1 {
-		return fmt.Errorf("slashing transaction must have exactly one input")
-	}
-
-	// Verify that the slashing transaction is non-replaceable.
-	if slashingTx.TxIn[0].Sequence != wire.MaxTxInSequenceNum {
-		return fmt.Errorf("slashing transaction must not be replaceable")
-	}
-
-	// Verify that lock time of the slashing transaction is 0.
-	if slashingTx.LockTime != 0 {
-		return fmt.Errorf("slashing tx must not have locktime")
-	}
-
-	// Verify that the slashing transaction has exactly two outputs.
-	if len(slashingTx.TxOut) != 2 {
-		return fmt.Errorf("slashing transaction must have exactly 2 outputs")
+	if err := CheckPreSignedSlashingTxSanity(slashingTx); err != nil {
+		return fmt.Errorf("invalid slashing tx: %w", err)
 	}
 
 	// Verify that at least staking output value * slashing rate is slashed.
@@ -324,13 +395,13 @@ func validateSlashingTx(
 	return nil
 }
 
-// CheckTransactions validates all relevant data of slashing and funding transaction.
+// CheckSlashingTxMatchFundingTx validates all relevant data of slashing and funding transaction.
 // - both transactions are valid from pov of BTC rules
 // - funding transaction has output committing to the provided script
 // - slashing transaction is valid
 // - slashing transaction input hash is pointing to funding transaction hash
 // - slashing transaction input index is pointing to funding transaction output committing to the script
-func CheckTransactions(
+func CheckSlashingTxMatchFundingTx(
 	slashingTx *wire.MsgTx,
 	fundingTransaction *wire.MsgTx,
 	fundingOutputIdx uint32,
@@ -345,10 +416,6 @@ func CheckTransactions(
 		return fmt.Errorf("slashing and funding transactions must not be nil")
 	}
 
-	if err := blockchain.CheckTransactionSanity(btcutil.NewTx(slashingTx)); err != nil {
-		return fmt.Errorf("slashing transaction does not obey BTC rules: %w", err)
-	}
-
 	if err := blockchain.CheckTransactionSanity(btcutil.NewTx(fundingTransaction)); err != nil {
 		return fmt.Errorf("funding transaction does not obey BTC rules: %w", err)
 	}

btcstaking/staking_test.go

@@ -193,7 +193,7 @@ func testSlashingTx(
 			require.ErrorIs(t, err, btcstaking.ErrDustOutputFound)
 		} else {
 			require.NoError(t, err)
-			err = btcstaking.CheckTransactions(
+			err = btcstaking.CheckSlashingTxMatchFundingTx(
 				slashingTx,
 				stakingTx,
 				uint32(stakingOutputIdx),
@@ -303,7 +303,7 @@ func TestSlashingTxWithOverflowMustNotAccepted(t *testing.T) {
 	slashingTx.TxOut[0].Value = math.MaxInt64 / 8
 	slashingTx.TxOut[1].Value = math.MaxInt64 / 8
 
-	err = btcstaking.CheckTransactions(
+	err = btcstaking.CheckSlashingTxMatchFundingTx(
 		slashingTx,
 		stakingTx,
 		uint32(0),
@@ -315,7 +315,7 @@ func TestSlashingTxWithOverflowMustNotAccepted(t *testing.T) {
 		&chaincfg.MainNetParams,
 	)
 	require.Error(t, err)
-	require.EqualError(t, err, "slashing transaction does not obey BTC rules: transaction output value is higher than max allowed value: 1152921504606846975 > 2.1e+15 ")
+	require.EqualError(t, err, "invalid slashing tx: btc transaction do not obey BTC rules: transaction output value is higher than max allowed value: 1152921504606846975 > 2.1e+15 ")
 }
 
 func TestNotAllowStakerKeyToBeFinalityProviderKey(t *testing.T) {
@@ -413,3 +413,148 @@ func TestNotAllowFinalityProviderKeysAsCovenantKeys(t *testing.T) {
 	require.Error(t, err)
 	require.True(t, errors.Is(err, btcstaking.ErrDuplicatedKeyInScript))
 }
+
+func TestCheckPreSignedTxSanity(t *testing.T) {
+	tests := []struct {
+		name           string
+		genTx          func() *wire.MsgTx
+		numInputs      uint32
+		numOutputs     uint32
+		maxTxVersion   int32
+		wantErr        bool
+		expectedErrMsg string
+	}{
+		{
+			name: "valid tx",
+			genTx: func() *wire.MsgTx {
+				tx := wire.NewMsgTx(2)
+				tx.AddTxIn(wire.NewTxIn(wire.NewOutPoint(&chainhash.Hash{}, 0), nil, nil))
+				tx.AddTxOut(wire.NewTxOut(1000, nil))
+				return tx
+			},
+			numInputs:    1,
+			numOutputs:   1,
+			maxTxVersion: 2,
+			wantErr:      false,
+		},
+		{
+			name: "non standard version tx",
+			genTx: func() *wire.MsgTx {
+				tx := wire.NewMsgTx(0)
+				tx.AddTxIn(wire.NewTxIn(wire.NewOutPoint(&chainhash.Hash{}, 0), nil, nil))
+				tx.AddTxOut(wire.NewTxOut(1000, nil))
+				return tx
+			},
+			numInputs:      1,
+			numOutputs:     1,
+			maxTxVersion:   2,
+			wantErr:        true,
+			expectedErrMsg: "tx version must be between 1 and 2",
+		},
+		{
+			name: "transaction with locktime",
+			genTx: func() *wire.MsgTx {
+				tx := wire.NewMsgTx(2)
+				tx.AddTxIn(wire.NewTxIn(wire.NewOutPoint(&chainhash.Hash{}, 0), nil, nil))
+				tx.AddTxOut(wire.NewTxOut(1000, nil))
+				tx.LockTime = 1
+				return tx
+			},
+			numInputs:      1,
+			numOutputs:     1,
+			maxTxVersion:   2,
+			wantErr:        true,
+			expectedErrMsg: "pre-signed tx must not have locktime",
+		},
+		{
+			name: "transaction with sig script",
+			genTx: func() *wire.MsgTx {
+				tx := wire.NewMsgTx(2)
+				tx.AddTxIn(wire.NewTxIn(wire.NewOutPoint(&chainhash.Hash{}, 0), nil, nil))
+				tx.AddTxOut(wire.NewTxOut(1000, nil))
+				tx.TxIn[0].SignatureScript = []byte{0x01, 0x02, 0x03}
+				return tx
+			},
+			numInputs:      1,
+			numOutputs:     1,
+			maxTxVersion:   2,
+			wantErr:        true,
+			expectedErrMsg: "pre-signed tx must not have signature script",
+		},
+		{
+			name: "transaction with invalid amount of inputs",
+			genTx: func() *wire.MsgTx {
+				tx := wire.NewMsgTx(2)
+				tx.AddTxIn(wire.NewTxIn(wire.NewOutPoint(&chainhash.Hash{}, 0), nil, nil))
+				tx.AddTxIn(wire.NewTxIn(wire.NewOutPoint(&chainhash.Hash{}, 1), nil, nil))
+				tx.AddTxOut(wire.NewTxOut(1000, nil))
+				return tx
+			},
+			numInputs:      1,
+			numOutputs:     1,
+			maxTxVersion:   2,
+			wantErr:        true,
+			expectedErrMsg: "tx must have exactly 1 inputs",
+		},
+		{
+			name: "transaction with invalid amount of outputs",
+			genTx: func() *wire.MsgTx {
+				tx := wire.NewMsgTx(2)
+				tx.AddTxIn(wire.NewTxIn(wire.NewOutPoint(&chainhash.Hash{}, 0), nil, nil))
+				tx.AddTxOut(wire.NewTxOut(1000, nil))
+				tx.AddTxOut(wire.NewTxOut(1000, nil))
+				return tx
+			},
+			numInputs:      1,
+			numOutputs:     1,
+			maxTxVersion:   2,
+			wantErr:        true,
+			expectedErrMsg: "tx must have exactly 1 outputs",
+		},
+		{
+			name: "replacable transaction",
+			genTx: func() *wire.MsgTx {
+				tx := wire.NewMsgTx(2)
+				tx.AddTxIn(wire.NewTxIn(wire.NewOutPoint(&chainhash.Hash{}, 0), nil, nil))
+				tx.AddTxOut(wire.NewTxOut(1000, nil))
+				tx.TxIn[0].Sequence = wire.MaxTxInSequenceNum - 1
+				return tx
+			},
+			numInputs:      1,
+			numOutputs:     1,
+			maxTxVersion:   2,
+			wantErr:        true,
+			expectedErrMsg: "pre-signed tx must not be replaceable",
+		},
+		{
+			name: "transaction with too big witness",
+			genTx: func() *wire.MsgTx {
+				tx := wire.NewMsgTx(2)
+				tx.AddTxIn(wire.NewTxIn(wire.NewOutPoint(&chainhash.Hash{}, 0), nil, nil))
+				tx.AddTxOut(wire.NewTxOut(1000, nil))
+				witness := [20000000]byte{}
+				tx.TxIn[0].Witness = [][]byte{witness[:]}
+				return tx
+			},
+			numInputs:      1,
+			numOutputs:     1,
+			maxTxVersion:   2,
+			wantErr:        true,
+			expectedErrMsg: "tx weight must not exceed 400000",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := btcstaking.CheckPreSignedTxSanity(
+				tt.genTx(), tt.numInputs, tt.numOutputs, tt.maxTxVersion,
+			)
+
+			if tt.wantErr {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tt.expectedErrMsg)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}

x/btcstaking/types/create_delegation_parser.go

@@ -10,7 +10,6 @@ import (
 	"github.com/btcsuite/btcd/wire"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
-	"github.com/babylonlabs-io/babylon/btcstaking"
 	bbn "github.com/babylonlabs-io/babylon/types"
 )
 
@@ -177,10 +176,6 @@ func ParseCreateDelegationMessage(msg *MsgCreateBTCDelegation) (*ParsedCreateDel
 		return nil, fmt.Errorf("failed to deserialize unbonding tx: %v", err)
 	}
 
-	if err := btcstaking.IsSimpleTransfer(unbondingTx.Transaction); err != nil {
-		return nil, fmt.Errorf("unbonding tx is not a simple transfer: %v", err)
-	}
-
 	unbondingSlashingTx, err := NewBtcTransaction(msg.UnbondingSlashingTx.MustMarshal())
 
 	if err != nil {