Add signature malleability check and emit an event when `incrementNon…

…ce` is called
ScopeLift · Feb 19, 2024 · e164fb0 · e164fb0
1 parent 864a0a0
commit e164fb0
Show file tree

Hide file tree

Showing 2 changed files with 78 additions and 1 deletion.
diff --git a/src/ERC6538Registry.sol b/src/ERC6538Registry.sol
@@ -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();
@@ -96,7 +101,11 @@ contract ERC6538Registry {
         s := mload(add(signature, 0x40))
         v := byte(0, mload(add(signature, 0x60)))
       }
-      recoveredAddress = ecrecover(dataHash, v, r, s);
+
+      // If the signature is valid and not malleable, `ecrecover` returns the signing address.
+      if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) {} else {
+        recoveredAddress = ecrecover(dataHash, v, r, s);
+      }
     }
 
     if (
@@ -118,6 +127,7 @@ contract ERC6538Registry {
     unchecked {
       nonceOf[msg.sender]++;
     }
+    emit NonceIncremented(msg.sender, nonceOf[msg.sender]);
   }
 
   /// @notice Returns the domain separator used in this contract.

diff --git a/test/ERC6538Registry.t.sol b/test/ERC6538Registry.t.sol
@@ -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();
@@ -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;
+    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
@@ -305,6 +332,38 @@ contract RegisterKeysOnBehalf is ERC6538RegistryTest {
     registry.registerKeysOnBehalf(registrant, schemeId, signature, stealthMetaAddress);
   }
 
+  function testFuzz_RevertIf_AManipulatedErc712SignatureIsUsedToRegister(
+    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);
+
+    vm.expectRevert(bytes(""));
+    registry.registerKeysOnBehalf(registrant, schemeId, manipulatedSignature, 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
+    );
+  }
+
   function testFuzz_RevertIf_TheErc1271SignatureIsNotValid(
     uint256 schemeId,
     bytes memory stealthMetaAddress,
@@ -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 {