Misc. Fixes to HybridUSDCTokenPool (#14922)

* copy files from PR on ccip repo and re-open

* forgot to include changeset file

* [Bot] Update changeset file with jira issues

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>

contracts/.changeset/moody-humans-judge.md

@@ -0,0 +1,10 @@
+---
+'@chainlink/contracts': patch
+---
+
+Minor fixes to formatting, pragma, imports, etc. for Hybrid USDC Token Pools #bugfix
+
+
+PR issue: CCIP-3014
+
+Solidity Review issue: CCIP-3966

contracts/gas-snapshots/ccip.gas-snapshot

@@ -15,6 +15,7 @@ BurnMintTokenPool_lockOrBurn:test_Setup_Success() (gas: 17851)
 BurnMintTokenPool_releaseOrMint:test_ChainNotAllowed_Revert() (gas: 28783)
 BurnMintTokenPool_releaseOrMint:test_PoolMintNotHealthy_Revert() (gas: 56259)
 BurnMintTokenPool_releaseOrMint:test_PoolMint_Success() (gas: 112372)
+BurnMintWithLockReleaseFlagTokenPool_lockOrBurn:test_LockOrBurn_CorrectReturnData_Success() (gas: 242288)
 BurnWithFromMintTokenPool_lockOrBurn:test_ChainNotAllowed_Revert() (gas: 28842)
 BurnWithFromMintTokenPool_lockOrBurn:test_PoolBurnRevertNotHealthy_Revert() (gas: 55271)
 BurnWithFromMintTokenPool_lockOrBurn:test_PoolBurn_Success() (gas: 244092)
@@ -197,33 +198,38 @@ FeeQuoter_validateDestFamilyAddress:test_InvalidEVMAddressPrecompiles_Revert() (
 FeeQuoter_validateDestFamilyAddress:test_InvalidEVMAddress_Revert() (gas: 10840)
 FeeQuoter_validateDestFamilyAddress:test_ValidEVMAddress_Success() (gas: 6775)
 FeeQuoter_validateDestFamilyAddress:test_ValidNonEVMAddress_Success() (gas: 6489)
-HybridUSDCTokenPoolMigrationTests:test_LockOrBurn_LockReleaseMechanism_then_switchToPrimary_Success() (gas: 209230)
-HybridUSDCTokenPoolMigrationTests:test_LockOrBurn_PrimaryMechanism_Success() (gas: 135879)
-HybridUSDCTokenPoolMigrationTests:test_LockOrBurn_WhileMigrationPause_Revert() (gas: 107090)
-HybridUSDCTokenPoolMigrationTests:test_LockOrBurn_onLockReleaseMechanism_Success() (gas: 144653)
-HybridUSDCTokenPoolMigrationTests:test_MintOrRelease_OnLockReleaseMechanism_Success() (gas: 214817)
-HybridUSDCTokenPoolMigrationTests:test_MintOrRelease_OnLockReleaseMechanism_then_switchToPrimary_Success() (gas: 423690)
-HybridUSDCTokenPoolMigrationTests:test_MintOrRelease_incomingMessageWithPrimaryMechanism() (gas: 268967)
-HybridUSDCTokenPoolMigrationTests:test_ReleaseOrMint_WhileMigrationPause_Revert() (gas: 111484)
-HybridUSDCTokenPoolMigrationTests:test_burnLockedUSDC_invalidPermissions_Revert() (gas: 39362)
-HybridUSDCTokenPoolMigrationTests:test_cancelExistingCCTPMigrationProposal() (gas: 33172)
-HybridUSDCTokenPoolMigrationTests:test_cannotCancelANonExistentMigrationProposal() (gas: 12714)
-HybridUSDCTokenPoolMigrationTests:test_cannotModifyLiquidityWithoutPermissions_Revert() (gas: 13329)
-HybridUSDCTokenPoolMigrationTests:test_cannotTransferLiquidityDuringPendingMigration_Revert() (gas: 160900)
-HybridUSDCTokenPoolMigrationTests:test_lockOrBurn_then_BurnInCCTPMigration_Success() (gas: 255982)
-HybridUSDCTokenPoolMigrationTests:test_transferLiquidity_Success() (gas: 165921)
-HybridUSDCTokenPoolMigrationTests:test_unstickManualTxAfterMigration_destChain_Success() (gas: 154307)
-HybridUSDCTokenPoolMigrationTests:test_unstickManualTxAfterMigration_homeChain_Success() (gas: 463780)
-HybridUSDCTokenPoolTests:test_LockOrBurn_LockReleaseMechanism_then_switchToPrimary_Success() (gas: 209230)
-HybridUSDCTokenPoolTests:test_LockOrBurn_PrimaryMechanism_Success() (gas: 135897)
-HybridUSDCTokenPoolTests:test_LockOrBurn_WhileMigrationPause_Revert() (gas: 107135)
-HybridUSDCTokenPoolTests:test_LockOrBurn_onLockReleaseMechanism_Success() (gas: 144607)
-HybridUSDCTokenPoolTests:test_MintOrRelease_OnLockReleaseMechanism_Success() (gas: 214795)
-HybridUSDCTokenPoolTests:test_MintOrRelease_OnLockReleaseMechanism_then_switchToPrimary_Success() (gas: 423668)
-HybridUSDCTokenPoolTests:test_MintOrRelease_incomingMessageWithPrimaryMechanism() (gas: 268949)
-HybridUSDCTokenPoolTests:test_ReleaseOrMint_WhileMigrationPause_Revert() (gas: 111528)
-HybridUSDCTokenPoolTests:test_cannotTransferLiquidityDuringPendingMigration_Revert() (gas: 160845)
-HybridUSDCTokenPoolTests:test_transferLiquidity_Success() (gas: 165904)
+HybridUSDCTokenPoolMigrationTests:test_LockOrBurn_LockReleaseMechanism_then_switchToPrimary_Success() (gas: 209283)
+HybridUSDCTokenPoolMigrationTests:test_LockOrBurn_PrimaryMechanism_Success() (gas: 135915)
+HybridUSDCTokenPoolMigrationTests:test_LockOrBurn_WhileMigrationPause_Revert() (gas: 109794)
+HybridUSDCTokenPoolMigrationTests:test_LockOrBurn_onLockReleaseMechanism_Success() (gas: 147082)
+HybridUSDCTokenPoolMigrationTests:test_MintOrRelease_OnLockReleaseMechanism_Success() (gas: 217024)
+HybridUSDCTokenPoolMigrationTests:test_MintOrRelease_OnLockReleaseMechanism_then_switchToPrimary_Success() (gas: 425962)
+HybridUSDCTokenPoolMigrationTests:test_MintOrRelease_incomingMessageWithPrimaryMechanism() (gas: 268984)
+HybridUSDCTokenPoolMigrationTests:test_ProposeMigration_ChainNotUsingLockRelease_Revert() (gas: 15843)
+HybridUSDCTokenPoolMigrationTests:test_ReleaseOrMint_WhileMigrationPause_Revert() (gas: 113503)
+HybridUSDCTokenPoolMigrationTests:test_burnLockedUSDC_invalidPermissions_Revert() (gas: 39300)
+HybridUSDCTokenPoolMigrationTests:test_cancelExistingCCTPMigrationProposal() (gas: 56208)
+HybridUSDCTokenPoolMigrationTests:test_cannotCancelANonExistentMigrationProposal() (gas: 12758)
+HybridUSDCTokenPoolMigrationTests:test_cannotModifyLiquidityWithoutPermissions_Revert() (gas: 13395)
+HybridUSDCTokenPoolMigrationTests:test_cannotProvideLiquidityWhenMigrationProposalPending_Revert() (gas: 67370)
+HybridUSDCTokenPoolMigrationTests:test_cannotRevertChainMechanism_afterMigration_Revert() (gas: 313252)
+HybridUSDCTokenPoolMigrationTests:test_cannotTransferLiquidityDuringPendingMigration_Revert() (gas: 177053)
+HybridUSDCTokenPoolMigrationTests:test_cnanotProvideLiquidity_AfterMigration_Revert() (gas: 313637)
+HybridUSDCTokenPoolMigrationTests:test_excludeTokensWhenNoMigrationProposalPending_Revert() (gas: 13657)
+HybridUSDCTokenPoolMigrationTests:test_lockOrBurn_then_BurnInCCTPMigration_Success() (gas: 309797)
+HybridUSDCTokenPoolMigrationTests:test_transferLiquidity_Success() (gas: 167051)
+HybridUSDCTokenPoolMigrationTests:test_unstickManualTxAfterMigration_destChain_Success() (gas: 156096)
+HybridUSDCTokenPoolMigrationTests:test_unstickManualTxAfterMigration_homeChain_Success() (gas: 516027)
+HybridUSDCTokenPoolTests:test_LockOrBurn_LockReleaseMechanism_then_switchToPrimary_Success() (gas: 209283)
+HybridUSDCTokenPoolTests:test_LockOrBurn_PrimaryMechanism_Success() (gas: 135915)
+HybridUSDCTokenPoolTests:test_LockOrBurn_WhileMigrationPause_Revert() (gas: 109794)
+HybridUSDCTokenPoolTests:test_LockOrBurn_onLockReleaseMechanism_Success() (gas: 147080)
+HybridUSDCTokenPoolTests:test_MintOrRelease_OnLockReleaseMechanism_Success() (gas: 217002)
+HybridUSDCTokenPoolTests:test_MintOrRelease_OnLockReleaseMechanism_then_switchToPrimary_Success() (gas: 425918)
+HybridUSDCTokenPoolTests:test_MintOrRelease_incomingMessageWithPrimaryMechanism() (gas: 268967)
+HybridUSDCTokenPoolTests:test_ReleaseOrMint_WhileMigrationPause_Revert() (gas: 113547)
+HybridUSDCTokenPoolTests:test_cannotTransferLiquidityDuringPendingMigration_Revert() (gas: 177009)
+HybridUSDCTokenPoolTests:test_transferLiquidity_Success() (gas: 167033)
 LockReleaseTokenPool_canAcceptLiquidity:test_CanAcceptLiquidity_Success() (gas: 2836138)
 LockReleaseTokenPool_lockOrBurn:test_LockOrBurnWithAllowList_Revert() (gas: 30062)
 LockReleaseTokenPool_lockOrBurn:test_LockOrBurnWithAllowList_Success() (gas: 79965)

contracts/src/v0.8/ccip/pools/USDC/BurnMintWithLockReleaseFlagTokenPool.sol

@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: BUSL-1.1
+pragma solidity 0.8.24;
+
+import {IBurnMintERC20} from "../../../shared/token/ERC20/IBurnMintERC20.sol";
+
+import {Pool} from "../../libraries/Pool.sol";
+import {BurnMintTokenPool} from "../BurnMintTokenPool.sol";
+import {LOCK_RELEASE_FLAG} from "./HybridLockReleaseUSDCTokenPool.sol";
+
+/// @notice A standard BurnMintTokenPool with modified destPoolData so that the remote pool knows to release tokens
+/// instead of minting. This enables interoperability with HybridLockReleaseUSDCTokenPool which uses
+// the destPoolData to determine whether to mint or release tokens.
+/// @dev The only difference between this contract and BurnMintTokenPool is the destPoolData returns the
+/// abi-encoded LOCK_RELEASE_FLAG instead of an empty string.
+contract BurnMintWithLockReleaseFlagTokenPool is BurnMintTokenPool {
+  constructor(
+    IBurnMintERC20 token,
+    address[] memory allowlist,
+    address rmnProxy,
+    address router
+  ) BurnMintTokenPool(token, allowlist, rmnProxy, router) {}
+
+  /// @notice Burn the token in the pool
+  /// @dev The _validateLockOrBurn check is an essential security check
+  /// @dev Performs the exact same functionality as BurnMintTokenPool, but returns the LOCK_RELEASE_FLAG
+  /// as the destPoolData to signal to the remote pool to release tokens instead of minting them.
+  function lockOrBurn(
+    Pool.LockOrBurnInV1 calldata lockOrBurnIn
+  ) external override returns (Pool.LockOrBurnOutV1 memory) {
+    _validateLockOrBurn(lockOrBurnIn);
+
+    _burn(lockOrBurnIn.amount);
+
+    emit Burned(msg.sender, lockOrBurnIn.amount);
+
+    // LOCK_RELEASE_FLAG = bytes4(keccak256("NO_CCTP_USE_LOCK_RELEASE"))
+    return Pool.LockOrBurnOutV1({
+      destTokenAddress: getRemoteToken(lockOrBurnIn.remoteChainSelector),
+      destPoolData: abi.encode(LOCK_RELEASE_FLAG)
+    });
+  }
+}

contracts/src/v0.8/ccip/pools/USDC/HybridLockReleaseUSDCTokenPool.sol

@@ -14,6 +14,9 @@ import {IERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/tok
 import {SafeERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/utils/SafeERC20.sol";
 import {EnumerableSet} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/utils/structs/EnumerableSet.sol";
 
+// bytes4(keccak256("NO_CCTP_USE_LOCK_RELEASE"))
+bytes4 constant LOCK_RELEASE_FLAG = 0xfa7c07de;
+
 /// @notice A token pool for USDC which uses CCTP for supported chains and Lock/Release for all others
 /// @dev The functionality from LockReleaseTokenPool.sol has been duplicated due to lack of compiler support for shared
 /// constructors between parents
@@ -34,9 +37,6 @@ contract HybridLockReleaseUSDCTokenPool is USDCTokenPool, USDCBridgeMigrator {
   error LanePausedForCCTPMigration(uint64 remoteChainSelector);
   error TokenLockingNotAllowedAfterMigration(uint64 remoteChainSelector);
 
-  /// bytes4(keccak256("NO_CTTP_USE_LOCK_RELEASE"))
-  bytes4 public constant LOCK_RELEASE_FLAG = 0xd43c7897;
-
   /// @notice The address of the liquidity provider for a specific chain.
   /// External liquidity is not required when there is one canonical token deployed to a chain,
   /// and CCIP is facilitating mint/burn on all the other chains, in which case the invariant
@@ -49,7 +49,7 @@ contract HybridLockReleaseUSDCTokenPool is USDCTokenPool, USDCBridgeMigrator {
     address[] memory allowlist,
     address rmnProxy,
     address router
-  ) USDCTokenPool(tokenMessenger, token, allowlist, rmnProxy, router) USDCBridgeMigrator(address(token), router) {}
+  ) USDCTokenPool(tokenMessenger, token, allowlist, rmnProxy, router) USDCBridgeMigrator(address(token)) {}
 
   // ================================================================
   // │                   Incoming/Outgoing Mechanisms               |
@@ -99,7 +99,7 @@ contract HybridLockReleaseUSDCTokenPool is USDCTokenPool, USDCBridgeMigrator {
     _validateReleaseOrMint(releaseOrMintIn);
 
     // Circle requires a supply-lock to prevent incoming messages once the migration process begins.
-    // This prevents new outgoing messages once the migration has begun to ensure any the procedure runs as expected
+    // This prevents new incoming messages once the migration has begun to ensure any the procedure runs as expected
     if (s_proposedUSDCMigrationChain == releaseOrMintIn.remoteChainSelector) {
       revert LanePausedForCCTPMigration(s_proposedUSDCMigrationChain);
     }
@@ -114,7 +114,6 @@ contract HybridLockReleaseUSDCTokenPool is USDCTokenPool, USDCBridgeMigrator {
       s_lockedTokensByChainSelector[releaseOrMintIn.remoteChainSelector] -= releaseOrMintIn.amount;
     }
 
-    // Release to the offRamp, which forwards it to the recipient
     getToken().safeTransfer(releaseOrMintIn.receiver, releaseOrMintIn.amount);
 
     emit Released(msg.sender, releaseOrMintIn.receiver, releaseOrMintIn.amount);
@@ -173,6 +172,16 @@ contract HybridLockReleaseUSDCTokenPool is USDCTokenPool, USDCBridgeMigrator {
   function provideLiquidity(uint64 remoteChainSelector, uint256 amount) external {
     if (s_liquidityProvider[remoteChainSelector] != msg.sender) revert TokenPool.Unauthorized(msg.sender);
 
+    // Prevent adding liquidity to a chain which has already been migrated
+    if (s_migratedChains.contains(remoteChainSelector)) {
+      revert TokenLockingNotAllowedAfterMigration(remoteChainSelector);
+    }
+
+    // prevent adding liquidity to a chain which has been proposed for migration
+    if (remoteChainSelector == s_proposedUSDCMigrationChain) {
+      revert LanePausedForCCTPMigration(remoteChainSelector);
+    }
+
     s_lockedTokensByChainSelector[remoteChainSelector] += amount;
 
     i_token.safeTransferFrom(msg.sender, address(this), amount);
@@ -187,7 +196,7 @@ contract HybridLockReleaseUSDCTokenPool is USDCTokenPool, USDCBridgeMigrator {
   /// withdrawn on this chain, otherwise a mismatch may occur between locked token balance and remote circulating supply
   /// which may block a potential future migration of the chain to CCTP.
   function withdrawLiquidity(uint64 remoteChainSelector, uint256 amount) external onlyOwner {
-    // Circle requires a supply-lock to prevent outgoing messages once the migration process begins.
+    // A supply-lock is required to prevent outgoing messages once the migration process begins.
     // This prevents new outgoing messages once the migration has begun to ensure any the procedure runs as expected
     if (remoteChainSelector == s_proposedUSDCMigrationChain) {
       revert LanePausedForCCTPMigration(remoteChainSelector);
@@ -213,16 +222,10 @@ contract HybridLockReleaseUSDCTokenPool is USDCTokenPool, USDCBridgeMigrator {
   /// @param from The address of the old pool.
   /// @param remoteChainSelector The chain for which liquidity is being transferred.
   function transferLiquidity(address from, uint64 remoteChainSelector) external onlyOwner {
-    // Prevent Liquidity Transfers when a migration is pending. This prevents requiring the new pool to manage
-    // token exclusions for edge-case messages and ensures that the migration is completed before any new liquidity
-    // is added to the pool.
-    if (HybridLockReleaseUSDCTokenPool(from).getCurrentProposedCCTPChainMigration() == remoteChainSelector) {
-      revert LanePausedForCCTPMigration(remoteChainSelector);
-    }
-
     OwnerIsCreator(from).acceptOwnership();
 
-    // Withdraw all available liquidity from the old pool.
+    // Withdraw all available liquidity from the old pool. No check is needed for pending migrations, as the old pool
+    // will revert if the migration has begun.
     uint256 withdrawAmount = HybridLockReleaseUSDCTokenPool(from).getLockedTokensForChain(remoteChainSelector);
     HybridLockReleaseUSDCTokenPool(from).withdrawLiquidity(remoteChainSelector, withdrawAmount);
 
@@ -237,23 +240,28 @@ contract HybridLockReleaseUSDCTokenPool is USDCTokenPool, USDCBridgeMigrator {
 
   /// @notice Return whether a lane should use the alternative L/R mechanism in the token pool.
   /// @param remoteChainSelector the remote chain the lane is interacting with
-  /// @return bool Return true if the alternative L/R mechanism should be used
+  /// @return bool Return true if the alternative L/R mechanism should be used, and is decided by the Owner
   function shouldUseLockRelease(
     uint64 remoteChainSelector
   ) public view virtual returns (bool) {
     return s_shouldUseLockRelease[remoteChainSelector];
   }
 
-  /// @notice Updates Updates designations for chains on whether to use primary or alt mechanism on CCIP messages
+  /// @notice Updates designations for chains on whether to use primary or alt mechanism on CCIP messages
   /// @param removes A list of chain selectors to disable Lock-Release, and enforce BM
-  /// @param adds A list of chain selectors to enable LR instead of BM
+  /// @param adds A list of chain selectors to enable LR instead of BM. These chains must not have been migrated
+  /// to CCTP yet or the transaction will revert
   function updateChainSelectorMechanisms(uint64[] calldata removes, uint64[] calldata adds) external onlyOwner {
     for (uint256 i = 0; i < removes.length; ++i) {
       delete s_shouldUseLockRelease[removes[i]];
       emit LockReleaseDisabled(removes[i]);
     }
 
     for (uint256 i = 0; i < adds.length; ++i) {
+      // Prevent enabling lock release on chains which have already been migrated
+      if (s_migratedChains.contains(adds[i])) {
+        revert TokenLockingNotAllowedAfterMigration(adds[i]);
+      }
       s_shouldUseLockRelease[adds[i]] = true;
       emit LockReleaseEnabled(adds[i]);
     }