test: fix evm signer instability

zeta-chain · Nov 8, 2024 · ab02935 · ab02935
1 parent 7c70809
commit ab02935
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 49 deletions.
diff --git a/zetaclient/chains/evm/signer/sign_test.go b/zetaclient/chains/evm/signer/sign_test.go
@@ -8,14 +8,15 @@ import (
 	"github.com/rs/zerolog"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"github.com/zeta-chain/node/pkg/chains"
 	"github.com/zeta-chain/node/zetaclient/testutils/mocks"
 )
 
 func TestSigner_SignConnectorOnReceive(t *testing.T) {
 	ctx := makeCtx(t)
 
 	// Setup evm signer
-	tss := mocks.NewTSSMainnet()
+	tss := mocks.NewDerivedTSS(chains.BitcoinMainnet)
 	evmSigner, err := getNewEvmSigner(tss)
 	require.NoError(t, err)
 
@@ -33,8 +34,7 @@ func TestSigner_SignConnectorOnReceive(t *testing.T) {
 		require.NoError(t, err)
 
 		// Verify Signature
-		tss := mocks.NewTSSMainnet()
-		verifyTxSignature(t, tx, tss.Pubkey(), evmSigner.EvmSigner())
+		verifyTxSender(t, tx, tss.EVMAddress(), evmSigner.EvmSigner())
 	})
 	t.Run("SignConnectorOnReceive - should fail if keysign fails", func(t *testing.T) {
 		// Pause tss to make keysign fail
@@ -53,8 +53,7 @@ func TestSigner_SignConnectorOnReceive(t *testing.T) {
 		require.NoError(t, err)
 
 		// Verify Signature
-		tss := mocks.NewTSSMainnet()
-		verifyTxSignature(t, tx, tss.Pubkey(), evmSigner.EvmSigner())
+		verifyTxSender(t, tx, tss.EVMAddress(), evmSigner.EvmSigner())
 
 		// check that by default tx type is legacy tx
 		assert.Equal(t, ethtypes.LegacyTxType, int(tx.Type()))
@@ -86,7 +85,7 @@ func TestSigner_SignConnectorOnReceive(t *testing.T) {
 		require.NoError(t, err)
 
 		// ASSERT
-		verifyTxSignature(t, tx, mocks.NewTSSMainnet().Pubkey(), evmSigner.EvmSigner())
+		verifyTxSender(t, tx, tss.EVMAddress(), evmSigner.EvmSigner())
 
 		// check that by default tx type is a dynamic fee tx
 		assert.Equal(t, ethtypes.DynamicFeeTxType, int(tx.Type()))
@@ -101,7 +100,7 @@ func TestSigner_SignConnectorOnRevert(t *testing.T) {
 	ctx := makeCtx(t)
 
 	// Setup evm signer
-	tss := mocks.NewTSSMainnet()
+	tss := mocks.NewDerivedTSS(chains.BitcoinMainnet)
 	evmSigner, err := getNewEvmSigner(tss)
 	require.NoError(t, err)
 
@@ -118,8 +117,7 @@ func TestSigner_SignConnectorOnRevert(t *testing.T) {
 		require.NoError(t, err)
 
 		// Verify tx signature
-		tss := mocks.NewTSSMainnet()
-		verifyTxSignature(t, tx, tss.Pubkey(), evmSigner.EvmSigner())
+		verifyTxSender(t, tx, tss.EVMAddress(), evmSigner.EvmSigner())
 
 		// Verify tx body basics
 		// Note: Revert tx calls connector contract with 0 gas token
@@ -140,7 +138,7 @@ func TestSigner_SignCancel(t *testing.T) {
 	ctx := makeCtx(t)
 
 	// Setup evm signer
-	tss := mocks.NewTSSMainnet()
+	tss := mocks.NewDerivedTSS(chains.BitcoinMainnet)
 	evmSigner, err := getNewEvmSigner(tss)
 	require.NoError(t, err)
 
@@ -157,12 +155,11 @@ func TestSigner_SignCancel(t *testing.T) {
 		require.NoError(t, err)
 
 		// Verify tx signature
-		tss := mocks.NewTSSMainnet()
-		verifyTxSignature(t, tx, tss.Pubkey(), evmSigner.EvmSigner())
+		verifyTxSender(t, tx, tss.EVMAddress(), evmSigner.EvmSigner())
 
 		// Verify tx body basics
 		// Note: Cancel tx sends 0 gas token to TSS self address
-		verifyTxBodyBasics(t, tx, evmSigner.TSS().EVMAddress(), txData.nonce, big.NewInt(0))
+		verifyTxBodyBasics(t, tx, tss.EVMAddress(), txData.nonce, big.NewInt(0))
 	})
 	t.Run("SignCancel - should fail if keysign fails", func(t *testing.T) {
 		// Pause tss to make keysign fail
@@ -179,7 +176,7 @@ func TestSigner_SignGasWithdraw(t *testing.T) {
 	ctx := makeCtx(t)
 
 	// Setup evm signer
-	tss := mocks.NewTSSMainnet()
+	tss := mocks.NewDerivedTSS(chains.BitcoinMainnet)
 	evmSigner, err := getNewEvmSigner(tss)
 	require.NoError(t, err)
 
@@ -196,8 +193,7 @@ func TestSigner_SignGasWithdraw(t *testing.T) {
 		require.NoError(t, err)
 
 		// Verify tx signature
-		tss := mocks.NewTSSMainnet()
-		verifyTxSignature(t, tx, tss.Pubkey(), evmSigner.EvmSigner())
+		verifyTxSender(t, tx, tss.EVMAddress(), evmSigner.EvmSigner())
 
 		// Verify tx body basics
 		verifyTxBodyBasics(t, tx, txData.to, txData.nonce, txData.amount)
@@ -217,7 +213,7 @@ func TestSigner_SignERC20Withdraw(t *testing.T) {
 	ctx := makeCtx(t)
 
 	// Setup evm signer
-	tss := mocks.NewTSSMainnet()
+	tss := mocks.NewDerivedTSS(chains.BitcoinMainnet)
 	evmSigner, err := getNewEvmSigner(tss)
 	require.NoError(t, err)
 
@@ -233,8 +229,7 @@ func TestSigner_SignERC20Withdraw(t *testing.T) {
 		require.NoError(t, err)
 
 		// Verify tx signature
-		tss := mocks.NewTSSMainnet()
-		verifyTxSignature(t, tx, tss.Pubkey(), evmSigner.EvmSigner())
+		verifyTxSender(t, tx, tss.EVMAddress(), evmSigner.EvmSigner())
 
 		// Verify tx body basics
 		// Note: Withdraw tx calls erc20 custody contract with 0 gas token

diff --git a/zetaclient/chains/evm/signer/signer_admin_test.go b/zetaclient/chains/evm/signer/signer_admin_test.go
@@ -7,6 +7,7 @@ import (
 
 	"github.com/rs/zerolog"
 	"github.com/stretchr/testify/require"
+	"github.com/zeta-chain/node/pkg/chains"
 	"github.com/zeta-chain/node/pkg/constant"
 	"github.com/zeta-chain/node/testutil/sample"
 	"github.com/zeta-chain/node/zetaclient/testutils/mocks"
@@ -16,7 +17,8 @@ func TestSigner_SignAdminTx(t *testing.T) {
 	ctx := makeCtx(t)
 
 	// Setup evm signer
-	evmSigner, err := getNewEvmSigner(nil)
+	tss := mocks.NewDerivedTSS(chains.BitcoinMainnet)
+	evmSigner, err := getNewEvmSigner(tss)
 	require.NoError(t, err)
 
 	// Setup txData struct
@@ -36,8 +38,7 @@ func TestSigner_SignAdminTx(t *testing.T) {
 		require.NoError(t, err)
 
 		// Verify tx signature
-		tss := mocks.NewTSSMainnet()
-		verifyTxSignature(t, tx, tss.Pubkey(), evmSigner.EvmSigner())
+		verifyTxSender(t, tx, tss.EVMAddress(), evmSigner.EvmSigner())
 
 		// Verify tx body basics
 		// Note: Revert tx calls erc20 custody contract with 0 gas token
@@ -57,8 +58,7 @@ func TestSigner_SignAdminTx(t *testing.T) {
 		require.NoError(t, err)
 
 		// Verify tx signature
-		tss := mocks.NewTSSMainnet()
-		verifyTxSignature(t, tx, tss.Pubkey(), evmSigner.EvmSigner())
+		verifyTxSender(t, tx, tss.EVMAddress(), evmSigner.EvmSigner())
 
 		// Verify tx body basics
 		// Note: Revert tx calls erc20 custody contract with 0 gas token
@@ -73,8 +73,7 @@ func TestSigner_SignAdminTx(t *testing.T) {
 		require.NoError(t, err)
 
 		// Verify tx signature
-		tss := mocks.NewTSSMainnet()
-		verifyTxSignature(t, tx, tss.Pubkey(), evmSigner.EvmSigner())
+		verifyTxSender(t, tx, tss.EVMAddress(), evmSigner.EvmSigner())
 
 		// Verify tx body basics
 		// Note: Revert tx calls erc20 custody contract with 0 gas token
@@ -88,8 +87,7 @@ func TestSigner_SignAdminTx(t *testing.T) {
 		require.NoError(t, err)
 
 		// Verify tx signature
-		tss := mocks.NewTSSMainnet()
-		verifyTxSignature(t, tx, tss.Pubkey(), evmSigner.EvmSigner())
+		verifyTxSender(t, tx, tss.EVMAddress(), evmSigner.EvmSigner())
 
 		// Verify tx body basics
 		verifyTxBodyBasics(t, tx, txData.to, txData.nonce, txData.amount)
@@ -100,7 +98,7 @@ func TestSigner_SignWhitelistERC20Cmd(t *testing.T) {
 	ctx := makeCtx(t)
 
 	// Setup evm signer
-	tss := mocks.NewTSSMainnet()
+	tss := mocks.NewDerivedTSS(chains.BitcoinMainnet)
 	evmSigner, err := getNewEvmSigner(tss)
 	require.NoError(t, err)
 
@@ -121,8 +119,7 @@ func TestSigner_SignWhitelistERC20Cmd(t *testing.T) {
 		require.NotNil(t, tx)
 
 		// Verify tx signature
-		tss := mocks.NewTSSMainnet()
-		verifyTxSignature(t, tx, tss.Pubkey(), evmSigner.EvmSigner())
+		verifyTxSender(t, tx, tss.EVMAddress(), evmSigner.EvmSigner())
 
 		// Verify tx body basics
 		verifyTxBodyBasics(t, tx, txData.to, txData.nonce, zeroValue)
@@ -178,8 +175,7 @@ func TestSigner_SignMigrateERC20CustodyFundsCmd(t *testing.T) {
 		require.NotNil(t, tx)
 
 		// Verify tx signature
-		tss := mocks.NewTSSMainnet()
-		verifyTxSignature(t, tx, tss.Pubkey(), evmSigner.EvmSigner())
+		verifyTxSender(t, tx, tss.EVMAddress(), evmSigner.EvmSigner())
 
 		// Verify tx body basics
 		verifyTxBodyBasics(t, tx, txData.to, txData.nonce, zeroValue)
@@ -238,8 +234,7 @@ func TestSigner_SignUpdateERC20CustodyPauseStatusCmd(t *testing.T) {
 		require.NotNil(t, tx)
 
 		// Verify tx signature
-		tss := mocks.NewTSSMainnet()
-		verifyTxSignature(t, tx, tss.Pubkey(), evmSigner.EvmSigner())
+		verifyTxSender(t, tx, tss.EVMAddress(), evmSigner.EvmSigner())
 
 		// Verify tx body basics
 		verifyTxBodyBasics(t, tx, txData.to, txData.nonce, zeroValue)
@@ -255,8 +250,7 @@ func TestSigner_SignUpdateERC20CustodyPauseStatusCmd(t *testing.T) {
 		require.NotNil(t, tx)
 
 		// Verify tx signature
-		tss := mocks.NewTSSMainnet()
-		verifyTxSignature(t, tx, tss.Pubkey(), evmSigner.EvmSigner())
+		verifyTxSender(t, tx, tss.EVMAddress(), evmSigner.EvmSigner())
 
 		// Verify tx body basics
 		verifyTxBodyBasics(t, tx, txData.to, txData.nonce, zeroValue)
@@ -293,7 +287,7 @@ func TestSigner_SignMigrateTssFundsCmd(t *testing.T) {
 	ctx := makeCtx(t)
 
 	// Setup evm signer
-	tss := mocks.NewTSSMainnet()
+	tss := mocks.NewDerivedTSS(chains.BitcoinMainnet)
 	evmSigner, err := getNewEvmSigner(tss)
 	require.NoError(t, err)
 
@@ -313,8 +307,7 @@ func TestSigner_SignMigrateTssFundsCmd(t *testing.T) {
 		require.NotNil(t, tx)
 
 		// Verify tx signature
-		tss := mocks.NewTSSMainnet()
-		verifyTxSignature(t, tx, tss.Pubkey(), evmSigner.EvmSigner())
+		verifyTxSender(t, tx, tss.EVMAddress(), evmSigner.EvmSigner())
 
 		// Verify tx body basics
 		verifyTxBodyBasics(t, tx, txData.to, txData.nonce, txData.amount)

diff --git a/zetaclient/chains/evm/signer/signer_test.go b/zetaclient/chains/evm/signer/signer_test.go
@@ -6,9 +6,9 @@ import (
 	"testing"
 
 	sdktypes "github.com/cosmos/cosmos-sdk/types"
+	"github.com/ethereum/go-ethereum/common"
 	ethcommon "github.com/ethereum/go-ethereum/common"
 	ethtypes "github.com/ethereum/go-ethereum/core/types"
-	"github.com/ethereum/go-ethereum/crypto"
 	"github.com/rs/zerolog"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
@@ -113,14 +113,14 @@ func getInvalidCCTX(t *testing.T) *crosschaintypes.CrossChainTx {
 	return cctx
 }
 
-// verifyTxSignature is a helper function to verify the signature of a transaction
-func verifyTxSignature(t *testing.T, tx *ethtypes.Transaction, tssPubkey []byte, signer ethtypes.Signer) {
-	_, r, s := tx.RawSignatureValues()
-	signature := append(r.Bytes(), s.Bytes()...)
-	hash := signer.Hash(tx)
-
-	verified := crypto.VerifySignature(tssPubkey, hash.Bytes(), signature)
-	require.True(t, verified)
+// verifyTxSender is a helper function to verify the signature of a transaction
+//
+// signer.Sender() will ecrecover the public key of the transaction internally
+// and will fail if the transaction is not valid or has been tampered with
+func verifyTxSender(t *testing.T, tx *ethtypes.Transaction, expectedSender common.Address, signer ethtypes.Signer) {
+	senderAddr, err := signer.Sender(tx)
+	require.NoError(t, err)
+	require.Equal(t, expectedSender.String(), senderAddr.String())
 }
 
 // verifyTxBodyBasics is a helper function to verify 'to', 'nonce' and 'amount' of a transaction

diff --git a/zetaclient/testutils/mocks/tss_signer.go b/zetaclient/testutils/mocks/tss_signer.go
@@ -63,6 +63,16 @@ func NewTSSAthens3() *TSS {
 	return NewMockTSS(chains.BscTestnet, testutils.TSSAddressEVMAthens3, testutils.TSSAddressBTCAthens3)
 }
 
+// NewDerivedTSS creates a TSS where evmAddress and btcAdresses are always derived from the test
+// private key
+func NewDerivedTSS(chain chains.Chain) *TSS {
+	return &TSS{
+		paused:  false,
+		chain:   chain,
+		PrivKey: TestPrivateKey,
+	}
+}
+
 func NewGeneratedTSS(t *testing.T, chain chains.Chain) *TSS {
 	pk, err := crypto.GenerateKey()
 	require.NoError(t, err)