feat(contracts/core): refactored to verify secp256k1 signatures (#2452)

Implemented secp256k1 signature verification in Staking.sol's
createValidator function.

issue: #2430

contracts/core/.gas-snapshot

@@ -1,12 +1,12 @@
-Admin_Test:test_pause_unpause() (gas: 27886590)
-Admin_Test:test_pause_unpause_bridge() (gas: 22916277)
-Admin_Test:test_pause_unpause_xcall() (gas: 27840520)
-Admin_Test:test_pause_unpause_xsubmit() (gas: 27840271)
-Admin_Test:test_upgrade() (gas: 31921765)
-AllocPredeploys_Test:test_num_allocs() (gas: 1181227495)
-AllocPredeploys_Test:test_predeploys() (gas: 1181163797)
-AllocPredeploys_Test:test_preinstalls() (gas: 1181880235)
-AllocPredeploys_Test:test_proxies() (gas: 1408807036)
+Admin_Test:test_pause_unpause() (gas: 28588918)
+Admin_Test:test_pause_unpause_bridge() (gas: 23618605)
+Admin_Test:test_pause_unpause_xcall() (gas: 28542848)
+Admin_Test:test_pause_unpause_xsubmit() (gas: 28542599)
+Admin_Test:test_upgrade() (gas: 32624093)
+AllocPredeploys_Test:test_num_allocs() (gas: 1181319043)
+AllocPredeploys_Test:test_predeploys() (gas: 1181300853)
+AllocPredeploys_Test:test_preinstalls() (gas: 1182017269)
+AllocPredeploys_Test:test_proxies() (gas: 1408944070)
 FeeOracleV1_Test:test_bulkSetFeeParams() (gas: 173154)
 FeeOracleV1_Test:test_feeFor() (gas: 122830)
 FeeOracleV1_Test:test_setBaseGasLimit() (gas: 32375)
@@ -143,8 +143,8 @@ Quorum_Test:test_verify_sigsNotSorted_reverts() (gas: 282992)
 Quorum_Test:test_verify_succeeds() (gas: 294059)
 Slashing_Test:test_stub() (gas: 143)
 Slashing_Test:test_unjail() (gas: 54734)
-Staking_Test:test_createValidator() (gas: 221028)
-Staking_Test:test_delegate() (gas: 109941)
+Staking_Test:test_createValidator() (gas: 143163)
+Staking_Test:test_delegate() (gas: 109986)
 Staking_Test:test_stub() (gas: 143)
 Upgrade_Test:test_stub() (gas: 143)
 XAppUpgradeable_Test:test_isXCall() (gas: 148924)

contracts/core/src/libraries/Secp256k1.sol

@@ -23,12 +23,38 @@ library Secp256k1 {
      */
     uint256 public constant PP = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEFFFFFC2F;
 
+    /**
+     * @notice Compress a public key
+     * @param x The x coordinate of the public key
+     * @param y The y coordinate of the public key
+     * @return compressedPubKey The compressed public key
+     */
+    function compressPublicKey(bytes32 x, bytes32 y) internal pure returns (bytes memory) {
+        // Set prefix based on y coordinate's parity
+        // 0x02 for even y, 0x03 for odd y
+        uint8 prefix = uint8(y[31] & 0x01) + 0x02;
+
+        // Concatenate prefix and x coordinate
+        bytes memory compressedPubKey = abi.encodePacked(bytes1(prefix), x);
+        return compressedPubKey;
+    }
+
+    /**
+     * @notice Verify a secp256k1 public key is on the curve
+     * @param x The x coordinate of the public key
+     * @param y The y coordinate of the public key
+     * @return True if the public key is valid, false otherwise
+     */
+    function verifyPubkey(bytes32 x, bytes32 y) internal pure returns (bool) {
+        return EllipticCurve.isOnCurve(uint256(x), uint256(y), AA, BB, PP);
+    }
+
     /**
      * @notice Validate a compressed secp256k1 public key
      * @param compressedPubKey The compressed public key to validate
      * @return True if the public key is valid, false otherwise
      */
-    function validatePubkey(bytes calldata compressedPubKey) internal pure returns (bool) {
+    function verifyPubkey(bytes calldata compressedPubKey) internal pure returns (bool) {
         require(compressedPubKey.length == 33, "Staking: invalid pubkey length");
         require(compressedPubKey[0] == 0x02 || compressedPubKey[0] == 0x03, "Staking: invalid pubkey prefix");
 
@@ -45,4 +71,23 @@ library Secp256k1 {
         // Verify the derived point lies on the curve
         return EllipticCurve.isOnCurve(x, y, AA, BB, PP);
     }
+
+    /**
+     * @notice Convert public key coordinates to an Ethereum address
+     * @param x The x coordinate of the public key
+     * @param y The y coordinate of the public key
+     * @return The Ethereum address corresponding to the public key
+     */
+    function pubkeyToAddress(bytes32 x, bytes32 y) internal pure returns (address) {
+        bytes memory pubKey = new bytes(64);
+        assembly {
+            // Store x and y coordinates
+            mstore(add(pubKey, 32), x)
+            mstore(add(pubKey, 64), y)
+        }
+
+        // Hash the public key and keep last 20 bytes
+        bytes32 hash = keccak256(pubKey);
+        return address(uint160(uint256(hash)));
+    }
 }

contracts/core/src/octane/Staking.sol

@@ -2,6 +2,8 @@
 pragma solidity =0.8.24;
 
 import { OwnableUpgradeable } from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
+import { EIP712Upgradeable } from "@openzeppelin/contracts-upgradeable/utils/cryptography/EIP712Upgradeable.sol";
+import { ECDSA } from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
 import { Secp256k1 } from "../libraries/Secp256k1.sol";
 
 /**
@@ -14,7 +16,7 @@ import { Secp256k1 } from "../libraries/Secp256k1.sol";
  *      iniitializers on the implementation are disabled via manual storage updates, rather than in a constructor.
  *      If an new implementation is required, a constructor should be added.
  */
-contract Staking is OwnableUpgradeable {
+contract Staking is OwnableUpgradeable, EIP712Upgradeable {
     /**
      * @notice Emitted when a validator is created
      * @param validator     (MsgCreateValidator.validator_addr) The address of the validator to create
@@ -64,7 +66,12 @@ contract Staking is OwnableUpgradeable {
     uint256 public constant MinDelegation = 1 ether;
 
     /**
-     * @notice True of the validator allowlist is enabled.
+     * @notice EIP-712 typehash
+     */
+    bytes32 private constant _EIP712_TYPEHASH = keccak256("ValidatorPublicKey(bytes32 x,bytes32 y)");
+
+    /**
+     * @notice True if the validator allowlist is enabled.
      */
     bool public isAllowlistEnabled;
 
@@ -77,21 +84,67 @@ contract Staking is OwnableUpgradeable {
         _disableInitializers();
     }
 
+    /**
+     * @notice Initialize the contract, used for fresh deployment
+     */
     function initialize(address owner_, bool isAllowlistEnabled_) public initializer {
+        __Ownable_init(owner_);
+        __EIP712_init("Staking", "1");
+        isAllowlistEnabled = isAllowlistEnabled_;
+    }
+
+    /**
+     * @notice Original initializer when first deployed publicly
+     */
+    function initializeV1(address owner_, bool isAllowlistEnabled_) public initializer {
         __Ownable_init(owner_);
         isAllowlistEnabled = isAllowlistEnabled_;
     }
 
+    /**
+     * @notice Initializer for upgrade deployment, unnecessary for fresh deployment
+     */
+    function initializeV2() public reinitializer(2) {
+        __EIP712_init("Staking", "1");
+    }
+
     /**
      * @notice Create a new validator
      * @param pubkey The validators consensus public key. 33 bytes compressed secp256k1 public key
      * @dev Proxies x/staking.MsgCreateValidator
+     * @dev NOTE: This function needs to be removed once Go codebase is migrated to the new functions below
      */
     function createValidator(bytes calldata pubkey) external payable {
         require(!isAllowlistEnabled || isAllowedValidator[msg.sender], "Staking: not allowed");
         require(msg.value >= MinDeposit, "Staking: insufficient deposit");
-        require(Secp256k1.validatePubkey(pubkey), "Staking: invalid pubkey");
+        require(Secp256k1.verifyPubkey(pubkey), "Staking: invalid pubkey");
+
+        emit CreateValidator(msg.sender, pubkey, msg.value);
+    }
+
+    /**
+     * @param x The x coordinate of the validators consensus public key
+     * @param y The y coordinate of the validators consensus public key
+     * @return Digest hash to be signed by the validators public key
+     */
+    function getValidatorPubkeyDigest(bytes32 x, bytes32 y) external view returns (bytes32) {
+        return _hashTypedDataV4(keccak256(abi.encode(_EIP712_TYPEHASH, x, y)));
+    }
+
+    /**
+     * @notice Create a new validator
+     * @param x The x coordinate of the validators consensus public key
+     * @param y The y coordinate of the validators consensus public key
+     * @param signature The signature of the validators consensus public key
+     * @dev Proxies x/staking.MsgCreateValidator
+     */
+    function createValidator(bytes32 x, bytes32 y, bytes calldata signature) external payable {
+        require(!isAllowlistEnabled || isAllowedValidator[msg.sender], "Staking: not allowed");
+        require(msg.value >= MinDeposit, "Staking: insufficient deposit");
+        require(Secp256k1.verifyPubkey(x, y), "Staking: invalid pubkey");
+        require(_verifySignature(x, y, signature), "Staking: invalid signature");
 
+        bytes memory pubkey = Secp256k1.compressPublicKey(x, y);
         emit CreateValidator(msg.sender, pubkey, msg.value);
     }
 
@@ -150,4 +203,21 @@ contract Staking is OwnableUpgradeable {
             emit ValidatorDisallowed(validators[i]);
         }
     }
+
+    //////////////////////////////////////////////////////////////////////////////
+    //                                 Internal                                 //
+    //////////////////////////////////////////////////////////////////////////////
+
+    /**
+     * @notice Verify a signature matches a secp256k1 public key
+     * @param x The x coordinate of the validators consensus public key
+     * @param y The y coordinate of the validators consensus public key
+     * @param signature The signature of the validators consensus public key
+     */
+    function _verifySignature(bytes32 x, bytes32 y, bytes calldata signature) internal view returns (bool) {
+        bytes32 digest = _hashTypedDataV4(keccak256(abi.encode(_EIP712_TYPEHASH, x, y)));
+        (address recovered,,) = ECDSA.tryRecover(digest, signature);
+        address pubKeyAddress = Secp256k1.pubkeyToAddress(x, y);
+        return recovered == pubKeyAddress;
+    }
 }

contracts/core/test/octane/Staking.t.sol

@@ -3,6 +3,7 @@ pragma solidity =0.8.24;
 
 import { Staking } from "src/octane/Staking.sol";
 import { Test, Vm } from "forge-std/Test.sol";
+import { Secp256k1 } from "src/libraries/Secp256k1.sol";
 
 /**
  * @title Staking_Test
@@ -27,7 +28,11 @@ contract Staking_Test is Test {
         address validator = makeAddr("validator");
         address[] memory validators = new address[](1);
         validators[0] = validator;
-        bytes memory pubkey = abi.encodePacked(hex"03440d290e4394cd9832cc7025769be18ab7975e34e4514c31c07da3d370fe0b05");
+        bytes32 x = 0x534d719d4f56544f42e22cab20886dd64fb713a5c72b31f929d856654a11dc0c;
+        bytes32 y = 0x5609e3c7f55c46a197ead4a96caa63eeade00b4a775e7709f6e673157a724d6c;
+        bytes memory pubkey = Secp256k1.compressPublicKey(x, y);
+        bytes memory signature =
+            hex"e847a133d1925786238a920c00e38ceb5fd5c2ffaba0bcfb63858d3724f40b370767f30ad1cc1a1130d9a199b5c0322b860fce2144147f993db48af4fd4ec2af1b";
         vm.deal(validator, staking.MinDeposit());
 
         // allowlist is disabled
@@ -40,7 +45,7 @@ contract Staking_Test is Test {
 
         // must be in allowlist
         vm.expectRevert("Staking: not allowed");
-        staking.createValidator(pubkey);
+        staking.createValidator(x, y, signature);
 
         // add to allowlist
         vm.prank(owner);
@@ -52,34 +57,29 @@ contract Staking_Test is Test {
 
         vm.expectRevert("Staking: insufficient deposit");
         vm.prank(validator);
-        staking.createValidator{ value: insufficientDeposit }(pubkey);
+        staking.createValidator{ value: insufficientDeposit }(x, y, signature);
 
-        // requires 33 byte
         uint256 deposit = staking.MinDeposit();
-        bytes memory pubkey32 = abi.encodePacked(keccak256("pubkey"));
-
-        vm.expectRevert("Staking: invalid pubkey length");
-        vm.prank(validator);
-        staking.createValidator{ value: deposit }(pubkey32);
-
-        // requires valid pubkey prefix
-        bytes memory invalidPubkey = abi.encodePacked(hex"01", pubkey32);
-        vm.expectRevert("Staking: invalid pubkey prefix");
-        vm.prank(validator);
-        staking.createValidator{ value: deposit }(invalidPubkey);
 
         // requires a valid pubkey on the secp256k1 curve
-        invalidPubkey = abi.encodePacked(hex"03", pubkey32);
+        bytes32 badY = bytes32(uint256(y) + 1);
         vm.expectRevert("Staking: invalid pubkey");
         vm.prank(validator);
-        staking.createValidator{ value: deposit }(invalidPubkey);
+        staking.createValidator{ value: deposit }(x, badY, signature);
+
+        // requires a valid signature
+        bytes memory badSignature = abi.encodePacked(signature);
+        badSignature[0] = bytes1(uint8(badSignature[0]) + 1);
+        vm.expectRevert("Staking: invalid signature");
+        vm.prank(validator);
+        staking.createValidator{ value: deposit }(x, y, badSignature);
 
         // succeeds with valid deposit and pubkey
         vm.expectEmit();
         emit CreateValidator(validator, pubkey, deposit);
 
         vm.prank(validator);
-        staking.createValidator{ value: deposit }(pubkey);
+        staking.createValidator{ value: deposit }(x, y, signature);
 
         // remove from allowlist
         vm.prank(owner);
@@ -90,7 +90,7 @@ contract Staking_Test is Test {
         vm.expectRevert("Staking: not allowed");
         vm.deal(validator, deposit);
         vm.prank(validator);
-        staking.createValidator{ value: deposit }(pubkey);
+        staking.createValidator{ value: deposit }(x, y, signature);
 
         // disable allowlist
         vm.prank(owner);
@@ -102,7 +102,7 @@ contract Staking_Test is Test {
         emit CreateValidator(validator, pubkey, deposit);
 
         vm.prank(validator);
-        staking.createValidator{ value: deposit }(pubkey);
+        staking.createValidator{ value: deposit }(x, y, signature);
     }
 
     function test_delegate() public {
@@ -144,10 +144,22 @@ contract Staking_Test is Test {
 
 /**
  * @title StakingHarness
- * @notice Wrapper around Staking.sol that allows setting owner in constructor
+ * @notice Wrapper around Staking.sol that allows setting owner and EIP-712 in constructor
  */
 contract StakingHarness is Staking {
+    bytes32 private constant EIP712StorageLocation = 0xa16a46d94261c7517cc8ff89f61c0ce93598e3c849801011dee649a6a557d100;
+
+    function getEIP712Storage() private pure returns (EIP712Storage storage $) {
+        assembly {
+            $.slot := EIP712StorageLocation
+        }
+    }
+
     constructor(address _owner) {
         _transferOwnership(_owner);
+
+        EIP712Storage storage $ = getEIP712Storage();
+        $._name = "Staking";
+        $._version = "1";
     }
 }