CCIP-4010 fix various nits (#14972)

* fix various nits

* rm redundant rpc call

* [Bot] Update changeset file with jira issues

* fix comments

* add test for router update

* undo RMNHome check

* fix forge fmt

---------

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

contracts/.changeset/metal-radios-push.md

@@ -0,0 +1,10 @@
+---
+'@chainlink/contracts': patch
+---
+
+#internal minor nits, allow Router updates even when the offRamp has been used. Remove getRouter from onRamp
+
+
+PR issue: CCIP-4010
+
+Solidity Review issue: CCIP-3966

contracts/src/v0.8/ccip/Router.sol

@@ -175,8 +175,8 @@ contract Router is IRouter, IRouterClient, ITypeAndVersion, OwnerIsCreator {
     return (success, retData, gasUsed);
   }
 
-  // @notice Merges a chain selector and offRamp address into a single uint256 by shifting the
-  // chain selector 160 bits to the left.
+  /// @notice Merges a chain selector and offRamp address into a single uint256 by shifting the
+  /// chain selector 160 bits to the left.
   function _mergeChainSelectorAndOffRamp(
     uint64 sourceChainSelector,
     address offRampAddress

contracts/src/v0.8/ccip/applications/CCIPClientExample.sol

@@ -140,8 +140,8 @@ contract CCIPClientExample is CCIPReceiver, OwnerIsCreator {
     emit MessageSent(messageId);
   }
 
-  // @notice user sends tokens to a receiver
-  // Approvals can be optimized with a whitelist of tokens and inf approvals if desired.
+  /// @notice user sends tokens to a receiver
+  /// Approvals can be optimized with a whitelist of tokens and inf approvals if desired.
   function sendTokens(
     uint64 destChainSelector,
     bytes memory receiver,

contracts/src/v0.8/ccip/capability/CCIPHome.sol

@@ -94,13 +94,6 @@ contract CCIPHome is OwnerIsCreator, ITypeAndVersion, ICapabilityConfiguration,
   error InvalidSelector(bytes4 selector);
   error DONIdMismatch(uint32 callDonId, uint32 capabilityRegistryDonId);
 
-  error InvalidStateTransition(
-    bytes32 currentActiveDigest,
-    bytes32 currentCandidateDigest,
-    bytes32 proposedActiveDigest,
-    bytes32 proposedCandidateDigest
-  );
-
   /// @notice Represents an oracle node in OCR3 configs part of the role DON.
   /// Every configured node should be a signer, but does not have to be a transmitter.
   struct OCR3Node {

contracts/src/v0.8/ccip/libraries/Internal.sol

@@ -11,9 +11,9 @@ library Internal {
   /// We include this in the offramp so that we can redeploy to adjust it
   /// should a hardfork change the gas costs of relevant opcodes in callWithExactGas.
   uint16 internal constant GAS_FOR_CALL_EXACT_CHECK = 5_000;
-  // @dev We limit return data to a selector plus 4 words. This is to avoid
-  // malicious contracts from returning large amounts of data and causing
-  // repeated out-of-gas scenarios.
+  /// @dev We limit return data to a selector plus 4 words. This is to avoid
+  /// malicious contracts from returning large amounts of data and causing
+  /// repeated out-of-gas scenarios.
   uint16 internal constant MAX_RET_BYTES = 4 + 4 * 32;
   /// @dev The expected number of bytes returned by the balanceOf function.
   uint256 internal constant MAX_BALANCE_OF_RET_BYTES = 32;
@@ -227,8 +227,8 @@ library Internal {
     // The source pool EVM address encoded to bytes. This value is trusted as it is obtained through the onRamp. It can be
     // relied upon by the destination pool to validate the source pool.
     bytes sourcePoolAddress;
-    address destTokenAddress; // ───╮ Address of destination token
-    uint32 destGasAmount; //────────╯ The amount of gas available for the releaseOrMint and transfer calls on the offRamp.
+    address destTokenAddress; // ─╮ Address of destination token
+    uint32 destGasAmount; //──────╯ The amount of gas available for the releaseOrMint and transfer calls on the offRamp.
     // Optional pool data to be transferred to the destination chain. Be default this is capped at
     // CCIP_LOCK_OR_BURN_V1_RET_BYTES bytes. If more data is required, the TokenTransferFeeConfig.destBytesOverhead
     // has to be set for the specific token.

contracts/src/v0.8/ccip/onRamp/OnRamp.sol

@@ -22,8 +22,8 @@ import {SafeERC20} from "../../vendor/openzeppelin-solidity/v4.8.3/contracts/tok
 import {EnumerableSet} from "../../vendor/openzeppelin-solidity/v5.0.2/contracts/utils/structs/EnumerableSet.sol";
 
 /// @notice The OnRamp is a contract that handles lane-specific fee logic.
-/// @dev The OnRamp and OffRamp form an xchain upgradeable unit. Any change to one of them
-/// results in an onchain upgrade of all 3.
+/// @dev The OnRamp and OffRamp form a cross chain upgradeable unit. Any change to one of them results in an onchain
+/// upgrade of both contracts.
 contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
   using SafeERC20 for IERC20;
   using EnumerableSet for EnumerableSet.AddressSet;
@@ -55,7 +55,7 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
   event AllowListSendersAdded(uint64 indexed destChainSelector, address[] senders);
   event AllowListSendersRemoved(uint64 indexed destChainSelector, address[] senders);
 
-  /// @dev Struct that contains the static configuration
+  /// @dev Struct that contains the static configuration.
   /// RMN depends on this struct, if changing, please notify the RMN maintainers.
   // solhint-disable-next-line gas-struct-packing
   struct StaticConfig {
@@ -68,20 +68,19 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
   /// @dev Struct that contains the dynamic configuration
   // solhint-disable-next-line gas-struct-packing
   struct DynamicConfig {
-    address feeQuoter; // FeeQuoter address
-    bool reentrancyGuardEntered; // Reentrancy protection
-    address messageInterceptor; // Optional message interceptor to validate outbound messages. Zero address = no interceptor
-    address feeAggregator; // Fee aggregator address
-    address allowlistAdmin; // authorized admin to add or remove allowed senders
+    address feeQuoter; // FeeQuoter address.
+    bool reentrancyGuardEntered; // Reentrancy protection.
+    address messageInterceptor; // Optional message interceptor to validate messages. Zero address = no interceptor.
+    address feeAggregator; // Fee aggregator address.
+    address allowlistAdmin; // authorized admin to add or remove allowed senders.
   }
 
-  /// @dev Struct to hold the configs for a destination chain
-  /// @dev sequenceNumber, allowlistEnabled, router will all be packed in 1 slot
+  /// @dev Struct to hold the configs for a single destination chain
   struct DestChainConfig {
     // The last used sequence number. This is zero in the case where no messages have yet been sent.
-    // 0 is not a valid sequence number for any real transaction.
+    // 0 is not a valid sequence number for any real transaction as this value will be incremented before use.
     uint64 sequenceNumber; // ──╮ The last used sequence number.
-    bool allowlistEnabled; //   │ boolean indicator to specify if allowlist check is enabled.
+    bool allowlistEnabled; //   │ True if the allowlist is enabled.
     IRouter router; // ─────────╯ Local router address  that is allowed to send messages to the destination chain.
     EnumerableSet.AddressSet allowedSendersList; // The list of addresses allowed to send messages.
   }
@@ -90,35 +89,35 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
   /// can be passed in the constructor and the applyDestChainConfigUpdates function
   // solhint-disable gas-struct-packing
   struct DestChainConfigArgs {
-    uint64 destChainSelector; // ─╮ Destination chain selector
-    IRouter router; //            │ Source router address
-    bool allowlistEnabled; // ────╯ Boolean indicator to specify if allowlist check is enabled
+    uint64 destChainSelector; // ─╮ Destination chain selector.
+    IRouter router; //            │ Source router address.
+    bool allowlistEnabled; // ────╯ True if the allowlist is enabled.
   }
 
   /// @dev Struct to hold the allowlist configuration args per dest chain.
   struct AllowlistConfigArgs {
     uint64 destChainSelector; // ──╮ Destination chain selector.
     bool allowlistEnabled; // ─────╯ True if the allowlist is enabled.
-    address[] addedAllowlistedSenders; // list of senders to be added to the allowedSendersList
-    address[] removedAllowlistedSenders; // list of senders to be removed from the allowedSendersList
+    address[] addedAllowlistedSenders; // list of senders to be added to the allowedSendersList.
+    address[] removedAllowlistedSenders; // list of senders to be removed from the allowedSendersList.
   }
 
   // STATIC CONFIG
   string public constant override typeAndVersion = "OnRamp 1.6.0-dev";
-  /// @dev The chain ID of the source chain that this contract is deployed to
+  /// @dev The chain ID of the source chain that this contract is deployed to.
   uint64 private immutable i_chainSelector;
-  /// @dev The rmn contract
+  /// @dev The rmn contract.
   IRMNRemote private immutable i_rmnRemote;
-  /// @dev The address of the nonce manager
+  /// @dev The address of the nonce manager.
   address private immutable i_nonceManager;
-  /// @dev The address of the token admin registry
+  /// @dev The address of the token admin registry.
   address private immutable i_tokenAdminRegistry;
 
   // DYNAMIC CONFIG
-  /// @dev The dynamic config for the onRamp
+  /// @dev The dynamic config for the onRamp.
   DynamicConfig private s_dynamicConfig;
 
-  /// @dev The destination chain specific configs
+  /// @dev The destination chain specific configs.
   mapping(uint64 destChainSelector => DestChainConfig destChainConfig) private s_destChainConfigs;
 
   constructor(
@@ -146,9 +145,9 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
   // │                          Messaging                           │
   // ================================================================
 
-  /// @notice Gets the next sequence number to be used in the onRamp
-  /// @param destChainSelector The destination chain selector
-  /// @return nextSequenceNumber The next sequence number to be used
+  /// @notice Gets the next sequence number to be used in the onRamp.
+  /// @param destChainSelector The destination chain selector.
+  /// @return nextSequenceNumber The next sequence number to be used.
   function getExpectedNextSequenceNumber(
     uint64 destChainSelector
   ) external view returns (uint64) {
@@ -162,16 +161,15 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
     uint256 feeTokenAmount,
     address originalSender
   ) external returns (bytes32) {
-    // We rely on a reentrancy guard here due to the untrusted calls performed to the pools
-    // This enables some optimizations by not following the CEI pattern
+    // We rely on a reentrancy guard here due to the untrusted calls performed to the pools. This enables some
+    // optimizations by not following the CEI pattern
     if (s_dynamicConfig.reentrancyGuardEntered) revert ReentrancyGuardReentrantCall();
-
     s_dynamicConfig.reentrancyGuardEntered = true;
 
     DestChainConfig storage destChainConfig = s_destChainConfigs[destChainSelector];
 
-    // NOTE: assumes the message has already been validated through the getFee call
-    // Validate message sender is set and allowed. Not validated in `getFee` since it is not user-driven.
+    // NOTE: assumes the message has already been validated through the getFee call.
+    // Validate originalSender is set and allowed. Not validated in `getFee` since it is not user-driven.
     if (originalSender == address(0)) revert RouterMustSetOriginalSender();
 
     if (destChainConfig.allowlistEnabled) {
@@ -180,7 +178,7 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
       }
     }
 
-    // Router address may be zero intentionally to pause.
+    // Router address may be zero intentionally to pause, which should stop all messages.
     if (msg.sender != address(destChainConfig.router)) revert MustBeCalledByRouter();
 
     {
@@ -193,14 +191,14 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
 
     Internal.EVM2AnyRampMessage memory newMessage = Internal.EVM2AnyRampMessage({
       header: Internal.RampMessageHeader({
-        // Should be generated after the message is complete
+        // Should be generated after the message is complete.
         messageId: "",
         sourceChainSelector: i_chainSelector,
         destChainSelector: destChainSelector,
-        // We need the next available sequence number so we increment before we use the value
+        // We need the next available sequence number so we increment before we use the value.
         sequenceNumber: ++destChainConfig.sequenceNumber,
-        // Only bump nonce for messages that specify allowOutOfOrderExecution == false. Otherwise, we
-        // may block ordered message nonces, which is not what we want.
+        // Only bump nonce for messages that specify allowOutOfOrderExecution == false. Otherwise, we may block ordered
+        // message nonces, which is not what we want.
         nonce: 0
       }),
       sender: originalSender,
@@ -210,7 +208,7 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
       feeToken: message.feeToken,
       feeTokenAmount: feeTokenAmount,
       feeValueJuels: 0, // calculated later
-      // Should be populated via lock / burn pool calls
+      // Should be populated via lock / burn pool calls.
       tokenAmounts: new Internal.EVM2AnyTokenTransfer[](message.tokenAmounts.length)
     });
 
@@ -221,8 +219,8 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
         _lockOrBurnSingleToken(tokenAmounts[i], destChainSelector, message.receiver, originalSender);
     }
 
-    // Convert message fee to juels and retrieve converted args
-    // Validate pool return data after it is populated (view function - no state changes)
+    // Convert message fee to juels and retrieve converted args.
+    // Validate pool return data after it is populated (view function - no state changes).
     bool isOutOfOrderExecution;
     bytes memory convertedExtraArgs;
     bytes[] memory destExecDataPerToken;
@@ -241,15 +239,15 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
       newMessage.tokenAmounts[i].destExecData = destExecDataPerToken[i];
     }
 
-    // Hash only after all fields have been set
+    // Hash only after all fields have been set.
     newMessage.header.messageId = Internal._hash(
       newMessage,
-      // Metadata hash preimage to ensure global uniqueness, ensuring 2 identical messages sent to 2 different
-      // lanes will have a distinct hash.
+      // Metadata hash preimage to ensure global uniqueness, ensuring 2 identical messages sent to 2 different lanes
+      // will have a distinct hash.
       keccak256(abi.encode(Internal.EVM_2_ANY_MESSAGE_HASH, i_chainSelector, destChainSelector, address(this)))
     );
 
-    // Emit message request
+    // Emit message request.
     // This must happen after any pool events as some tokens (e.g. USDC) emit events that we expect to precede this
     // event in the offchain code.
     emit CCIPMessageSent(destChainSelector, newMessage.header.sequenceNumber, newMessage);
@@ -259,12 +257,12 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
     return newMessage.header.messageId;
   }
 
-  /// @notice Uses a pool to lock or burn a token
-  /// @param tokenAndAmount Token address and amount to lock or burn
-  /// @param destChainSelector Target destination chain selector of the message
-  /// @param receiver Message receiver
-  /// @param originalSender Message sender
-  /// @return evm2AnyTokenTransfer EVM2Any token and amount data
+  /// @notice Uses a pool to lock or burn a token.
+  /// @param tokenAndAmount Token address and amount to lock or burn.
+  /// @param destChainSelector Target destination chain selector of the message.
+  /// @param receiver Message receiver.
+  /// @param originalSender Message sender.
+  /// @return evm2AnyTokenTransfer EVM2Any token and amount data.
   function _lockOrBurnSingleToken(
     Client.EVMTokenAmount memory tokenAndAmount,
     uint64 destChainSelector,
@@ -291,13 +289,13 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
       })
     );
 
-    // NOTE: pool data validations are outsourced to the FeeQuoter to handle family-specific logic handling
+    // NOTE: pool data validations are outsourced to the FeeQuoter to handle family-specific logic handling.
     return Internal.EVM2AnyTokenTransfer({
       sourcePoolAddress: address(sourcePool),
       destTokenAddress: poolReturnData.destTokenAddress,
       extraData: poolReturnData.destPoolData,
       amount: tokenAndAmount.amount,
-      destExecData: "" // This is set in the processPoolReturnData function
+      destExecData: "" // This is set in the processPoolReturnData function.
     });
   }
 
@@ -331,15 +329,6 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
     _setDynamicConfig(dynamicConfig);
   }
 
-  /// @notice Gets the source router for a destination chain
-  /// @param destChainSelector The destination chain selector
-  /// @return router the router for the provided destination chain
-  function getRouter(
-    uint64 destChainSelector
-  ) external view returns (IRouter) {
-    return s_destChainConfigs[destChainSelector].router;
-  }
-
   /// @notice Internal version of setDynamicConfig to allow for reuse in the constructor.
   function _setDynamicConfig(
     DynamicConfig memory dynamicConfig
@@ -393,11 +382,11 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
     }
   }
 
-  /// @notice get ChainConfig configured for the DestinationChainSelector
-  /// @param destChainSelector The destination chain selector
-  /// @return sequenceNumber The last used sequence number
-  /// @return allowlistEnabled boolean indicator to specify if allowlist check is enabled
-  /// @return router address of the router
+  /// @notice get ChainConfig configured for the DestinationChainSelector.
+  /// @param destChainSelector The destination chain selector.
+  /// @return sequenceNumber The last used sequence number.
+  /// @return allowlistEnabled boolean indicator to specify if allowlist check is enabled.
+  /// @return router address of the router.
   function getDestChainConfig(
     uint64 destChainSelector
   ) external view returns (uint64 sequenceNumber, bool allowlistEnabled, address router) {

contracts/src/v0.8/ccip/pools/LockReleaseTokenPool.sol

@@ -9,6 +9,7 @@ import {TokenPool} from "./TokenPool.sol";
 
 import {IERC20} from "../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/IERC20.sol";
 import {SafeERC20} from "../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/utils/SafeERC20.sol";
+import {IERC165} from "../../vendor/openzeppelin-solidity/v5.0.2/contracts/utils/introspection/IERC165.sol";
 
 /// @notice Token pool used for tokens on their native chain. This uses a lock and release mechanism.
 /// Because of lock/unlock requiring liquidity, this pool contract also has function to add and remove
@@ -69,7 +70,7 @@ contract LockReleaseTokenPool is TokenPool, ILiquidityContainer, ITypeAndVersion
     return Pool.ReleaseOrMintOutV1({destinationAmount: releaseOrMintIn.amount});
   }
 
-  // @inheritdoc IERC165
+  /// @inheritdoc IERC165
   function supportsInterface(
     bytes4 interfaceId
   ) public pure virtual override returns (bool) {