refactor: separate BLSSignatureChecker storage

Author: 0xClandestine

src/BLSApkRegistryStorage.sol

@@ -23,7 +23,7 @@ abstract contract BLSApkRegistryStorage is Initializable, IBLSApkRegistry {
 
     /// @inheritdoc IBLSApkRegistry
     mapping(bytes32 pubkeyHash => address operator) public pubkeyHashToOperator;
-    
+
     /// @inheritdoc IBLSApkRegistry
     mapping(address operator => BN254.G1Point pubkeyG1) public operatorToPubkey;
 

src/BLSSignatureChecker.sol

@@ -1,91 +1,45 @@
 // SPDX-License-Identifier: BUSL-1.1
 pragma solidity ^0.8.27;
 
-import {IBLSSignatureChecker} from "./interfaces/IBLSSignatureChecker.sol";
-import {IRegistryCoordinator} from "./interfaces/IRegistryCoordinator.sol";
-import {IBLSApkRegistry} from "./interfaces/IBLSApkRegistry.sol";
-import {IStakeRegistry, IDelegationManager} from "./interfaces/IStakeRegistry.sol";
-
 import {BitmapUtils} from "./libraries/BitmapUtils.sol";
 import {BN254} from "./libraries/BN254.sol";
 
+import "./BLSSignatureCheckerStorage.sol";
+
 /**
  * @title Used for checking BLS aggregate signatures from the operators of a `BLSRegistry`.
  * @author Layr Labs, Inc.
  * @notice Terms of Service: https://docs.eigenlayer.xyz/overview/terms-of-service
  * @notice This is the contract for checking the validity of aggregate operator signatures.
  */
-contract BLSSignatureChecker is IBLSSignatureChecker {
+contract BLSSignatureChecker is BLSSignatureCheckerStorage {
     using BN254 for BN254.G1Point;
 
-    // CONSTANTS & IMMUTABLES
-
-    // gas cost of multiplying 2 pairings
-    uint256 internal constant PAIRING_EQUALITY_CHECK_GAS = 120_000;
-
-    IRegistryCoordinator public immutable registryCoordinator;
-    IStakeRegistry public immutable stakeRegistry;
-    IBLSApkRegistry public immutable blsApkRegistry;
-    IDelegationManager public immutable delegation;
-    /// @notice If true, check the staleness of the operator stakes and that its within the delegation withdrawalDelayBlocks window.
-    bool public staleStakesForbidden;
+    /// MODIFIERS
 
     modifier onlyCoordinatorOwner() {
         require(msg.sender == registryCoordinator.owner(), OnlyRegistryCoordinatorOwner());
         _;
     }
 
+    /// CONSTRUCTION
+
     constructor(
         IRegistryCoordinator _registryCoordinator
-    ) {
-        registryCoordinator = _registryCoordinator;
-        stakeRegistry = _registryCoordinator.stakeRegistry();
-        blsApkRegistry = _registryCoordinator.blsApkRegistry();
-        delegation = stakeRegistry.delegation();
-    }
+    ) BLSSignatureCheckerStorage(_registryCoordinator) {}
+
+    /// ACTIONS
 
-    /**
-     * /**
-     * RegistryCoordinator owner can either enforce or not that operator stakes are staler
-     * than the delegation.minWithdrawalDelayBlocks() window.
-     * @param value to toggle staleStakesForbidden
-     */
+    /// @inheritdoc IBLSSignatureChecker
     function setStaleStakesForbidden(
         bool value
     ) external onlyCoordinatorOwner {
         _setStaleStakesForbidden(value);
     }
 
-    struct NonSignerInfo {
-        uint256[] quorumBitmaps;
-        bytes32[] pubkeyHashes;
-    }
+    /// VIEW
 
-    /**
-     * @notice This function is called by disperser when it has aggregated all the signatures of the operators
-     * that are part of the quorum for a particular taskNumber and is asserting them into onchain. The function
-     * checks that the claim for aggregated signatures are valid.
-     *
-     * The thesis of this procedure entails:
-     * - getting the aggregated pubkey of all registered nodes at the time of pre-commit by the
-     * disperser (represented by apk in the parameters),
-     * - subtracting the pubkeys of all the signers not in the quorum (nonSignerPubkeys) and storing
-     * the output in apk to get aggregated pubkey of all operators that are part of quorum.
-     * - use this aggregated pubkey to verify the aggregated signature under BLS scheme.
-     *
-     * @dev Before signature verification, the function verifies operator stake information.  This includes ensuring that the provided `referenceBlockNumber`
-     * is correct, i.e., ensure that the stake returned from the specified block number is recent enough and that the stake is either the most recent update
-     * for the total stake (of the operator) or latest before the referenceBlockNumber.
-     * @param msgHash is the hash being signed
-     * @dev NOTE: Be careful to ensure `msgHash` is collision-resistant! This method does not hash
-     * `msgHash` in any way, so if an attacker is able to pass in an arbitrary value, they may be able
-     * to tamper with signature verification.
-     * @param quorumNumbers is the bytes array of quorum numbers that are being signed for
-     * @param referenceBlockNumber is the block number at which the stake information is being verified
-     * @param params is the struct containing information on nonsigners, stakes, quorum apks, and the aggregate signature
-     * @return quorumStakeTotals is the struct containing the total and signed stake for each quorum
-     * @return signatoryRecordHash is the hash of the signatory record, which is used for fraud proofs
-     */
+    /// @inheritdoc IBLSSignatureChecker
     function checkSignatures(
         bytes32 msgHash,
         bytes calldata quorumNumbers,
@@ -253,15 +207,7 @@ contract BLSSignatureChecker is IBLSSignatureChecker {
         return (stakeTotals, signatoryRecordHash);
     }
 
-    /**
-     * trySignatureAndApkVerification verifies a BLS aggregate signature and the veracity of a calculated G1 Public key
-     * @param msgHash is the hash being signed
-     * @param apk is the claimed G1 public key
-     * @param apkG2 is provided G2 public key
-     * @param sigma is the G1 point signature
-     * @return pairingSuccessful is true if the pairing precompile call was successful
-     * @return siganatureIsValid is true if the signature is valid
-     */
+    /// @inheritdoc IBLSSignatureChecker
     function trySignatureAndApkVerification(
         bytes32 msgHash,
         BN254.G1Point memory apk,
@@ -300,8 +246,4 @@ contract BLSSignatureChecker is IBLSSignatureChecker {
         staleStakesForbidden = value;
         emit StaleStakesForbiddenUpdate(value);
     }
-
-    // storage gap for upgradeability
-    // slither-disable-next-line shadowing-state
-    uint256[49] private __GAP;
 }

src/BLSSignatureCheckerStorage.sol

@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: BUSL-1.1
+pragma solidity ^0.8.27;
+
+import {IBLSSignatureChecker} from "./interfaces/IBLSSignatureChecker.sol";
+import {IRegistryCoordinator} from "./interfaces/IRegistryCoordinator.sol";
+import {IBLSApkRegistry} from "./interfaces/IBLSApkRegistry.sol";
+import {IStakeRegistry, IDelegationManager} from "./interfaces/IStakeRegistry.sol";
+
+abstract contract BLSSignatureCheckerStorage is IBLSSignatureChecker {
+    /// @dev Returns the assumed gas cost of multiplying 2 pairings.
+    uint256 internal constant PAIRING_EQUALITY_CHECK_GAS = 120_000;
+
+    /// @inheritdoc IBLSSignatureChecker
+    IRegistryCoordinator public immutable registryCoordinator;
+    /// @inheritdoc IBLSSignatureChecker
+    IStakeRegistry public immutable stakeRegistry;
+    /// @inheritdoc IBLSSignatureChecker
+    IBLSApkRegistry public immutable blsApkRegistry;
+    /// @inheritdoc IBLSSignatureChecker
+    IDelegationManager public immutable delegation;
+
+    /// STATE
+
+    /// @inheritdoc IBLSSignatureChecker
+    bool public staleStakesForbidden;
+
+    constructor(
+        IRegistryCoordinator _registryCoordinator
+    ) {
+        registryCoordinator = _registryCoordinator;
+        stakeRegistry = _registryCoordinator.stakeRegistry();
+        blsApkRegistry = _registryCoordinator.blsApkRegistry();
+        delegation = stakeRegistry.delegation();
+    }
+    
+    // slither-disable-next-line shadowing-state
+    uint256[49] private __GAP;
+}

src/interfaces/IBLSApkRegistry.sol

@@ -84,12 +84,16 @@ interface IBLSApkRegistry is IRegistry, IBLSApkRegistryErrors, IBLSApkRegistryEv
     /// @notice Maps `operator` to their BLS public key hash (`operatorId`).
     /// @param operator The address of the operator.
     /// @return operatorId The hash of the operator's BLS public key.
-    function operatorToPubkeyHash(address operator) external view returns (bytes32 operatorId);
+    function operatorToPubkeyHash(
+        address operator
+    ) external view returns (bytes32 operatorId);
 
     /// @notice Maps `pubkeyHash` to their corresponding `operator` address.
     /// @param pubkeyHash The hash of a BLS public key.
     /// @return operator The address of the operator who registered this public key.
-    function pubkeyHashToOperator(bytes32 pubkeyHash) external view returns (address operator);
+    function pubkeyHashToOperator(
+        bytes32 pubkeyHash
+    ) external view returns (address operator);
 
     /// @notice Maps `operator` to their BLS public key in G1.
     /// @dev Returns a non-encoded BN254.G1Point.

src/interfaces/IBLSSignatureChecker.sol

@@ -31,25 +31,41 @@ interface IBLSSignatureCheckerErrors {
 }
 
 interface IBLSSignatureCheckerTypes {
+    /// @notice Contains bitmap and pubkey hash information for non-signing operators.
+    /// @param quorumBitmaps Array of bitmaps indicating which quorums each non-signer was registered for.
+    /// @param pubkeyHashes Array of BLS public key hashes for each non-signer.
+    struct NonSignerInfo {
+        uint256[] quorumBitmaps;
+        bytes32[] pubkeyHashes;
+    }
+
+    /// @notice Contains non-signer information and aggregated signature data for BLS verification.
+    /// @param nonSignerQuorumBitmapIndices The indices of all non-signer quorum bitmaps.
+    /// @param nonSignerPubkeys The G1 public keys of all non-signers.
+    /// @param quorumApks The aggregate G1 public key of each quorum.
+    /// @param apkG2 The aggregate G2 public key of all signers.
+    /// @param sigma The aggregate G1 signature of all signers.
+    /// @param quorumApkIndices The indices of each quorum's aggregate public key in the APK registry.
+    /// @param totalStakeIndices The indices of each quorum's total stake in the stake registry.
+    /// @param nonSignerStakeIndices The indices of each non-signer's stake within each quorum.
+    /// @dev Used as input to checkSignatures() to verify BLS signatures.
     struct NonSignerStakesAndSignature {
-        uint32[] nonSignerQuorumBitmapIndices; // is the indices of all nonsigner quorum bitmaps
-        BN254.G1Point[] nonSignerPubkeys; // is the G1 pubkeys of all nonsigners
-        BN254.G1Point[] quorumApks; // is the aggregate G1 pubkey of each quorum
-        BN254.G2Point apkG2; // is the aggregate G2 pubkey of all signers
-        BN254.G1Point sigma; // is the aggregate G1 signature of all signers
-        uint32[] quorumApkIndices; // is the indices of each quorum aggregate pubkey
-        uint32[] totalStakeIndices; // is the indices of each quorums total stake
-        uint32[][] nonSignerStakeIndices; // is the indices of each non signers stake within a quorum
+        uint32[] nonSignerQuorumBitmapIndices;
+        BN254.G1Point[] nonSignerPubkeys;
+        BN254.G1Point[] quorumApks;
+        BN254.G2Point apkG2;
+        BN254.G1Point sigma;
+        uint32[] quorumApkIndices;
+        uint32[] totalStakeIndices;
+        uint32[][] nonSignerStakeIndices;
     }
 
-    /**
-     * @notice this data structure is used for recording the details on the total stake of the registered
-     * operators and those operators who are part of the quorum for a particular taskNumber
-     */
+    /// @notice Records the total stake amounts for operators in each quorum.
+    /// @param signedStakeForQuorum Array of total stake amounts from operators who signed, per quorum.
+    /// @param totalStakeForQuorum Array of total stake amounts from all operators, per quorum.
+    /// @dev Used to track stake distribution and calculate quorum thresholds. Array indices correspond to quorum numbers.
     struct QuorumStakeTotals {
-        // total stake of the operators in each quorum
         uint96[] signedStakeForQuorum;
-        // total amount staked by all operators in each quorum
         uint96[] totalStakeForQuorum;
     }
 }
@@ -62,49 +78,73 @@ interface IBLSSignatureCheckerEvents is IBLSSignatureCheckerTypes {
 interface IBLSSignatureChecker is IBLSSignatureCheckerErrors, IBLSSignatureCheckerEvents {
     /// STATE
 
+    /// @notice Returns the address of the registry coordinator contract.
+    /// @return The address of the registry coordinator.
+    /// @dev This value is immutable and set during contract construction.
     function registryCoordinator() external view returns (IRegistryCoordinator);
 
+    /// @notice Returns the address of the stake registry contract.
+    /// @return The address of the stake registry.
+    /// @dev This value is immutable and set during contract construction.
     function stakeRegistry() external view returns (IStakeRegistry);
-    
+
+    /// @notice Returns the address of the BLS APK registry contract.
+    /// @return The address of the BLS APK registry.
+    /// @dev This value is immutable and set during contract construction.
     function blsApkRegistry() external view returns (IBLSApkRegistry);
-    
+
+    /// @notice Returns the address of the delegation manager contract.
+    /// @return The address of the delegation manager.
+    /// @dev This value is immutable and set during contract construction.
     function delegation() external view returns (IDelegationManager);
-    
+
+    /// @notice Returns whether stale stakes are forbidden in signature verification.
+    /// @return True if stale stakes are forbidden, false otherwise.
     function staleStakesForbidden() external view returns (bool);
 
     // ACTIONS
 
-    function setStaleStakesForbidden(bool value) external;
+    /// @notice Sets `value` as the new staleStakesForbidden flag.
+    /// @param value True to forbid stale stakes, false to allow them.
+    /// @dev Access restricted to the registry coordinator owner.
+    function setStaleStakesForbidden(
+        bool value
+    ) external;
 
     /// VIEW
 
-    /**
-     * @notice This function is called by disperser when it has aggregated all the signatures of the operators
-     * that are part of the quorum for a particular taskNumber and is asserting them into onchain. The function
-     * checks that the claim for aggregated signatures are valid.
-     *
-     * The thesis of this procedure entails:
-     * - getting the aggregated pubkey of all registered nodes at the time of pre-commit by the
-     * disperser (represented by apk in the parameters),
-     * - subtracting the pubkeys of all the signers not in the quorum (nonSignerPubkeys) and storing
-     * the output in apk to get aggregated pubkey of all operators that are part of quorum.
-     * - use this aggregated pubkey to verify the aggregated signature under BLS scheme.
-     *
-     * @dev Before signature verification, the function verifies operator stake information.  This includes ensuring that the provided `referenceBlockNumber`
-     * is correct, i.e., ensure that the stake returned from the specified block number is recent enough and that the stake is either the most recent update
-     * for the total stake (or the operator) or latest before the referenceBlockNumber.
-     */
+    /// @notice Verifies aggregated BLS signatures and stake information for message hash `msgHash` across `quorumNumbers` at block `referenceBlockNumber`.
+    /// @param msgHash The hash of the message that was signed.
+    /// @param quorumNumbers The quorum numbers to verify signatures for, where each byte is an 8-bit integer.
+    /// @param referenceBlockNumber The block number to use for stake information.
+    /// @param nonSignerStakesAndSignature Contains non-signer information and aggregated signature data.
+    /// @return Stake totals for each quorum and a hash of the verification data.
+    /// @dev The thesis of this procedure entails:
+    ///      1. Getting the aggregated pubkey of all registered nodes at the time of pre-commit
+    ///      2. Subtracting the pubkeys of all non-signers (nonSignerPubkeys)
+    ///      3. Using the resulting aggregated pubkey to verify the aggregated signature under BLS scheme
+    /// @dev Before signature verification, the function verifies:
+    ///      1. The reference block number is valid and recent enough
+    ///      2. Stake information is either most recent or latest before reference block
+    ///      3. The aggregated signature is valid under the BLS scheme
     function checkSignatures(
         bytes32 msgHash,
         bytes calldata quorumNumbers,
         uint32 referenceBlockNumber,
         NonSignerStakesAndSignature memory nonSignerStakesAndSignature
     ) external view returns (QuorumStakeTotals memory, bytes32);
 
+    /// @notice Attempts to verify signature `sigma` against message hash `msgHash` using aggregate public keys `apk` and `apkG2`.
+    /// @param msgHash The hash of the message that was signed.
+    /// @param apk The aggregate public key in G1.
+    /// @param apkG2 The aggregate public key in G2.
+    /// @param sigma The signature to verify.
+    /// @return pairingSuccessful True if the pairing check succeeded.
+    /// @return siganatureIsValid True if the signature is valid.
     function trySignatureAndApkVerification(
         bytes32 msgHash,
         BN254.G1Point memory apk,
         BN254.G2Point memory apkG2,
         BN254.G1Point memory sigma
     ) external view returns (bool pairingSuccessful, bool siganatureIsValid);
-}
+}

test/unit/BLSSignatureCheckerUnit.t.sol

@@ -3,7 +3,10 @@ pragma solidity ^0.8.27;
 
 import "../../src/BLSSignatureChecker.sol";
 import "../utils/BLSMockAVSDeployer.sol";
-import {IBLSSignatureCheckerErrors, IBLSSignatureCheckerTypes} from "../../src/interfaces/IBLSSignatureChecker.sol";
+import {
+    IBLSSignatureCheckerErrors,
+    IBLSSignatureCheckerTypes
+} from "../../src/interfaces/IBLSSignatureChecker.sol";
 import {IBLSApkRegistryErrors} from "../../src/interfaces/IBLSApkRegistry.sol";
 import {QuorumBitmapHistoryLib} from "../../src/libraries/QuorumBitmapHistoryLib.sol";
 import {IStakeRegistryErrors} from "../../src/interfaces/IStakeRegistry.sol";