Fix for signature malleability (#1335)

* Fix for signature malleability

* Update go.mod

* Add test

* Improve test

* Add comments

* Add command for scan beefy signatures

* Ignore zero prefix error

* Fix test

* More logs

* Update relayer/relays/beefy/parameters.go

Co-authored-by: Alistair Singh <[email protected]>

* Update relayer/relays/beefy/parameters.go

Co-authored-by: Alistair Singh <[email protected]>

* Test ecrecover with malleable signature

* Update beefy config

---------

Co-authored-by: Alistair Singh <[email protected]>

go.work.sum

@@ -100,7 +100,6 @@ github.com/fatih/structs v1.1.0 h1:Q7juDM0QtcnhCpeyLGQKyg4TOIghuNXrkL32pHAUMxo=
 github.com/fatih/structs v1.1.0/go.mod h1:9NiDSp5zOcgEDl+j00MP/WkGVPOlPRLejGD8Ga6PJ7M=
 github.com/fjl/gencodec v0.0.0-20230517082657-f9840df7b83e h1:bBLctRc7kr01YGvaDfgLbTwjFNW5jdp5y5rj8XXBHfY=
 github.com/fjl/gencodec v0.0.0-20230517082657-f9840df7b83e/go.mod h1:AzA8Lj6YtixmJWL+wkKoBGsLWy9gFrAzi4g+5bCKwpY=
-github.com/fjl/memsize v0.0.2/go.mod h1:VvhXpOYNQvB+uIk2RvXzuaQtkQJzzIx6lSBe1xv7hi0=
 github.com/flosch/pongo2/v4 v4.0.2 h1:gv+5Pe3vaSVmiJvh/BZa82b7/00YUGm0PIyVVLop0Hw=
 github.com/flosch/pongo2/v4 v4.0.2/go.mod h1:B5ObFANs/36VwxxlgKpdchIJHMvHB562PW+BWPhwZD8=
 github.com/garslo/gogen v0.0.0-20170306192744-1d203ffc1f61 h1:IZqZOB2fydHte3kUgxrzK5E1fW7RQGeDwE8F/ZZnUYc=
@@ -173,7 +172,8 @@ github.com/hashicorp/golang-lru v0.5.4 h1:YDjusn29QI/Das2iO9M0BHnIbxPeyuCHsjMW+l
 github.com/hashicorp/golang-lru v0.5.4/go.mod h1:iADmTwqILo4mZ8BN3D2Q6+9jd8WM5uGBxy+E8yxSoD4=
 github.com/hashicorp/serf v0.10.1 h1:Z1H2J60yRKvfDYAOZLd2MU0ND4AH/WDz7xYHDWQsIPY=
 github.com/hashicorp/serf v0.10.1/go.mod h1:yL2t6BqATOLGc5HF7qbFkTfXoPIY0WZdWHfEvMqbG+4=
-github.com/holiman/billy v0.0.0-20240216141850-2abb0c79d3c4/go.mod h1:5GuXa7vkL8u9FkFuWdVvfR5ix8hRB7DbOAaYULamFpc=
+github.com/holiman/uint256 v1.3.1 h1:JfTzmih28bittyHM8z360dCjIA9dbPIBlcTI6lmctQs=
+github.com/holiman/uint256 v1.3.1/go.mod h1:EOMSn4q6Nyt9P6efbI3bueV4e1b3dGlUCXeiRV4ng7E=
 github.com/hpcloud/tail v1.0.0 h1:nfCOvKYfkgYP8hkirhJocXT2+zOD8yUNjXaWfTlyFKI=
 github.com/hydrogen18/memlistener v1.0.0 h1:JR7eDj8HD6eXrc5fWLbSUnfcQFL06PYvCc0DKQnWfaU=
 github.com/hydrogen18/memlistener v1.0.0/go.mod h1:qEIFzExnS6016fRpRfxrExeVn2gbClQA99gQhnIcdhE=

relayer/cmd/root.go

@@ -22,9 +22,9 @@ var rootCmd = &cobra.Command{
 func init() {
 	rootCmd.AddCommand(run.Command())
 	rootCmd.AddCommand(getBlockCmd())
-	//rootCmd.AddCommand(fetchMessagesCmd())
 	rootCmd.AddCommand(subBeefyCmd())
 	rootCmd.AddCommand(scanBeefyCmd())
+	rootCmd.AddCommand(scanSingleBeefyBlockCmd())
 	rootCmd.AddCommand(leafCmd())
 	rootCmd.AddCommand(basicChannelLeafProofCmd())
 	rootCmd.AddCommand(parachainHeadProofCmd())

relayer/cmd/scan_beefy.go

@@ -2,14 +2,17 @@ package cmd
 
 import (
 	"context"
+	"fmt"
 	"log"
 	"os"
 	"os/signal"
 	"syscall"
 
 	"github.com/sirupsen/logrus"
+	"github.com/snowfork/go-substrate-rpc-client/v4/types"
 	"github.com/snowfork/snowbridge/relayer/chain/relaychain"
 	"github.com/snowfork/snowbridge/relayer/relays/beefy"
+	"github.com/snowfork/snowbridge/relayer/relays/util"
 	"github.com/spf13/cobra"
 	"golang.org/x/sync/errgroup"
 )
@@ -25,9 +28,6 @@ func scanBeefyCmd() *cobra.Command {
 	cmd.Flags().StringP("polkadot-url", "p", "ws://127.0.0.1:9944", "Polkadot URL.")
 	cmd.Flags().Uint64P("beefy-block", "b", 0, "Beefy block.")
 	cmd.MarkFlagRequired("beefy-block")
-	cmd.Flags().Uint64P("validator-set-id", "v", 0, "Validator set id.")
-	cmd.MarkFlagRequired("validator-set-id")
-	cmd.Flags().Uint64P("fast-forward-depth", "f", 10000, "Fast forward depth.")
 	return cmd
 }
 
@@ -43,24 +43,19 @@ func ScanBeefyFn(cmd *cobra.Command, _ []string) error {
 	relaychainConn := relaychain.NewConnection(polkadotUrl)
 	relaychainConn.Connect(ctx)
 
-	fastForwardDepth, _ := cmd.Flags().GetUint64("fast-forward-depth")
-	config := beefy.SourceConfig{
-		FastForwardDepth: fastForwardDepth,
-	}
+	config := beefy.SourceConfig{}
 	polkadotListener := beefy.NewPolkadotListener(
 		&config,
 		relaychainConn,
 	)
 
 	beefyBlock, _ := cmd.Flags().GetUint64("beefy-block")
-	validatorSetID, _ := cmd.Flags().GetUint64("validator-set-id")
 	logrus.WithFields(logrus.Fields{
-		"polkadot-url":     polkadotUrl,
-		"beefy-block":      beefyBlock,
-		"validator-set-id": validatorSetID,
+		"polkadot-url": polkadotUrl,
+		"beefy-block":  beefyBlock,
 	}).Info("Connected to relaychain.")
 
-	commitments, err := polkadotListener.Start(ctx, eg, beefyBlock, validatorSetID)
+	commitments, err := polkadotListener.Start(ctx, eg, beefyBlock)
 	if err != nil {
 		logrus.WithError(err).Fatalf("could not start")
 	}
@@ -103,3 +98,93 @@ func ScanBeefyFn(cmd *cobra.Command, _ []string) error {
 
 	return nil
 }
+
+func scanSingleBeefyBlockCmd() *cobra.Command {
+	cmd := &cobra.Command{
+		Use:   "scan-single-beefy-block",
+		Short: "Scan a single block which contains beefy commitment",
+		Args:  cobra.ExactArgs(0),
+		RunE:  ScanSingleBeefyBlockFn,
+	}
+
+	cmd.Flags().StringP("polkadot-url", "p", "ws://127.0.0.1:9944", "Polkadot URL.")
+	cmd.Flags().Uint64P("beefy-block", "b", 0, "Beefy block.")
+	cmd.MarkFlagRequired("beefy-block")
+	return cmd
+}
+
+func ScanSingleBeefyBlockFn(cmd *cobra.Command, _ []string) error {
+	ctx := cmd.Context()
+	log.SetOutput(logrus.WithFields(logrus.Fields{"logger": "stdlib"}).WriterLevel(logrus.InfoLevel))
+	logrus.SetLevel(logrus.DebugLevel)
+
+	polkadotUrl, _ := cmd.Flags().GetString("polkadot-url")
+	relaychainConn := relaychain.NewConnection(polkadotUrl)
+	err := relaychainConn.Connect(ctx)
+	if err != nil {
+		fmt.Errorf("connect: %w", err)
+		return err
+	}
+	api := relaychainConn.API()
+	// metadata := relaychainConn.Metadata()
+
+	beefyBlockNumber, _ := cmd.Flags().GetUint64("beefy-block")
+	logrus.WithFields(logrus.Fields{
+		"polkadot-url": polkadotUrl,
+		"beefy-block":  beefyBlockNumber,
+	}).Info("Connected to relaychain.")
+
+	beefyBlockHash, err := api.RPC.Chain.GetBlockHash(beefyBlockNumber)
+	if err != nil {
+		return fmt.Errorf("fetch hash: %w", err)
+	}
+
+	beefyBlock, err := api.RPC.Chain.GetBlock(beefyBlockHash)
+	if err != nil {
+		return fmt.Errorf("fetch block: %w", err)
+	}
+
+	var commitment *types.SignedCommitment
+	for j := range beefyBlock.Justifications {
+		sc := types.OptionalSignedCommitment{}
+		if beefyBlock.Justifications[j].EngineID() == "BEEF" {
+			err := types.DecodeFromBytes(beefyBlock.Justifications[j].Payload(), &sc)
+			if err != nil {
+				return fmt.Errorf("decode BEEFY signed commitment: %w", err)
+			}
+			ok, value := sc.Unwrap()
+			if ok {
+				commitment = &value
+			}
+		}
+	}
+	if commitment == nil {
+		return fmt.Errorf("beefy block without a valid commitment")
+	}
+	if len(commitment.Signatures) == 0 {
+		return fmt.Errorf("no signature in the commitment")
+	}
+	var emptyNum uint
+	var errNum uint
+	var revertedNum uint
+	for _, s := range commitment.Signatures {
+		ok, beefySig := s.Unwrap()
+		if !ok {
+			emptyNum++
+			continue
+		}
+		sBefore := util.BytesToHexString(beefySig[32:64])
+		_, _, s, reverted, err := beefy.CleanSignature(beefySig)
+		if err != nil {
+			logrus.WithError(err).Warn("cleanSignature")
+			errNum++
+		}
+		sAfter := util.BytesToHexString(s[:])
+		if reverted {
+			revertedNum++
+			logrus.Info(fmt.Sprintf("s is reverted, before clean:%s, after clean:%s", sBefore, sAfter))
+		}
+	}
+	logrus.Info(fmt.Sprintf("number of total signatures:%d,empty signatures:%d,invalid signatures:%d,reverted signatures:%d", len(commitment.Signatures), emptyNum, errNum, revertedNum))
+	return nil
+}

relayer/go.mod

@@ -20,6 +20,7 @@ require (
 	github.com/stretchr/testify v1.8.4
 	golang.org/x/exp v0.0.0-20240110193028-0dcbfd608b1e
 	golang.org/x/sync v0.6.0
+	github.com/holiman/uint256 v1.3.1
 )
 
 require (

relayer/relays/beefy/config.go

@@ -2,6 +2,7 @@ package beefy
 
 import (
 	"fmt"
+
 	"github.com/snowfork/snowbridge/relayer/config"
 )
 
@@ -12,10 +13,6 @@ type Config struct {
 
 type SourceConfig struct {
 	Polkadot config.PolkadotConfig `mapstructure:"polkadot"`
-	// Depth to ignore the beefy updates too far away (in number of blocks)
-	FastForwardDepth uint64 `mapstructure:"fast-forward-depth"`
-	// Period to sample the beefy updates (in number of blocks)
-	UpdatePeriod uint64 `mapstructure:"update-period"`
 }
 
 type SinkConfig struct {

relayer/relays/beefy/main.go

@@ -68,7 +68,7 @@ func (relay *Relay) Start(ctx context.Context, eg *errgroup.Group) error {
 		"validatorSetID": initialState.CurrentValidatorSetID,
 	}).Info("Retrieved current BeefyClient state")
 
-	requests, err := relay.polkadotListener.Start(ctx, eg, initialState.LatestBeefyBlock, initialState.CurrentValidatorSetID)
+	requests, err := relay.polkadotListener.Start(ctx, eg, initialState.LatestBeefyBlock)
 	if err != nil {
 		return fmt.Errorf("initialize polkadot listener: %w", err)
 	}

relayer/relays/beefy/parameters.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"math/big"
 
+	"github.com/holiman/uint256"
 	"github.com/sirupsen/logrus"
 	"github.com/snowfork/go-substrate-rpc-client/v4/types"
 	"github.com/snowfork/snowbridge/relayer/contracts"
@@ -63,7 +64,10 @@ func (r *Request) MakeSubmitInitialParams(valAddrIndex int64, initialBitfield []
 		return nil, fmt.Errorf("convert to ethereum address: %w", err)
 	}
 
-	v, _r, s := cleanSignature(validatorSignature)
+	v, _r, s, _, err := CleanSignature(validatorSignature)
+	if err != nil {
+		return nil, fmt.Errorf("clean signature: %w", err)
+	}
 
 	msg := InitialRequestParams{
 		Commitment: *commitment,
@@ -89,13 +93,43 @@ func toBeefyClientCommitment(c *types.Commitment) *contracts.BeefyClientCommitme
 	}
 }
 
-func cleanSignature(input types.BeefySignature) (uint8, [32]byte, [32]byte) {
+// Implementation derived from https://github.com/OpenZeppelin/openzeppelin-contracts/blob/5bb3f3e788c6b2c806d562ef083b438354f969d7/contracts/utils/cryptography/ECDSA.sol#L139-L145
+func CleanSignature(input types.BeefySignature) (v uint8, r [32]byte, s [32]byte, reverted bool, err error) {
 	// Update signature format (Polkadot uses recovery IDs 0 or 1, Eth uses 27 or 28, so we need to add 27)
 	// Split signature into r, s, v and add 27 to v
-	r := *(*[32]byte)(input[:32])
-	s := *(*[32]byte)(input[32:64])
-	v := byte(uint8(input[64]) + 27)
-	return v, r, s
+	r = *(*[32]byte)(input[:32])
+	s = *(*[32]byte)(input[32:64])
+	v = uint8(input[64])
+	if v < 27 {
+		v += 27
+	}
+	if v != 27 && v != 28 {
+		return v, r, s, reverted, fmt.Errorf("invalid V:%d", v)
+	}
+	var N *uint256.Int = uint256.NewInt(0)
+	N.SetFromHex("0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141")
+	var halfN *uint256.Int = uint256.NewInt(0)
+	halfN.SetFromHex("0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0")
+	var s256 *uint256.Int = uint256.NewInt(0)
+	err = s256.SetFromHex(util.BytesToHexString(s[:]))
+	if err != nil && err != uint256.ErrLeadingZero {
+		return v, r, s, reverted, fmt.Errorf("invalid S:%s,error is:%w", util.BytesToHexString(s[:]), err)
+	}
+	// If polkadot library generates malleable signatures, such as s-values in the upper range, calculate a new s-value
+	// with 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141 - s1 and flip v from 27 to 28 or
+	// vice versa.
+	if s256.Gt(halfN) {
+		var negativeS256 *uint256.Int = uint256.NewInt(0)
+		negativeS256 = negativeS256.Sub(N, s256)
+		s = negativeS256.Bytes32()
+		if v%2 == 0 {
+			v = v - 1
+		} else {
+			v = v + 1
+		}
+		reverted = true
+	}
+	return v, r, s, reverted, nil
 }
 
 func (r *Request) generateValidatorAddressProof(validatorIndex int64) ([][32]byte, error) {
@@ -132,7 +166,10 @@ func (r *Request) MakeSubmitFinalParams(validatorIndices []uint64, initialBitfie
 			return nil, fmt.Errorf("signature is empty")
 		}
 
-		v, _r, s := cleanSignature(beefySig)
+		v, _r, s, _, err := CleanSignature(beefySig)
+		if err != nil {
+			return nil, fmt.Errorf("clean signature: %w", err)
+		}
 		account, err := r.Validators[validatorIndex].IntoEthereumAddress()
 		if err != nil {
 			return nil, fmt.Errorf("convert to ethereum address: %w", err)

relayer/relays/beefy/parameters_test.go

@@ -0,0 +1,68 @@
+package beefy
+
+import (
+	"testing"
+
+	"github.com/ethereum/go-ethereum/crypto"
+	"github.com/snowfork/go-substrate-rpc-client/v4/types"
+	"github.com/snowfork/snowbridge/relayer/relays/util"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestCleanSignatureNochange(t *testing.T) {
+	hash, _ := util.HexStringTo32Bytes("0x3ea2f1d0abf3fc66cf29eebb70cbd4e7fe762ef8a09bcc06c8edf641230afec0")
+	r, _ := util.HexStringTo32Bytes("0xc1d9e2b5dd63860d27c38a8b276e5a5ab5e19a97452b0cb24094613bcbd517d8")
+	s, _ := util.HexStringTo32Bytes("0x6dc0d1a7743c3328bfcfe05a2f8691e114f9143776a461ddad6e8b858bb19c1d")
+	v := uint8(1)
+	signature := buildSignature(v, r, s)
+	publicKey, err := crypto.Ecrecover(hash[:], signature[:])
+	if err != nil {
+		t.Fatal(err)
+	}
+	assert.Equal(t, len(publicKey), 65)
+	vAfter, rAfter, sAfter, reverted, err := CleanSignature(signature)
+	if err != nil {
+		t.Fatal(err)
+	}
+	assert.Equal(t, reverted, false)
+	assert.Equal(t, vAfter, v+27)
+	assert.Equal(t, rAfter, r)
+	assert.Equal(t, sAfter, s)
+}
+
+func TestCleanSignatureWithSConverted(t *testing.T) {
+	hash, _ := util.HexStringTo32Bytes("0x3ea2f1d0abf3fc66cf29eebb70cbd4e7fe762ef8a09bcc06c8edf641230afec0")
+	r, _ := util.HexStringTo32Bytes("0xc1d9e2b5dd63860d27c38a8b276e5a5ab5e19a97452b0cb24094613bcbd517d8")
+	s, _ := util.HexStringTo32Bytes("0x6dc0d1a7743c3328bfcfe05a2f8691e114f9143776a461ddad6e8b858bb19c1d")
+	v := uint8(1)
+	signature := buildSignature(v, r, s)
+	publicKey, err := crypto.Ecrecover(hash[:], signature[:])
+	if err != nil {
+		t.Fatal(err)
+	}
+	negativeS, _ := util.HexStringTo32Bytes("0x923f2e588bc3ccd740301fa5d0796e1da5b5c8af38a43e5e1263d3074484a524")
+	negativeV := byte(0)
+	negativeSignature := buildSignature(negativeV, r, negativeS)
+	vAfter, rAfter, sAfter, reverted, err := CleanSignature(negativeSignature)
+	if err != nil {
+		t.Fatal(err)
+	}
+	assert.Equal(t, reverted, true)
+	assert.Equal(t, vAfter, v+27)
+	assert.Equal(t, rAfter, r)
+	assert.Equal(t, sAfter, s)
+	publicKeyAfter, err := crypto.Ecrecover(hash[:], negativeSignature[:])
+	if err != nil {
+		t.Fatal(err)
+	}
+	assert.Equal(t, len(publicKeyAfter), 65)
+	assert.Equal(t, publicKey, publicKeyAfter)
+}
+
+func buildSignature(v uint8, r [32]byte, s [32]byte) (signature types.BeefySignature) {
+	var input []byte
+	input = append(r[:], s[:]...)
+	input = append(input, v)
+	copy(signature[:], input)
+	return signature
+}

relayer/relays/beefy/polkadot-listener.go

@@ -32,13 +32,12 @@ func (li *PolkadotListener) Start(
 	ctx context.Context,
 	eg *errgroup.Group,
 	currentBeefyBlock uint64,
-	currentValidatorSetID uint64,
 ) (<-chan Request, error) {
 	requests := make(chan Request, 1)
 
 	eg.Go(func() error {
 		defer close(requests)
-		err := li.scanCommitments(ctx, currentBeefyBlock, currentValidatorSetID, requests)
+		err := li.scanCommitments(ctx, currentBeefyBlock, requests)
 		if err != nil {
 			return err
 		}
@@ -51,7 +50,6 @@ func (li *PolkadotListener) Start(
 func (li *PolkadotListener) scanCommitments(
 	ctx context.Context,
 	currentBeefyBlock uint64,
-	currentValidatorSet uint64,
 	requests chan<- Request,
 ) error {
 	in, err := ScanCommitments(ctx, li.conn.Metadata(), li.conn.API(), currentBeefyBlock+1)
@@ -92,7 +90,6 @@ func (li *PolkadotListener) scanCommitments(
 					"validatorSetID":     validatorSetID,
 					"nextValidatorSetID": nextValidatorSetID,
 				},
-				"validatorSetID": currentValidatorSet,
 			}).Info("Sending BEEFY commitment to ethereum writer")
 
 			select {