ECRecover: missing handling(?) of invalid signature bubble up.

[mratsim]

We are passing the tests in https://github.com/mratsim/constantine/blob/master/tests/protocol_ethereum_evm_precompiles/ecRecover.json but I'm not sure what code path is doing that.

So Looking at the high-level function, it claims that on invalid signature, the neutral element is returned:

constantine/constantine/ethereum_evm_precompiles.nim

Lines 1308 to 1380 in b581c0f

	func eth_evm_ecrecover*(r: var openArray[byte],
	input: openArray[byte]): CttEVMStatus {.libPrefix: prefix_ffi, meter.} =
	## Attempts to recover the public key, which was used to sign the given `data`
	## to obtain the given signature `sig`.
	##
	## If the signature is invalid, the result array `r` will contain the neutral
	## element of the curve.
	##
	## Inputs:
	## - `r`: Array of the recovered public key. An elliptic curve point in affine
	## coordinates (`EC_ShortW_Aff[Fp[Secp256k1], G1]`).
	## - `input`: The input data as an array of 128 bytes. The data is as follows:
	## - 32 byte: `keccak256` digest of the message that was signed
	## - 32 byte: `v`, decides if the even or odd coordinate in `R` was used
	## - 32 byte: `r` of the signature, scalar `Fr[Secp256k1]`
	## - 32 byte: `s` of the signature, scalar `Fr[Secp256k1]`
	##
	## Implementation follows Geth here:
	## https://github.com/ethereum/go-ethereum/blob/341647f1865dab437a690dc1424ba71495de2dd8/core/vm/contracts.go#L243-L272
	##
	## and to a lesser extent the Ethereum Yellow Paper in appendix F:
	## https://ethereum.github.io/yellowpaper/paper.pdf
	##
	## Internal Geth implementation in:
	## https://github.com/ethereum/go-ethereum/blob/master/signer/core/signed_data.go#L292-L319
	if len(input) != 128:
	return cttEVM_InvalidInputSize
	if len(r) != 32:
	return cttEVM_InvalidOutputSize
	# 1. construct message hash as scalar in field `Fr[Secp256k1]`
	var msgBI {.noinit.}: BigInt[256]
	msgBI.unmarshal(input.toOpenArray(0, 32-1), bigEndian)
	var msgHash {.noinit.}: Fr[Secp256k1]
	msgHash.fromBig(msgBI)
	# 2. verify `v` data is valid
	## XXX: Or construct a `BigInt[256]` instead and compare? (or compare with uint64s?)
	for i in 32 ..< 63: # first 31 bytes must be zero for a valid `v`
	if input[i] != byte 0:
	return cttEVM_MalformedSignature
	let v = input[63]
	if v notin [byte 0, 1, 27, 28]:
	return cttEVM_MalformedSignature
	# 2a. determine if even or odd `y` coordinate
	let evenY = v in [byte 0, 27] # 0 / 27 indicates `y` to be even, 1 / 28 odd
	# 3. unmarshal signature data
	var signature {.noinit.}: Signature
	privateAccess(Signature)
	var rSig {.noinit}, sSig {.noinit.}: BigInt[256]
	rSig.unmarshal(input.toOpenArray(64, 96-1), bigEndian)
	sSig.unmarshal(input.toOpenArray(96, 128-1), bigEndian)
	signature.r = Fr[Secp256k1].fromBig(rSig)
	signature.s = Fr[Secp256k1].fromBig(sSig)
	# 4. perform pubkey recovery
	var pubKey {.noinit.}: PublicKey
	pubKey.recoverPubkeyFromDigest(msgHash, signature, evenY)
	# 4. now calculate the Ethereum address of the public key (keccak256)
	privateAccess(PublicKey)
	var rawPubkey {.noinit.}: array[64, byte] # `[x, y]` coordinates of public key
	rawPubkey.toOpenArray( 0, 32-1).marshal(pubKey.raw.x, bigEndian)
	rawPubkey.toOpenArray(32, 64-1).marshal(pubKey.raw.y, bigEndian)
	var dgst {.noinit.}: array[32, byte] # keccak256 digest
	keccak256.hash(dgst, rawPubkey)
	# 5. and effectively truncate to last 20 bytes of digest
	r.rawCopy(12, dgst, 12, 20)
	result = cttEVM_Success

However there is no setZero called at a high-level for the obvious MalformedSignature (this can be handled by caller)
and there is no part of the code that handles seemingly valid inputs revealed invalid by this internal proc:

constantine/constantine/signatures/ecdsa.nim

Lines 312 to 334 in b581c0f

	proc recoverPubkeyImpl_vartime*[Name: static Algebra; Sig](
	recovered: var EC_ShortW_Aff[Fp[Name], G1],
	signature: Sig,
	msgHash: Fr[Name],
	evenY: bool) =
	## Attempts to recover an associated public key to the given `signature` and
	## hash of a message `msgHash`.
	##
	## Note that as the signature is only dependent on the `x` coordinate of the
	## curve point `R`, two public keys verify the signature. The one with even
	## and the one with odd `y` coordinate (one even & one odd due to curve prime
	## order).
	##
	## `evenY` decides whether we recover the public key associated with the even
	## `y` coordinate of `R` or the odd one. Both verify the (message, signature)
	## pair.
	##
	## If the signature is invalid, `recovered` will be set to the neutral element.
	type
	ECAff = EC_ShortW_Aff[Fp[Name], G1]
	ECJac = EC_ShortW_Jac[Fp[Name], G1]
	# 1. Set to neutral so if we don't find a valid signature, return neutral
	recovered.setNeutral()

We go straight to keccak-ing the recovered pubkey without checking if it's the neutral element

constantine/constantine/ethereum_evm_precompiles.nim

Lines 1365 to 1380 in b581c0f

	# 4. perform pubkey recovery
	var pubKey {.noinit.}: PublicKey
	pubKey.recoverPubkeyFromDigest(msgHash, signature, evenY)
	# 4. now calculate the Ethereum address of the public key (keccak256)
	privateAccess(PublicKey)
	var rawPubkey {.noinit.}: array[64, byte] # `[x, y]` coordinates of public key
	rawPubkey.toOpenArray( 0, 32-1).marshal(pubKey.raw.x, bigEndian)
	rawPubkey.toOpenArray(32, 64-1).marshal(pubKey.raw.y, bigEndian)
	var dgst {.noinit.}: array[32, byte] # keccak256 digest
	keccak256.hash(dgst, rawPubkey)
	# 5. and effectively truncate to last 20 bytes of digest
	r.rawCopy(12, dgst, 12, 20)
	result = cttEVM_Success

Instead we should be shortcutting here, and possibly return an error code (and replace MalformedSignature with VerificationFailure)