fix(contracts/core): include msg sender in createValidator digest (#2779

)

Include msg.sender in digest to be signed by cons pub key 
passed in createValidator(). This prevents front running createValidator
transactions.

This is not an active issue, as we have an allowlist of validators. But
will be important when that allowlist is disabled.

Additional:
- refactor to use 33 byte compressed pubkey, instead of x y coords
- cleanup
- add back tests for createValidator w/o signature

issue: #2773

cli/cmd/staking.go

@@ -214,7 +214,7 @@ func createValidator(ctx context.Context, cfg createValConfig) error {
 		return err
 	}
 
-	tx, err := contract.CreateValidator(txOpts, crypto.CompressPubkey(consPubkey))
+	tx, err := contract.CreateValidator0(txOpts, crypto.CompressPubkey(consPubkey))
 	if err != nil {
 		return errors.Wrap(err, "create validator")
 	}

contracts/core/.gas-snapshot

@@ -1,12 +1,12 @@
-Admin_Test:test_pause_unpause() (gas: 34028415)
-Admin_Test:test_pause_unpause_bridge() (gas: 29058168)
-Admin_Test:test_pause_unpause_xcall() (gas: 33982389)
-Admin_Test:test_pause_unpause_xsubmit() (gas: 33982096)
-Admin_Test:test_upgrade() (gas: 38063590)
-AllocPredeploys_Test:test_num_allocs() (gas: 1181319931)
-AllocPredeploys_Test:test_predeploys() (gas: 1181301741)
-AllocPredeploys_Test:test_preinstalls() (gas: 1182018157)
-AllocPredeploys_Test:test_proxies() (gas: 1408944958)
+Admin_Test:test_pause_unpause() (gas: 33980665)
+Admin_Test:test_pause_unpause_bridge() (gas: 29010418)
+Admin_Test:test_pause_unpause_xcall() (gas: 33934639)
+Admin_Test:test_pause_unpause_xsubmit() (gas: 33934346)
+Admin_Test:test_upgrade() (gas: 38015840)
+AllocPredeploys_Test:test_num_allocs() (gas: 1181359031)
+AllocPredeploys_Test:test_predeploys() (gas: 1181295355)
+AllocPredeploys_Test:test_preinstalls() (gas: 1182011771)
+AllocPredeploys_Test:test_proxies() (gas: 1408938572)
 FeeOracleV1_Test:test_bulkSetFeeParams() (gas: 173154)
 FeeOracleV1_Test:test_feeFor() (gas: 122830)
 FeeOracleV1_Test:test_setBaseGasLimit() (gas: 32375)
@@ -149,8 +149,9 @@ 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: 145288)
-Staking_Test:test_delegate() (gas: 98848)
+Staking_Test:test_createValidator_withSignature() (gas: 363511)
+Staking_Test:test_createValidator_withoutSignature() (gas: 228383)
+Staking_Test:test_delegate() (gas: 102798)
 Staking_Test:test_stub() (gas: 143)
 Upgrade_Test:test_stub() (gas: 143)
 XAppUpgradeable_Test:test_isXCall() (gas: 148924)

contracts/core/script/admin/StakingPostUpgradeTest.sol

@@ -69,10 +69,10 @@ contract StakingPostUpgradeTest is Test {
 
     function _testCreateValidator() internal {
         bytes32 privkey = 0xf0e1605dd50ce33553290b778b0f53b2cde5e47a8794c0e7d2815e456e6da3b9;
-        bytes32 x = 0x3b12d750493ed6b12b390447f6dd38f587af12ed04ab8d6858e818cf0c63607c;
-        bytes32 y = 0x044e0321a3e57de51e95f2b230b9e4ffed2318578baab1a80652234fe0115d13;
-        bytes memory pubkey = Secp256k1.compressPublicKey(x, y);
-        bytes32 digest = staking.getValidatorPubkeyDigest(x, y);
+        uint256 x = 0x3b12d750493ed6b12b390447f6dd38f587af12ed04ab8d6858e818cf0c63607c;
+        uint256 y = 0x044e0321a3e57de51e95f2b230b9e4ffed2318578baab1a80652234fe0115d13;
+        bytes memory pubkey = Secp256k1.compress(x, y);
+        bytes32 digest = staking.getConsPubkeyDigest(validator);
         (uint8 v, bytes32 r, bytes32 s) = vm.sign(uint256(privkey), digest);
         bytes memory signature = abi.encodePacked(r, s, v);
         uint256 deposit = staking.MinDeposit();
@@ -81,7 +81,7 @@ contract StakingPostUpgradeTest is Test {
         vm.expectEmit();
         emit Staking.CreateValidator(validator, pubkey, deposit);
         vm.prank(validator);
-        staking.createValidator{ value: deposit }(x, y, signature);
+        staking.createValidator{ value: deposit }(pubkey, signature);
     }
 
     function _testDelegate() internal {

contracts/core/src/libraries/Secp256k1.sol

@@ -24,61 +24,67 @@ library Secp256k1 {
     uint256 public constant PP = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEFFFFFC2F;
 
     /**
-     * @notice Compress a public key
+     * @notice Returns 33 byte compressed public key for the given x, y coordinates
      * @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) {
+    function compress(uint256 x, uint256 y) internal pure returns (bytes memory) {
+        bytes32 _y = bytes32(y);
+
         // Set prefix based on y coordinate's parity
         // 0x02 for even y, 0x03 for odd y
-        uint8 prefix = uint8(y[31] & 0x01) + 0x02;
+        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);
+        return abi.encodePacked(bytes1(prefix), x);
     }
 
     /**
-     * @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
+     * @notice Returns x,y coordinates of an compressed public key
+     *         Reverts if the public key is invalid
+     * @param pubkey The 33 byte compressed public key
      */
-    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");
+    function decompress(bytes calldata pubkey) internal pure returns (uint256, uint256) {
+        require(pubkey.length == 33, "Secp256k1: pubkey not 33 bytes");
+        require(pubkey[0] == 0x02 || pubkey[0] == 0x03, "Secp256k1: invalid pubkey prefix");
 
         // Extract x coordinate
         uint256 x;
         assembly {
-            let ptr := add(compressedPubKey.offset, 1)
+            let ptr := add(pubkey.offset, 1)
             x := calldataload(ptr)
         }
 
         // Derive y coordinate
-        uint256 y = EllipticCurve.deriveY(uint8(compressedPubKey[0]), x, AA, BB, PP);
+        uint256 y = EllipticCurve.deriveY(uint8(pubkey[0]), x, AA, BB, PP);
+
+        require(onCurve(x, y), "Secp256k1: pubkey not on curve");
 
-        // Verify the derived point lies on the curve
+        return (x, y);
+    }
+
+    /**
+     * @notice Return true if x, y are on the secp256k1 curve
+     */
+    function onCurve(uint256 x, uint256 y) internal pure returns (bool) {
         return EllipticCurve.isOnCurve(x, y, AA, BB, PP);
     }
 
+    /**
+     * @notice Revert if the public key is invalid
+     * @param pubkey The compressed public key to validate
+     */
+    function verify(bytes calldata pubkey) internal pure {
+        decompress(pubkey);
+    }
+
     /**
      * @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) {
+    function pubkeyToAddress(uint256 x, uint256 y) internal pure returns (address) {
         bytes memory pubKey = new bytes(64);
         assembly {
             // Store x and y coordinates

contracts/core/src/octane/Staking.sol

@@ -110,41 +110,40 @@ contract Staking is OwnableUpgradeable, EIP712Upgradeable {
 
     /**
      * @notice Create a new validator
-     * @param pubkey The validators consensus public key. 33 bytes compressed secp256k1 public key
+     * @param pubkey The validators consensus public key. 33 byte 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.verifyPubkey(pubkey), "Staking: invalid pubkey");
+        Secp256k1.verify(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
+     * @param validator The validator address (msg.sender in createValidator)
+     * @return Digest hash to be signed by the validators consesnsus public key,
+     *         authorizing the validator to use that consensus key.
      */
-    function getValidatorPubkeyDigest(bytes32 x, bytes32 y) external view returns (bytes32) {
-        return _hashTypedDataV4(keccak256(abi.encode(_EIP712_TYPEHASH, x, y)));
+    function getConsPubkeyDigest(address validator) public view returns (bytes32) {
+        return _hashTypedDataV4(keccak256(abi.encode(_EIP712_TYPEHASH, validator)));
     }
 
     /**
      * @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
+     * @param pubkey    The validators consensus public key. 33 byte compressed secp256k1 public key
+     * @param signature Signature of getConsPubkeyDigest(validator) by pubkey
      * @dev Proxies x/staking.MsgCreateValidator
      */
-    function createValidator(bytes32 x, bytes32 y, bytes calldata signature) external payable {
+    function createValidator(bytes calldata pubkey, 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);
+        (uint256 x, uint256 y) = Secp256k1.decompress(pubkey);
+        _verifySignature(x, y, msg.sender, signature);
+
         emit CreateValidator(msg.sender, pubkey, msg.value);
     }
 
@@ -211,16 +210,16 @@ contract Staking is OwnableUpgradeable, EIP712Upgradeable {
     //////////////////////////////////////////////////////////////////////////////
 
     /**
-     * @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
+     * @notice Verifies signature is getConsPubkeyDigest(validator) signed by x,y pubkey
+     *         Revokes the signature if invalid
+     * @param x         The x coordinate of the validators consensus public key
+     * @param y         The y coordinate of the validators consensus public key
+     * @param validator The validator address signed by the consensus key
+     * @param signature The signature of the validator adddress, by the 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;
+    function _verifySignature(uint256 x, uint256 y, address validator, bytes calldata signature) internal view {
+        (address recovered,,) = ECDSA.tryRecover(getConsPubkeyDigest(validator), signature);
+        require(recovered == Secp256k1.pubkeyToAddress(x, y), "Staking: invalid signature");
     }
 
     /**