diff --git a/src/foundation/Errors.sol b/src/foundation/Errors.sol index c9dcc36..23b25a3 100644 --- a/src/foundation/Errors.sol +++ b/src/foundation/Errors.sol @@ -196,6 +196,20 @@ library Errors { /// @notice Consensus proof-of-possession is empty error InvalidConsensusPopLength(); + /// @notice Consensus pubkey commitment has already been recorded under this sender + /// @dev See audit #580: commit-reveal binds a registrant to a pubkey before the pubkey + /// is publicly observable, so a mempool front-runner cannot replay a victim's + /// (pubkey, pop) into their own pool. + error ConsensusPubkeyCommitAlreadyExists(); + + /// @notice No matching consensus pubkey commitment found for caller + error ConsensusPubkeyCommitNotFound(); + + /// @notice Consensus pubkey commitment was recorded in the current block; must wait at least one block + /// @dev Prevents same-block commit+reveal, which would let a front-runner that sees a + /// reveal tx in the mempool submit (commit, register) in the same block and still win. + error ConsensusPubkeyCommitTooRecent(); + // ======================================================================== // RECONFIGURATION ERRORS // ======================================================================== diff --git a/src/staking/IValidatorManagement.sol b/src/staking/IValidatorManagement.sol index 1af6faa..db3ce6c 100644 --- a/src/staking/IValidatorManagement.sol +++ b/src/staking/IValidatorManagement.sol @@ -116,6 +116,12 @@ interface IValidatorManagement { /// @param enabled True if permissionless join is now active (whitelist bypassed) event PermissionlessJoinEnabledUpdated(bool enabled); + /// @notice Emitted when a caller commits to a future consensus pubkey registration/rotation + /// @param committer Address that submitted the commitment (must later be msg.sender of register/rotate) + /// @param commitment keccak256(abi.encode(pubkey, stakePool, chainid)) + /// @param blockNumber Block number at which the commitment was recorded + event ConsensusPubkeyCommitted(address indexed committer, bytes32 indexed commitment, uint256 blockNumber); + // ======================================================================== // INITIALIZATION // ======================================================================== @@ -136,10 +142,29 @@ interface IValidatorManagement { // REGISTRATION // ======================================================================== + /// @notice Commit to a future consensus-pubkey registration or rotation + /// @dev Audit #580 mitigation. The BLS PoP precompile only proves "someone has sk", + /// not "this sender has sk" — so a front-runner that observes a victim's + /// (pubkey, pop) in the mempool can replay the pair into their own pool and + /// DoS the victim's registration. The commit-reveal flow forces registration + /// to be preceded by an opaque `keccak256(pubkey, stakePool, chainid)` commit + /// from the same `msg.sender`, so the registrant is bound to the pubkey + /// before the pubkey is publicly observable. + /// + /// The commit is keyed by msg.sender; it is consumed (deleted) on the matching + /// `registerValidator` / `rotateConsensusKey` call. A subsequent call that + /// re-uses the same (pubkey, stakePool) needs a fresh commit. + /// @param commitment keccak256(abi.encode(pubkey, stakePool, chainid)) + function commitConsensusPubkey( + bytes32 commitment + ) external; + /// @notice Register a new validator with a stake pool /// @dev Only callable by the stake pool's operator. /// Requires stake pool to have voting power >= minimumBond. /// The stakePool address becomes the validator identity. + /// Caller must have previously submitted a matching `commitConsensusPubkey` + /// in a strictly earlier block (see audit #580). /// @param stakePool Address of the stake pool (must be created by Staking factory) /// @param moniker Display name for the validator (max 31 bytes) /// @param consensusPubkey BLS public key for consensus @@ -218,7 +243,11 @@ interface IValidatorManagement { /// @notice Rotate the validator's consensus key /// @dev Only callable by the validator's operator. - /// New key takes effect immediately (no epoch delay). + /// Rotation uses the same commit-reveal flow as registration (audit #580): + /// caller must have previously submitted a matching `commitConsensusPubkey` + /// for (newPubkey, stakePool) in a strictly earlier block. + /// The new key is reserved immediately but only takes effect at the next + /// epoch boundary (see rotateConsensusKey implementation). /// @param stakePool Address of the validator's stake pool /// @param newPubkey New BLS public key /// @param newPop New proof of possession diff --git a/src/staking/ValidatorManagement.sol b/src/staking/ValidatorManagement.sol index fbaf52f..9231e5d 100644 --- a/src/staking/ValidatorManagement.sol +++ b/src/staking/ValidatorManagement.sol @@ -117,6 +117,19 @@ contract ValidatorManagement is IValidatorManagement { /// permissionless operation. Defaults to false at genesis. bool internal _permissionlessJoinEnabled; + /// @notice Consensus-pubkey commitments from commit-reveal registration (audit #580) + /// @dev committer => keccak256(abi.encode(pubkey, stakePool, chainid)) => block.number recorded. + /// A zero value means "no commit". Values are consumed (set back to zero) by + /// registerValidator / rotateConsensusKey when they match. + /// + /// The commit-reveal flow exists because the BLS PoP precompile + /// (gravity-reth bls_precompile.rs) only signs the pubkey itself — there is no + /// binding between the PoP and the registering operator/pool. Without commit-reveal, + /// a mempool front-runner that sees a victim's in-flight registerValidator tx can + /// replay the (pubkey, pop) pair into their own pool and claim + /// `_pubkeyToValidator[keccak(pubkey)]` first, DoSing the victim's registration. + mapping(address => mapping(bytes32 => uint256)) internal _pubkeyCommits; + // ======================================================================== // INITIALIZATION // ======================================================================== @@ -245,6 +258,17 @@ contract ValidatorManagement is IValidatorManagement { // REGISTRATION // ======================================================================== + /// @inheritdoc IValidatorManagement + function commitConsensusPubkey( + bytes32 commitment + ) external { + if (_pubkeyCommits[msg.sender][commitment] != 0) { + revert Errors.ConsensusPubkeyCommitAlreadyExists(); + } + _pubkeyCommits[msg.sender][commitment] = block.number; + emit ConsensusPubkeyCommitted(msg.sender, commitment, block.number); + } + /// @inheritdoc IValidatorManagement function registerValidator( address stakePool, @@ -262,12 +286,35 @@ contract ValidatorManagement is IValidatorManagement { // Validate inputs and get required data _validateRegistration(stakePool, moniker); + // Audit #580: consume the commit-reveal commitment that binds this msg.sender + // to (consensusPubkey, stakePool) BEFORE doing any state writes. + _consumeConsensusPubkeyCommit(stakePool, consensusPubkey); + // Create validator record _createValidatorRecord(stakePool, moniker, consensusPubkey, consensusPop, networkAddresses, fullnodeAddresses); emit ValidatorRegistered(stakePool, moniker); } + /// @notice Consume a matching consensus-pubkey commitment for msg.sender + /// @dev Reverts if there is no commitment, or if the commitment was recorded in the + /// current block (same-block commit+reveal would let a front-runner still win — + /// the commit must be in a strictly earlier block than the reveal). + function _consumeConsensusPubkeyCommit( + address stakePool, + bytes calldata consensusPubkey + ) internal { + bytes32 commitment = keccak256(abi.encode(consensusPubkey, stakePool, block.chainid)); + uint256 committedAt = _pubkeyCommits[msg.sender][commitment]; + if (committedAt == 0) { + revert Errors.ConsensusPubkeyCommitNotFound(); + } + if (block.number <= committedAt) { + revert Errors.ConsensusPubkeyCommitTooRecent(); + } + delete _pubkeyCommits[msg.sender][commitment]; + } + /// @notice Validate registration inputs function _validateRegistration( address stakePool, @@ -549,6 +596,11 @@ contract ValidatorManagement is IValidatorManagement { // Validate consensus pubkey with proof of possession _validateConsensusPubkey(newPubkey, newPop); + // Audit #580: rotation uses the same commit-reveal binding as registration; + // without this, an attacker could squat a victim's freshly-generated rotation key + // via mempool front-run exactly as in the registration scenario. + _consumeConsensusPubkeyCommit(stakePool, newPubkey); + ValidatorRecord storage validator = _validators[stakePool]; // Check new pubkey is unique (against both active keys and other pending keys) diff --git a/test/unit/integration/ConsensusEngineFlow.t.sol b/test/unit/integration/ConsensusEngineFlow.t.sol index e5d2740..63772ae 100644 --- a/test/unit/integration/ConsensusEngineFlow.t.sol +++ b/test/unit/integration/ConsensusEngineFlow.t.sol @@ -224,6 +224,13 @@ contract ConsensusEngineFlowTest is Test { pool = _createStakePool(owner, stakeAmount); // Generate unique 48-byte pubkey based on pool address (BLS12-381 G1 compressed size) bytes memory uniquePubkey = abi.encodePacked(pool, bytes28(keccak256(abi.encodePacked(pool)))); + + // Audit #580: commit-reveal precondition for registerValidator. + bytes32 commitment = keccak256(abi.encode(uniquePubkey, pool, block.chainid)); + vm.prank(owner); + validatorManager.commitConsensusPubkey(commitment); + vm.roll(block.number + 1); + vm.prank(owner); validatorManager.registerValidator( pool, moniker, uniquePubkey, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES diff --git a/test/unit/staking/ValidatorManagement.t.sol b/test/unit/staking/ValidatorManagement.t.sol index f1f39e6..04911dc 100644 --- a/test/unit/staking/ValidatorManagement.t.sol +++ b/test/unit/staking/ValidatorManagement.t.sol @@ -139,6 +139,20 @@ contract ValidatorManagementTest is Test { pool = staking.createPool{ value: stakeAmount }(owner, owner, owner, owner, lockedUntil); } + /// @notice Submit a consensus-pubkey commit for `caller` binding `pubkey` to `pool`, + /// then advance one block so the subsequent reveal passes the + /// `block.number > committedAt` check (audit #580). + function _commitPubkey( + address caller, + address pool, + bytes memory pubkey + ) internal { + bytes32 commitment = keccak256(abi.encode(pubkey, pool, block.chainid)); + vm.prank(caller); + validatorManager.commitConsensusPubkey(commitment); + vm.roll(block.number + 1); + } + /// @notice Create a stake pool and register as validator /// @dev Generates unique consensusPubkey to avoid DuplicateConsensusPubkey function _createAndRegisterValidator( @@ -149,6 +163,7 @@ contract ValidatorManagementTest is Test { pool = _createStakePool(owner, stakeAmount); // Generate unique 48-byte pubkey based on pool address (BLS12-381 G1 compressed size) bytes memory uniquePubkey = abi.encodePacked(pool, bytes28(keccak256(abi.encodePacked(pool)))); + _commitPubkey(owner, pool, uniquePubkey); vm.prank(owner); // owner is also operator by default validatorManager.registerValidator( pool, moniker, uniquePubkey, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES @@ -182,6 +197,7 @@ contract ValidatorManagementTest is Test { function test_registerValidator_success() public { address pool = _createStakePool(alice, MIN_BOND); + _commitPubkey(alice, pool, CONSENSUS_PUBKEY); vm.prank(alice); validatorManager.registerValidator( pool, "alice-validator", CONSENSUS_PUBKEY, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES @@ -199,6 +215,7 @@ contract ValidatorManagementTest is Test { function test_registerValidator_emitsEvent() public { address pool = _createStakePool(alice, MIN_BOND); + _commitPubkey(alice, pool, CONSENSUS_PUBKEY); vm.prank(alice); vm.expectEmit(true, false, false, true); emit IValidatorManagement.ValidatorRegistered(pool, "alice-validator"); @@ -440,6 +457,7 @@ contract ValidatorManagementTest is Test { // Once reconfiguration completes, registration succeeds again mockReconfiguration.setTransitionInProgress(false); + _commitPubkey(alice, pool, CONSENSUS_PUBKEY); vm.prank(alice); validatorManager.registerValidator( pool, "alice", CONSENSUS_PUBKEY, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES @@ -748,6 +766,7 @@ contract ValidatorManagementTest is Test { hex"a666d31d6e3c5e8aab7e0f2e926f0b4307bbad66166a5598c8dde1152f2e16e964ad3e42f5e7c73e2e35c6a69b108f4e"; bytes memory newPop = hex"cafebabe"; + _commitPubkey(alice, pool, newPubkey); vm.prank(alice); validatorManager.rotateConsensusKey(pool, newPubkey, newPop); @@ -776,6 +795,7 @@ contract ValidatorManagementTest is Test { bytes memory newPubkey = hex"a666d31d6e3c5e8aab7e0f2e926f0b4307bbad66166a5598c8dde1152f2e16e964ad3e42f5e7c73e2e35c6a69b108f4e"; + _commitPubkey(alice, pool, newPubkey); vm.prank(alice); vm.expectEmit(true, false, false, true); emit IValidatorManagement.ConsensusKeyRotated(pool, newPubkey); @@ -792,13 +812,17 @@ contract ValidatorManagementTest is Test { address alicePool = _createStakePool(alice, MIN_BOND); bytes memory alicePubkey = hex"a1cecafe0000000100000000000000000000000000000000000000000000000000000000000000000000000000000000"; + _commitPubkey(alice, alicePool, alicePubkey); vm.prank(alice); validatorManager.registerValidator( alicePool, "alice", alicePubkey, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES ); - // Bob tries to register with the same pubkey - should fail + // Bob tries to register with the same pubkey - should fail with DuplicateConsensusPubkey. + // Bob must first pass the commit-reveal precondition (audit #580) before reaching the + // duplicate-pubkey check. address bobPool = _createStakePool(bob, MIN_BOND); + _commitPubkey(bob, bobPool, alicePubkey); vm.prank(bob); vm.expectRevert(abi.encodeWithSelector(Errors.DuplicateConsensusPubkey.selector, alicePubkey)); validatorManager.registerValidator( @@ -812,6 +836,7 @@ contract ValidatorManagementTest is Test { address alicePool = _createStakePool(alice, MIN_BOND); bytes memory alicePubkey = hex"a1cecafe0000000100000000000000000000000000000000000000000000000000000000000000000000000000000000"; + _commitPubkey(alice, alicePool, alicePubkey); vm.prank(alice); validatorManager.registerValidator( alicePool, "alice", alicePubkey, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES @@ -820,12 +845,15 @@ contract ValidatorManagementTest is Test { address bobPool = _createStakePool(bob, MIN_BOND); bytes memory bobPubkey = hex"b0b0b0b0b01234aa00000000000000000000000000000000000000000000000000000000000000000000000000000000"; + _commitPubkey(bob, bobPool, bobPubkey); vm.prank(bob); validatorManager.registerValidator( bobPool, "bob", bobPubkey, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES ); - // Alice tries to rotate to Bob's pubkey - should fail + // Alice tries to rotate to Bob's pubkey - should fail with DuplicateConsensusPubkey. + // Alice must first pass commit-reveal (audit #580) to reach the duplicate check. + _commitPubkey(alice, alicePool, bobPubkey); vm.prank(alice); vm.expectRevert(abi.encodeWithSelector(Errors.DuplicateConsensusPubkey.selector, bobPubkey)); validatorManager.rotateConsensusKey(alicePool, bobPubkey, hex"abcd1234"); @@ -837,6 +865,7 @@ contract ValidatorManagementTest is Test { address alicePool = _createStakePool(alice, MIN_BOND); bytes memory aliceOldPubkey = hex"a1cecafe0000000100000000000000000000000000000000000000000000000000000000000000000000000000000000"; + _commitPubkey(alice, alicePool, aliceOldPubkey); vm.prank(alice); validatorManager.registerValidator( alicePool, "alice", aliceOldPubkey, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES @@ -845,12 +874,14 @@ contract ValidatorManagementTest is Test { // Alice rotates to a new key (pending) bytes memory aliceNewPubkey = hex"a1ecafe00000000200000000000000000000000000000000000000000000000000000000000000000000000000000000"; + _commitPubkey(alice, alicePool, aliceNewPubkey); vm.prank(alice); validatorManager.rotateConsensusKey(alicePool, aliceNewPubkey, hex"abcd1234"); // Old pubkey is still reserved (active key) until epoch boundary. // Bob should NOT be able to use Alice's old key before epoch processes. address bobPool = _createStakePool(bob, MIN_BOND); + _commitPubkey(bob, bobPool, aliceOldPubkey); vm.prank(bob); vm.expectRevert(abi.encodeWithSelector(Errors.DuplicateConsensusPubkey.selector, aliceOldPubkey)); validatorManager.registerValidator( @@ -863,6 +894,8 @@ contract ValidatorManagementTest is Test { _processEpoch(); // Now old pubkey should be freed. Bob can use it. + // Bob's earlier failed register reverted the whole tx (including commit deletion), + // so his commit is still live — no need to re-commit. vm.prank(bob); validatorManager.registerValidator( bobPool, "bob", aliceOldPubkey, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES @@ -881,6 +914,7 @@ contract ValidatorManagementTest is Test { address alicePool = _createStakePool(alice, MIN_BOND); bytes memory alicePubkey = hex"a1cecafe0000000100000000000000000000000000000000000000000000000000000000000000000000000000000000"; + _commitPubkey(alice, alicePool, alicePubkey); vm.prank(alice); validatorManager.registerValidator( alicePool, "alice", alicePubkey, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES @@ -889,6 +923,7 @@ contract ValidatorManagementTest is Test { address bobPool = _createStakePool(bob, MIN_BOND); bytes memory bobPubkey = hex"b0b0b0b0b01234aa00000000000000000000000000000000000000000000000000000000000000000000000000000000"; + _commitPubkey(bob, bobPool, bobPubkey); vm.prank(bob); validatorManager.registerValidator( bobPool, "bob", bobPubkey, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES @@ -897,6 +932,7 @@ contract ValidatorManagementTest is Test { // Alice rotates to a completely new key (not Bob's) bytes memory aliceNewPubkey = hex"a1ecafe00000000300000000000000000000000000000000000000000000000000000000000000000000000000000000"; + _commitPubkey(alice, alicePool, aliceNewPubkey); vm.prank(alice); validatorManager.rotateConsensusKey(alicePool, aliceNewPubkey, hex"abcd1234"); @@ -944,6 +980,157 @@ contract ValidatorManagementTest is Test { assertEq(record.pendingFeeRecipient, address(0), "Pending should be cleared"); } + // ======================================================================== + // AUDIT #580: CONSENSUS PUBKEY COMMIT-REVEAL + // ======================================================================== + // + // The BLS PoP precompile (gravity-reth bls_precompile.rs) only signs the pubkey + // itself, with no binding to the registering operator/pool. Without commit-reveal, + // a mempool front-runner that observes a victim's (pubkey, pop) could register + // the pubkey under their own pool first and claim _pubkeyToValidator, DoSing + // the victim. Commit-reveal forces the registrant to pre-commit to the + // (pubkey, stakePool, chainid) tuple in a strictly earlier block, so the pubkey + // is not publicly observable when the commit is made. + // ======================================================================== + + function test_audit580_registerValidator_revertsWithoutCommit() public { + address pool = _createStakePool(alice, MIN_BOND); + + vm.prank(alice); + vm.expectRevert(Errors.ConsensusPubkeyCommitNotFound.selector); + validatorManager.registerValidator( + pool, "alice", CONSENSUS_PUBKEY, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES + ); + } + + function test_audit580_registerValidator_revertsSameBlockReveal() public { + address pool = _createStakePool(alice, MIN_BOND); + bytes32 commitment = keccak256(abi.encode(CONSENSUS_PUBKEY, pool, block.chainid)); + + vm.prank(alice); + validatorManager.commitConsensusPubkey(commitment); + // Intentionally no vm.roll — commit and reveal in the same block must fail. + + vm.prank(alice); + vm.expectRevert(Errors.ConsensusPubkeyCommitTooRecent.selector); + validatorManager.registerValidator( + pool, "alice", CONSENSUS_PUBKEY, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES + ); + } + + function test_audit580_commit_revertsOnDuplicateFromSameSender() public { + bytes32 commitment = keccak256(abi.encode(CONSENSUS_PUBKEY, alice, block.chainid)); + + vm.prank(alice); + validatorManager.commitConsensusPubkey(commitment); + + vm.prank(alice); + vm.expectRevert(Errors.ConsensusPubkeyCommitAlreadyExists.selector); + validatorManager.commitConsensusPubkey(commitment); + } + + function test_audit580_commit_emitsEvent() public { + bytes32 commitment = keccak256(abi.encode(CONSENSUS_PUBKEY, alice, block.chainid)); + + vm.expectEmit(true, true, false, true); + emit IValidatorManagement.ConsensusPubkeyCommitted(alice, commitment, block.number); + vm.prank(alice); + validatorManager.commitConsensusPubkey(commitment); + } + + /// @notice An attacker's commit binds the commitment to the attacker's msg.sender, + /// so it cannot be consumed by the victim's legitimate register call (or vice versa). + function test_audit580_commit_isKeyedBySender() public { + address victimPool = _createStakePool(alice, MIN_BOND); + bytes32 victimCommitment = keccak256(abi.encode(CONSENSUS_PUBKEY, victimPool, block.chainid)); + + // Attacker pre-commits the SAME hash under their own sender. + vm.prank(bob); + validatorManager.commitConsensusPubkey(victimCommitment); + vm.roll(block.number + 1); + + // Victim has not committed, so their register still fails. + vm.prank(alice); + vm.expectRevert(Errors.ConsensusPubkeyCommitNotFound.selector); + validatorManager.registerValidator( + victimPool, "alice", CONSENSUS_PUBKEY, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES + ); + } + + /// @notice End-to-end front-run scenario: victim commits in block N, attacker observes + /// the reveal tx containing the pubkey in block N+1 and tries to race. + /// The attacker can at best commit in block N+1, but then their register must + /// wait until block N+2, by which time victim has already claimed the pubkey. + function test_audit580_frontRun_squattingIsBlocked() public { + address victimPool = _createStakePool(alice, MIN_BOND); + address attackerPool = _createStakePool(bob, MIN_BOND); + + bytes memory pk = CONSENSUS_PUBKEY; + + // Block N: victim commits. + _commitPubkey(alice, victimPool, pk); + // _commitPubkey advances one block internally, so we are now in block N+1. + + // Attacker sees pk in victim's pending reveal tx and races to commit + register. + bytes32 attackerCommitment = keccak256(abi.encode(pk, attackerPool, block.chainid)); + vm.prank(bob); + validatorManager.commitConsensusPubkey(attackerCommitment); + + // Attacker's same-block register must fail (CommitTooRecent). + vm.prank(bob); + vm.expectRevert(Errors.ConsensusPubkeyCommitTooRecent.selector); + validatorManager.registerValidator( + attackerPool, "attacker", pk, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES + ); + + // Victim's register succeeds in the same block because their commit was in the PREVIOUS block. + vm.prank(alice); + validatorManager.registerValidator( + victimPool, "alice", pk, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES + ); + assertTrue(validatorManager.isValidator(victimPool), "victim registration should win the race"); + + // When the attacker's commit matures in the next block, the pubkey is already owned, + // so their register now reverts with DuplicateConsensusPubkey instead of succeeding. + vm.roll(block.number + 1); + vm.prank(bob); + vm.expectRevert(abi.encodeWithSelector(Errors.DuplicateConsensusPubkey.selector, pk)); + validatorManager.registerValidator( + attackerPool, "attacker", pk, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES + ); + } + + function test_audit580_rotateConsensusKey_revertsWithoutCommit() public { + address pool = _createAndRegisterValidator(alice, MIN_BOND, "alice"); + bytes memory newPubkey = + hex"a666d31d6e3c5e8aab7e0f2e926f0b4307bbad66166a5598c8dde1152f2e16e964ad3e42f5e7c73e2e35c6a69b108f4e"; + + vm.prank(alice); + vm.expectRevert(Errors.ConsensusPubkeyCommitNotFound.selector); + validatorManager.rotateConsensusKey(pool, newPubkey, hex"aa01"); + } + + /// @notice A successful register consumes the commit, so a second attempt with the same + /// (pubkey, stakePool) requires a fresh commit. + function test_audit580_commit_consumedOnSuccessfulReveal() public { + address pool = _createStakePool(alice, MIN_BOND); + _commitPubkey(alice, pool, CONSENSUS_PUBKEY); + vm.prank(alice); + validatorManager.registerValidator( + pool, "alice", CONSENSUS_PUBKEY, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES + ); + + // Alice registered, but the commit has been cleared. A hypothetical retry (e.g. after + // a later un-register flow were added) would need a brand-new commit. We exercise the + // "commit cleared" property directly by trying to re-commit the same hash — this must + // succeed because the slot was zeroed on consume. + bytes32 commitment = keccak256(abi.encode(CONSENSUS_PUBKEY, pool, block.chainid)); + vm.prank(alice); + validatorManager.commitConsensusPubkey(commitment); + // If the commit had NOT been cleared on consume, this re-commit would have reverted + // with ConsensusPubkeyCommitAlreadyExists. + } + // ======================================================================== // VIEW FUNCTION TESTS // ======================================================================== @@ -978,6 +1165,7 @@ contract ValidatorManagementTest is Test { validatorManager.getValidatorStatus(pool); // Register - INACTIVE + _commitPubkey(alice, pool, CONSENSUS_PUBKEY); vm.prank(alice); validatorManager.registerValidator( pool, "alice", CONSENSUS_PUBKEY, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES @@ -1016,6 +1204,7 @@ contract ValidatorManagementTest is Test { bondAmount = bound(bondAmount, MIN_BOND, MAX_BOND); address pool = _createStakePool(alice, bondAmount); + _commitPubkey(alice, pool, CONSENSUS_PUBKEY); vm.prank(alice); validatorManager.registerValidator( pool, "alice", CONSENSUS_PUBKEY, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES @@ -2478,6 +2667,7 @@ contract ValidatorManagementTest is Test { bytes memory newPubkey = hex"a666d31d6e3c5e8aab7e0f2e926f0b4307bbad66166a5598c8dde1152f2e16e964ad3e42f5e7c73e2e35c6a69b108f4e"; bytes memory newPop = hex"cafebabe"; + _commitPubkey(alice, pool, newPubkey); vm.prank(alice); validatorManager.rotateConsensusKey(pool, newPubkey, newPop); @@ -2506,16 +2696,19 @@ contract ValidatorManagementTest is Test { hex"b777e42e7f4d6f9bbc8f1f3f037f1c5418ccbe77277b66a9d9eef22630f27fa75be4f530f8d84f3f46d7b7ac219050ff"; // First rotation + _commitPubkey(alice, pool, key1); vm.prank(alice); validatorManager.rotateConsensusKey(pool, key1, hex"aa01"); // Second rotation should work (releases key1 reservation) + _commitPubkey(alice, pool, key2); vm.prank(alice); validatorManager.rotateConsensusKey(pool, key2, hex"bb02"); // key1 should now be available for others address bobPool = _createAndRegisterValidator(bob, MIN_BOND, "bob"); // Bob should be able to rotate to key1 (it was released by Alice's second rotation) + _commitPubkey(bob, bobPool, key1); vm.prank(bob); validatorManager.rotateConsensusKey(bobPool, key1, hex"cc03"); diff --git a/test/unit/staking/ValidatorWhitelist.t.sol b/test/unit/staking/ValidatorWhitelist.t.sol index 0cf0956..04bd8f9 100644 --- a/test/unit/staking/ValidatorWhitelist.t.sol +++ b/test/unit/staking/ValidatorWhitelist.t.sol @@ -121,10 +121,15 @@ contract ValidatorWhitelistTest is Test { address pool, string memory moniker ) internal { + bytes memory pk = _uniquePubkey(pool); + // Audit #580: commit-reveal precondition. + bytes32 commitment = keccak256(abi.encode(pk, pool, block.chainid)); vm.prank(owner); - validatorManager.registerValidator( - pool, moniker, _uniquePubkey(pool), CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES - ); + validatorManager.commitConsensusPubkey(commitment); + vm.roll(block.number + 1); + + vm.prank(owner); + validatorManager.registerValidator(pool, moniker, pk, CONSENSUS_POP, NETWORK_ADDRESSES, FULLNODE_ADDRESSES); } function _allow(