CCIP-4659 New Modified ERC165Checker library

contracts/.changeset/violet-lamps-pump.md

@@ -0,0 +1,10 @@
+---
+'@chainlink/contracts': patch
+---
+
+Create a new version of the ERC165Checker library which checks for sufficient gas before making an external call to prevent message delivery issues. #bugfix
+
+
+PR issue: CCIP-4659
+
+Solidity Review issue: CCIP-3966

contracts/gas-snapshots/ccip.gas-snapshot

@@ -28,7 +28,8 @@ CCIPHome_setCandidate:test_setCandidate() (gas: 1365392)
 CCIPHome_supportsInterface:test_supportsInterface() (gas: 9885)
 DefensiveExampleTest:test_HappyPath() (gas: 200517)
 DefensiveExampleTest:test_Recovery() (gas: 424996)
-E2E:test_E2E_3MessagesMMultiOffRampSuccess_gas() (gas: 1489913)
+E2E:test_E2E_3MessagesMMultiOffRampSuccess_gas() (gas: 1490237)
+ERC165CheckerReverting_supportsInterfaceReverting:test__supportsInterfaceReverting() (gas: 10445)
 EtherSenderReceiverTest_ccipReceive:test_ccipReceive_fallbackToWethTransfer() (gas: 96964)
 EtherSenderReceiverTest_ccipReceive:test_ccipReceive_happyPath() (gas: 49797)
 EtherSenderReceiverTest_ccipReceive:test_ccipReceive_wrongToken() (gas: 17460)
@@ -195,11 +196,11 @@ NonceManager_applyPreviousRampsUpdates:test_MultipleRampsUpdates() (gas: 123595)
 NonceManager_applyPreviousRampsUpdates:test_PreviousRampAlreadySet_overrideAllowed() (gas: 45935)
 NonceManager_applyPreviousRampsUpdates:test_SingleRampUpdate() (gas: 66937)
 NonceManager_applyPreviousRampsUpdates:test_ZeroInput() (gas: 12145)
-NonceManager_getInboundNonce:test_getInboundNonce_NoPrevOffRampForChain() (gas: 178870)
-NonceManager_getInboundNonce:test_getInboundNonce_Upgraded() (gas: 146059)
-NonceManager_getInboundNonce:test_getInboundNonce_UpgradedNonceNewSenderStartsAtZero() (gas: 182340)
-NonceManager_getInboundNonce:test_getInboundNonce_UpgradedNonceStartsAtV1Nonce() (gas: 245017)
-NonceManager_getInboundNonce:test_getInboundNonce_UpgradedOffRampNonceSkipsIfMsgInFlight() (gas: 213699)
+NonceManager_getInboundNonce:test_getInboundNonce_NoPrevOffRampForChain() (gas: 178906)
+NonceManager_getInboundNonce:test_getInboundNonce_Upgraded() (gas: 146095)
+NonceManager_getInboundNonce:test_getInboundNonce_UpgradedNonceNewSenderStartsAtZero() (gas: 182376)
+NonceManager_getInboundNonce:test_getInboundNonce_UpgradedNonceStartsAtV1Nonce() (gas: 245089)
+NonceManager_getInboundNonce:test_getInboundNonce_UpgradedOffRampNonceSkipsIfMsgInFlight() (gas: 213735)
 NonceManager_getInboundNonce:test_getInboundNonce_UpgradedSenderNoncesReadsPreviousRamp() (gas: 60418)
 NonceManager_getIncrementedOutboundNonce:test_getIncrementedOutboundNonce() (gas: 37974)
 NonceManager_getIncrementedOutboundNonce:test_incrementInboundNonce() (gas: 38746)
@@ -215,12 +216,12 @@ OffRamp_applySourceChainConfigUpdates:test_ApplyZeroUpdates() (gas: 16671)
 OffRamp_applySourceChainConfigUpdates:test_ReplaceExistingChain() (gas: 180998)
 OffRamp_applySourceChainConfigUpdates:test_ReplaceExistingChainOnRamp() (gas: 168513)
 OffRamp_applySourceChainConfigUpdates:test_allowNonOnRampUpdateAfterLaneIsUsed() (gas: 284861)
-OffRamp_batchExecute:test_MultipleReportsDifferentChains() (gas: 325920)
-OffRamp_batchExecute:test_MultipleReportsDifferentChainsSkipCursedChain() (gas: 170816)
-OffRamp_batchExecute:test_MultipleReportsSameChain() (gas: 269219)
-OffRamp_batchExecute:test_MultipleReportsSkipDuplicate() (gas: 161775)
-OffRamp_batchExecute:test_SingleReport() (gas: 149581)
-OffRamp_batchExecute:test_Unhealthy() (gas: 533110)
+OffRamp_batchExecute:test_MultipleReportsDifferentChains() (gas: 326028)
+OffRamp_batchExecute:test_MultipleReportsDifferentChainsSkipCursedChain() (gas: 170852)
+OffRamp_batchExecute:test_MultipleReportsSameChain() (gas: 269327)
+OffRamp_batchExecute:test_MultipleReportsSkipDuplicate() (gas: 161811)
+OffRamp_batchExecute:test_SingleReport() (gas: 149617)
+OffRamp_batchExecute:test_Unhealthy() (gas: 533326)
 OffRamp_commit:test_OnlyGasPriceUpdates() (gas: 112973)
 OffRamp_commit:test_OnlyTokenPriceUpdates() (gas: 112927)
 OffRamp_commit:test_PriceSequenceNumberCleared() (gas: 355397)
@@ -229,51 +230,51 @@ OffRamp_commit:test_ReportOnlyRootSuccess_gas() (gas: 141051)
 OffRamp_commit:test_RootWithRMNDisabled() (gas: 153873)
 OffRamp_commit:test_StaleReportWithRoot() (gas: 232101)
 OffRamp_commit:test_ValidPriceUpdateThenStaleReportWithRoot() (gas: 206722)
-OffRamp_constructor:test_Constructor() (gas: 6268112)
-OffRamp_execute:test_LargeBatch() (gas: 3372780)
-OffRamp_execute:test_MultipleReports() (gas: 291350)
-OffRamp_execute:test_MultipleReportsWithPartialValidationFailures() (gas: 364740)
-OffRamp_execute:test_SingleReport() (gas: 168814)
-OffRamp_executeSingleMessage:test_executeSingleMessage_NoTokens() (gas: 51574)
+OffRamp_constructor:test_Constructor() (gas: 6269512)
+OffRamp_execute:test_LargeBatch() (gas: 3373860)
+OffRamp_execute:test_MultipleReports() (gas: 291458)
+OffRamp_execute:test_MultipleReportsWithPartialValidationFailures() (gas: 364776)
+OffRamp_execute:test_SingleReport() (gas: 168850)
+OffRamp_executeSingleMessage:test_executeSingleMessage_NoTokens() (gas: 51610)
 OffRamp_executeSingleMessage:test_executeSingleMessage_NonContract() (gas: 20514)
-OffRamp_executeSingleMessage:test_executeSingleMessage_NonContractWithTokens() (gas: 230346)
-OffRamp_executeSingleMessage:test_executeSingleMessage_WithMessageInterceptor() (gas: 87429)
-OffRamp_executeSingleMessage:test_executeSingleMessage_WithTokens() (gas: 259827)
-OffRamp_executeSingleReport:test_InvalidSourcePoolAddress() (gas: 455214)
-OffRamp_executeSingleReport:test_ReceiverError() (gas: 180761)
-OffRamp_executeSingleReport:test_SingleMessageNoTokens() (gas: 205198)
-OffRamp_executeSingleReport:test_SingleMessageNoTokensOtherChain() (gas: 241285)
-OffRamp_executeSingleReport:test_SingleMessageNoTokensUnordered() (gas: 185191)
-OffRamp_executeSingleReport:test_SingleMessageToNonCCIPReceiver() (gas: 243935)
-OffRamp_executeSingleReport:test_SingleMessagesNoTokensSuccess_gas() (gas: 134620)
+OffRamp_executeSingleMessage:test_executeSingleMessage_NonContractWithTokens() (gas: 230418)
+OffRamp_executeSingleMessage:test_executeSingleMessage_WithMessageInterceptor() (gas: 87465)
+OffRamp_executeSingleMessage:test_executeSingleMessage_WithTokens() (gas: 259935)
+OffRamp_executeSingleReport:test_InvalidSourcePoolAddress() (gas: 455358)
+OffRamp_executeSingleReport:test_ReceiverError() (gas: 180797)
+OffRamp_executeSingleReport:test_SingleMessageNoTokens() (gas: 205270)
+OffRamp_executeSingleReport:test_SingleMessageNoTokensOtherChain() (gas: 241357)
+OffRamp_executeSingleReport:test_SingleMessageNoTokensUnordered() (gas: 185263)
+OffRamp_executeSingleReport:test_SingleMessageToNonCCIPReceiver() (gas: 243920)
+OffRamp_executeSingleReport:test_SingleMessagesNoTokensSuccess_gas() (gas: 134656)
 OffRamp_executeSingleReport:test_SkippedIncorrectNonce() (gas: 58298)
-OffRamp_executeSingleReport:test_SkippedIncorrectNonceStillExecutes() (gas: 392286)
-OffRamp_executeSingleReport:test_TwoMessagesWithTokensAndGE() (gas: 562211)
-OffRamp_executeSingleReport:test_TwoMessagesWithTokensSuccess_gas() (gas: 510592)
-OffRamp_executeSingleReport:test_Unhealthy() (gas: 528733)
-OffRamp_executeSingleReport:test_WithCurseOnAnotherSourceChain() (gas: 439083)
-OffRamp_executeSingleReport:test__execute_SkippedAlreadyExecutedMessage() (gas: 158002)
-OffRamp_executeSingleReport:test__execute_SkippedAlreadyExecutedMessageUnordered() (gas: 128386)
+OffRamp_executeSingleReport:test_SkippedIncorrectNonceStillExecutes() (gas: 392394)
+OffRamp_executeSingleReport:test_TwoMessagesWithTokensAndGE() (gas: 562427)
+OffRamp_executeSingleReport:test_TwoMessagesWithTokensSuccess_gas() (gas: 510808)
+OffRamp_executeSingleReport:test_Unhealthy() (gas: 528949)
+OffRamp_executeSingleReport:test_WithCurseOnAnotherSourceChain() (gas: 439299)
+OffRamp_executeSingleReport:test__execute_SkippedAlreadyExecutedMessage() (gas: 158038)
+OffRamp_executeSingleReport:test__execute_SkippedAlreadyExecutedMessageUnordered() (gas: 128422)
 OffRamp_getExecutionState:test_FillExecutionState() (gas: 3955662)
 OffRamp_getExecutionState:test_GetDifferentChainExecutionState() (gas: 121311)
 OffRamp_getExecutionState:test_GetExecutionState() (gas: 90102)
-OffRamp_manuallyExecute:test_manuallyExecute() (gas: 212296)
-OffRamp_manuallyExecute:test_manuallyExecute_DoesNotRevertIfUntouched() (gas: 165706)
-OffRamp_manuallyExecute:test_manuallyExecute_LowGasLimit() (gas: 479073)
-OffRamp_manuallyExecute:test_manuallyExecute_ReentrancyFails() (gas: 2229590)
-OffRamp_manuallyExecute:test_manuallyExecute_WithGasOverride() (gas: 212846)
-OffRamp_manuallyExecute:test_manuallyExecute_WithMultiReportGasOverride() (gas: 731858)
-OffRamp_manuallyExecute:test_manuallyExecute_WithPartialMessages() (gas: 336871)
-OffRamp_releaseOrMintSingleToken:test__releaseOrMintSingleToken() (gas: 94593)
-OffRamp_releaseOrMintTokens:test_releaseOrMintTokens() (gas: 161085)
-OffRamp_releaseOrMintTokens:test_releaseOrMintTokens_WithGasOverride() (gas: 162951)
-OffRamp_releaseOrMintTokens:test_releaseOrMintTokens_destDenominatedDecimals() (gas: 174204)
+OffRamp_manuallyExecute:test_manuallyExecute() (gas: 212368)
+OffRamp_manuallyExecute:test_manuallyExecute_DoesNotRevertIfUntouched() (gas: 165742)
+OffRamp_manuallyExecute:test_manuallyExecute_LowGasLimit() (gas: 479145)
+OffRamp_manuallyExecute:test_manuallyExecute_ReentrancyFails() (gas: 2229662)
+OffRamp_manuallyExecute:test_manuallyExecute_WithGasOverride() (gas: 212918)
+OffRamp_manuallyExecute:test_manuallyExecute_WithMultiReportGasOverride() (gas: 732218)
+OffRamp_manuallyExecute:test_manuallyExecute_WithPartialMessages() (gas: 337015)
+OffRamp_releaseOrMintSingleToken:test__releaseOrMintSingleToken() (gas: 94629)
+OffRamp_releaseOrMintTokens:test_releaseOrMintTokens() (gas: 161157)
+OffRamp_releaseOrMintTokens:test_releaseOrMintTokens_WithGasOverride() (gas: 163023)
+OffRamp_releaseOrMintTokens:test_releaseOrMintTokens_destDenominatedDecimals() (gas: 174276)
 OffRamp_setDynamicConfig:test_SetDynamicConfig() (gas: 25442)
 OffRamp_setDynamicConfig:test_SetDynamicConfigWithInterceptor() (gas: 47493)
-OffRamp_trialExecute:test_trialExecute() (gas: 263527)
-OffRamp_trialExecute:test_trialExecute_RateLimitError() (gas: 120685)
-OffRamp_trialExecute:test_trialExecute_TokenHandlingErrorIsCaught() (gas: 131995)
-OffRamp_trialExecute:test_trialExecute_TokenPoolIsNotAContract() (gas: 281272)
+OffRamp_trialExecute:test_trialExecute() (gas: 263635)
+OffRamp_trialExecute:test_trialExecute_RateLimitError() (gas: 120721)
+OffRamp_trialExecute:test_trialExecute_TokenHandlingErrorIsCaught() (gas: 132031)
+OffRamp_trialExecute:test_trialExecute_TokenPoolIsNotAContract() (gas: 281380)
 OnRampTokenPoolReentrancy:test_OnRampTokenPoolReentrancy() (gas: 244294)
 OnRamp_applyAllowlistUpdates:test_applyAllowlistUpdates() (gas: 325979)
 OnRamp_applyAllowlistUpdates:test_applyAllowlistUpdates_InvalidAllowListRequestDisabledAllowListWithAdds() (gas: 17190)

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

@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+import {IERC165} from "../../vendor/openzeppelin-solidity/v4.8.3/contracts/utils/introspection/IERC165.sol";
+
+/// @notice Library used to query support of an interface declared via {IERC165}.
+/// @dev These functions return the actual result of the query: they do not `revert` if an interface is not supported.
+library ERC165CheckerReverting {
+  error InsufficientGasForStaticCall();
+
+  // As per the EIP-165 spec, no interface should ever match 0xffffffff.
+  bytes4 private constant INTERFACE_ID_INVALID = 0xffffffff;
+
+  /// @dev 30k gas is required to make the staticcall. Under the 63/64 rule this means that 30,477 gas must be available
+  /// to ensure that at least 30k is forwarded. Checking for at least 31,000 ensures that after additional
+  /// operations are performed there is still >= 30,477 gas remaining.
+  /// 30,000 = ((30,477 * 63) / 64)
+  uint256 private constant MINIMUM_GAS_REQUIREMENT = 31_000;
+
+  /// @notice Returns true if `account` supports a defined interface.
+  /// @dev The function must support both the interfaceId and interfaces specified by ERC165 generally as per the standard.
+  /// @param account the contract to be queried for support.
+  /// @param interfaceId the interface being checked for support.
+  /// @return true if the contract at account indicates support of the interface with, false otherwise.
+  function _supportsInterfaceReverting(address account, bytes4 interfaceId) internal view returns (bool) {
+    // As a gas optimization, short circuit return false if interfaceId is not supported, as it is most likely interfaceId
+    // to be unsupported by the target.
+    return _supportsERC165InterfaceUncheckedReverting(account, interfaceId)
+      && !_supportsERC165InterfaceUncheckedReverting(account, INTERFACE_ID_INVALID)
+      && _supportsERC165InterfaceUncheckedReverting(account, type(IERC165).interfaceId);
+  }
+
+  /// @notice Query if a contract implements an interface, does not check ERC165 support
+  /// @param account The address of the contract to query for support of an interface
+  /// @param interfaceId The interface identifier, as specified in ERC-165
+  /// @return true if the contract at account indicates support of the interface with
+  /// identifier interfaceId, false otherwise
+  /// @dev Assumes that account contains a contract that supports ERC165, otherwise
+  /// the behavior of this method is undefined. This precondition can be checked.
+  /// @dev Function will only revert if the minimum gas requirement is not met before the staticcall is performed.
+  function _supportsERC165InterfaceUncheckedReverting(address account, bytes4 interfaceId) internal view returns (bool) {
+    bytes memory encodedParams = abi.encodeWithSelector(IERC165.supportsInterface.selector, interfaceId);
+
+    bool success;
+    uint256 returnSize;
+    uint256 returnValue;
+
+    bytes4 notEnoughGasSelector = InsufficientGasForStaticCall.selector;
+
+    assembly {
+      // The EVM does not return a specific error code if a revert is due to OOG. This check ensures that
+      // the message will not throw an OOG error by requiring that the amount of gas for the following
+      // staticcall exists before invoking it.
+      if lt(gas(), MINIMUM_GAS_REQUIREMENT) {
+        mstore(0x0, notEnoughGasSelector)
+        revert(0x0, 0x4)
+      }
+
+      success := staticcall(30000, account, add(encodedParams, 0x20), mload(encodedParams), 0x00, 0x20)
+      returnSize := returndatasize()
+      returnValue := mload(0x00)
+    }
+    return success && returnSize >= 0x20 && returnValue > 0;
+  }
+}

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

@@ -13,13 +13,13 @@ import {ITokenAdminRegistry} from "../interfaces/ITokenAdminRegistry.sol";
 
 import {CallWithExactGas} from "../../shared/call/CallWithExactGas.sol";
 import {Client} from "../libraries/Client.sol";
+import {ERC165CheckerReverting} from "../libraries/ERC165CheckerReverting.sol";
 import {Internal} from "../libraries/Internal.sol";
 import {MerkleMultiProof} from "../libraries/MerkleMultiProof.sol";
 import {Pool} from "../libraries/Pool.sol";
 import {MultiOCR3Base} from "../ocr/MultiOCR3Base.sol";
 
 import {IERC20} from "../../vendor/openzeppelin-solidity/v5.0.2/contracts/token/ERC20/IERC20.sol";
-import {ERC165Checker} from "../../vendor/openzeppelin-solidity/v5.0.2/contracts/utils/introspection/ERC165Checker.sol";
 import {EnumerableSet} from "../../vendor/openzeppelin-solidity/v5.0.2/contracts/utils/structs/EnumerableSet.sol";
 
 /// @notice OffRamp enables OCR networks to execute multiple messages in an OffRamp in a single transaction.
@@ -28,7 +28,7 @@ import {EnumerableSet} from "../../vendor/openzeppelin-solidity/v5.0.2/contracts
 /// @dev MultiOCR3Base is used to store multiple OCR configs for the OffRamp. The execution plugin type has to be
 /// configured without signature verification, and the commit plugin type with verification.
 contract OffRamp is ITypeAndVersion, MultiOCR3Base {
-  using ERC165Checker for address;
+  using ERC165CheckerReverting for address;
   using EnumerableSet for EnumerableSet.UintSet;
 
   error ZeroChainSelectorNotAllowed();
@@ -604,9 +604,12 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
     // 3. If the receiver is a contract but it does not support the IAny2EVMMessageReceiver interface.
     //
     // The ordering of these checks is important, as the first check is the cheapest to execute.
+    //
+    // To prevent message delivery bypass issues, a modified version of the ERC165Checker is used
+    // which checks for sufficient gas before making the external call.
     if (
       (message.data.length == 0 && message.gasLimit == 0) || message.receiver.code.length == 0
-        || !message.receiver.supportsInterface(type(IAny2EVMMessageReceiver).interfaceId)
+        || !message.receiver._supportsInterfaceReverting(type(IAny2EVMMessageReceiver).interfaceId)
     ) return;
 
     (bool success, bytes memory returnData,) = s_sourceChainConfigs[message.header.sourceChainSelector]
@@ -647,7 +650,7 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
     // This is done to prevent a pool from reverting the entire transaction if it doesn't support the interface.
     // The call gets a max or 30k gas per instance, of which there are three. This means offchain gas estimations should
     // account for 90k gas overhead due to the interface check.
-    if (localPoolAddress == address(0) || !localPoolAddress.supportsInterface(Pool.CCIP_POOL_V1)) {
+    if (localPoolAddress == address(0) || !localPoolAddress._supportsInterfaceReverting(Pool.CCIP_POOL_V1)) {
       revert NotACompatiblePool(localPoolAddress);
     }
 

contracts/src/v0.8/ccip/test/libraries/ERC165CheckerReverting.supportsInterfaceReverting.t.sol

@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: BUSL-1.1
+pragma solidity ^0.8.24;
+
+import {IAny2EVMMessageReceiver} from "../../interfaces/IAny2EVMMessageReceiver.sol";
+
+import {ERC165CheckerReverting} from "../../libraries/ERC165CheckerReverting.sol";
+import {MaybeRevertMessageReceiver} from "../helpers/receivers/MaybeRevertMessageReceiver.sol";
+
+import {Test} from "forge-std/Test.sol";
+
+contract ERC165CheckerReverting_supportsInterfaceReverting is Test {
+  using ERC165CheckerReverting for address;
+
+  address internal s_receiver;
+
+  bytes4 internal constant EXAMPLE_INTERFACE_ID = 0xdeadbeef;
+
+  error InsufficientGasForStaticCall();
+
+  constructor() {
+    s_receiver = address(new MaybeRevertMessageReceiver(false));
+  }
+
+  function test__supportsInterfaceReverting() public view {
+    assertTrue(s_receiver._supportsInterfaceReverting(type(IAny2EVMMessageReceiver).interfaceId));
+  }
+
+  // Reverts
+
+  function test__supportsInterfaceReverting_RevertWhen_NotEnoughGasForSupportsInterface() public {
+    vm.expectRevert(InsufficientGasForStaticCall.selector);
+
+    // Library calls cannot be called with gas limit overrides, so a public function must be exposed
+    // instead which can proxy the call to the library.
+
+    // The gas limit was chosen so that after overhead, <31k would remain to trigger the error.
+    this.invokeERC165Checker{gas: 33_000}();
+  }
+
+  // Meant to test the call with a manual gas limit override
+  function invokeERC165Checker() external view {
+    s_receiver._supportsInterfaceReverting(EXAMPLE_INTERFACE_ID);
+  }
+}