From 644f9069664b107ca0cd208cb8bd0f57ef0df2b4 Mon Sep 17 00:00:00 2001
From: Facundo Medica <14063057+facundomedica@users.noreply.github.com>
Date: Wed, 23 Nov 2022 14:16:00 -0300
Subject: [PATCH] refactor: (secp256k1) cleaner way to check for lower S form
 signatures (#13884)

## Description

TLDR; check for `s.IsOverHalfOrder()` with less steps

Before this PR:

```go
        // parse the signature:
	signature := signatureFromBytes(sigStr)
	// Reject malleable signatures. libsecp256k1 does this check but btcec doesn't.
	// see: https://github.com/ethereum/go-ethereum/blob/f9401ae011ddf7f8d2d95020b7446c17f8d98dc1/crypto/signature_nocgo.go#L90-L93
	// Serialize() would negate S value if it is over half order.
	// Hence, if the signature is different after Serialize() if should be rejected.
	modifiedSignature, parseErr := ecdsa.ParseDERSignature(signature.Serialize())
	if parseErr != nil {
		return false
	}
	if !signature.IsEqual(modifiedSignature) {
		return false
	}

```

It's serializing the signature into a new variable and then comparing if both are equal. Inside `Serialize()` we have:

https://github.com/decred/dcrd/blob/5d537320a0fe2357daf444cc12c62680579689da/dcrec/secp256k1/ecdsa/signature.go#L88-L95

```go
	// Ensure the S component of the signature is less than or equal to the half
	// order of the group because both S and its negation are valid signatures
	// modulo the order, so this forces a consistent choice to reduce signature
	// malleability.
	sigS := new(secp256k1.ModNScalar).Set(&sig.s)
	if sigS.IsOverHalfOrder() {
		sigS.Negate()
	}
```

Before btcec update to v2:

Until btcec update this was simpler because S was exported in the signature: https://github.com/cosmos/cosmos-sdk/commit/ed9cd41396815bd236770f73bb30e083e63e8b64#diff-30a6b594e157a12a28bf8a26f65ab4de1dc3c65c8419e5756f0b9c25d9ce1a93L46

```go
if signature.S.Cmp(secp256k1halfN) > 0 {
    return false
}
```


Closes: #XXXX



---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
---
 crypto/keys/secp256k1/secp256k1_nocgo.go | 25 ++++++++++++------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/crypto/keys/secp256k1/secp256k1_nocgo.go b/crypto/keys/secp256k1/secp256k1_nocgo.go
index ac7f521488..e28baca3ee 100644
--- a/crypto/keys/secp256k1/secp256k1_nocgo.go
+++ b/crypto/keys/secp256k1/secp256k1_nocgo.go
@@ -4,6 +4,8 @@
 package secp256k1
 
 import (
+	"errors"
+
 	secp256k1 "github.com/btcsuite/btcd/btcec/v2"
 	"github.com/btcsuite/btcd/btcec/v2/ecdsa"
 
@@ -32,17 +34,9 @@ func (pubKey *PubKey) VerifySignature(msg []byte, sigStr []byte) bool {
 	if err != nil {
 		return false
 	}
-	// parse the signature:
-	signature := signatureFromBytes(sigStr)
-	// Reject malleable signatures. libsecp256k1 does this check but btcec doesn't.
-	// see: https://github.com/ethereum/go-ethereum/blob/f9401ae011ddf7f8d2d95020b7446c17f8d98dc1/crypto/signature_nocgo.go#L90-L93
-	// Serialize() would negate S value if it is over half order.
-	// Hence, if the signature is different after Serialize() if should be rejected.
-	modifiedSignature, parseErr := ecdsa.ParseDERSignature(signature.Serialize())
-	if parseErr != nil {
-		return false
-	}
-	if !signature.IsEqual(modifiedSignature) {
+	// parse the signature, will return error if it is not in lower-S form
+	signature, err := signatureFromBytes(sigStr)
+	if err != nil {
 		return false
 	}
 	return signature.Verify(crypto.Sha256(msg), pub)
@@ -50,10 +44,15 @@ func (pubKey *PubKey) VerifySignature(msg []byte, sigStr []byte) bool {
 
 // Read Signature struct from R || S. Caller needs to ensure
 // that len(sigStr) == 64.
-func signatureFromBytes(sigStr []byte) *ecdsa.Signature {
+// Rejects malleable signatures (if S value if it is over half order).
+func signatureFromBytes(sigStr []byte) (*ecdsa.Signature, error) {
 	var r secp256k1.ModNScalar
 	r.SetByteSlice(sigStr[:32])
 	var s secp256k1.ModNScalar
 	s.SetByteSlice(sigStr[32:64])
-	return ecdsa.NewSignature(&r, &s)
+	if s.IsOverHalfOrder() {
+		return nil, errors.New("signature is not in lower-S form")
+	}
+
+	return ecdsa.NewSignature(&r, &s), nil
 }