Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CCIP-4223 bubble up revert for estimation #15504

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

CCIP-4223 bubble up revert for estimation #15504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
29f1697
feat: fix estimation by adding a reverting clause
0xsuryansh Dec 4, 2024
68e00c0
CCIP-4223 changeset
0xsuryansh Dec 4, 2024
175d47b
[Bot] Update changeset file with jira issues
app-token-issuer-infra-releng[bot] Dec 4, 2024
e9e6277
Merge 175d47b143cca8edf2be7f60abe1cd6c463ca9fe into 52676f13cd629f575…
0xsuryansh Dec 4, 2024
ac70f46
Update gethwrappers
app-token-issuer-infra-releng[bot] Dec 4, 2024
a76641b
Merge branch 'develop' into CCIP-4223_fix-gas-estimation
0xsuryansh Dec 8, 2024
3db41bf
updates wrapper and gas snapshot
0xsuryansh Dec 8, 2024
d1f8ec9
add tests
0xsuryansh Dec 8, 2024
5686b69
fix test vars
0xsuryansh Dec 8, 2024
94abe32
add comments reasoning for address(1)
0xsuryansh Dec 9, 2024
ab7ffa1
reorder import
0xsuryansh Dec 9, 2024
6c884cb
fix comments
0xsuryansh Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions contracts/.changeset/twelve-pianos-chew.md
View file
Open in desktop
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
71 changes: 37 additions & 34 deletions contracts/gas-snapshots/ccip.gas-snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,9 +343,9 @@ MultiOCR3Base_transmit:test_TransmitSigners_gas_Success() (gas: 33589)
MultiOCR3Base_transmit:test_TransmitWithExtraCalldataArgs_Revert() (gas: 47082)
MultiOCR3Base_transmit:test_TransmitWithLessCalldataArgs_Revert() (gas: 25583)
MultiOCR3Base_transmit:test_TransmitWithoutSignatureVerification_gas_Success() (gas: 18615)
MultiOCR3Base_transmit:test_UnAuthorizedTransmitter_Revert() (gas: 24193)
MultiOCR3Base_transmit:test_UnAuthorizedTransmitter_Revert() (gas: 24232)
MultiOCR3Base_transmit:test_UnauthorizedSigner_Revert() (gas: 60994)
MultiOCR3Base_transmit:test_UnconfiguredPlugin_Revert() (gas: 39824)
MultiOCR3Base_transmit:test_UnconfiguredPlugin_Revert() (gas: 39863)
MultiOCR3Base_transmit:test_ZeroSignatures_Revert() (gas: 32920)
NonceManager_NonceIncrementation:test_getIncrementedOutboundNonce_Success() (gas: 37956)
NonceManager_NonceIncrementation:test_incrementInboundNonce_Skip() (gas: 23706)
Expand All @@ -369,7 +369,7 @@ NonceManager_applyPreviousRampsUpdates:test_PreviousRampAlreadySet_overrideAllow
NonceManager_applyPreviousRampsUpdates:test_SingleRampUpdate_success() (gas: 66889)
NonceManager_applyPreviousRampsUpdates:test_ZeroInput_success() (gas: 12213)
NonceManager_typeAndVersion:test_typeAndVersion() (gas: 9705)
OffRamp_afterOC3ConfigSet:test_afterOCR3ConfigSet_SignatureVerificationDisabled_Revert() (gas: 5872422)
OffRamp_afterOC3ConfigSet:test_afterOCR3ConfigSet_SignatureVerificationDisabled_Revert() (gas: 5921426)
OffRamp_applySourceChainConfigUpdates:test_AddMultipleChains_Success() (gas: 626094)
OffRamp_applySourceChainConfigUpdates:test_AddNewChain_Success() (gas: 166505)
OffRamp_applySourceChainConfigUpdates:test_ApplyZeroUpdates_Success() (gas: 16719)
Expand All @@ -393,8 +393,8 @@ OffRamp_commit:test_FailedRMNVerification_Reverts() (gas: 63117)
OffRamp_commit:test_InvalidIntervalMinLargerThanMax_Revert() (gas: 69655)
OffRamp_commit:test_InvalidInterval_Revert() (gas: 65803)
OffRamp_commit:test_InvalidRootRevert() (gas: 64898)
OffRamp_commit:test_NoConfigWithOtherConfigPresent_Revert() (gas: 6633259)
OffRamp_commit:test_NoConfig_Revert() (gas: 6216677)
OffRamp_commit:test_NoConfigWithOtherConfigPresent_Revert() (gas: 6682263)
OffRamp_commit:test_NoConfig_Revert() (gas: 6265681)
OffRamp_commit:test_OnlyGasPriceUpdates_Success() (gas: 112728)
OffRamp_commit:test_OnlyPriceUpdateStaleReport_Revert() (gas: 120561)
OffRamp_commit:test_OnlyTokenPriceUpdates_Success() (gas: 112660)
Expand All @@ -405,27 +405,27 @@ OffRamp_commit:test_RootAlreadyCommitted_Revert() (gas: 147631)
OffRamp_commit:test_RootWithRMNDisabled_success() (gas: 153596)
OffRamp_commit:test_SourceChainNotEnabled_Revert() (gas: 61365)
OffRamp_commit:test_StaleReportWithRoot_Success() (gas: 231709)
OffRamp_commit:test_UnauthorizedTransmitter_Revert() (gas: 125027)
OffRamp_commit:test_UnauthorizedTransmitter_Revert() (gas: 125066)
OffRamp_commit:test_Unhealthy_Revert() (gas: 60177)
OffRamp_commit:test_ValidPriceUpdateThenStaleReportWithRoot_Success() (gas: 206221)
OffRamp_commit:test_ZeroEpochAndRound_Revert() (gas: 53305)
OffRamp_constructor:test_Constructor_Success() (gas: 6179080)
OffRamp_constructor:test_SourceChainSelector_Revert() (gas: 136555)
OffRamp_constructor:test_ZeroChainSelector_Revert() (gas: 103592)
OffRamp_constructor:test_ZeroNonceManager_Revert() (gas: 101441)
OffRamp_constructor:test_ZeroOnRampAddress_Revert() (gas: 162036)
OffRamp_constructor:test_ZeroRMNRemote_Revert() (gas: 101358)
OffRamp_constructor:test_ZeroTokenAdminRegistry_Revert() (gas: 101362)
OffRamp_constructor:test_Constructor_Success() (gas: 6228086)
OffRamp_constructor:test_SourceChainSelector_Revert() (gas: 136656)
OffRamp_constructor:test_ZeroChainSelector_Revert() (gas: 103692)
OffRamp_constructor:test_ZeroNonceManager_Revert() (gas: 101541)
OffRamp_constructor:test_ZeroOnRampAddress_Revert() (gas: 162136)
OffRamp_constructor:test_ZeroRMNRemote_Revert() (gas: 101458)
OffRamp_constructor:test_ZeroTokenAdminRegistry_Revert() (gas: 101462)
OffRamp_execute:test_IncorrectArrayType_Revert() (gas: 17532)
OffRamp_execute:test_LargeBatch_Success() (gas: 3378447)
OffRamp_execute:test_MultipleReportsWithPartialValidationFailures_Success() (gas: 371209)
OffRamp_execute:test_MultipleReportsWithPartialValidationFailures_Success() (gas: 371289)
OffRamp_execute:test_MultipleReports_Success() (gas: 298806)
OffRamp_execute:test_NoConfigWithOtherConfigPresent_Revert() (gas: 7041684)
OffRamp_execute:test_NoConfig_Revert() (gas: 6266154)
OffRamp_execute:test_NoConfigWithOtherConfigPresent_Revert() (gas: 7090727)
OffRamp_execute:test_NoConfig_Revert() (gas: 6315197)
OffRamp_execute:test_NonArray_Revert() (gas: 27572)
OffRamp_execute:test_SingleReport_Success() (gas: 175631)
OffRamp_execute:test_UnauthorizedTransmitter_Revert() (gas: 147790)
OffRamp_execute:test_WrongConfigWithSigners_Revert() (gas: 6933352)
OffRamp_execute:test_UnauthorizedTransmitter_Revert() (gas: 147829)
OffRamp_execute:test_WrongConfigWithSigners_Revert() (gas: 6982356)
OffRamp_execute:test_ZeroReports_Revert() (gas: 17248)
OffRamp_executeSingleMessage:test_executeSingleMessage_NoTokens() (gas: 56213)
OffRamp_executeSingleMessage:test_executeSingleMessage_NonContract() (gas: 20508)
Expand All @@ -434,14 +434,14 @@ OffRamp_executeSingleMessage:test_executeSingleMessage_WithMessageInterceptor()
OffRamp_executeSingleMessage:test_executeSingleMessage_WithTokens() (gas: 268135)
OffRamp_executeSingleReport:test_DisabledSourceChain_Revert() (gas: 28659)
OffRamp_executeSingleReport:test_EmptyReport_Revert() (gas: 15530)
OffRamp_executeSingleReport:test_InvalidSourcePoolAddress() (gas: 474650)
OffRamp_executeSingleReport:test_InvalidSourcePoolAddress() (gas: 474690)
OffRamp_executeSingleReport:test_ManualExecutionNotYetEnabled_Revert() (gas: 48296)
OffRamp_executeSingleReport:test_MismatchingDestChainSelector_Revert() (gas: 34101)
OffRamp_executeSingleReport:test_NonExistingSourceChain_Revert() (gas: 28824)
OffRamp_executeSingleReport:test_ReceiverError_Success() (gas: 187677)
OffRamp_executeSingleReport:test_RetryFailedMessageWithoutManualExecution_Revert() (gas: 197809)
OffRamp_executeSingleReport:test_ReceiverError_Success() (gas: 187717)
OffRamp_executeSingleReport:test_RetryFailedMessageWithoutManualExecution_Revert() (gas: 197849)
OffRamp_executeSingleReport:test_RootNotCommitted_Revert() (gas: 40687)
OffRamp_executeSingleReport:test_RouterYULCall_Revert() (gas: 405023)
OffRamp_executeSingleReport:test_RouterYULCall_Revert() (gas: 405063)
OffRamp_executeSingleReport:test_SingleMessageNoTokensOtherChain_Success() (gas: 248786)
OffRamp_executeSingleReport:test_SingleMessageNoTokensUnordered_Success() (gas: 192430)
OffRamp_executeSingleReport:test_SingleMessageNoTokens_Success() (gas: 212456)
Expand All @@ -463,20 +463,20 @@ OffRamp_getExecutionState:test_GetDifferentChainExecutionState_Success() (gas: 1
OffRamp_getExecutionState:test_GetExecutionState_Success() (gas: 89737)
OffRamp_manuallyExecute:test_ManualExecGasLimitMismatchSingleReport_Revert() (gas: 81694)
OffRamp_manuallyExecute:test_manuallyExecute_DestinationGasAmountCountMismatch_Revert() (gas: 74284)
OffRamp_manuallyExecute:test_manuallyExecute_DoesNotRevertIfUntouched_Success() (gas: 172639)
OffRamp_manuallyExecute:test_manuallyExecute_FailedTx_Revert() (gas: 213251)
OffRamp_manuallyExecute:test_manuallyExecute_DoesNotRevertIfUntouched_Success() (gas: 172679)
OffRamp_manuallyExecute:test_manuallyExecute_FailedTx_Revert() (gas: 213331)
OffRamp_manuallyExecute:test_manuallyExecute_ForkedChain_Revert() (gas: 27248)
OffRamp_manuallyExecute:test_manuallyExecute_GasLimitMismatchMultipleReports_Revert() (gas: 165935)
OffRamp_manuallyExecute:test_manuallyExecute_InvalidReceiverExecutionGasLimit_Revert() (gas: 27774)
OffRamp_manuallyExecute:test_manuallyExecute_InvalidTokenGasOverride_Revert() (gas: 55362)
OffRamp_manuallyExecute:test_manuallyExecute_LowGasLimit_Success() (gas: 489669)
OffRamp_manuallyExecute:test_manuallyExecute_MultipleReportsWithSingleCursedLane_Revert() (gas: 314861)
OffRamp_manuallyExecute:test_manuallyExecute_LowGasLimit_Success() (gas: 489709)
OffRamp_manuallyExecute:test_manuallyExecute_MultipleReportsWithSingleCursedLane_Revert() (gas: 314981)
OffRamp_manuallyExecute:test_manuallyExecute_ReentrancyFails_Success() (gas: 2224794)
OffRamp_manuallyExecute:test_manuallyExecute_SourceChainSelectorMismatch_Revert() (gas: 165330)
OffRamp_manuallyExecute:test_manuallyExecute_Success() (gas: 226161)
OffRamp_manuallyExecute:test_manuallyExecute_WithGasOverride_Success() (gas: 226701)
OffRamp_manuallyExecute:test_manuallyExecute_WithMultiReportGasOverride_Success() (gas: 774719)
OffRamp_manuallyExecute:test_manuallyExecute_WithPartialMessages_Success() (gas: 344726)
OffRamp_manuallyExecute:test_manuallyExecute_Success() (gas: 226201)
OffRamp_manuallyExecute:test_manuallyExecute_WithGasOverride_Success() (gas: 226741)
OffRamp_manuallyExecute:test_manuallyExecute_WithMultiReportGasOverride_Success() (gas: 774919)
OffRamp_manuallyExecute:test_manuallyExecute_WithPartialMessages_Success() (gas: 344766)
OffRamp_releaseOrMintSingleToken:test__releaseOrMintSingleToken_NotACompatiblePool_Revert() (gas: 37632)
OffRamp_releaseOrMintSingleToken:test__releaseOrMintSingleToken_Success() (gas: 101465)
OffRamp_releaseOrMintSingleToken:test_releaseOrMintToken_InvalidDataLength_Revert() (gas: 36790)
Expand All @@ -491,10 +491,13 @@ OffRamp_setDynamicConfig:test_FeeQuoterZeroAddress_Revert() (gas: 11465)
OffRamp_setDynamicConfig:test_NonOwner_Revert() (gas: 13975)
OffRamp_setDynamicConfig:test_SetDynamicConfigWithInterceptor_Success() (gas: 47491)
OffRamp_setDynamicConfig:test_SetDynamicConfig_Success() (gas: 25464)
OffRamp_trialExecute:test_trialExecute() (gas: 271771)
OffRamp_trialExecute:test_trialExecute_RateLimitError() (gas: 127457)
OffRamp_trialExecute:test_trialExecute_TokenHandlingErrorIsCaught() (gas: 138767)
OffRamp_trialExecute:test_trialExecute_TokenPoolIsNotAContract() (gas: 289412)
OffRamp_trialExecute:test_trialExecute() (gas: 271751)
OffRamp_trialExecute:test_trialExecute_CallWithExactGasRevertsAndSenderIsNotGasEstimator() (gas: 56330)
OffRamp_trialExecute:test_trialExecute_RateLimitError() (gas: 127460)
OffRamp_trialExecute:test_trialExecute_RevertsWhen_NoEnoughGasForCallSigAndSenderIsGasEstimator() (gas: 61213)
OffRamp_trialExecute:test_trialExecute_RevertsWhen_NoGasForCallExactCheckAndSenderIsGasEstimator() (gas: 61347)
OffRamp_trialExecute:test_trialExecute_TokenHandlingErrorIsCaught() (gas: 138682)
OffRamp_trialExecute:test_trialExecute_TokenPoolIsNotAContract() (gas: 289388)
OnRampTokenPoolReentrancy:test_OnRampTokenPoolReentrancy_Success() (gas: 251706)
OnRamp_applyAllowlistUpdates:test_applyAllowlistUpdates_InvalidAllowListRequestDisabledAllowListWithAdds() (gas: 17227)
OnRamp_applyAllowlistUpdates:test_applyAllowlistUpdates_Revert() (gas: 67101)
Expand Down
14 changes: 13 additions & 1 deletion contracts/src/v0.8/ccip/ocr/MultiOCR3Base.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,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 @@ -115,8 +116,17 @@ abstract contract MultiOCR3Base is ITypeAndVersion, Ownable2StepMsgSender {

uint256 internal immutable i_chainID;

/// @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 internal immutable i_gasEstimationSender;

constructor() {
i_chainID = block.chainid;

/// We use address(1) because it's the ecrecover precompile and therefore guaranteed to
/// never have any code on any EVM chain.
i_gasEstimationSender = address(1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you include a comment as to why this is being set as address(1) please.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we use a hard coded value to set an immutable value? Doesn't that mean it's simply a constant?

}

/// @notice Sets offchain reporting protocol configuration incl. participating oracles.
Expand Down Expand Up @@ -274,7 +284,9 @@ abstract contract MultiOCR3Base is ITypeAndVersion, Ownable2StepMsgSender {
&& msg.sender == s_ocrConfigs[ocrPluginType].transmitters[transmitter.index]
)
) {
revert UnauthorizedTransmitter();
if (msg.sender != i_gasEstimationSender) {
revert UnauthorizedTransmitter();
}
}
}

Expand Down
8 changes: 8 additions & 0 deletions contracts/src/v0.8/ccip/offRamp/OffRamp.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,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 == i_gasEstimationSender) {
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
67 changes: 67 additions & 0 deletions contracts/src/v0.8/ccip/test/offRamp/OffRamp/OffRamp.trialExecute.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,12 @@ pragma solidity 0.8.24;

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";

import {CallWithExactGas} from "../../../../shared/call/CallWithExactGas.sol";

import {IERC20} from "../../../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/IERC20.sol";

contract OffRamp_trialExecute is OffRampSetup {
Expand Down Expand Up @@ -117,4 +120,68 @@ 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 {
jhweintraub marked this conversation as resolved.
Show resolved Hide resolved
uint256[] memory amounts = new uint256[](2);
amounts[0] = 1000;
amounts[1] = 50;
Internal.Any2EVMRampMessage memory message =
_generateAny2EVMMessageWithTokens(SOURCE_CHAIN_SELECTOR_1, ON_RAMP_ADDRESS_1, 1, amounts);

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

vm.mockCallRevert(
address(s_offRamp),
abi.encodeWithSelector(s_offRamp.executeSingleMessage.selector, 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 {
uint256[] memory amounts = new uint256[](2);
amounts[0] = 1000;
amounts[1] = 50;
Internal.Any2EVMRampMessage memory message =
_generateAny2EVMMessageWithTokens(SOURCE_CHAIN_SELECTOR_1, ON_RAMP_ADDRESS_1, 1, amounts);

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

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

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

function test_trialExecute_RevertsWhen_NoEnoughGasForCallSigAndSenderIsGasEstimator() public {
uint256[] memory amounts = new uint256[](2);
amounts[0] = 1000;
amounts[1] = 50;
Internal.Any2EVMRampMessage memory message =
_generateAny2EVMMessageWithTokens(SOURCE_CHAIN_SELECTOR_1, ON_RAMP_ADDRESS_1, 1, amounts);

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

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

vm.expectRevert(MultiOCR3Base.InsufficientGasForCallWithExact.selector);
changePrank(address(1));
s_offRamp.trialExecute(message, offchainTokenData, tokenGasOverrides);
}
}
Loading
Loading