-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
EcRecover #3633
base: HF_Echidna
Are you sure you want to change the base?
EcRecover #3633
Conversation
var messageHash = message.Sha256(); | ||
var recover = Crypto.ECRecover(Neo.Cryptography.ECC.ECCurve.Secp256k1, signature, messageHash, SignatureFormat.Fixed32); | ||
|
||
recover.Length.Should().Be(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without V we can get more than one address
|
||
// TODO: Not tested, check if it works | ||
case NamedCurveHash.secp256r1Keccak256: | ||
case NamedCurveHash.secp256r1SHA256: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it will work with these curves
/// <param name="curveHash">A pair of the curve to be used by the ECDSA algorithm and the hasher function to be used to hash message.</param> | ||
/// <returns><see langword="true"/> if the signature is valid; otherwise, <see langword="false"/>.</returns> | ||
[ContractMethod(Hardfork.HF_Aspidochelone, CpuFee = 1 << 10)] | ||
public static ECPoint[] ECrecover(byte[] message, byte[] signature, NamedCurveHash curveHash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't have the v
we can get multiples public keys
case NamedCurveHash.secp256k1SHA256: | ||
{ | ||
return Crypto.ECRecover(ECCurve.Secp256k1, signature, messageHash, | ||
// TODO: only accept 65? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
64bytes signatures MAY store a parity bit in the first bit of the s
value which is always 0.
From what I deduce, you assume the parity value to be 0 if the size is 64 so checking the first bit of s
will not change the current behavior if this is not a compressed signature (i.e. parity will always be 0) But in case it is a compressed ERC-2098 signature, it will be supported out-of-the-box
Note: this is predicated on the assumption that N3 contracts might need to verify signatures that originate in NEOX where EIP standards might apply (eg: someone migrates a platform written for Ethereum to NeoX)
for reference, this is the compressed signature standard I am referring to: https://eips.ethereum.org/EIPS/eip-2098
default: throw new InvalidOperationException("Invalid signature format"); | ||
} | ||
|
||
// Validate values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on the topic of validation, maybe having the s value in the lower half of the domain would be a good guard against signature maleability. At least that's how that was addressed in ethereum where the wallets only generate (r,s) pairs with s lower than n/2 (where n is the order of the curve). Because a pair (r, n-s) is also a valid solution leading to possible issues.
} | ||
} | ||
} | ||
catch { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several lines below we do throw new NotSupportedException(nameof(ch.Hasher));
if hashing algorythm is not supported. This exception should be properly thrown, so we don't need to catch it here.
Maybe we can actually have two methods: ==Specification== ===Native Contract Interface=== Two methods will be added to CryptoLib in ====Method 1: Recovery from Signature==== { "name": "secp256k1Recover", "safe": true, "parameters": [ { "name": "message", "type": "ByteArray" }, { "name": "hasher", "type": "Integer" }, { "name": "signature", "type": "ByteArray" } ], "returntype": "ByteArray" } ====Method 2: Recovery from Components==== { "name": "secp256k1Recover", "safe": true, "parameters": [ { "name": "message", "type": "ByteArray" }, { "name": "hasher", "type": "Integer" }, { "name": "r", "type": "ByteArray" }, { "name": "s", "type": "ByteArray" }, { "name": "v", "type": "Integer" } ], "returntype": "ByteArray" } ===Method Specification=== Both methods
|
if (r.SignValue < 0) throw new ArgumentException("r should be positive"); | ||
if (s.SignValue < 0) throw new ArgumentException("s should be positive"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (r.CompareTo(BigInteger.One) < 0 || r.CompareTo(curve.N) > 0) throw new ArgumentException($"Invalid r value {r}; expected [1, {curve.N}]");
if (s.CompareTo(BigInteger.One) < 0 || s.CompareTo(curve.N) > 0) throw new ArgumentException($"Invalid s value {s}; expected [1, {curve.N}]");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optionally we can require s < n / 2 if v is not specified, for Ethereum
Co-authored-by: Anna Shaleva <[email protected]>
@Jim8y could you change it? |
Description
Close #3628
Type of change
How Has This Been Tested?
Test Configuration:
Checklist: