Skip to content

Commit

Permalink
CCIP-4223 bubble up revert for estimation (#15504)
Browse files Browse the repository at this point in the history
* feat: fix estimation by adding a reverting clause

* CCIP-4223 changeset

* [Bot] Update changeset file with jira issues

* Update gethwrappers

* updates wrapper and gas snapshot

* add tests

* fix test vars

* add comments reasoning for address(1)

* reorder import

* fix comments

* updated code after via ir merge

* update sender with C11 address

* update wrappers

* fmt

* move GAS_ESTIMATION_SENDER to Internal.sol + minor refactoring

* make gas estimator const public and use encodeCall

* lint fix

* fix test

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
  • Loading branch information
1 parent 3dcfd1c commit 437ef64
Show file tree
Hide file tree
Showing 9 changed files with 108 additions and 21 deletions.
10 changes: 10 additions & 0 deletions contracts/.changeset/twelve-pianos-chew.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
'@chainlink/contracts': minor
---

#internal Fix gas estimation by adding a reverting clause

PR issue: CCIP-4223


Solidity Review issue: CCIP-3966
31 changes: 17 additions & 14 deletions contracts/gas-snapshots/ccip.gas-snapshot
Original file line number Diff line number Diff line change
Expand Up @@ -226,18 +226,18 @@ 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: 6269512)
OffRamp_constructor:test_Constructor() (gas: 6311247)
OffRamp_execute:test_LargeBatch() (gas: 3373860)
OffRamp_execute:test_MultipleReports() (gas: 291458)
OffRamp_execute:test_MultipleReportsWithPartialValidationFailures() (gas: 364776)
OffRamp_execute:test_MultipleReportsWithPartialValidationFailures() (gas: 364826)
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: 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_InvalidSourcePoolAddress() (gas: 455383)
OffRamp_executeSingleReport:test_ReceiverError() (gas: 180822)
OffRamp_executeSingleReport:test_SingleMessageNoTokens() (gas: 205270)
OffRamp_executeSingleReport:test_SingleMessageNoTokensOtherChain() (gas: 241357)
OffRamp_executeSingleReport:test_SingleMessageNoTokensUnordered() (gas: 185263)
Expand All @@ -254,23 +254,26 @@ OffRamp_executeSingleReport:test__execute_SkippedAlreadyExecutedMessageUnordered
OffRamp_getExecutionState:test_FillExecutionState() (gas: 3955662)
OffRamp_getExecutionState:test_GetDifferentChainExecutionState() (gas: 121311)
OffRamp_getExecutionState:test_GetExecutionState() (gas: 90102)
OffRamp_manuallyExecute:test_manuallyExecute() (gas: 212368)
OffRamp_manuallyExecute:test_manuallyExecute_DoesNotRevertIfUntouched() (gas: 165742)
OffRamp_manuallyExecute:test_manuallyExecute_LowGasLimit() (gas: 479145)
OffRamp_manuallyExecute:test_manuallyExecute() (gas: 212393)
OffRamp_manuallyExecute:test_manuallyExecute_DoesNotRevertIfUntouched() (gas: 165767)
OffRamp_manuallyExecute:test_manuallyExecute_LowGasLimit() (gas: 479170)
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_manuallyExecute:test_manuallyExecute_WithGasOverride() (gas: 212943)
OffRamp_manuallyExecute:test_manuallyExecute_WithMultiReportGasOverride() (gas: 732343)
OffRamp_manuallyExecute:test_manuallyExecute_WithPartialMessages() (gas: 337040)
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: 263635)
OffRamp_trialExecute:test_trialExecute_RateLimitError() (gas: 120721)
OffRamp_trialExecute:test_trialExecute_TokenHandlingErrorIsCaught() (gas: 132031)
OffRamp_trialExecute:test_trialExecute_TokenPoolIsNotAContract() (gas: 281380)
OffRamp_trialExecute:test_trialExecute() (gas: 263614)
OffRamp_trialExecute:test_trialExecute_CallWithExactGasRevertsAndSenderIsNotGasEstimator() (gas: 24490)
OffRamp_trialExecute:test_trialExecute_RateLimitError() (gas: 120710)
OffRamp_trialExecute:test_trialExecute_RevertsWhen_NoEnoughGasForCallSigAndSenderIsGasEstimator() (gas: 29391)
OffRamp_trialExecute:test_trialExecute_RevertsWhen_NoGasForCallExactCheckAndSenderIsGasEstimator() (gas: 29539)
OffRamp_trialExecute:test_trialExecute_TokenHandlingErrorIsCaught() (gas: 131932)
OffRamp_trialExecute:test_trialExecute_TokenPoolIsNotAContract() (gas: 281327)
OnRampTokenPoolReentrancy:test_OnRampTokenPoolReentrancy() (gas: 244294)
OnRamp_applyAllowlistUpdates:test_applyAllowlistUpdates() (gas: 325979)
OnRamp_applyAllowlistUpdates:test_applyAllowlistUpdates_InvalidAllowListRequestDisabledAllowListWithAdds() (gas: 17190)
Expand Down
5 changes: 5 additions & 0 deletions contracts/src/v0.8/ccip/libraries/Internal.sol
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ library Internal {
/// @dev The expected number of bytes returned by the balanceOf function.
uint256 internal constant MAX_BALANCE_OF_RET_BYTES = 32;

/// @dev The address used to send calls for gas estimation.
/// You only need to use this address if the minimum gas limit specified by the user is not actually enough to execute the
/// given message and you're attempting to estimate the actual necessary gas limit
address public constant GAS_ESTIMATION_SENDER = address(0xC11C11C11C11C11C11C11C11C11C11C11C11C1);

/// @notice A collection of token price and gas price updates.
/// @dev RMN depends on this struct, if changing, please notify the RMN maintainers.
struct PriceUpdates {
Expand Down
6 changes: 5 additions & 1 deletion contracts/src/v0.8/ccip/ocr/MultiOCR3Base.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ pragma solidity ^0.8.4;

import {Ownable2StepMsgSender} from "../../shared/access/Ownable2StepMsgSender.sol";
import {ITypeAndVersion} from "../../shared/interfaces/ITypeAndVersion.sol";
import {Internal} from "../libraries/Internal.sol";

/// @notice Onchain verification of reports from the offchain reporting protocol with multiple OCR plugin support.
abstract contract MultiOCR3Base is ITypeAndVersion, Ownable2StepMsgSender {
Expand Down Expand Up @@ -42,6 +43,7 @@ abstract contract MultiOCR3Base is ITypeAndVersion, Ownable2StepMsgSender {
error NonUniqueSignatures();
error OracleCannotBeZeroAddress();
error StaticConfigCannotBeChanged(uint8 ocrPluginType);
error InsufficientGasForCallWithExact();

/// @dev Packing these fields used on the hot path in a ConfigInfo variable reduces the retrieval of all
/// of them to a minimum number of SLOADs.
Expand Down Expand Up @@ -274,7 +276,9 @@ abstract contract MultiOCR3Base is ITypeAndVersion, Ownable2StepMsgSender {
&& msg.sender == s_ocrConfigs[ocrPluginType].transmitters[transmitter.index]
)
) {
revert UnauthorizedTransmitter();
if (msg.sender != Internal.GAS_ESTIMATION_SENDER) {
revert UnauthorizedTransmitter();
}
}
}

Expand Down
8 changes: 8 additions & 0 deletions contracts/src/v0.8/ccip/offRamp/OffRamp.sol
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,14 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
) internal returns (Internal.MessageExecutionState executionState, bytes memory) {
try this.executeSingleMessage(message, offchainTokenData, tokenGasOverrides) {}
catch (bytes memory err) {
if (msg.sender == Internal.GAS_ESTIMATION_SENDER) {
if (
CallWithExactGas.NOT_ENOUGH_GAS_FOR_CALL_SIG == bytes4(err)
|| CallWithExactGas.NO_GAS_FOR_CALL_EXACT_CHECK_SIG == bytes4(err)
) {
revert InsufficientGasForCallWithExact();
}
}
// return the message execution state as FAILURE and the revert data.
// Max length of revert data is Router.MAX_RET_BYTES, max length of err is 4 + Router.MAX_RET_BYTES.
return (Internal.MessageExecutionState.FAILURE, err);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.24;

import {CallWithExactGas} from "../../../../shared/call/CallWithExactGas.sol";
import {Internal} from "../../../libraries/Internal.sol";
import {RateLimiter} from "../../../libraries/RateLimiter.sol";
import {MultiOCR3Base} from "../../../ocr/MultiOCR3Base.sol";
import {OffRamp} from "../../../offRamp/OffRamp.sol";
import {OffRampSetup} from "./OffRampSetup.t.sol";

Expand Down Expand Up @@ -117,4 +119,59 @@ contract OffRamp_trialExecute is OffRampSetup {
assertEq(uint256(Internal.MessageExecutionState.FAILURE), uint256(newState));
assertEq(abi.encodeWithSelector(OffRamp.NotACompatiblePool.selector, address(0)), err);
}

function test_trialExecute_CallWithExactGasRevertsAndSenderIsNotGasEstimator() public {
Internal.Any2EVMRampMessage memory message =
_generateAny2EVMMessageNoTokens(SOURCE_CHAIN_SELECTOR_1, ON_RAMP_ADDRESS_1, 1);

bytes[] memory offchainTokenData = new bytes[](message.tokenAmounts.length);
uint32[] memory tokenGasOverrides = new uint32[](0);

vm.mockCallRevert(
address(s_offRamp),
abi.encodeCall(s_offRamp.executeSingleMessage, (message, offchainTokenData, tokenGasOverrides)),
abi.encodeWithSelector(CallWithExactGas.NOT_ENOUGH_GAS_FOR_CALL_SIG, "")
);

(Internal.MessageExecutionState newState, bytes memory err) =
s_offRamp.trialExecute(message, offchainTokenData, tokenGasOverrides);
assertEq(uint256(Internal.MessageExecutionState.FAILURE), uint256(newState));
assertEq(CallWithExactGas.NotEnoughGasForCall.selector, bytes4(err));
}

function test_trialExecute_RevertsWhen_NoGasForCallExactCheckAndSenderIsGasEstimator() public {
Internal.Any2EVMRampMessage memory message =
_generateAny2EVMMessageNoTokens(SOURCE_CHAIN_SELECTOR_1, ON_RAMP_ADDRESS_1, 1);

bytes[] memory offchainTokenData = new bytes[](message.tokenAmounts.length);
uint32[] memory tokenGasOverrides = new uint32[](0);

vm.mockCallRevert(
address(s_offRamp),
abi.encodeCall(s_offRamp.executeSingleMessage, (message, offchainTokenData, tokenGasOverrides)),
abi.encodeWithSelector(CallWithExactGas.NO_GAS_FOR_CALL_EXACT_CHECK_SIG, "")
);

changePrank(Internal.GAS_ESTIMATION_SENDER);
vm.expectRevert(MultiOCR3Base.InsufficientGasForCallWithExact.selector);
s_offRamp.trialExecute(message, offchainTokenData, tokenGasOverrides);
}

function test_trialExecute_RevertsWhen_NoEnoughGasForCallSigAndSenderIsGasEstimator() public {
Internal.Any2EVMRampMessage memory message =
_generateAny2EVMMessageNoTokens(SOURCE_CHAIN_SELECTOR_1, ON_RAMP_ADDRESS_1, 1);

bytes[] memory offchainTokenData = new bytes[](message.tokenAmounts.length);
uint32[] memory tokenGasOverrides = new uint32[](0);

vm.mockCallRevert(
address(s_offRamp),
abi.encodeCall(s_offRamp.executeSingleMessage, (message, offchainTokenData, tokenGasOverrides)),
abi.encodeWithSelector(CallWithExactGas.NOT_ENOUGH_GAS_FOR_CALL_SIG, "")
);

changePrank(Internal.GAS_ESTIMATION_SENDER);
vm.expectRevert(MultiOCR3Base.InsufficientGasForCallWithExact.selector);
s_offRamp.trialExecute(message, offchainTokenData, tokenGasOverrides);
}
}

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions core/gethwrappers/ccip/generated/offramp/offramp.go

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ message_hasher: ../../../contracts/solc/ccip/MessageHasher/MessageHasher.sol/Mes
mock_usdc_token_messenger: ../../../contracts/solc/ccip/MockE2EUSDCTokenMessenger/MockE2EUSDCTokenMessenger.sol/MockE2EUSDCTokenMessenger.abi.json ../../../contracts/solc/ccip/MockE2EUSDCTokenMessenger/MockE2EUSDCTokenMessenger.sol/MockE2EUSDCTokenMessenger.bin ad7902d63667e582b93b2fad139aa53111f9fddcedf92b1d6d122d1ab7ec4bab
mock_usdc_token_transmitter: ../../../contracts/solc/ccip/MockE2EUSDCTransmitter/MockE2EUSDCTransmitter.sol/MockE2EUSDCTransmitter.abi.json ../../../contracts/solc/ccip/MockE2EUSDCTransmitter/MockE2EUSDCTransmitter.sol/MockE2EUSDCTransmitter.bin ae0d090105bc248f4eccd337836ec1db760c506d6f5578e662305abbbc520fcd
multi_aggregate_rate_limiter: ../../../contracts/solc/ccip/MultiAggregateRateLimiter/MultiAggregateRateLimiter.sol/MultiAggregateRateLimiter.abi.json ../../../contracts/solc/ccip/MultiAggregateRateLimiter/MultiAggregateRateLimiter.sol/MultiAggregateRateLimiter.bin d462b10c87ad74b73502c3c97a7fc53771b915adb9a0fbee781e744f3827d179
multi_ocr3_helper: ../../../contracts/solc/ccip/MultiOCR3Helper/MultiOCR3Helper.sol/MultiOCR3Helper.abi.json ../../../contracts/solc/ccip/MultiOCR3Helper/MultiOCR3Helper.sol/MultiOCR3Helper.bin 6fa79484ac09342282df342055494853521ea5332adaee0b709c6e21bcd17869
multi_ocr3_helper: ../../../contracts/solc/ccip/MultiOCR3Helper/MultiOCR3Helper.sol/MultiOCR3Helper.abi.json ../../../contracts/solc/ccip/MultiOCR3Helper/MultiOCR3Helper.sol/MultiOCR3Helper.bin 71514db63a2ac5e3bebe0e6b393061ee1b414c9405084f4bbd890cfc76c21b0f
nonce_manager: ../../../contracts/solc/ccip/NonceManager/NonceManager.sol/NonceManager.abi.json ../../../contracts/solc/ccip/NonceManager/NonceManager.sol/NonceManager.bin ac76c64749ce07dd2ec1b9346d3401dcc5538253e516aecc4767c4308817ea59
offramp: ../../../contracts/solc/ccip/OffRamp/OffRamp.sol/OffRamp.abi.json ../../../contracts/solc/ccip/OffRamp/OffRamp.sol/OffRamp.bin 0b6526e1dfc331b45fe742560622a6538d9f134b0aeb560e84cccc7802425be1
offramp: ../../../contracts/solc/ccip/OffRamp/OffRamp.sol/OffRamp.abi.json ../../../contracts/solc/ccip/OffRamp/OffRamp.sol/OffRamp.bin 61002b1524baea33d1d6a71d007511ccebc4cf8c3105385774cc27e9c00f046e
onramp: ../../../contracts/solc/ccip/OnRamp/OnRamp.sol/OnRamp.abi.json ../../../contracts/solc/ccip/OnRamp/OnRamp.sol/OnRamp.bin a829e4efe4d8f600dc20589505701fbce872b09bc763b1a5abded28ef4a3afc9
ping_pong_demo: ../../../contracts/solc/ccip/PingPongDemo/PingPongDemo.sol/PingPongDemo.abi.json ../../../contracts/solc/ccip/PingPongDemo/PingPongDemo.sol/PingPongDemo.bin c87b6e1a8961a9dd2fab1eced0df12d0c1ef47bb1b2511f372b7e33443a20683
registry_module_owner_custom: ../../../contracts/solc/ccip/RegistryModuleOwnerCustom/RegistryModuleOwnerCustom.sol/RegistryModuleOwnerCustom.abi.json ../../../contracts/solc/ccip/RegistryModuleOwnerCustom/RegistryModuleOwnerCustom.sol/RegistryModuleOwnerCustom.bin ce04722cdea2e96d791e48c6a99f64559125d34cd24e19cfd5281892d2ed8ef0
Expand Down

0 comments on commit 437ef64

Please sign in to comment.