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: 1365438)
 CCIPHome_supportsInterface:test_supportsInterface() (gas: 9885)
 DefensiveExampleTest:test_HappyPath() (gas: 200521)
 DefensiveExampleTest:test_Recovery() (gas: 425013)
-E2E:test_E2E_3MessagesMMultiOffRampSuccess_gas() (gas: 1487929)
+E2E:test_E2E_3MessagesMMultiOffRampSuccess_gas() (gas: 1488253)
+ERC165CheckerReverting_supportsInterfaceReverting:test__supportsInterfaceReverting() (gas: 10445)
 EtherSenderReceiverTest_ccipReceive:test_ccipReceive_fallbackToWethTransfer() (gas: 96980)
 EtherSenderReceiverTest_ccipReceive:test_ccipReceive_happyPath() (gas: 49812)
 EtherSenderReceiverTest_ccipReceive:test_ccipReceive_wrongToken() (gas: 17479)
@@ -195,11 +196,11 @@ NonceManager_applyPreviousRampsUpdates:test_MultipleRampsUpdates() (gas: 123593)
 NonceManager_applyPreviousRampsUpdates:test_PreviousRampAlreadySet_overrideAllowed() (gas: 45963)
 NonceManager_applyPreviousRampsUpdates:test_SingleRampUpdate() (gas: 66943)
 NonceManager_applyPreviousRampsUpdates:test_ZeroInput() (gas: 12191)
-NonceManager_getInboundNonce:test_getInboundNonce_NoPrevOffRampForChain() (gas: 178734)
-NonceManager_getInboundNonce:test_getInboundNonce_Upgraded() (gas: 145889)
-NonceManager_getInboundNonce:test_getInboundNonce_UpgradedNonceNewSenderStartsAtZero() (gas: 182209)
-NonceManager_getInboundNonce:test_getInboundNonce_UpgradedNonceStartsAtV1Nonce() (gas: 244710)
-NonceManager_getInboundNonce:test_getInboundNonce_UpgradedOffRampNonceSkipsIfMsgInFlight() (gas: 213585)
+NonceManager_getInboundNonce:test_getInboundNonce_NoPrevOffRampForChain() (gas: 178770)
+NonceManager_getInboundNonce:test_getInboundNonce_Upgraded() (gas: 145925)
+NonceManager_getInboundNonce:test_getInboundNonce_UpgradedNonceNewSenderStartsAtZero() (gas: 182245)
+NonceManager_getInboundNonce:test_getInboundNonce_UpgradedNonceStartsAtV1Nonce() (gas: 244782)
+NonceManager_getInboundNonce:test_getInboundNonce_UpgradedOffRampNonceSkipsIfMsgInFlight() (gas: 213621)
 NonceManager_getInboundNonce:test_getInboundNonce_UpgradedSenderNoncesReadsPreviousRamp() (gas: 60520)
 NonceManager_getIncrementedOutboundNonce:test_getIncrementedOutboundNonce() (gas: 37979)
 NonceManager_getIncrementedOutboundNonce:test_incrementInboundNonce() (gas: 38756)
@@ -215,12 +216,12 @@ OffRamp_applySourceChainConfigUpdates:test_ApplyZeroUpdates() (gas: 16720)
 OffRamp_applySourceChainConfigUpdates:test_ReplaceExistingChain() (gas: 181035)
 OffRamp_applySourceChainConfigUpdates:test_ReplaceExistingChainOnRamp() (gas: 168558)
 OffRamp_applySourceChainConfigUpdates:test_allowNonOnRampUpdateAfterLaneIsUsed() (gas: 284716)
-OffRamp_batchExecute:test_MultipleReportsDifferentChains() (gas: 325065)
-OffRamp_batchExecute:test_MultipleReportsDifferentChainsSkipCursedChain() (gas: 170394)
-OffRamp_batchExecute:test_MultipleReportsSameChain() (gas: 268364)
-OffRamp_batchExecute:test_MultipleReportsSkipDuplicate() (gas: 161411)
-OffRamp_batchExecute:test_SingleReport() (gas: 149342)
-OffRamp_batchExecute:test_Unhealthy() (gas: 532022)
+OffRamp_batchExecute:test_MultipleReportsDifferentChains() (gas: 325173)
+OffRamp_batchExecute:test_MultipleReportsDifferentChainsSkipCursedChain() (gas: 170430)
+OffRamp_batchExecute:test_MultipleReportsSameChain() (gas: 268472)
+OffRamp_batchExecute:test_MultipleReportsSkipDuplicate() (gas: 161447)
+OffRamp_batchExecute:test_SingleReport() (gas: 149378)
+OffRamp_batchExecute:test_Unhealthy() (gas: 532238)
 OffRamp_commit:test_OnlyGasPriceUpdates() (gas: 112719)
 OffRamp_commit:test_OnlyTokenPriceUpdates() (gas: 112673)
 OffRamp_commit:test_PriceSequenceNumberCleared() (gas: 354787)
@@ -229,51 +230,51 @@ OffRamp_commit:test_ReportOnlyRootSuccess_gas() (gas: 140878)
 OffRamp_commit:test_RootWithRMNDisabled() (gas: 153674)
 OffRamp_commit:test_StaleReportWithRoot() (gas: 231732)
 OffRamp_commit:test_ValidPriceUpdateThenStaleReportWithRoot() (gas: 206199)
-OffRamp_constructor:test_Constructor() (gas: 6210275)
-OffRamp_execute:test_LargeBatch() (gas: 3354418)
-OffRamp_execute:test_MultipleReports() (gas: 290585)
-OffRamp_execute:test_MultipleReportsWithPartialValidationFailures() (gas: 364141)
-OffRamp_execute:test_SingleReport() (gas: 168585)
-OffRamp_executeSingleMessage:test_executeSingleMessage_NoTokens() (gas: 51593)
+OffRamp_constructor:test_Constructor() (gas: 6211675)
+OffRamp_execute:test_LargeBatch() (gas: 3355498)
+OffRamp_execute:test_MultipleReports() (gas: 290693)
+OffRamp_execute:test_MultipleReportsWithPartialValidationFailures() (gas: 364177)
+OffRamp_execute:test_SingleReport() (gas: 168621)
+OffRamp_executeSingleMessage:test_executeSingleMessage_NoTokens() (gas: 51629)
 OffRamp_executeSingleMessage:test_executeSingleMessage_NonContract() (gas: 20508)
-OffRamp_executeSingleMessage:test_executeSingleMessage_NonContractWithTokens() (gas: 230350)
-OffRamp_executeSingleMessage:test_executeSingleMessage_WithMessageInterceptor() (gas: 87432)
-OffRamp_executeSingleMessage:test_executeSingleMessage_WithTokens() (gas: 259856)
-OffRamp_executeSingleReport:test_InvalidSourcePoolAddress() (gas: 454636)
-OffRamp_executeSingleReport:test_ReceiverError() (gas: 180665)
-OffRamp_executeSingleReport:test_SingleMessageNoTokens() (gas: 204824)
-OffRamp_executeSingleReport:test_SingleMessageNoTokensOtherChain() (gas: 241135)
-OffRamp_executeSingleReport:test_SingleMessageNoTokensUnordered() (gas: 184833)
-OffRamp_executeSingleReport:test_SingleMessageToNonCCIPReceiver() (gas: 243821)
-OffRamp_executeSingleReport:test_SingleMessagesNoTokensSuccess_gas() (gas: 134435)
+OffRamp_executeSingleMessage:test_executeSingleMessage_NonContractWithTokens() (gas: 230422)
+OffRamp_executeSingleMessage:test_executeSingleMessage_WithMessageInterceptor() (gas: 87468)
+OffRamp_executeSingleMessage:test_executeSingleMessage_WithTokens() (gas: 259964)
+OffRamp_executeSingleReport:test_InvalidSourcePoolAddress() (gas: 454780)
+OffRamp_executeSingleReport:test_ReceiverError() (gas: 180701)
+OffRamp_executeSingleReport:test_SingleMessageNoTokens() (gas: 204896)
+OffRamp_executeSingleReport:test_SingleMessageNoTokensOtherChain() (gas: 241207)
+OffRamp_executeSingleReport:test_SingleMessageNoTokensUnordered() (gas: 184905)
+OffRamp_executeSingleReport:test_SingleMessageToNonCCIPReceiver() (gas: 243806)
+OffRamp_executeSingleReport:test_SingleMessagesNoTokensSuccess_gas() (gas: 134471)
 OffRamp_executeSingleReport:test_SkippedIncorrectNonce() (gas: 58298)
-OffRamp_executeSingleReport:test_SkippedIncorrectNonceStillExecutes() (gas: 391797)
-OffRamp_executeSingleReport:test_TwoMessagesWithTokensAndGE() (gas: 561276)
-OffRamp_executeSingleReport:test_TwoMessagesWithTokensSuccess_gas() (gas: 509640)
-OffRamp_executeSingleReport:test_Unhealthy() (gas: 527805)
-OffRamp_executeSingleReport:test_WithCurseOnAnotherSourceChain() (gas: 438850)
-OffRamp_executeSingleReport:test__execute_SkippedAlreadyExecutedMessage() (gas: 157823)
-OffRamp_executeSingleReport:test__execute_SkippedAlreadyExecutedMessageUnordered() (gas: 128214)
+OffRamp_executeSingleReport:test_SkippedIncorrectNonceStillExecutes() (gas: 391905)
+OffRamp_executeSingleReport:test_TwoMessagesWithTokensAndGE() (gas: 561492)
+OffRamp_executeSingleReport:test_TwoMessagesWithTokensSuccess_gas() (gas: 509856)
+OffRamp_executeSingleReport:test_Unhealthy() (gas: 528021)
+OffRamp_executeSingleReport:test_WithCurseOnAnotherSourceChain() (gas: 439066)
+OffRamp_executeSingleReport:test__execute_SkippedAlreadyExecutedMessage() (gas: 157859)
+OffRamp_executeSingleReport:test__execute_SkippedAlreadyExecutedMessageUnordered() (gas: 128250)
 OffRamp_getExecutionState:test_FillExecutionState() (gas: 3905742)
 OffRamp_getExecutionState:test_GetDifferentChainExecutionState() (gas: 121049)
 OffRamp_getExecutionState:test_GetExecutionState() (gas: 89738)
-OffRamp_manuallyExecute:test_manuallyExecute() (gas: 211986)
-OffRamp_manuallyExecute:test_manuallyExecute_DoesNotRevertIfUntouched() (gas: 165552)
-OffRamp_manuallyExecute:test_manuallyExecute_LowGasLimit() (gas: 475495)
-OffRamp_manuallyExecute:test_manuallyExecute_ReentrancyFails() (gas: 2214817)
-OffRamp_manuallyExecute:test_manuallyExecute_WithGasOverride() (gas: 212536)
-OffRamp_manuallyExecute:test_manuallyExecute_WithMultiReportGasOverride() (gas: 729871)
-OffRamp_manuallyExecute:test_manuallyExecute_WithPartialMessages() (gas: 335954)
-OffRamp_releaseOrMintSingleToken:test__releaseOrMintSingleToken() (gas: 94597)
-OffRamp_releaseOrMintTokens:test_releaseOrMintTokens() (gas: 161092)
-OffRamp_releaseOrMintTokens:test_releaseOrMintTokens_WithGasOverride() (gas: 162962)
-OffRamp_releaseOrMintTokens:test_releaseOrMintTokens_destDenominatedDecimals() (gas: 174201)
+OffRamp_manuallyExecute:test_manuallyExecute() (gas: 212058)
+OffRamp_manuallyExecute:test_manuallyExecute_DoesNotRevertIfUntouched() (gas: 165588)
+OffRamp_manuallyExecute:test_manuallyExecute_LowGasLimit() (gas: 475567)
+OffRamp_manuallyExecute:test_manuallyExecute_ReentrancyFails() (gas: 2214889)
+OffRamp_manuallyExecute:test_manuallyExecute_WithGasOverride() (gas: 212608)
+OffRamp_manuallyExecute:test_manuallyExecute_WithMultiReportGasOverride() (gas: 730231)
+OffRamp_manuallyExecute:test_manuallyExecute_WithPartialMessages() (gas: 336098)
+OffRamp_releaseOrMintSingleToken:test__releaseOrMintSingleToken() (gas: 94633)
+OffRamp_releaseOrMintTokens:test_releaseOrMintTokens() (gas: 161164)
+OffRamp_releaseOrMintTokens:test_releaseOrMintTokens_WithGasOverride() (gas: 163034)
+OffRamp_releaseOrMintTokens:test_releaseOrMintTokens_destDenominatedDecimals() (gas: 174273)
 OffRamp_setDynamicConfig:test_SetDynamicConfig() (gas: 25397)
 OffRamp_setDynamicConfig:test_SetDynamicConfigWithInterceptor() (gas: 47448)
-OffRamp_trialExecute:test_trialExecute() (gas: 263492)
-OffRamp_trialExecute:test_trialExecute_RateLimitError() (gas: 120655)
-OffRamp_trialExecute:test_trialExecute_TokenHandlingErrorIsCaught() (gas: 131965)
-OffRamp_trialExecute:test_trialExecute_TokenPoolIsNotAContract() (gas: 281133)
+OffRamp_trialExecute:test_trialExecute() (gas: 263600)
+OffRamp_trialExecute:test_trialExecute_RateLimitError() (gas: 120691)
+OffRamp_trialExecute:test_trialExecute_TokenHandlingErrorIsCaught() (gas: 132001)
+OffRamp_trialExecute:test_trialExecute_TokenPoolIsNotAContract() (gas: 281241)
 OnRampTokenPoolReentrancy:test_OnRampTokenPoolReentrancy() (gas: 244196)
 OnRamp_applyAllowlistUpdates:test_applyAllowlistUpdates() (gas: 325984)
 OnRamp_applyAllowlistUpdates:test_applyAllowlistUpdates_InvalidAllowListRequestDisabledAllowListWithAdds() (gas: 17227)

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);
+  }
+}