fix: address tmnt 158 (#16456)

Please read [contributing guidelines](CONTRIBUTING.md) and remove this
line.

For audit-related pull requests, please use the [audit PR
template](?expand=1&template=audit.md).

Author: LHerskind

l1-contracts/src/governance/GSE.sol

@@ -184,13 +184,6 @@ contract GSECore is IGSECore, Ownable {
   // intended.
   IERC20 public immutable ASSET;
 
-  // the `gap` pushes the `checkProofOfPossession` into its own slot
-  // so we don't have the trouble of being in the middle of a slot
-  uint256 private gap = 0;
-
-  // @note  Always true, exists to override to false for testing only.
-  bool public checkProofOfPossession = true;
-
   // The GSE's history of rollups.
   Checkpoints.Trace224 internal rollups;
   // Mapping from instance address to its historical attester information.
@@ -332,28 +325,7 @@ contract GSECore is IGSECore, Ownable {
       instances[recipientInstance].attesters.add(_attester), Errors.GSE__AlreadyRegistered(recipientInstance, _attester)
     );
 
-    if (checkProofOfPossession) {
-      // Make sure the attester has not registered before
-      G1Point memory previouslyRegisteredPoint = configOf[_attester].publicKey;
-      require(
-        (previouslyRegisteredPoint.x == 0 && previouslyRegisteredPoint.y == 0),
-        Errors.GSE__CannotChangePublicKeys(previouslyRegisteredPoint.x, previouslyRegisteredPoint.y)
-      );
-
-      // Make sure the incoming point has not been seen before
-      // NOTE: we only need to check for the existence of Pk1, and not also for Pk2,
-      // as the Pk2 will be constrained to have the same underlying secret key as part of the proofOfPossession,
-      // so existence/correctness of Pk2 is implied by existence/correctness of Pk1.
-      bytes32 hashedIncomingPoint = keccak256(abi.encodePacked(_publicKeyInG1.x, _publicKeyInG1.y));
-      require((!ownedPKs[hashedIncomingPoint]), Errors.GSE__ProofOfPossessionAlreadySeen(hashedIncomingPoint));
-
-      require(
-        BN254Lib.proofOfPossession(_publicKeyInG1, _publicKeyInG2, _proofOfPossession),
-        Errors.GSE__InvalidProofOfPossession()
-      );
-
-      ownedPKs[hashedIncomingPoint] = true;
-    }
+    _checkProofOfPossession(_attester, _publicKeyInG1, _publicKeyInG2, _proofOfPossession);
 
     // This is the ONLY place where we set the configuration for an attester.
     // This means that their withdrawer and public keys are set once, globally.
@@ -616,6 +588,33 @@ contract GSECore is IGSECore, Ownable {
     getGovernance().vote(_proposalId, _amount, _support);
   }
 
+  function _checkProofOfPossession(
+    address _attester,
+    G1Point memory _publicKeyInG1,
+    G2Point memory _publicKeyInG2,
+    G1Point memory _proofOfPossession
+  ) internal virtual {
+    // Make sure the attester has not registered before
+    G1Point memory previouslyRegisteredPoint = configOf[_attester].publicKey;
+    require(
+      (previouslyRegisteredPoint.x == 0 && previouslyRegisteredPoint.y == 0),
+      Errors.GSE__CannotChangePublicKeys(previouslyRegisteredPoint.x, previouslyRegisteredPoint.y)
+    );
+
+    // Make sure the incoming point has not been seen before
+    // NOTE: we only need to check for the existence of Pk1, and not also for Pk2,
+    // as the Pk2 will be constrained to have the same underlying secret key as part of the proofOfPossession,
+    // so existence/correctness of Pk2 is implied by existence/correctness of Pk1.
+    bytes32 hashedIncomingPoint = keccak256(abi.encodePacked(_publicKeyInG1.x, _publicKeyInG1.y));
+    require((!ownedPKs[hashedIncomingPoint]), Errors.GSE__ProofOfPossessionAlreadySeen(hashedIncomingPoint));
+    ownedPKs[hashedIncomingPoint] = true;
+
+    require(
+      BN254Lib.proofOfPossession(_publicKeyInG1, _publicKeyInG2, _proofOfPossession),
+      Errors.GSE__InvalidProofOfPossession()
+    );
+  }
+
   function _pendingThrough(uint256 _proposalId) internal view returns (Timestamp) {
     return getGovernance().getProposal(_proposalId).pendingThroughMemory();
   }

l1-contracts/test/GSEWithSkip.sol

@@ -0,0 +1,35 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright 2024 Aztec Labs.
+pragma solidity >=0.8.27;
+
+import {GSE} from "@aztec/governance/GSE.sol";
+import {IERC20} from "@oz/token/ERC20/IERC20.sol";
+import {BN254Lib, G1Point, G2Point} from "@aztec/shared/libraries/BN254Lib.sol";
+
+contract GSEWithSkip is GSE {
+  // the `gap` pushes the `checkProofOfPossession` into its own slot
+  // so we don't have the trouble of being in the middle of a slot
+  uint256 private gap = 0;
+
+  // @note  Always true, exists to override to false for testing only.
+  bool public checkProofOfPossession = true;
+
+  constructor(address __owner, IERC20 _asset, uint256 _activationThreshold, uint256 _ejectionThreshold)
+    GSE(__owner, _asset, _activationThreshold, _ejectionThreshold)
+  {}
+
+  function setCheckProofOfPossession(bool _shouldCheck) external {
+    checkProofOfPossession = _shouldCheck;
+  }
+
+  function _checkProofOfPossession(
+    address _attester,
+    G1Point memory _publicKeyInG1,
+    G2Point memory _publicKeyInG2,
+    G1Point memory _proofOfPossession
+  ) internal override {
+    if (checkProofOfPossession) {
+      super._checkProofOfPossession(_attester, _publicKeyInG1, _publicKeyInG2, _proofOfPossession);
+    }
+  }
+}

l1-contracts/test/builder/GseBuilder.sol

@@ -4,6 +4,7 @@
 pragma solidity >=0.8.27;
 
 import {GSE} from "@aztec/governance/GSE.sol";
+import {GSEWithSkip} from "@test/GSEWithSkip.sol";
 import {TestBase} from "@test/base/Base.sol";
 import {TestConstants} from "@test/harnesses/TestConstants.sol";
 
@@ -70,12 +71,12 @@ contract GSEBuilder is TestBase {
     }
 
     if (address(config.gse) == address(0)) {
-      config.gse =
-        new GSE(address(this), config.testERC20, TestConstants.ACTIVATION_THRESHOLD, TestConstants.EJECTION_THRESHOLD);
-      vm.label(address(config.gse), "GSE");
-      stdstore.target(address(config.gse)).sig("checkProofOfPossession()").checked_write(
-        config.flags.checkProofOfPossession
+      config.gse = new GSEWithSkip(
+        address(this), config.testERC20, TestConstants.ACTIVATION_THRESHOLD, TestConstants.EJECTION_THRESHOLD
       );
+      vm.label(address(config.gse), "GSE");
+
+      GSEWithSkip(address(config.gse)).setCheckProofOfPossession(config.flags.checkProofOfPossession);
     }
 
     if (address(config.registry) == address(0)) {

l1-contracts/test/builder/RollupBuilder.sol

@@ -19,6 +19,7 @@ import {Test} from "forge-std/Test.sol";
 import {MultiAdder, CheatDepositArgs} from "@aztec/mock/MultiAdder.sol";
 import {CoinIssuer} from "@aztec/governance/CoinIssuer.sol";
 import {stdStorage, StdStorage} from "forge-std/Test.sol";
+import {GSEWithSkip} from "@test/GSEWithSkip.sol";
 
 // Stack the layers to avoid the stack too deep 🧌
 struct ConfigFlags {
@@ -228,12 +229,11 @@ contract RollupBuilder is Test {
     }
 
     if (address(config.gse) == address(0)) {
-      config.gse =
-        new GSE(address(this), config.testERC20, TestConstants.ACTIVATION_THRESHOLD, TestConstants.EJECTION_THRESHOLD);
-
-      stdstore.target(address(config.gse)).sig("checkProofOfPossession()").checked_write(
-        config.flags.checkProofOfPossession
+      config.gse = new GSEWithSkip(
+        address(this), config.testERC20, TestConstants.ACTIVATION_THRESHOLD, TestConstants.EJECTION_THRESHOLD
       );
+
+      GSEWithSkip(address(config.gse)).setCheckProofOfPossession(config.flags.checkProofOfPossession);
     }
 
     if (address(config.registry) == address(0)) {

l1-contracts/test/governance/gse/gse/depositBN254.t.sol

@@ -6,6 +6,7 @@ import {Errors} from "@aztec/governance/libraries/Errors.sol";
 import {AttesterConfig} from "@aztec/governance/GSE.sol";
 import {stdStorage, StdStorage} from "forge-std/Test.sol";
 import {WithGSE} from "./base.sol";
+import {GSEWithSkip} from "@test/GSEWithSkip.sol";
 
 contract DepositBN254Test is WithGSE {
   using stdStorage for StdStorage;
@@ -23,7 +24,7 @@ contract DepositBN254Test is WithGSE {
 
   function setUp() public override {
     super.setUp();
-    stdstore.target(address(gse)).sig("checkProofOfPossession()").checked_write(true);
+    GSEWithSkip(address(gse)).setCheckProofOfPossession(true);
     // See yarn-project/ethereum/src/test/bn254_registration.test.ts for construction of pk2
     // Prefilling here, and the rest of the data will be generated using the helper
     // generateProofsOfPossession()

l1-contracts/test/staking/invalidDeposit.t.sol

@@ -13,6 +13,7 @@ import {Rollup} from "@aztec/core/Rollup.sol";
 import {G1Point, G2Point} from "@aztec/shared/libraries/BN254Lib.sol";
 import {BN254Fixtures} from "../shared/BN254Fixtures.t.sol";
 import {stdStorage, StdStorage} from "forge-std/Test.sol";
+import {GSEWithSkip} from "@test/GSEWithSkip.sol";
 
 contract InvalidPointsFlushEntryQueueTest is StakingBase, BN254Fixtures {
   using stdStorage for StdStorage;
@@ -31,7 +32,7 @@ contract InvalidPointsFlushEntryQueueTest is StakingBase, BN254Fixtures {
     StakingBase.setUp();
 
     GSE gse = staking.getGSE();
-    stdstore.target(address(gse)).sig("checkProofOfPossession()").checked_write(true);
+    GSEWithSkip(address(gse)).setCheckProofOfPossession(true);
 
     StakingQueueConfig memory stakingQueueConfig = StakingQueueConfig({
       bootstrapValidatorSetSize: 0,