Skip to content

Commit

Permalink
ecdsa: Make de/serialization revert if signature malleable
Browse files Browse the repository at this point in the history 
Closes #25.
  • Loading branch information
@pmerkleplant
pmerkleplant authored Jun 28, 2024
1 parent aa1960c commit 1e38ad0
Show file tree
Hide file tree
Showing 3 changed files with 103 additions and 36 deletions.
40 changes: 36 additions & 4 deletions src/onchain/secp256k1/signatures/ECDSA.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@
// SPDX-License-Identifier: MIT OR Apache-2.0
pragma solidity ^0.8.16;

import {Message} from "../../common/Message.sol";

import {Secp256k1, SecretKey, PublicKey} from "../Secp256k1.sol";

/**
Expand Down Expand Up @@ -188,6 +186,7 @@ library ECDSA {
///
/// @dev Reverts if:
/// Length not 65 bytes
/// ∨ Deserialized ECDSA signature malleable
///
/// @dev Expects 65 bytes encoding:
/// [256-bit r value][256-bit s value][8-bit v value]
Expand All @@ -200,6 +199,7 @@ library ECDSA {
revert("LengthInvalid()");
}

// Read (v, r, s) triplet.
uint8 v;
bytes32 r;
bytes32 s;
Expand All @@ -209,18 +209,33 @@ library ECDSA {
v := byte(0, mload(add(blob, 0x60)))
}

return Signature(v, r, s);
// Make signature.
Signature memory sig = Signature(v, r, s);

// Revert if signature malleable.
if (sig.isMalleable()) {
revert("SignatureMalleable()");
}

return sig;
}

/// @dev Encodes ECDSA signature `sig` as ABI-encoded bytes.
///
/// @dev Reverts if:
/// ECDSA signature malleable
///
/// @dev Provides 65 bytes encoding:
/// [256-bit r value][256-bit s value][8-bit v value]
function toEncoded(Signature memory sig)
internal
pure
returns (bytes memory)
{
if (sig.isMalleable()) {
revert("SignatureMalleable()");
}

return abi.encodePacked(sig.r, sig.s, sig.v);
}

Expand All @@ -229,6 +244,7 @@ library ECDSA {
///
/// @dev Reverts if:
/// Length not 64 bytes
/// ∨ Deserialized ECDSA signature malleable
///
/// @dev Expects compact 64 bytes encoding:
/// [256-bit r value][1-bit yParity value][255-bit s value]
Expand All @@ -243,6 +259,7 @@ library ECDSA {
revert("LengthInvalid()");
}

// Read (v, r, s) triplet.
uint8 v;
bytes32 r;
bytes32 s;
Expand All @@ -260,11 +277,22 @@ library ECDSA {
v := add(shr(255, yParityAndS), 27)
}

return Signature(v, r, s);
// Make signature.
Signature memory sig = Signature(v, r, s);

// Revert if signature malleable.
if (sig.isMalleable()) {
revert("SignatureMalleable()");
}

return sig;
}

/// @dev Encodes ECDSA signature `sig` as [EIP-2098] compact encoded bytes.
///
/// @dev Reverts if:
/// ECDSA signature malleable
///
/// @dev Provides 64 bytes encoding:
/// [256-bit r value][1-bit yParity value][255-bit s value]
///
Expand All @@ -274,6 +302,10 @@ library ECDSA {
pure
returns (bytes memory)
{
if (sig.isMalleable()) {
revert("SignatureMalleable()");
}

bytes memory blob;

uint8 v = sig.v;
Expand Down
97 changes: 65 additions & 32 deletions test/onchain/secp256k1/signatures/ECDSA.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import {
PublicKey
} from "src/onchain/secp256k1/Secp256k1.sol";

import {ECDSAOffchain} from
"src/offchain/secp256k1/signatures/ECDSAOffchain.sol";
import {ECDSA, Signature} from "src/onchain/secp256k1/signatures/ECDSA.sol";
import {ECDSAUnsafe} from "src/unsafe/secp256k1/signatures/ECDSAUnsafe.sol";

Expand All @@ -22,6 +24,7 @@ contract ECDSATest is Test {
using Secp256k1 for SecretKey;
using Secp256k1 for PublicKey;

using ECDSAOffchain for SecretKey;
using ECDSA for address;
using ECDSA for SecretKey;
using ECDSA for PublicKey;
Expand All @@ -43,12 +46,7 @@ contract ECDSATest is Test {
PublicKey memory pk = sk.toPublicKey();
bytes32 digest = keccak256(message);

uint8 v;
bytes32 r;
bytes32 s;
(v, r, s) = vm.sign(sk.asUint(), digest);

Signature memory sig = Signature(v, r, s);
Signature memory sig = sk.sign(digest);

assertTrue(wrapper.verify(pk, message, sig));
assertTrue(wrapper.verify(pk, digest, sig));
Expand All @@ -69,16 +67,11 @@ contract ECDSATest is Test {
PublicKey memory pk = sk.toPublicKey();
bytes32 digest = keccak256(message);

uint8 v;
bytes32 r;
bytes32 s;
(v, r, s) = vm.sign(sk.asUint(), digest);

v ^= vMask;
r = bytes32(uint(r) ^ rMask);
s = bytes32(uint(s) ^ sMask);
Signature memory sig = sk.sign(digest);

Signature memory sig = Signature(v, r, s);
sig.v ^= vMask;
sig.r = bytes32(uint(sig.r) ^ rMask);
sig.s = bytes32(uint(sig.s) ^ sMask);

// Note that verify() reverts if signature is malleable.
sig.intoNonMalleable();
Expand All @@ -98,24 +91,19 @@ contract ECDSATest is Test {
PublicKey memory pk = sk.toPublicKey();
bytes32 digest = keccak256(message);

uint8 v;
bytes32 r;
bytes32 s;
(v, r, s) = vm.sign(sk.asUint(), digest);

Signature memory badSig = Signature(v, r, s).intoMalleable();
Signature memory sig = sk.sign(digest).intoMalleable();

vm.expectRevert("SignatureMalleable()");
wrapper.verify(pk, message, badSig);
wrapper.verify(pk, message, sig);

vm.expectRevert("SignatureMalleable()");
wrapper.verify(pk, digest, badSig);
wrapper.verify(pk, digest, sig);

vm.expectRevert("SignatureMalleable()");
wrapper.verify(pk.toAddress(), message, badSig);
wrapper.verify(pk.toAddress(), message, sig);

vm.expectRevert("SignatureMalleable()");
wrapper.verify(pk.toAddress(), digest, badSig);
wrapper.verify(pk.toAddress(), digest, sig);
}

function testFuzz_verify_RevertsIf_PublicKeyInvalid(
Expand Down Expand Up @@ -169,16 +157,18 @@ contract ECDSATest is Test {

// -- Signature <-> Encoded

function testFuzz_signatureFromEncoded(uint8 v, bytes32 r, bytes32 s)
public
{
bytes memory blob = abi.encodePacked(r, s, v);
// TODO: Not a good test, also already implemented as property.
// Can we get test vectors?
function testFuzz_signatureFromEncoded(Signature memory sig) public {
vm.assume(!sig.isMalleable());

bytes memory blob = sig.toEncoded();

Signature memory got = wrapper.signatureFromEncoded(blob);

assertEq(got.v, v);
assertEq(got.r, r);
assertEq(got.s, s);
assertEq(got.v, sig.v);
assertEq(got.r, sig.r);
assertEq(got.s, sig.s);
}

function testFuzz_signatureFromEncoded_RevertsIf_LengthInvalid(
Expand All @@ -190,13 +180,38 @@ contract ECDSATest is Test {
wrapper.signatureFromEncoded(blob);
}

function testFuzz_signatureFromEncoded_RevertsIf_SignatureMalleable(
SecretKey sk,
bytes32 digest
) public {
vm.assume(sk.isValid());

Signature memory sig = sk.sign(digest).intoMalleable();

bytes memory blob = abi.encodePacked(sig.r, sig.s, sig.v);

vm.expectRevert("SignatureMalleable()");
wrapper.signatureFromEncoded(blob);
}

function testFuzz_Signature_toEncoded(Signature memory sig) public {
vm.assume(!sig.isMalleable());

bytes memory got = wrapper.toEncoded(sig);
bytes memory want = abi.encodePacked(sig.r, sig.s, sig.v);

assertEq(got, want);
}

function testFuzz_Signature_toEncoded_RevertsIf_SignatureMalleable(
Signature memory sig
) public {
vm.assume(sig.isMalleable());

vm.expectRevert("SignatureMalleable()");
wrapper.toEncoded(sig);
}

// -- Signature <-> Compact Encoded

function test_signatureFromCompactEncoded() public {
Expand Down Expand Up @@ -242,6 +257,15 @@ contract ECDSATest is Test {
wrapper.signatureFromCompactEncoded(blob);
}

function test_signatureFromCompactEncoded_RevertsIf_SignatureMalleable()
public
{
bytes memory blob = abi.encodePacked(type(uint).max, type(uint).max);

vm.expectRevert("SignatureMalleable()");
wrapper.signatureFromCompactEncoded(blob);
}

function test_Signature_toCompactEncoded() public {
// Note that test cases are taken from EIP-2098.

Expand Down Expand Up @@ -271,6 +295,15 @@ contract ECDSATest is Test {
);
assertEq(got2, want2);
}

function test_Signature_toCompactEncoded_RevertsIf_SignatureMalleable(
Signature memory sig
) public {
vm.assume(sig.isMalleable());

vm.expectRevert("SignatureMalleable()");
wrapper.toCompactEncoded(sig);
}
}

/**
Expand Down
2 changes: 2 additions & 0 deletions test/onchain/secp256k1/signatures/ECDSAProperties.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ contract ECDSAPropertiesTest is Test {
function testProperty_Encoding_SerializationLoop(Signature memory start)
public
{
vm.assume(!start.isMalleable());

Signature memory end = ECDSA.signatureFromEncoded(start.toEncoded());

assertEq(start.v, end.v);
Expand Down

0 comments on commit 1e38ad0

Please sign in to comment.