EcRecover #3633

shargon · 2024-12-18T12:06:50Z

Description

Close #3628

Type of change

Optimization (the change is only an optimization)
Style (the change is only a code style for better maintenance or standard purpose)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Crypto tests

Test Configuration:

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

shargon · 2024-12-18T12:07:55Z

tests/Neo.UnitTests/Cryptography/UT_Crypto.cs

+            var messageHash = message.Sha256();
+            var recover = Crypto.ECRecover(Neo.Cryptography.ECC.ECCurve.Secp256k1, signature, messageHash, SignatureFormat.Fixed32);
+
+            recover.Length.Should().Be(2);


Without V we can get more than one address

shargon · 2024-12-18T12:08:14Z

src/Neo/SmartContract/Native/CryptoLib.cs

+
+                    // TODO: Not tested, check if it works
+                    case NamedCurveHash.secp256r1Keccak256:
+                    case NamedCurveHash.secp256r1SHA256:


Not sure if it will work with these curves

shargon · 2024-12-18T12:10:21Z

src/Neo/SmartContract/Native/CryptoLib.cs

+        /// <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)


If we don't have the v we can get multiples public keys

OT-kraftchain · 2024-12-18T13:40:15Z

src/Neo/SmartContract/Native/CryptoLib.cs

+                    case NamedCurveHash.secp256k1SHA256:
+                        {
+                            return Crypto.ECRecover(ECCurve.Secp256k1, signature, messageHash,
+                                // TODO: only accept 65?


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

OT-kraftchain · 2024-12-18T14:13:28Z

src/Neo/Cryptography/Crypto.cs

+                default: throw new InvalidOperationException("Invalid signature format");
+            }
+
+            // Validate values


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.

src/Neo/SmartContract/Native/CryptoLib.cs

AnnaShaleva · 2024-12-18T17:29:17Z

src/Neo/SmartContract/Native/CryptoLib.cs

+                        }
+                }
+            }
+            catch { }


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.

Jim8y · 2024-12-19T02:55:01Z

Maybe we can actually have two methods:

==Specification==

===Native Contract Interface===

Two methods will be added to CryptoLib in HF_Echidna:

====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 MUST follow these rules:

Input Requirements for secp256k1Recover:
- message: Original message bytes
- hasher: Hash function ID
  - 1: SHA256
  - 2: RIPEMD160
  - Others: Reserved for future use
- signature: 65 bytes (32 bytes r + 32 bytes s + 1 byte v)
Input Requirements for secp256k1Recover (components):
- message: Original message bytes
- hasher: Hash function ID (same as above)
- r: 32 bytes
- s: 32 bytes
- v: Recovery ID (27 or 28)
Return Value (both methods):
- Success: 33-byte compressed public key in SEC format
- Failure: Returns null if:
  - Invalid signature/component length
  - Invalid recovery value (v)
  - Invalid signature format
  - Recovery failure

Hecate2 · 2024-12-19T08:22:39Z

src/Neo/Cryptography/Crypto.cs

+            if (r.SignValue < 0) throw new ArgumentException("r should be positive");
+            if (s.SignValue < 0) throw new ArgumentException("s should be positive");


https://github.com/Hecate2/ECrecover/blob/c3ec9b4ca4f74e66c426b48452f86cf643a9f6fb/Program.cs#L83-L84

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}]");

Optionally we can require s < n / 2 if v is not specified, for Ethereum

Co-authored-by: Anna Shaleva <shaleva.ann@nspcc.ru>

shargon · 2024-12-19T10:44:32Z

Maybe we can actually have two methods:

==Specification==

===Native Contract Interface===

Two methods will be added to CryptoLib in HF_Echidna:

====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 MUST follow these rules:

Input Requirements for secp256k1Recover:

message: Original message bytes

hasher: Hash function ID

1: SHA256

2: RIPEMD160

Others: Reserved for future use

signature: 65 bytes (32 bytes r + 32 bytes s + 1 byte v)

Input Requirements for secp256k1Recover (components):

message: Original message bytes

hasher: Hash function ID (same as above)

r: 32 bytes

s: 32 bytes

v: Recovery ID (27 or 28)

Return Value (both methods):

Success: 33-byte compressed public key in SEC format

Failure: Returns null if:

Invalid signature/component length

Invalid recovery value (v)

Invalid signature format

Recovery failure

@Jim8y could you change it?

EC Recover

9e28008

shargon requested a review from Jim8y December 18, 2024 12:06

shargon changed the base branch from master to HF_Echidna December 18, 2024 12:07

shargon commented Dec 18, 2024

View reviewed changes

shargon marked this pull request as ready for review December 18, 2024 12:09

shargon changed the title ~~Ec recover~~ EcRecover Dec 18, 2024

shargon commented Dec 18, 2024

View reviewed changes

shargon added 2 commits December 18, 2024 13:10

Fix

73d90c9

Fix HF

2ac18ea

OT-kraftchain reviewed Dec 18, 2024

View reviewed changes

AnnaShaleva reviewed Dec 18, 2024

View reviewed changes

AnnaShaleva added the Hardfork label Dec 18, 2024

Hecate2 reviewed Dec 19, 2024

View reviewed changes

Update src/Neo/SmartContract/Native/CryptoLib.cs

81bd7f6

Co-authored-by: Anna Shaleva <shaleva.ann@nspcc.ru>

Jim8y mentioned this pull request Dec 19, 2024

Add hardfork HF_Echidna #3454

Draft

11 tasks

Jim8y added 3 commits December 26, 2024 15:17

update with two ecrevover

2f98dae

fix issues.

6658ad6

remove not appliable tests

b2bbaba

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EcRecover #3633

EcRecover #3633

shargon commented Dec 18, 2024

shargon Dec 18, 2024

shargon Dec 18, 2024

shargon Dec 18, 2024 •

edited

Loading

OT-kraftchain Dec 18, 2024

OT-kraftchain Dec 18, 2024

AnnaShaleva Dec 18, 2024

Jim8y commented Dec 19, 2024

Hecate2 Dec 19, 2024

Hecate2 Dec 19, 2024

shargon commented Dec 19, 2024

		if (r.SignValue < 0) throw new ArgumentException("r should be positive");
		if (s.SignValue < 0) throw new ArgumentException("s should be positive");

EcRecover #3633

Are you sure you want to change the base?

EcRecover #3633

Conversation

shargon commented Dec 18, 2024

Description

Type of change

How Has This Been Tested?

Checklist:

shargon Dec 18, 2024

Choose a reason for hiding this comment

shargon Dec 18, 2024

Choose a reason for hiding this comment

shargon Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

OT-kraftchain Dec 18, 2024

Choose a reason for hiding this comment

OT-kraftchain Dec 18, 2024

Choose a reason for hiding this comment

AnnaShaleva Dec 18, 2024

Choose a reason for hiding this comment

Jim8y commented Dec 19, 2024

Hecate2 Dec 19, 2024

Choose a reason for hiding this comment

Hecate2 Dec 19, 2024

Choose a reason for hiding this comment

shargon commented Dec 19, 2024

shargon Dec 18, 2024 •

edited

Loading