Skip to content

Commit

Permalink
fix: Make de/serialization functions revert for invalid objects
Browse files Browse the repository at this point in the history
  • Loading branch information
@pmerkleplant
pmerkleplant committed Jun 27, 2024
1 parent 2ebcf59 commit 8f81931
Show file tree
Hide file tree
Showing 6 changed files with 212 additions and 47 deletions.
40 changes: 32 additions & 8 deletions src/onchain/secp256k1/Secp256k1.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -255,10 +255,11 @@ library Secp256k1 {
//----------------------------------
// Secret Key

/// @dev Returns bytes `blob` as secret key.
/// @dev Decodes secret key from bytes `blob`.
///
/// @dev Reverts if:
/// Length not 32 bytes
/// ∨ Deserialized secret key invalid
function secretKeyFromBytes(bytes memory blob)
internal
pure
Expand All @@ -273,14 +274,26 @@ library Secp256k1 {
scalar := mload(add(blob, 0x20))
}

// Note to not use secretKeyFromUint(uint) to not revert in case secret
// key is invalid.
// This responsibility is delegated to the caller.
return SecretKey.wrap(scalar);
// Make secret key.
SecretKey sk = SecretKey.wrap(scalar);

// Revert if secret key invalid.
if (!sk.isValid()) {
revert("SecretKeyInvalid()");
}

return sk;
}

/// @dev Returns secret key `sk` as bytes.
/// @dev Encodes secret key `sk` as bytes.
///
/// @dev Reverts if:
/// Secret key invalid
function toBytes(SecretKey sk) internal pure returns (bytes memory) {
if (!sk.isValid()) {
revert("SecretKeyInvalid()");
}

return abi.encodePacked(sk.asUint());
}

Expand All @@ -291,6 +304,7 @@ library Secp256k1 {
///
/// @dev Reverts if:
/// Length not 64 bytes
/// ∨ Deserialized public key invalid
///
/// @dev Expects 64 bytes encoding:
/// [32 bytes x coordinate][32 bytes y coordinate]
Expand All @@ -315,20 +329,30 @@ library Secp256k1 {
// Make public key.
PublicKey memory pk = PublicKey(x, y);

// Note that public key's validity is not verified.
// This responsibility is delegated to the caller.
// Revert if public key invalid.
if (!pk.isValid()) {
revert("PublicKeyInvalid()");
}

return pk;
}

/// @dev Encodes public key `pk` as ABI-encoded bytes.
///
/// @dev Reverts if:
/// Public key invalid
///
/// @dev Provides 64 bytes encoding:
/// [32 bytes x coordinate][32 bytes y coordinate]
function toBytes(PublicKey memory pk)
internal
pure
returns (bytes memory)
{
if (!pk.isValid()) {
revert("PublicKeyInvalid()");
}

return abi.encodePacked(pk.x, pk.y);
}
}
56 changes: 40 additions & 16 deletions src/onchain/secp256k1/Secp256k1Arithmetic.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -430,9 +430,10 @@ library Secp256k1Arithmetic {
/// @dev Decodes point from [SEC-1 v2] encoded bytes `blob`.
///
/// @dev Reverts if:
/// Blob not 0x00
/// ∧ Length not 65 bytes
/// ∨ Prefix byte not 0x04
/// Blob not 0x00
/// ∧ Length not 65 bytes
/// ∨ Prefix byte not 0x04
/// ∨ Deserialized point not on curve
///
/// @dev Expects uncompressed 65 bytes encoding if point is not identity:
/// [0x04 prefix][32 bytes x coordinate][32 bytes y coordinate]
Expand Down Expand Up @@ -475,15 +476,22 @@ library Secp256k1Arithmetic {
y := mload(add(blob, 0x41))
}

// Return as new point.
//
// Note that point's validity is not verified.
// This responsibility is delegated to the caller.
return Point(x, y);
// Make point.
Point memory point = Point(x, y);

// Revert if point not on curve.
if (!point.isOnCurve()) {
revert("PointNotOnCurve()");
}

return point;
}

/// @dev Encodes point `point` as [SEC-1 v2] encoded bytes.
///
/// @dev Reverts if:
/// Point not on curve
///
/// @dev Provides uncompressed 65 bytes encoding if point is not identity:
/// [0x04 prefix][32 bytes x coordinate][32 bytes y coordinate]
///
Expand All @@ -496,6 +504,10 @@ library Secp256k1Arithmetic {
pure
returns (bytes memory blob)
{
if (!point.isOnCurve()) {
revert("PointNotOnCurve()");
}

// Note to catch special encoding for identity.
if (point.isIdentity()) {
return bytes(hex"00");
Expand All @@ -507,9 +519,10 @@ library Secp256k1Arithmetic {
/// @dev Decodes point from [SEC-1 v2] compressed encoded bytes `blob`.
///
/// @dev Reverts if:
/// Blob not 0x00
/// ∧ Length not 33 bytes
/// ∨ Prefix byte not one in [0x02, 0x03]
/// Blob not 0x00
/// ∧ Length not 33 bytes
/// ∨ Prefix byte not one in [0x02, 0x03]
/// ∨ Deserialized point not on curve
///
/// @dev Expects compressed 33 bytes encoding if point is not identity:
/// [0x02 or 0x03 prefix][32 bytes x coordinate]
Expand Down Expand Up @@ -566,15 +579,22 @@ library Secp256k1Arithmetic {
y = beta & 1 == prefix & 1 ? beta : P - beta;
}

// Return as new point.
//
// Note that point's validity is not verified.
// This responsibility is delegated to the caller.
return Point(x, y);
// Make point.
Point memory point = Point(x, y);

// Revert if point not on curve.
if (!point.isOnCurve()) {
revert("PointNotOnCurve()");
}

return point;
}

/// @dev Encodes point `point` as [SEC-1 v2] compressed encoded bytes.
///
/// @dev Reverts if:
/// Point not on curve
///
/// @dev Provides compressed 33 bytes encoding if point is not identity:
/// [0x02 or 0x03 prefix][32 bytes x coordinate]
///
Expand All @@ -587,6 +607,10 @@ library Secp256k1Arithmetic {
pure
returns (bytes memory blob)
{
if (!point.isOnCurve()) {
revert("PointNotOnCurve()");
}

// Note to catch special encoding for identity.
if (point.isIdentity()) {
return bytes(hex"00");
Expand Down
74 changes: 69 additions & 5 deletions test/onchain/secp256k1/Secp256k1.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,12 +243,14 @@ contract Secp256k1Test is Test {

// -- SecretKey <-> Bytes

function testFuzz_secretKeyFromBytes(uint scalar) public {
bytes memory blob = abi.encodePacked(scalar);
function testFuzz_secretKeyFromBytes(SecretKey sk1) public {
vm.assume(sk1.isValid());

SecretKey sk = wrapper.secretKeyFromBytes(blob);
bytes memory blob = abi.encodePacked(sk1.asUint());

assertEq(sk.asUint(), scalar);
SecretKey sk2 = wrapper.secretKeyFromBytes(blob);

assertEq(sk1.asUint(), sk2.asUint());
}

function testFuzz_secretKeyFromBytes_RevertsIf_LengthNot32Bytes(
Expand All @@ -260,6 +262,25 @@ contract Secp256k1Test is Test {
wrapper.secretKeyFromBytes(blob);
}

function test_secretKeyFromBytes_RevertsIf_DeserializedSecretKeyInvalid_SecretKeyZero(
) public {
bytes memory blob = abi.encodePacked(uint(0));

vm.expectRevert("SecretKeyInvalid()");
wrapper.secretKeyFromBytes(blob);
}

function testFuzz_secretKeyFromBytes_RevertsIf_DeserializedSecretKeyInvalid_SecretKeyGreaterOrEqualToQ(
uint seed
) public {
uint scalar = _bound(seed, Secp256k1.Q, type(uint).max);

bytes memory blob = abi.encodePacked(scalar);

vm.expectRevert("SecretKeyInvalid()");
wrapper.secretKeyFromBytes(blob);
}

function testFuzz_SecretKey_toBytes(SecretKey sk) public {
vm.assume(sk.isValid());

Expand All @@ -268,12 +289,35 @@ contract Secp256k1Test is Test {
assertEq(sk.asUint(), Secp256k1.secretKeyFromBytes(blob).asUint());
}

function test_SecretKey_toBytes_RevertsIf_SecretKeyInvalid_SecretKeyZero()
public
{
SecretKey sk = SecretKey.wrap(0);

vm.expectRevert("SecretKeyInvalid()");
wrapper.toBytes(sk);
}

function testFuzz_SecretKey_toBytes_RevertsIf_SecretKeyInvalid_SecretKeyGreaterOrEqualToQ(
uint seed
) public {
uint scalar = _bound(seed, Secp256k1.Q, type(uint).max);
SecretKey sk = SecretKey.wrap(scalar);

vm.expectRevert("SecretKeyInvalid()");
wrapper.toBytes(sk);
}

//----------------------------------
// Public Key

// -- PublicKey <-> Bytes

function testFuzz_publicKeyFromBytes(PublicKey memory pk1) public {
// TODO: PublicKey <-> Bytes: Need test vectors.
function testFuzz_publicKeyFromBytes(SecretKey sk) public {
vm.assume(sk.isValid());

PublicKey memory pk1 = sk.toPublicKey();
bytes memory blob = pk1.toBytes();

PublicKey memory pk2 = wrapper.publicKeyFromBytes(blob);
Expand All @@ -290,6 +334,17 @@ contract Secp256k1Test is Test {
wrapper.publicKeyFromBytes(blob);
}

function testFuzz_publicKeyFromBytes_RevertsIf_DeserializedPublicKeyInvalid(
PublicKey memory pk
) public {
vm.assume(!pk.isValid());

bytes memory blob = abi.encodePacked(pk.x, pk.y);

vm.expectRevert("PublicKeyInvalid()");
wrapper.publicKeyFromBytes(blob);
}

function testFuzz_PublicKey_toBytes(SecretKey sk) public {
vm.assume(sk.isValid());

Expand All @@ -301,6 +356,15 @@ contract Secp256k1Test is Test {
PublicKey memory pk2 = Secp256k1.publicKeyFromBytes(blob);
assertTrue(pk1.eq(pk2));
}

function testFuzz_PublicKey_toBytes_RevertsIf_PublicKeyInvalid(
PublicKey memory pk
) public {
vm.assume(!pk.isValid());

vm.expectRevert("PublicKeyInvalid()");
wrapper.toBytes(pk);
}
}

/**
Expand Down
Loading

0 comments on commit 8f81931

Please sign in to comment.