CCIP-3730 Misc ccip onchain changes (#14845)

* improve return value get allowlist

* update order of applyFeeTokensUpdates

improve tests

* consistent naming of allowlist

* fix typo, dedup error

* improve comments

* fix offchain

* changeset + fix lint

* remove non-allowed state transition

* fix naming of allowListUpdates

* re run CI

.github/workflows/solidity-foundry.yml

@@ -28,7 +28,7 @@ jobs:
           cat <<EOF > matrix.json
           [
             { "name": "automation", "setup": { "run-coverage": false, "min-coverage": 98.5, "run-gas-snapshot": false, "run-forge-fmt": false }},
-            { "name": "ccip", "setup": { "run-coverage": true, "min-coverage": 97.6, "run-gas-snapshot": true, "run-forge-fmt": true }},
+            { "name": "ccip", "setup": { "run-coverage": true, "min-coverage": 98.8, "run-gas-snapshot": true, "run-forge-fmt": true }},
             { "name": "functions", "setup": { "run-coverage": false, "min-coverage": 98.5, "run-gas-snapshot": true, "run-forge-fmt": false }},
             { "name": "keystone", "setup": { "run-coverage": true, "min-coverage": 72.8, "run-gas-snapshot": false, "run-forge-fmt": false }},
             { "name": "l2ep", "setup": { "run-coverage": true, "min-coverage": 61.0, "run-gas-snapshot": true, "run-forge-fmt": false }},

.gitignore

@@ -114,3 +114,4 @@ ocr_soak_report.csv
 vendor/*
 
 *.wasm
+contracts/lcov.info

contracts/.changeset/nice-rocks-trade.md

@@ -0,0 +1,9 @@
+---
+'@chainlink/contracts': patch
+---
+
+#internal Minor fixes and changing the order of removes/adds in feeToken config
+
+CCIP-3730
+CCIP-3727
+CCIP-3725

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

@@ -1,20 +1,20 @@
 // SPDX-License-Identifier: BUSL-1.1
 pragma solidity 0.8.24;
 
+import {IReceiver} from "../keystone/interfaces/IReceiver.sol";
 import {ITypeAndVersion} from "../shared/interfaces/ITypeAndVersion.sol";
 import {IFeeQuoter} from "./interfaces/IFeeQuoter.sol";
 import {IPriceRegistry} from "./interfaces/IPriceRegistry.sol";
 
+import {KeystoneFeedsPermissionHandler} from "../keystone/KeystoneFeedsPermissionHandler.sol";
+import {KeystoneFeedDefaultMetadataLib} from "../keystone/lib/KeystoneFeedDefaultMetadataLib.sol";
 import {AuthorizedCallers} from "../shared/access/AuthorizedCallers.sol";
 import {AggregatorV3Interface} from "./../shared/interfaces/AggregatorV3Interface.sol";
 import {Client} from "./libraries/Client.sol";
 import {Internal} from "./libraries/Internal.sol";
 import {Pool} from "./libraries/Pool.sol";
 import {USDPriceWith18Decimals} from "./libraries/USDPriceWith18Decimals.sol";
 
-import {KeystoneFeedsPermissionHandler} from "../keystone/KeystoneFeedsPermissionHandler.sol";
-import {IReceiver} from "../keystone/interfaces/IReceiver.sol";
-import {KeystoneFeedDefaultMetadataLib} from "../keystone/lib/KeystoneFeedDefaultMetadataLib.sol";
 import {EnumerableSet} from "../vendor/openzeppelin-solidity/v5.0.2/contracts/utils/structs/EnumerableSet.sol";
 
 /// @notice The FeeQuoter contract responsibility is to:
@@ -59,8 +59,8 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
 
   /// @dev Token price data feed configuration
   struct TokenPriceFeedConfig {
-    address dataFeedAddress; // ──╮ AggregatorV3Interface contract (0 - feed is unset)
-    uint8 tokenDecimals; // ──────╯ Decimals of the token that the feed represents
+    address dataFeedAddress; // ─╮ AggregatorV3Interface contract (0 - feed is unset)
+    uint8 tokenDecimals; // ─────╯ Decimals of the token that the feed represents
   }
 
   /// @dev Token price data feed update
@@ -81,9 +81,9 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
 
   /// @dev The struct representing the received CCIP feed report from keystone IReceiver.onReport()
   struct ReceivedCCIPFeedReport {
-    address token; // Token address
-    uint224 price; // ─────────╮ Price of the token in USD with 18 decimals
-    uint32 timestamp; // ──────╯ Timestamp of the price update
+    address token; //       Token address
+    uint224 price; // ────╮ Price of the token in USD with 18 decimals
+    uint32 timestamp; // ─╯ Timestamp of the price update
   }
 
   /// @dev Struct to hold the fee & validation configs for a destination chain
@@ -119,13 +119,13 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
 
   /// @dev Struct to hold the transfer fee configuration for token transfers
   struct TokenTransferFeeConfig {
-    uint32 minFeeUSDCents; // ──────────╮ Minimum fee to charge per token transfer, multiples of 0.01 USD
-    uint32 maxFeeUSDCents; //           │ Maximum fee to charge per token transfer, multiples of 0.01 USD
-    uint16 deciBps; //                  │ Basis points charged on token transfers, multiples of 0.1bps, or 1e-5
-    uint32 destGasOverhead; //          │ Gas charged to execute the token transfer on the destination chain
-    //                                  │ Extra data availability bytes that are returned from the source pool and sent
-    uint32 destBytesOverhead; //        │ to the destination pool. Must be >= Pool.CCIP_LOCK_OR_BURN_V1_RET_BYTES
-    bool isEnabled; // ─────────────────╯ Whether this token has custom transfer fees
+    uint32 minFeeUSDCents; // ────╮ Minimum fee to charge per token transfer, multiples of 0.01 USD
+    uint32 maxFeeUSDCents; //     │ Maximum fee to charge per token transfer, multiples of 0.01 USD
+    uint16 deciBps; //            │ Basis points charged on token transfers, multiples of 0.1bps, or 1e-5
+    uint32 destGasOverhead; //    │ Gas charged to execute the token transfer on the destination chain
+    //                            │ Extra data availability bytes that are returned from the source pool and sent
+    uint32 destBytesOverhead; //  │ to the destination pool. Must be >= Pool.CCIP_LOCK_OR_BURN_V1_RET_BYTES
+    bool isEnabled; // ───────────╯ Whether this token has custom transfer fees
   }
 
   /// @dev Struct to hold the token transfer fee configurations for a token, same as TokenTransferFeeConfig but with the token address included so
@@ -154,8 +154,8 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
   /// the token address included so that an array of these can be passed in the constructor and
   /// applyPremiumMultiplierWeiPerEthUpdates to set the mapping
   struct PremiumMultiplierWeiPerEthArgs {
-    address token; // // ───────────────────╮ Token address
-    uint64 premiumMultiplierWeiPerEth; // ──╯ Multiplier for destination chain specific premiums.
+    address token; // // ──────────────────╮ Token address
+    uint64 premiumMultiplierWeiPerEth; // ─╯ Multiplier for destination chain specific premiums.
   }
 
   /// @dev The base decimals for cost calculations
@@ -196,7 +196,7 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
   mapping(uint64 destChainSelector => mapping(address token => TokenTransferFeeConfig tranferFeeConfig)) private
     s_tokenTransferFeeConfig;
 
-  /// @dev Maximum fee that can be charged for a message. This is a guard to prevent massively overcharging due to misconfiguation.
+  /// @dev Maximum fee that can be charged for a message. This is a guard to prevent massively overcharging due to misconfiguration.
   uint96 internal immutable i_maxFeeJuelsPerMsg;
   /// @dev The link token address
   address internal immutable i_linkToken;
@@ -226,7 +226,7 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
     i_maxFeeJuelsPerMsg = staticConfig.maxFeeJuelsPerMsg;
     i_tokenPriceStalenessThreshold = staticConfig.tokenPriceStalenessThreshold;
 
-    _applyFeeTokensUpdates(feeTokens, new address[](0));
+    _applyFeeTokensUpdates(new address[](0), feeTokens);
     _updateTokenPriceFeeds(tokenPriceFeeds);
     _applyDestChainConfigUpdates(destChainConfigArgs);
     _applyPremiumMultiplierWeiPerEthUpdates(premiumMultiplierWeiPerEthArgs);
@@ -418,27 +418,27 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
   /// @param feeTokensToAdd The addresses of the tokens which are now considered fee tokens
   /// and can be used to calculate fees.
   function applyFeeTokensUpdates(
-    address[] memory feeTokensToAdd,
-    address[] memory feeTokensToRemove
+    address[] memory feeTokensToRemove,
+    address[] memory feeTokensToAdd
   ) external onlyOwner {
-    _applyFeeTokensUpdates(feeTokensToAdd, feeTokensToRemove);
+    _applyFeeTokensUpdates(feeTokensToRemove, feeTokensToAdd);
   }
 
   /// @notice Add and remove tokens from feeTokens set.
   /// @param feeTokensToRemove The addresses of the tokens which are no longer considered feeTokens.
   /// @param feeTokensToAdd The addresses of the tokens which are now considered fee tokens
   /// and can be used to calculate fees.
-  function _applyFeeTokensUpdates(address[] memory feeTokensToAdd, address[] memory feeTokensToRemove) private {
-    for (uint256 i = 0; i < feeTokensToAdd.length; ++i) {
-      if (s_feeTokens.add(feeTokensToAdd[i])) {
-        emit FeeTokenAdded(feeTokensToAdd[i]);
-      }
-    }
+  function _applyFeeTokensUpdates(address[] memory feeTokensToRemove, address[] memory feeTokensToAdd) private {
     for (uint256 i = 0; i < feeTokensToRemove.length; ++i) {
       if (s_feeTokens.remove(feeTokensToRemove[i])) {
         emit FeeTokenRemoved(feeTokensToRemove[i]);
       }
     }
+    for (uint256 i = 0; i < feeTokensToAdd.length; ++i) {
+      if (s_feeTokens.add(feeTokensToAdd[i])) {
+        emit FeeTokenAdded(feeTokensToAdd[i]);
+      }
+    }
   }
 
   // ================================================================
@@ -449,7 +449,7 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
   function updatePrices(
     Internal.PriceUpdates calldata priceUpdates
   ) external override {
-    // The caller must be the fee updater
+    // The caller must be a fee updater
     _validateCaller();
 
     uint256 tokenUpdatesLength = priceUpdates.tokenPriceUpdates.length;
@@ -512,7 +512,7 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
         revert TokenNotSupported(feeds[i].token);
       }
       // Keystone reports prices in USD with 18 decimals, so we passing it as 18 in the _calculateRebasedValue function
-      uint224 rebasedValue = _calculateRebasedValue(18, tokenDecimals, feeds[i].price);
+      uint224 rebasedValue = _calculateRebasedValue(uint8(KEYSTONE_PRICE_DECIMALS), tokenDecimals, feeds[i].price);
 
       //if stale update then revert
       if (feeds[i].timestamp < s_usdPerToken[feeds[i].token].timestamp) {
@@ -1017,7 +1017,8 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
         revert InvalidDestChainConfig(destChainSelector);
       }
 
-      // The chain family selector cannot be zero - indicates that it is a new chain
+      // If the chain family selector is zero, it indicates that the chain was never configured and we
+      // are adding a new chain
       if (s_destChainConfigs[destChainSelector].chainFamilySelector == 0) {
         emit DestChainAdded(destChainSelector, destChainConfig);
       } else {

contracts/src/v0.8/ccip/offRamp/OffRamp.sol

@@ -12,7 +12,6 @@ import {IRouter} from "../interfaces/IRouter.sol";
 import {ITokenAdminRegistry} from "../interfaces/ITokenAdminRegistry.sol";
 
 import {CallWithExactGas} from "../../shared/call/CallWithExactGas.sol";
-import {EnumerableMapAddresses} from "../../shared/enumerable/EnumerableMapAddresses.sol";
 import {Client} from "../libraries/Client.sol";
 import {Internal} from "../libraries/Internal.sol";
 import {MerkleMultiProof} from "../libraries/MerkleMultiProof.sol";
@@ -32,7 +31,6 @@ import {EnumerableSet} from "../../vendor/openzeppelin-solidity/v5.0.2/contracts
 /// plugin type with verification.
 contract OffRamp is ITypeAndVersion, MultiOCR3Base {
   using ERC165Checker for address;
-  using EnumerableMapAddresses for EnumerableMapAddresses.AddressToAddressMap;
   using EnumerableSet for EnumerableSet.UintSet;
 
   error ZeroChainSelectorNotAllowed();
@@ -54,7 +52,8 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
   error ReceiverError(bytes err);
   error TokenHandlingError(bytes err);
   error ReleaseOrMintBalanceMismatch(uint256 amountReleased, uint256 balancePre, uint256 balancePost);
-  error EmptyReport();
+  error EmptyReport(uint64 sourceChainSelector);
+  error EmptyBatch();
   error CursedByRMN(uint64 sourceChainSelector);
   error NotACompatiblePool(address notPool);
   error InvalidDataLength(uint256 expected, uint256 got);
@@ -340,7 +339,7 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
     Internal.ExecutionReport[] memory reports,
     GasLimitOverride[][] memory manualExecGasOverrides
   ) internal {
-    if (reports.length == 0) revert EmptyReport();
+    if (reports.length == 0) revert EmptyBatch();
 
     bool areManualGasLimitsEmpty = manualExecGasOverrides.length == 0;
     // Cache array for gas savings in the loop's condition
@@ -375,7 +374,7 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
     bytes memory onRamp = _getEnabledSourceChainConfig(sourceChainSelector).onRamp;
 
     uint256 numMsgs = report.messages.length;
-    if (numMsgs == 0) revert EmptyReport();
+    if (numMsgs == 0) revert EmptyReport(report.sourceChainSelector);
     if (numMsgs != report.offchainTokenData.length) revert UnexpectedTokenData();
 
     bytes32[] memory hashedLeaves = new bytes32[](numMsgs);
@@ -445,7 +444,7 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
         bool isOldCommitReport =
           (block.timestamp - timestampCommitted) > s_dynamicConfig.permissionLessExecutionThresholdSeconds;
         // Manually execution is fine if we previously failed or if the commit report is just too old
-        // Acceptable state transitions: FAILURE->SUCCESS, UNTOUCHED->SUCCESS, FAILURE->FAILURE
+        // Acceptable state transitions: UNTOUCHED->SUCCESS, UNTOUCHED->FAILURE, FAILURE->SUCCESS
         if (!(isOldCommitReport || originalState == Internal.MessageExecutionState.FAILURE)) {
           revert ManualExecutionNotYetEnabled(sourceChainSelector);
         }
@@ -466,7 +465,6 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
       // Nonce changes per state transition (these only apply for ordered messages):
       // UNTOUCHED -> FAILURE  nonce bump
       // UNTOUCHED -> SUCCESS  nonce bump
-      // FAILURE   -> FAILURE  no nonce bump
       // FAILURE   -> SUCCESS  no nonce bump
       // UNTOUCHED messages MUST be executed in order always
       // If nonce == 0 then out of order execution is allowed
@@ -493,9 +491,8 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
         _trialExecute(message, offchainTokenData, tokenGasOverrides);
       _setExecutionState(sourceChainSelector, message.header.sequenceNumber, newState);
 
-      // Since it's hard to estimate whether manual execution will succeed, we
-      // revert the entire transaction if it fails. This will show the user if
-      // their manual exec will fail before they submit it.
+      // Since it's hard to estimate whether manual execution will succeed, we revert the entire transaction
+      // if it fails. This will show the user if their manual exec will fail before they submit it.
       if (manualExecution) {
         if (newState == Internal.MessageExecutionState.FAILURE) {
           if (originalState != Internal.MessageExecutionState.UNTOUCHED) {
@@ -678,7 +675,7 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
     // Wrap and rethrow the error so we can catch it lower in the stack
     if (!success) revert TokenHandlingError(returnData);
 
-    // If the call was successful, the returnData should be the local token address.
+    // If the call was successful, the returnData should be the local token amount.
     if (returnData.length != Pool.CCIP_POOL_V1_RET_BYTES) {
       revert InvalidDataLength(Pool.CCIP_POOL_V1_RET_BYTES, returnData.length);
     }
@@ -734,6 +731,8 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
   /// @param receiver The address that will receive the tokens.
   /// @param sourceChainSelector The remote source chain selector.
   /// @param offchainTokenData Array of token data fetched offchain by the DON.
+  /// @param tokenGasOverrides Array of override gas limits to use for token transfers. If empty, the normal gas limit
+  /// as defined on the source chain is used.
   /// @return destTokenAmounts local token addresses with amounts
   /// @dev This function wraps the token pool call in a try catch block to gracefully handle
   /// any non-rate limiting errors that may occur. If we encounter a rate limiting related error
@@ -974,8 +973,8 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
       } else {
         if (currentConfig.minSeqNr != 1) {
           // OnRamp updates should only happens due to a misconfiguration
-          // If an OnRamp is misconfigured not reports should have been committed and no messages should have been executed
-          // This is enforced byt the onRamp address check in the commit function
+          // If an OnRamp is misconfigured, no reports should have been committed and no messages should have been executed
+          // This is enforced by the onRamp address check in the commit function
           revert InvalidOnRampUpdate(sourceChainSelector);
         }
       }