CCIP-5046 Minor FeeQuoter Fixes (#16090)

* init FeeQuoter and Typo Fixes * enforced-> enforce * SVM_EXTRA_EXTRA_ARGS_V1_TAG -> SVM_EXTRA_ARGS_V1_TAG * restore destChainConfig.isEnabled check and add chain family selector check to applyDestChainConfigUpdates * remove redundant address cast inn message transformer * update wrappers * [Bot] Update changeset file with jira issues * fmt * add fuzz for chain family selector in testFuzz_applyDestChainConfigUpdates_Success * simplify testFuzz_applyDestChainConfigUpdates_Success * fix test * add tokenReceiver validation to getValidatedFee * updated wrappers --------- Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
smartcontractkit · Jan 29, 2025 · 2efb46c · 2efb46c
1 parent ef0bf69
commit 2efb46c
Show file tree

Hide file tree

Showing 12 changed files with 219 additions and 162 deletions.
diff --git a/contracts/.changeset/seven-worms-yawn.md b/contracts/.changeset/seven-worms-yawn.md
@@ -0,0 +1,10 @@
+---
+'@chainlink/contracts': patch
+---
+
+#internal Minor FeeQuoter audit fixes
+
+PR issue: CCIP-5046
+
+
+Solidity Review issue: CCIP-3966
diff --git a/contracts/gas-snapshots/ccip.gas-snapshot b/contracts/gas-snapshots/ccip.gas-snapshot
diff --git a/contracts/src/v0.8/ccip/FeeQuoter.sol b/contracts/src/v0.8/ccip/FeeQuoter.sol
@@ -555,12 +555,13 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
     Client.EVM2AnyMessage calldata message
   ) external view returns (uint256 feeTokenAmount) {
     DestChainConfig memory destChainConfig = s_destChainConfigs[destChainSelector];
+    if (!destChainConfig.isEnabled) revert DestinationChainNotEnabled(destChainSelector);
     if (!s_feeTokens.contains(message.feeToken)) revert FeeTokenNotSupported(message.feeToken);
 
     uint256 numberOfTokens = message.tokenAmounts.length;
-    uint256 gasLimit = _resolveGasLimitForDestination(message.extraArgs, destChainConfig);
-
-    _validateMessage(destChainConfig, message.data.length, numberOfTokens, gasLimit, message.receiver);
+    uint256 gasLimit = _validateMessageAndResolveGasLimitForDestination(
+      destChainConfig, message.data.length, numberOfTokens, message.extraArgs, message.receiver
+    );
 
     // The below call asserts that feeToken is a supported token.
     uint224 feeTokenPrice = _getValidatedTokenPrice(message.feeToken);
@@ -890,44 +891,24 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
     revert InvalidChainFamilySelector(chainFamilySelector);
   }
 
-  function _resolveGasLimitForDestination(
-    bytes calldata extraArgs,
-    DestChainConfig memory destChainConfig
-  ) internal pure returns (uint256) {
-    if (destChainConfig.chainFamilySelector == Internal.CHAIN_FAMILY_SELECTOR_EVM) {
-      return _parseEVMExtraArgsFromBytes(
-        extraArgs,
-        destChainConfig.defaultTxGasLimit,
-        destChainConfig.maxPerMsgGasLimit,
-        destChainConfig.enforceOutOfOrder
-      ).gasLimit;
-    }
-    if (destChainConfig.chainFamilySelector == Internal.CHAIN_FAMILY_SELECTOR_SVM) {
-      return _parseSVMExtraArgsFromBytes(
-        extraArgs, destChainConfig.maxPerMsgGasLimit, destChainConfig.enforceOutOfOrder
-      ).computeUnits;
-    }
-    revert InvalidChainFamilySelector(destChainConfig.chainFamilySelector);
-  }
-
   /// @notice Parse and validate the SVM specific Extra Args Bytes.
   function _parseSVMExtraArgsFromBytes(
     bytes calldata extraArgs,
     uint256 maxPerMsgGasLimit,
-    bool enforcedOutOfOrder
+    bool enforceOutOfOrder
   ) internal pure returns (Client.SVMExtraArgsV1 memory svmExtraArgs) {
     if (extraArgs.length == 0) {
       revert InvalidExtraArgsData();
     }
 
     bytes4 tag = bytes4(extraArgs[:4]);
-    if (tag != Client.SVM_EXTRA_EXTRA_ARGS_V1_TAG) {
+    if (tag != Client.SVM_EXTRA_ARGS_V1_TAG) {
       revert InvalidExtraArgsTag();
     }
 
     svmExtraArgs = abi.decode(extraArgs[4:], (Client.SVMExtraArgsV1));
 
-    if (enforcedOutOfOrder && !svmExtraArgs.allowOutOfOrderExecution) {
+    if (enforceOutOfOrder && !svmExtraArgs.allowOutOfOrderExecution) {
       revert ExtraArgOutOfOrderExecutionMustBeTrue();
     }
 
@@ -993,21 +974,44 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
   /// @param dataLength The length of the data field of the message.
   /// @param numberOfTokens The number of tokens to be sent.
   /// @param receiver Message receiver on the dest chain.
-  function _validateMessage(
+  /// @return gasLimit The gas limit to use for the message.
+  function _validateMessageAndResolveGasLimitForDestination(
     DestChainConfig memory destChainConfig,
     uint256 dataLength,
     uint256 numberOfTokens,
-    uint256 gasLimit,
+    bytes calldata extraArgs,
     bytes memory receiver
-  ) internal pure {
+  ) internal pure returns (uint256 gasLimit) {
     // Check that payload is formed correctly.
     if (dataLength > uint256(destChainConfig.maxDataBytes)) {
       revert MessageTooLarge(uint256(destChainConfig.maxDataBytes), dataLength);
     }
     if (numberOfTokens > uint256(destChainConfig.maxNumberOfTokensPerMsg)) {
       revert UnsupportedNumberOfTokens(numberOfTokens, destChainConfig.maxNumberOfTokensPerMsg);
     }
+
+    // resolve gas limit and validate chainFamilySelector
+    if (destChainConfig.chainFamilySelector == Internal.CHAIN_FAMILY_SELECTOR_EVM) {
+      gasLimit = _parseEVMExtraArgsFromBytes(
+        extraArgs,
+        destChainConfig.defaultTxGasLimit,
+        destChainConfig.maxPerMsgGasLimit,
+        destChainConfig.enforceOutOfOrder
+      ).gasLimit;
+    } else if (destChainConfig.chainFamilySelector == Internal.CHAIN_FAMILY_SELECTOR_SVM) {
+      Client.SVMExtraArgsV1 memory svmExtraArgsV1 =
+        _parseSVMExtraArgsFromBytes(extraArgs, destChainConfig.maxPerMsgGasLimit, destChainConfig.enforceOutOfOrder);
+      if (numberOfTokens > 0 && svmExtraArgsV1.tokenReceiver == bytes32(0)) {
+        revert InvalidTokenReceiver();
+      }
+      gasLimit = svmExtraArgsV1.computeUnits;
+    } else {
+      revert InvalidChainFamilySelector(destChainConfig.chainFamilySelector);
+    }
+
     _validateDestFamilyAddress(destChainConfig.chainFamilySelector, receiver, gasLimit);
+
+    return gasLimit;
   }
 
   /// @inheritdoc IFeeQuoter
@@ -1150,9 +1154,14 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
       DestChainConfig memory destChainConfig = destChainConfigArg.destChainConfig;
 
       // destChainSelector must be non-zero, defaultTxGasLimit must be set, must be less than maxPerMsgGasLimit
+      // supported chain family is EVM and SVM
       if (
         destChainSelector == 0 || destChainConfig.defaultTxGasLimit == 0
           || destChainConfig.defaultTxGasLimit > destChainConfig.maxPerMsgGasLimit
+          || (
+            destChainConfig.chainFamilySelector != Internal.CHAIN_FAMILY_SELECTOR_EVM
+              && destChainConfig.chainFamilySelector != Internal.CHAIN_FAMILY_SELECTOR_SVM
+          )
       ) {
         revert InvalidDestChainConfig(destChainSelector);
       }

diff --git a/contracts/src/v0.8/ccip/libraries/Client.sol b/contracts/src/v0.8/ccip/libraries/Client.sol
@@ -43,7 +43,7 @@ library Client {
   bytes4 public constant EVM_EXTRA_ARGS_V2_TAG = 0x181dcf10;
 
   // bytes4(keccak256("CCIP SVMExtraArgsV1"));
-  bytes4 public constant SVM_EXTRA_EXTRA_ARGS_V1_TAG = 0x1f3b3aba;
+  bytes4 public constant SVM_EXTRA_ARGS_V1_TAG = 0x1f3b3aba;
 
   /// @param gasLimit: gas limit for the callback on the destination chain.
   /// @param allowOutOfOrderExecution: if true, it indicates that the message can be executed in any order relative to
@@ -71,6 +71,6 @@ library Client {
   function _svmArgsToBytes(
     SVMExtraArgsV1 memory extraArgs
   ) internal pure returns (bytes memory bts) {
-    return abi.encodeWithSelector(SVM_EXTRA_EXTRA_ARGS_V1_TAG, extraArgs);
+    return abi.encodeWithSelector(SVM_EXTRA_ARGS_V1_TAG, extraArgs);
   }
 }
diff --git a/contracts/src/v0.8/ccip/offRamp/OffRampWithMessageTransformer.sol b/contracts/src/v0.8/ccip/offRamp/OffRampWithMessageTransformer.sol
@@ -16,7 +16,7 @@ contract OffRampWithMessageTransformer is OffRamp {
     SourceChainConfigArgs[] memory sourceChainConfigs,
     address messageTransformerAddr
   ) OffRamp(staticConfig, dynamicConfig, sourceChainConfigs) {
-    if (address(messageTransformerAddr) == address(0)) {
+    if (messageTransformerAddr == address(0)) {
       revert ZeroAddressNotAllowed();
     }
     s_messageTransformer = messageTransformerAddr;
@@ -33,7 +33,7 @@ contract OffRampWithMessageTransformer is OffRamp {
   function setMessageTransformer(
     address messageTransformerAddr
   ) external onlyOwner {
-    if (address(messageTransformerAddr) == address(0)) {
+    if (messageTransformerAddr == address(0)) {
       revert ZeroAddressNotAllowed();
     }
     s_messageTransformer = messageTransformerAddr;

diff --git a/contracts/src/v0.8/ccip/onRamp/OnRampWithMessageTransformer.sol b/contracts/src/v0.8/ccip/onRamp/OnRampWithMessageTransformer.sol
@@ -18,7 +18,7 @@ contract OnRampWithMessageTransformer is OnRamp {
     DestChainConfigArgs[] memory destChainConfigs,
     address messageTransformerAddr
   ) OnRamp(staticConfig, dynamicConfig, destChainConfigs) {
-    if (address(messageTransformerAddr) == address(0)) {
+    if (messageTransformerAddr == address(0)) {
       revert ZeroAddressNotAllowed();
     }
     s_messageTransformer = messageTransformerAddr;
@@ -35,7 +35,7 @@ contract OnRampWithMessageTransformer is OnRamp {
   function setMessageTransformer(
     address messageTransformerAddr
   ) external onlyOwner {
-    if (address(messageTransformerAddr) == address(0)) {
+    if (messageTransformerAddr == address(0)) {
       revert ZeroAddressNotAllowed();
     }
     s_messageTransformer = messageTransformerAddr;

diff --git a/contracts/src/v0.8/ccip/test/feeQuoter/FeeQuoter.applyDestChainConfigUpdates.t.sol b/contracts/src/v0.8/ccip/test/feeQuoter/FeeQuoter.applyDestChainConfigUpdates.t.sol
@@ -6,6 +6,9 @@ import {Internal} from "../../libraries/Internal.sol";
 import {FeeQuoterSetup} from "./FeeQuoterSetup.t.sol";
 
 contract FeeQuoter_applyDestChainConfigUpdates is FeeQuoterSetup {
+  bytes4[] internal s_validChainFamilySelector =
+    [Internal.CHAIN_FAMILY_SELECTOR_EVM, Internal.CHAIN_FAMILY_SELECTOR_SVM];
+
   function testFuzz_applyDestChainConfigUpdates_Success(
     FeeQuoter.DestChainConfigArgs memory destChainConfigArgs
   ) public {
@@ -16,26 +19,25 @@ contract FeeQuoter_applyDestChainConfigUpdates is FeeQuoterSetup {
         destChainConfigArgs.destChainConfig.defaultTxGasLimit, 1, destChainConfigArgs.destChainConfig.maxPerMsgGasLimit
       )
     );
-    destChainConfigArgs.destChainConfig.chainFamilySelector = Internal.CHAIN_FAMILY_SELECTOR_EVM;
 
-    bool isNewChain = destChainConfigArgs.destChainSelector != DEST_CHAIN_SELECTOR;
+    for (uint256 i = 0; i < s_validChainFamilySelector.length; i++) {
+      destChainConfigArgs.destChainConfig.chainFamilySelector = s_validChainFamilySelector[i];
+      destChainConfigArgs.destChainSelector = uint64(
+        uint256(keccak256(abi.encodePacked(destChainConfigArgs.destChainSelector, s_validChainFamilySelector[i])))
+      );
 
-    FeeQuoter.DestChainConfigArgs[] memory newDestChainConfigArgs = new FeeQuoter.DestChainConfigArgs[](1);
-    newDestChainConfigArgs[0] = destChainConfigArgs;
+      FeeQuoter.DestChainConfigArgs[] memory newDestChainConfigArgs = new FeeQuoter.DestChainConfigArgs[](1);
+      newDestChainConfigArgs[0] = destChainConfigArgs;
 
-    if (isNewChain) {
       vm.expectEmit();
       emit FeeQuoter.DestChainAdded(destChainConfigArgs.destChainSelector, destChainConfigArgs.destChainConfig);
-    } else {
-      vm.expectEmit();
-      emit FeeQuoter.DestChainConfigUpdated(destChainConfigArgs.destChainSelector, destChainConfigArgs.destChainConfig);
-    }
 
-    s_feeQuoter.applyDestChainConfigUpdates(newDestChainConfigArgs);
+      s_feeQuoter.applyDestChainConfigUpdates(newDestChainConfigArgs);
 
-    _assertFeeQuoterDestChainConfigsEqual(
-      destChainConfigArgs.destChainConfig, s_feeQuoter.getDestChainConfig(destChainConfigArgs.destChainSelector)
-    );
+      _assertFeeQuoterDestChainConfigsEqual(
+        destChainConfigArgs.destChainConfig, s_feeQuoter.getDestChainConfig(destChainConfigArgs.destChainSelector)
+      );
+    }
   }
 
   function test_applyDestChainConfigUpdates() public {
@@ -109,4 +111,16 @@ contract FeeQuoter_applyDestChainConfigUpdates is FeeQuoterSetup {
     );
     s_feeQuoter.applyDestChainConfigUpdates(destChainConfigArgs);
   }
+
+  function test_RevertWhen_InvalidChainFamilySelector() public {
+    FeeQuoter.DestChainConfigArgs[] memory destChainConfigArgs = _generateFeeQuoterDestChainConfigArgs();
+    FeeQuoter.DestChainConfigArgs memory destChainConfigArg = destChainConfigArgs[0];
+
+    destChainConfigArg.destChainConfig.chainFamilySelector = bytes4(uint32(1));
+
+    vm.expectRevert(
+      abi.encodeWithSelector(FeeQuoter.InvalidDestChainConfig.selector, destChainConfigArg.destChainSelector)
+    );
+    s_feeQuoter.applyDestChainConfigUpdates(destChainConfigArgs);
+  }
 }
diff --git a/contracts/src/v0.8/ccip/test/feeQuoter/FeeQuoter.getValidatedFee.t.sol b/contracts/src/v0.8/ccip/test/feeQuoter/FeeQuoter.getValidatedFee.t.sol
@@ -223,6 +223,11 @@ contract FeeQuoter_getValidatedFee is FeeQuoterFeeSetup {
 
   // Reverts
 
+  function test_RevertWhen_DestinationChainNotEnabled() public {
+    vm.expectRevert(abi.encodeWithSelector(FeeQuoter.DestinationChainNotEnabled.selector, DEST_CHAIN_SELECTOR + 1));
+    s_feeQuoter.getValidatedFee(DEST_CHAIN_SELECTOR + 1, _generateEmptyMessage());
+  }
+
   function test_RevertWhen_EnforceOutOfOrder() public {
     // Update config to enforce allowOutOfOrderExecution = true.
     vm.stopPrank();
@@ -283,4 +288,30 @@ contract FeeQuoter_getValidatedFee is FeeQuoterFeeSetup {
 
     s_feeQuoter.getValidatedFee(DEST_CHAIN_SELECTOR, message);
   }
+
+  function test_RevertWhen_SVMMessageWithTokenTransferAndInvalidTokenReceiver() public {
+    vm.stopPrank();
+    //setup to set chainFamilySelector for SVM so that token receiver's check flow is enabled
+    vm.startPrank(OWNER);
+
+    FeeQuoter.DestChainConfigArgs[] memory destChainConfigArgs = _generateFeeQuoterDestChainConfigArgs();
+    destChainConfigArgs[0].destChainConfig.chainFamilySelector = Internal.CHAIN_FAMILY_SELECTOR_SVM;
+
+    s_feeQuoter.applyDestChainConfigUpdates(destChainConfigArgs);
+    vm.stopPrank();
+
+    Client.EVM2AnyMessage memory message = _generateSingleTokenMessage(s_sourceFeeToken, 1);
+    // replace with SVM Extra Args
+    message.extraArgs = Client._svmArgsToBytes(
+      Client.SVMExtraArgsV1({
+        computeUnits: GAS_LIMIT,
+        accountIsWritableBitmap: 0,
+        allowOutOfOrderExecution: true,
+        tokenReceiver: bytes32(0),
+        accounts: new bytes32[](0)
+      })
+    );
+    vm.expectRevert(FeeQuoter.InvalidTokenReceiver.selector);
+    s_feeQuoter.getValidatedFee(DEST_CHAIN_SELECTOR, message);
+  }
 }
diff --git a/contracts/src/v0.8/ccip/test/feeQuoter/FeeQuoter.parseSVMExtraArgsFromBytes.t.sol b/contracts/src/v0.8/ccip/test/feeQuoter/FeeQuoter.parseSVMExtraArgsFromBytes.t.sol
@@ -27,7 +27,7 @@ contract FeeQuoter_parseSVMExtraArgsFromBytes is FeeQuoterSetup {
   }
 
   function test_SVMExtraArgsV1TagSelector() public view {
-    assertEq(Client.SVM_EXTRA_EXTRA_ARGS_V1_TAG, bytes4(keccak256("CCIP SVMExtraArgsV1")));
+    assertEq(Client.SVM_EXTRA_ARGS_V1_TAG, bytes4(keccak256("CCIP SVMExtraArgsV1")));
   }
 
   function test_SVMExtraArgsV1() public view {
@@ -89,12 +89,12 @@ contract FeeQuoter_parseSVMExtraArgsFromBytes is FeeQuoterSetup {
 
   function test_RevertWhen_ExtraArgOutOfOrderExecutionIsFalse() public {
     bytes memory inputExtraArgs = abi.encodeWithSelector(
-      Client.SVM_EXTRA_EXTRA_ARGS_V1_TAG,
+      Client.SVM_EXTRA_ARGS_V1_TAG,
       Client.SVMExtraArgsV1({
         computeUnits: 1_000_000,
         accountIsWritableBitmap: 0,
         tokenReceiver: bytes32(0),
-        allowOutOfOrderExecution: false, // mismatch with enforcedOutOfOrder = true
+        allowOutOfOrderExecution: false, // mismatch with enforceOutOfOrder = true
         accounts: new bytes32[](0)
       })
     );

diff --git a/contracts/src/v0.8/ccip/test/helpers/FeeQuoterHelper.sol b/contracts/src/v0.8/ccip/test/helpers/FeeQuoterHelper.sol
@@ -112,11 +112,4 @@ contract FeeQuoterHelper is FeeQuoter {
   ) external pure returns (uint224) {
     return _calculateRebasedValue(dataFeedDecimal, tokenDecimal, feedValue);
   }
-
-  function resolveGasLimitForDestination(
-    bytes calldata extraArgs,
-    DestChainConfig memory destChainConfig
-  ) external pure returns (uint256 gasLimit) {
-    return _resolveGasLimitForDestination(extraArgs, destChainConfig);
-  }
 }
diff --git a/core/gethwrappers/ccip/generated/fee_quoter/fee_quoter.go b/core/gethwrappers/ccip/generated/fee_quoter/fee_quoter.go
diff --git a/core/gethwrappers/ccip/generation/generated-wrapper-dependency-versions-do-not-edit.txt b/core/gethwrappers/ccip/generation/generated-wrapper-dependency-versions-do-not-edit.txt
@@ -6,7 +6,7 @@ ccip_encoding_utils: ../../../contracts/solc/ccip/EncodingUtils/EncodingUtils.so
 ccip_home: ../../../contracts/solc/ccip/CCIPHome/CCIPHome.sol/CCIPHome.abi.json ../../../contracts/solc/ccip/CCIPHome/CCIPHome.sol/CCIPHome.bin 39de1fbc907a2b573e9358e716803bf5ac3b0a2e622d5bc0069ab60daf38949b
 ccip_reader_tester: ../../../contracts/solc/ccip/CCIPReaderTester/CCIPReaderTester.sol/CCIPReaderTester.abi.json ../../../contracts/solc/ccip/CCIPReaderTester/CCIPReaderTester.sol/CCIPReaderTester.bin b8e597d175ec5ff4990d98b4e3b8a8cf06c6ae22977dd6f0e58c0f4107639e8f
 ether_sender_receiver: ../../../contracts/solc/ccip/EtherSenderReceiver/EtherSenderReceiver.sol/EtherSenderReceiver.abi.json ../../../contracts/solc/ccip/EtherSenderReceiver/EtherSenderReceiver.sol/EtherSenderReceiver.bin 88973abc1bfbca23a23704e20087ef46f2e20581a13477806308c8f2e664844e
-fee_quoter: ../../../contracts/solc/ccip/FeeQuoter/FeeQuoter.sol/FeeQuoter.abi.json ../../../contracts/solc/ccip/FeeQuoter/FeeQuoter.sol/FeeQuoter.bin 7be986f71fb72d1790b05033ba39531679284ff6a1b8f4978aea11763d932e73
+fee_quoter: ../../../contracts/solc/ccip/FeeQuoter/FeeQuoter.sol/FeeQuoter.abi.json ../../../contracts/solc/ccip/FeeQuoter/FeeQuoter.sol/FeeQuoter.bin 3efd18088f1a27b497ec5b0936f54931e0e151033d5cb32649d497525a3964fa
 lock_release_token_pool: ../../../contracts/solc/ccip/LockReleaseTokenPool/LockReleaseTokenPool.sol/LockReleaseTokenPool.abi.json ../../../contracts/solc/ccip/LockReleaseTokenPool/LockReleaseTokenPool.sol/LockReleaseTokenPool.bin 2e73ee0da6f9a9a5722294289b969e4202476706e5d7cdb623e728831c79c28b
 log_message_data_receiver: ../../../contracts/solc/ccip/LogMessageDataReceiver/LogMessageDataReceiver.sol/LogMessageDataReceiver.abi.json ../../../contracts/solc/ccip/LogMessageDataReceiver/LogMessageDataReceiver.sol/LogMessageDataReceiver.bin 6fe60e48711884eae82dd95cabb1c66a5644336719fa1219df1ceceec11e6bce
 maybe_revert_message_receiver: ../../../contracts/solc/ccip/MaybeRevertMessageReceiver/MaybeRevertMessageReceiver.sol/MaybeRevertMessageReceiver.abi.json ../../../contracts/solc/ccip/MaybeRevertMessageReceiver/MaybeRevertMessageReceiver.sol/MaybeRevertMessageReceiver.bin d1eb951af1027ca20cbee2c34df80fddbfd861e1695989aeebd29327cfe56584