Merge pull request #273 from ava-labs/kms-signature-fix

EIP-2 cap s-value
ava-labs · May 1, 2024 · c74441a · c74441a
2 parents 366a137 + 0b94adc
commit c74441a
Show file tree

Hide file tree

Showing 2 changed files with 88 additions and 5 deletions.
diff --git a/vms/evm/signer/kms_signer.go b/vms/evm/signer/kms_signer.go
@@ -109,22 +109,31 @@ func (s *KMSSigner) SignTx(tx *types.Transaction, evmChainID *big.Int) (*types.T
 	if err != nil {
 		return nil, err
 	}
-
-	sigBytes, err := s.getEthereumSignature(h, sigAsn1.R.Bytes, sigAsn1.S.Bytes)
+	sigBytes, err := s.recoverEIP155Signature(h, sigAsn1.R.Bytes, sigAsn1.S.Bytes)
 	if err != nil {
 		return nil, err
 	}
+
 	return tx.WithSignature(signer, sigBytes)
 }
 
 func (s *KMSSigner) Address() common.Address {
 	return s.eoa
 }
 
-// Recover the EIP-155 signature from the KMS signature
+// Recover the EIP-155 signature from the KMS signature.
 // KMS returns the signature in ASN.1 format, but the EIP-155 signature is in R || S || V format,
-// so we need to test both V = 0 and V = 1 against the recovered public key
-func (s *KMSSigner) getEthereumSignature(txHash []byte, rBytes []byte, sBytes []byte) ([]byte, error) {
+// so we need to test both V = 0 and V = 1 against the recovered public key.
+// Additionally, EIP-2 S-values are capped at secp256k1n/2, so adjust that if necessary.
+func (s *KMSSigner) recoverEIP155Signature(txHash []byte, rBytes []byte, sBytes []byte) ([]byte, error) {
+	sBigInt := big.NewInt(0).SetBytes(sBytes)
+	secp256k1N := crypto.S256().Params().N
+	secp256k1HalfN := big.NewInt(0).Div(secp256k1N, big.NewInt(2))
+
+	if sBigInt.Cmp(secp256k1HalfN) > 0 {
+		sBytes = big.NewInt(0).Sub(secp256k1N, sBigInt).Bytes()
+	}
+
 	rsSignature := append(adjustSignatureLength(rBytes), adjustSignatureLength(sBytes)...)
 	if len(rsSignature) != signatureLength {
 		return nil, errors.New("rs signature length is not 64 bytes")

diff --git a/vms/evm/signer/kms_signer_test.go b/vms/evm/signer/kms_signer_test.go
@@ -0,0 +1,74 @@
+package signer
+
+import (
+	"testing"
+
+	"github.com/ethereum/go-ethereum/common"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestRecoverEIP155Signature(t *testing.T) {
+	testCases := []struct {
+		name              string
+		txHash            string
+		rValue            string
+		sValue            string
+		pubKey            string
+		expectedSignature string
+		expectedError     bool
+	}{
+		{
+			name:              "valid signature",
+			txHash:            "69963cf4839e149fea0e7b0969dd6834ea4b22fa7f9209a46683982320e5edfd",
+			rValue:            "00a94bfca53b42454acc43e7328ce7a8f244a629026095c36c0ee82607377cbee4",
+			sValue:            "5964b0dd9bf23723570b6a073cb945fd1ba853ebf5579c8d11bb4fdb305cd079",
+			pubKey:            "04a3b664aa1f37bf6c46a3f2cdb209091e16070208b30244838e2cb9fe1465c4911ba2ab0c54bfc39972740ef7b24904f8740b0a69aca2bbfce0f2829429b9a5c5",
+			expectedSignature: "a94bfca53b42454acc43e7328ce7a8f244a629026095c36c0ee82607377cbee45964b0dd9bf23723570b6a073cb945fd1ba853ebf5579c8d11bb4fdb305cd07901",
+			expectedError:     false,
+		},
+		{
+			name:              "invalid r value",
+			txHash:            "69963cf4839e149fea0e7b0969dd6834ea4b22fa7f9209a46683982320e5edfd",
+			rValue:            "10a94bfca53b42454acc43e7328ce7a8f244a629026095c36c0ee82607377cbee4",
+			sValue:            "5964b0dd9bf23723570b6a073cb945fd1ba853ebf5579c8d11bb4fdb305cd079",
+			pubKey:            "04a3b664aa1f37bf6c46a3f2cdb209091e16070208b30244838e2cb9fe1465c4911ba2ab0c54bfc39972740ef7b24904f8740b0a69aca2bbfce0f2829429b9a5c5",
+			expectedSignature: "a94bfca53b42454acc43e7328ce7a8f244a629026095c36c0ee82607377cbee45964b0dd9bf23723570b6a073cb945fd1ba853ebf5579c8d11bb4fdb305cd07901",
+			expectedError:     true,
+		},
+		{
+			name:              "invalid s value",
+			txHash:            "69963cf4839e149fea0e7b0969dd6834ea4b22fa7f9209a46683982320e5edfd",
+			rValue:            "00a94bfca53b42454acc43e7328ce7a8f244a629026095c36c0ee82607377cbee4",
+			sValue:            "1964b0dd9bf23723570b6a073cb945fd1ba853ebf5579c8d11bb4fdb305cd079",
+			pubKey:            "04a3b664aa1f37bf6c46a3f2cdb209091e16070208b30244838e2cb9fe1465c4911ba2ab0c54bfc39972740ef7b24904f8740b0a69aca2bbfce0f2829429b9a5c5",
+			expectedSignature: "a94bfca53b42454acc43e7328ce7a8f244a629026095c36c0ee82607377cbee45964b0dd9bf23723570b6a073cb945fd1ba853ebf5579c8d11bb4fdb305cd07901",
+			expectedError:     true,
+		},
+		{
+			name:              "s-value too high",
+			txHash:            "e29dc6b15950a5433e239155a2208156ba8fd81eeb81ade45329d4b2fb2e8421",
+			rValue:            "00d993636ed097bf04b7c982e0394d572ead3c8f23ecc8baba0bbdfaa91aba4376",
+			sValue:            "00d1ac23b517ae2569522626096fa1922681c87612cceac971e855d42103fea7c6", // This is > secp256k1n/2
+			pubKey:            "04a3b664aa1f37bf6c46a3f2cdb209091e16070208b30244838e2cb9fe1465c4911ba2ab0c54bfc39972740ef7b24904f8740b0a69aca2bbfce0f2829429b9a5c5",
+			expectedSignature: "d993636ed097bf04b7c982e0394d572ead3c8f23ecc8baba0bbdfaa91aba43762e53dc4ae851da96add9d9f6905e6dd838e666d3e25dd6c9d77c8a6bcc37997b00",
+			expectedError:     false,
+		},
+	}
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			signer := &KMSSigner{
+				pubKey: common.Hex2Bytes(testCase.pubKey),
+			}
+			txHash := common.Hex2Bytes(testCase.txHash)
+			rValue := common.Hex2Bytes(testCase.rValue)
+			sValue := common.Hex2Bytes(testCase.sValue)
+			signature, err := signer.recoverEIP155Signature(txHash, rValue, sValue)
+			if testCase.expectedError {
+				assert.NotNil(t, err)
+			} else {
+				assert.Nil(t, err)
+				assert.Equal(t, common.Hex2Bytes(testCase.expectedSignature), signature)
+			}
+		})
+	}
+}