Skip to content
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

Add signature malleability test and emit an event when incrementNonce is called #9

Merged
merged 10 commits into from
Apr 24, 2024
6 changes: 6 additions & 0 deletions src/ERC6538Registry.sol
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ contract ERC6538Registry {
address indexed registrant, uint256 indexed schemeId, bytes stealthMetaAddress
);

/// @notice Emitted when a registrant increments their nonce.
/// @param registrant The account that incremented the nonce.
/// @param newNonce The new nonce value.
event NonceIncremented(address indexed registrant, uint256 newNonce);

constructor() {
INITIAL_CHAIN_ID = block.chainid;
INITIAL_DOMAIN_SEPARATOR = _computeDomainSeparator();
Expand Down Expand Up @@ -118,6 +123,7 @@ contract ERC6538Registry {
unchecked {
nonceOf[msg.sender]++;
}
emit NonceIncremented(msg.sender, nonceOf[msg.sender]);
}

/// @notice Returns the domain separator used in this contract.
Expand Down
5 changes: 5 additions & 0 deletions src/interfaces/IERC6538Registry.sol
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ interface IERC6538Registry {
address indexed registrant, uint256 indexed schemeId, bytes stealthMetaAddress
);

/// @notice Emitted when a registrant increments their nonce.
/// @param registrant The account that incremented the nonce.
/// @param newNonce The new nonce value.
event NonceIncremented(address indexed registrant, uint256 newNonce);

/// @notice Sets the caller's stealth meta-address for the given scheme ID.
/// @param schemeId Identifier corresponding to the applied stealth address scheme, e.g. 0 for
/// secp256k1, as specified in ERC-5564.
Expand Down
67 changes: 67 additions & 0 deletions test/ERC6538Registry.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ contract ERC6538RegistryTest is Test, Deploy {
event StealthMetaAddressSet(
address indexed registrant, uint256 indexed schemeId, bytes stealthMetaAddress
);
event NonceIncremented(address indexed registrant, uint256 newNonce);

function setUp() public {
Deploy.run();
Expand Down Expand Up @@ -55,6 +56,32 @@ contract ERC6538RegistryTest is Test, Deploy {
vm.assume(_address != address(vm));
vm.assume(_address != address(address(0x000000000000000000636F6e736F6c652e6c6f67)));
}

function manipulateSignature(bytes memory signature) public pure returns (bytes memory) {
(uint8 v, bytes32 r, bytes32 s) = splitSignature(signature);

uint8 manipulatedV = v % 2 == 0 ? v - 1 : v + 1;
garyghayrat marked this conversation as resolved.
Show resolved Hide resolved
uint256 manipulatedS = modNegS(uint256(s));
bytes memory manipulatedSignature = abi.encodePacked(r, bytes32(manipulatedS), manipulatedV);

return manipulatedSignature;
}

function splitSignature(bytes memory sig) public pure returns (uint8 v, bytes32 r, bytes32 s) {
require(sig.length == 65, "Invalid signature length");
assembly {
r := mload(add(sig, 32))
s := mload(add(sig, 64))
v := byte(0, mload(add(sig, 96)))
}
if (v < 27) v += 27;
require(v == 27 || v == 28, "Invalid signature v value");
}

function modNegS(uint256 s) public pure returns (uint256) {
uint256 n = 0xfffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141;
return n - s;
}
}

// Test harness to expose internal contract methods for test purpose only
Expand Down Expand Up @@ -227,6 +254,20 @@ contract RegisterKeysOnBehalf is ERC6538RegistryTest {
}
}

function testFuzz_AManipulatedErc712SignatureIsUsedToRegisterTheSameStealthMetaAddress(
apbendi marked this conversation as resolved.
Show resolved Hide resolved
string memory registrantSeed,
uint256 schemeId,
bytes memory stealthMetaAddress
) external {
(address registrant, uint256 registrantPrivateKey) = makeAddrAndKey(registrantSeed);
bytes memory signature =
_generateRegistrationSignature(registrantPrivateKey, schemeId, stealthMetaAddress, 0);
bytes memory manipulatedSignature = manipulateSignature(signature);

registry.registerKeysOnBehalf(registrant, schemeId, manipulatedSignature, stealthMetaAddress);
assertEq(registry.stealthMetaAddressOf(address(registrant), schemeId), stealthMetaAddress);
}

function testFuzz_RevertIf_TheDataIsErc712SignedByAnAddressOtherThanTheRegistrant(
string memory seed,
address registrant,
Expand Down Expand Up @@ -305,6 +346,24 @@ contract RegisterKeysOnBehalf is ERC6538RegistryTest {
registry.registerKeysOnBehalf(registrant, schemeId, signature, stealthMetaAddress);
}

function testFuzz_RevertIf_AManipulatedErc712SignatureIsUsedToRegisterADifferentStealthMetaAddress(
string memory registrantSeed,
uint256 schemeId,
bytes memory stealthMetaAddress,
bytes memory attackerStealthMetaAddress
) external {
vm.assume(keccak256(stealthMetaAddress) != keccak256(attackerStealthMetaAddress));
(address registrant, uint256 registrantPrivateKey) = makeAddrAndKey(registrantSeed);
bytes memory signature =
_generateRegistrationSignature(registrantPrivateKey, schemeId, stealthMetaAddress, 0);
bytes memory manipulatedSignature = manipulateSignature(signature);

vm.expectRevert(bytes(""));
registry.registerKeysOnBehalf(
registrant, schemeId, manipulatedSignature, attackerStealthMetaAddress
);
garyghayrat marked this conversation as resolved.
Show resolved Hide resolved
}

function testFuzz_RevertIf_TheErc1271SignatureIsNotValid(
uint256 schemeId,
bytes memory stealthMetaAddress,
Expand Down Expand Up @@ -339,6 +398,14 @@ contract IncrementNonce is ERC6538RegistryTest {
assertEq(registry.nonceOf(registrant), i);
}
}

function testFuzz_EmitsANonceIncrementedEvent(address registrant) external {
uint256 expectedNonce = registry.nonceOf(registrant) + 1;
vm.expectEmit();
emit NonceIncremented(registrant, expectedNonce);
vm.prank(registrant);
registry.incrementNonce();
}
}

contract Domain_Separator is ERC6538RegistryTest {
Expand Down
Loading