ecrecover edge case chains fix (#58)

* Added edge case test and refactored logic

* Wip specific tests still failing

* Update invalid tests to use actual invalid chainid

@austinabell made this change

core/types/transaction_signing.go

@@ -163,7 +163,7 @@ func (s ChainIdSigner) PublicKey(tx *Transaction) ([]byte, error) {
 		return nil, ErrInvalidChainId
 	}
 
-	V := normaliseV(s, tx.data.V)
+	V := normaliseV(s, tx.data.V) - 27
 	if !crypto.ValidateSignatureValues(V, tx.data.R, tx.data.S, true) {
 		return nil, ErrInvalidSig
 	}
@@ -173,7 +173,7 @@ func (s ChainIdSigner) PublicKey(tx *Transaction) ([]byte, error) {
 	sig := make([]byte, 65)
 	copy(sig[32-len(R):32], R)
 	copy(sig[64-len(S):64], S)
-	sig[64] = V - 27
+	sig[64] = V
 
 	// recover the public key from the signature
 	hash := s.Hash(tx)
@@ -275,7 +275,7 @@ func (fs BasicSigner) PublicKey(tx *Transaction) ([]byte, error) {
 		return nil, ErrInvalidSig
 	}
 
-	V := byte(tx.data.V.Uint64())
+	V := byte(tx.data.V.Uint64()) - 27
 	if !crypto.ValidateSignatureValues(V, tx.data.R, tx.data.S, false) {
 		return nil, ErrInvalidSig
 	}
@@ -284,7 +284,7 @@ func (fs BasicSigner) PublicKey(tx *Transaction) ([]byte, error) {
 	sig := make([]byte, 65)
 	copy(sig[32-len(r):32], r)
 	copy(sig[64-len(s):64], s)
-	sig[64] = V - 27
+	sig[64] = V
 
 	// recover the public key from the snature
 	hash := fs.Hash(tx)

core/vm/contracts.go

@@ -218,8 +218,7 @@ func ecrecoverFunc(in []byte) ([]byte, error) {
 	r := new(big.Int).SetBytes(in[64:96])
 	s := new(big.Int).SetBytes(in[96:128])
 	// Treat V as a 256bit integer
-	vbig := new(big.Int).SetBytes(in[32:64])
-	v := byte(vbig.Uint64())
+	v := in[63] - 27
 
 	// tighter sig s values in homestead only apply to tx sigs
 	if !allZero(in[32:63]) || !crypto.ValidateSignatureValues(v, r, s, false) {
@@ -228,10 +227,7 @@ func ecrecoverFunc(in []byte) ([]byte, error) {
 	}
 
 	// v needs to be at the end and normalized for libsecp256k1
-	vbignormal := new(big.Int).Sub(vbig, big.NewInt(27))
-	vnormal := byte(vbignormal.Uint64())
-	rsv := append(in[64:128], vnormal)
-	pubKey, err := crypto.Ecrecover(in[:32], rsv)
+	pubKey, err := crypto.Ecrecover(in[:32], append(in[64:128], v))
 	// make sure the public key is a valid one
 	if err != nil {
 		glog.V(logger.Detail).Infoln("ECRECOVER error: ", err)

core/vm/contracts_test.go

@@ -0,0 +1,30 @@
+package vm
+
+import (
+	"testing"
+
+	"github.com/eth-classic/go-ethereum/common"
+)
+
+type precompiledTest struct {
+	input, expected string
+	gas             uint64
+}
+
+func testEcRecover(test precompiledTest, t *testing.T) {
+	in := common.Hex2Bytes(test.input)
+	if res, err := ecrecoverFunc(in); err != nil {
+		t.Error(err)
+	} else if common.Bytes2Hex(res) != test.expected {
+		t.Errorf("Expected %v, got %v", test.expected, common.Bytes2Hex(res))
+	}
+}
+
+func TestPrecompiledEcRecover(t *testing.T) {
+	test := precompiledTest{
+		input:    "38d18acb67d25c8bb9942764b62f18e17054f66a817bd4295423adf9ed98873e000000000000000000000000000000000000000000000000000000000000001b38d18acb67d25c8bb9942764b62f18e17054f66a817bd4295423adf9ed98873e789d1dd423d25f0772d2748d60f7e4b81bb14d086eba8e8e8efb6dcff8a4ae02",
+		expected: "000000000000000000000000ceaccac640adf55b2028469bd36ba501f28b699d",
+	}
+
+	testEcRecover(test, t)
+}

crypto/crypto.go

@@ -173,7 +173,7 @@ func ValidateSignatureValues(v byte, r, s *big.Int, homestead bool) bool {
 	if s.Cmp(secp256k1.N) >= 0 {
 		return false
 	}
-	if r.Cmp(secp256k1.N) < 0 && (vint == 27 || vint == 28) {
+	if r.Cmp(secp256k1.N) < 0 && (vint == 0 || vint == 1) {
 		return true
 	} else {
 		return false

crypto/crypto_test.go

@@ -226,38 +226,38 @@ func TestValidateSignatureValues(t *testing.T) {
 	secp256k1nMinus1 := new(big.Int).Sub(secp256k1.N, common.Big1)
 
 	// correct v,r,s
-	check(true, 27, one, one)
-	check(true, 28, one, one)
+	check(true, 0, one, one)
+	check(true, 1, one, one)
 	// incorrect v, correct r,s,
 	check(false, 30, one, one)
 	check(false, 26, one, one)
 
 	// incorrect v, combinations of incorrect/correct r,s at lower limit
+	check(false, 3, zero, zero)
+	check(false, 3, zero, one)
+	check(false, 3, one, zero)
+	check(false, 3, one, one)
+
+	// correct v for any combination of incorrect r,s
 	check(false, 0, zero, zero)
 	check(false, 0, zero, one)
 	check(false, 0, one, zero)
-	check(false, 0, one, one)
-
-	// correct v for any combination of incorrect r,s
-	check(false, 27, zero, zero)
-	check(false, 27, zero, one)
-	check(false, 27, one, zero)
 
-	check(false, 28, zero, zero)
-	check(false, 28, zero, one)
-	check(false, 28, one, zero)
+	check(false, 1, zero, zero)
+	check(false, 1, zero, one)
+	check(false, 1, one, zero)
 
 	// correct sig with max r,s
-	check(true, 27, secp256k1nMinus1, secp256k1nMinus1)
+	check(true, 0, secp256k1nMinus1, secp256k1nMinus1)
 	// correct v, combinations of incorrect r,s at upper limit
-	check(false, 27, secp256k1.N, secp256k1nMinus1)
-	check(false, 27, secp256k1nMinus1, secp256k1.N)
-	check(false, 27, secp256k1.N, secp256k1.N)
+	check(false, 0, secp256k1.N, secp256k1nMinus1)
+	check(false, 0, secp256k1nMinus1, secp256k1.N)
+	check(false, 0, secp256k1.N, secp256k1.N)
 
 	// current callers ensures r,s cannot be negative, but let's test for that too
 	// as crypto package could be used stand-alone
-	check(false, 27, minusOne, one)
-	check(false, 27, one, minusOne)
+	check(false, 0, minusOne, one)
+	check(false, 0, one, minusOne)
 }
 
 func checkhash(t *testing.T, name string, f func([]byte) []byte, msg, exp []byte) {