Skip to content

Commit

Permalink
scep256k1: Updates identity integration
Browse files Browse the repository at this point in the history 
The identity is updated to (0, 0) and is now considered an invalid PublicKey.
  • Loading branch information
@pmerkleplant
pmerkleplant authored Jun 27, 2024
1 parent ac04a6b commit 8a9b22c
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 89 deletions.
10 changes: 4 additions & 6 deletions src/onchain/secp256k1/Secp256k1.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,13 +188,11 @@ library Secp256k1 {

/// @dev Returns whether public key `pk` is a valid secp256k1 public key.
///
/// @dev Note that a public key is valid if its either on the curve or the
/// identity (aka point at infinity) point.
/// @dev Note that the identity point is not a valid public key.
function isValid(PublicKey memory pk) internal pure returns (bool) {
// TODO: Should identity be a valid public key?
// Point memory p = pk.intoPoint();
// return p.isOnCurve() && !p.isIdentity();
return pk.intoPoint().isOnCurve();
Point memory p = pk.intoPoint();

return p.isOnCurve() && !p.isIdentity();
}

/// @dev Returns the y parity of public key `pk`.
Expand Down
44 changes: 12 additions & 32 deletions src/onchain/secp256k1/Secp256k1Arithmetic.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ pragma solidity ^0.8.16;
* @notice Point is a secp256k1 point in affine coordinates
*
* @dev The identity, aka point at infinity, is represented via:
* x = y = type(uint).max
* x = y = 0
*/
struct Point {
uint x;
Expand Down Expand Up @@ -75,7 +75,7 @@ library Secp256k1Arithmetic {

/// @dev Used as substitute for `Identity().intoPublicKey().toAddress()`.
address private constant IDENTITY_ADDRESS =
0x2dCC482901728b6df477f4fB2F192733A005d396;
0x3f17f1962B36e491b30A40b2405849e597Ba5FB5;

//--------------------------------------------------------------------------
// Secp256k1 Constants
Expand Down Expand Up @@ -111,28 +111,18 @@ library Secp256k1Arithmetic {
//--------------------------------------------------------------------------
// Point

/// @dev Returns the zero point.
///
/// @dev Note that the zero point is invalid and this function only provided
/// for convenience.
function ZeroPoint() internal pure returns (Point memory) {
return Point(0, 0);
}

/// @dev Returns whether point `point` is the zero point.
function isZeroPoint(Point memory point) internal pure returns (bool) {
return (point.x | point.y) == 0;
}

// TODO: Use (0, 0) as infinity? Follows general convention...
/// @dev Returns the additive identity.
///
/// @dev Note that the identity is represented via:
/// point.x = point.y = type(uint).max
/// point.x = point.y = 0
///
/// @dev Note that the identity is a valid point on the curve to enable
/// arithmetic functionality. However, the identity is not a valid
/// PublicKey and MUST NOT be used as cryptographic object.
///
/// @dev Note that the identity is also called point at infinity.
function Identity() internal pure returns (Point memory) {
return Point(type(uint).max, type(uint).max);
return Point(0, 0);
}

/// @dev Returns whether point `point` is the identity.
Expand All @@ -142,7 +132,7 @@ library Secp256k1Arithmetic {
///
/// @dev Note that the identity is also called point at infinity.
function isIdentity(Point memory point) internal pure returns (bool) {
return (point.x & point.y) == type(uint).max;
return (point.x | point.y) == 0;
}

/// @dev Returns whether point `point` is on the curve.
Expand All @@ -152,18 +142,8 @@ library Secp256k1Arithmetic {
/// a = 0
/// b = 7
///
/// @dev Note that the identity is also on the curve.
/// @dev Note that the identity is on the curve.
function isOnCurve(Point memory point) internal pure returns (bool) {
// TODO: Issue with whether identity is on curve:
//
// isOnCurve should return true to enable arithmetic functionality.
// However, publicKey.isValid should return false! Identity is only
// valid inside Arithmetic package. For crypto package it is invalid.
//
// See also go's issue: https://github.com/golang/go/issues/37294.
//
// Important: It MUST NOT be possible to de-/serialize identity to
// normal public key.
if (point.isIdentity()) {
return true;
}
Expand Down Expand Up @@ -396,8 +376,8 @@ library Secp256k1Arithmetic {
assembly ("memory-safe") {
p := point
}
p.x = type(uint).max;
p.y = type(uint).max;
p.x = 0;
p.y = 0;
return p;
}

Expand Down
10 changes: 2 additions & 8 deletions test/onchain/secp256k1/Secp256k1.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,21 +165,15 @@ contract Secp256k1Test is Test {
assertTrue(wrapper.isValid(sk.toPublicKey()));
}

function test_PublicKey_isValid_If_Identity() public {
function test_PublicKey_isValid_FailsIf_Identity() public {
PublicKey memory pk = Secp256k1Arithmetic.Identity().intoPublicKey();

assertTrue(pk.isValid());
assertFalse(wrapper.isValid(pk));
}

function test_PublicKey_isValid_FailsIf_PointNotOnCurve() public {
PublicKey memory pk;

// Zero point not on curve.
pk.x = 0;
pk.y = 0;
assertFalse(wrapper.isValid(pk));

// Some other point.
pk.x = 1;
pk.x = 3;
assertFalse(wrapper.isValid(pk));
Expand Down
26 changes: 1 addition & 25 deletions test/onchain/secp256k1/Secp256k1Arithmetic.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,22 +61,6 @@ contract Secp256k1ArithmeticTest is Test {
//--------------------------------------------------------------------------
// Test: Point

// -- ZeroPoint

function test_ZeroPoint() public {
assertTrue(wrapper.ZeroPoint().isZeroPoint());
}

// -- isZeroPoint

function testFuzz_Point_isZeroPoint(Point memory point) public {
if (point.x == 0 && point.y == 0) {
assertTrue(wrapper.isZeroPoint(point));
} else {
assertFalse(wrapper.isZeroPoint(point));
}
}

// -- Identity

function test_Identity() public {
Expand All @@ -86,7 +70,7 @@ contract Secp256k1ArithmeticTest is Test {
// -- isIdentity

function testFuzz_Point_isIdentity(Point memory point) public {
if (point.x == type(uint).max && point.y == type(uint).max) {
if (point.x == 0 && point.y == 0) {
assertTrue(wrapper.isIdentity(point));
} else {
assertFalse(wrapper.isIdentity(point));
Expand Down Expand Up @@ -569,14 +553,6 @@ contract Secp256k1ArithmeticWrapper {
//--------------------------------------------------------------------------
// Point

function ZeroPoint() public pure returns (Point memory) {
return Secp256k1Arithmetic.ZeroPoint();
}

function isZeroPoint(Point memory point) public pure returns (bool) {
return point.isZeroPoint();
}

function Identity() public pure returns (Point memory) {
return Secp256k1Arithmetic.Identity();
}
Expand Down
19 changes: 1 addition & 18 deletions test/onchain/secp256k1/Secp256k1ArithmeticProperties.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ contract Secp256k1ArithmeticPropertiesTest is Test {

Point memory sum = p1.add(p2).intoPoint();

assertFalse(sum.isZeroPoint());
assertFalse(sum.x == 0 && sum.y == 0);
}

function testProperty_ProjectivePoint_add_ResultIsOnCurve(
Expand All @@ -64,29 +64,12 @@ contract Secp256k1ArithmeticPropertiesTest is Test {

// -- mul

function testProperty_ProjectivePoint_mul_NeverReturnsZeroPoint(
SecretKey sk,
uint scalar
) public {
vm.assume(sk.isValid());

// TODO: Make Felt type to circumvent manual bounding?
vm.assume(scalar < Secp256k1Arithmetic.Q);

ProjectivePoint memory p = sk.toPublicKey().toProjectivePoint();

Point memory product = p.mul(scalar).intoPoint();

assertFalse(product.isZeroPoint());
}

function testProperty_ProjectivePoint_mul_ResultIsOnCurve(
SecretKey sk,
uint scalar
) public {
vm.assume(sk.isValid());

// TODO: Make Felt type to circumvent manual bounding?
vm.assume(scalar < Secp256k1Arithmetic.Q);

ProjectivePoint memory p = sk.toPublicKey().toProjectivePoint();
Expand Down

0 comments on commit 8a9b22c

Please sign in to comment.