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

Fix/onramp allowlist race condition #1480

Merged
merged 13 commits into from
Oct 8, 2024
Merged

Fix/onramp allowlist race condition #1480

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
1a40763
fix: add allowListEnabled to DestChainConfigArgs for avoiding race co…
0xsuryansh Oct 2, 2024
a16401b
formatting
0xsuryansh Oct 2, 2024
b62ed8a
Merge a16401b0e86d0588886d395f8291c67966a51e2f into 7d4f8e56e27faba20…
0xsuryansh Oct 2, 2024
a39272f
Update gethwrappers
app-token-issuer-infra-releng[bot] Oct 2, 2024
46d641b
Merge branch 'ccip-develop' into fix/onramp-allowlist-race-condition
0xsuryansh Oct 3, 2024
d7e8c4b
Merge branch 'ccip-develop' into fix/onramp-allowlist-race-condition
0xsuryansh Oct 4, 2024
0b96b55
chore: gen wrappers
0xsuryansh Oct 4, 2024
a0cc771
add struct packing comment
0xsuryansh Oct 4, 2024
549b19d
add test
0xsuryansh Oct 4, 2024
52162b4
comments fix + making the test simple
0xsuryansh Oct 7, 2024
4ddff04
Merge branch 'ccip-develop' into fix/onramp-allowlist-race-condition
0xsuryansh Oct 7, 2024
6b592cb
chore: wrappers and snapshot
0xsuryansh Oct 7, 2024
1f26f49
Merge branch 'ccip-develop' into fix/onramp-allowlist-race-condition
0xsuryansh Oct 8, 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
41 changes: 21 additions & 20 deletions contracts/gas-snapshots/ccip.gas-snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,8 +564,8 @@ MultiOCR3Base_transmit:test_UnAuthorizedTransmitter_Revert() (gas: 24234)
MultiOCR3Base_transmit:test_UnauthorizedSigner_Revert() (gas: 61275)
MultiOCR3Base_transmit:test_UnconfiguredPlugin_Revert() (gas: 39933)
MultiOCR3Base_transmit:test_ZeroSignatures_Revert() (gas: 33049)
MultiOnRampTokenPoolReentrancy:test_OnRampTokenPoolReentrancy_Success() (gas: 233701)
MultiRampsE2E:test_E2E_3MessagesMMultiOffRampSuccess_gas() (gas: 1501791)
MultiOnRampTokenPoolReentrancy:test_OnRampTokenPoolReentrancy_Success() (gas: 233635)
MultiRampsE2E:test_E2E_3MessagesMMultiOffRampSuccess_gas() (gas: 1501725)
NonceManager_NonceIncrementation:test_getIncrementedOutboundNonce_Success() (gas: 37934)
NonceManager_NonceIncrementation:test_incrementInboundNonce_Skip() (gas: 23706)
NonceManager_NonceIncrementation:test_incrementInboundNonce_Success() (gas: 38778)
Expand Down Expand Up @@ -748,23 +748,24 @@ OffRamp_trialExecute:test_TokenHandlingErrorIsCaught_Success() (gas: 227999)
OffRamp_trialExecute:test_TokenPoolIsNotAContract_Success() (gas: 295396)
OffRamp_trialExecute:test_trialExecute_Success() (gas: 277896)
OnRampTokenPoolReentrancy:test_OnRampTokenPoolReentrancy_Success() (gas: 390842)
OnRamp_applyAllowListUpdates:test_applyAllowListUpdates_InvalidAllowListRequestDisabledAllowListWithAdds() (gas: 18030)
OnRamp_applyAllowListUpdates:test_applyAllowListUpdates_Revert() (gas: 67426)
OnRamp_applyAllowListUpdates:test_applyAllowListUpdates_Success() (gas: 325083)
OnRamp_applyDestChainConfigUpdates:test_ApplyDestChainConfigUpdates_Success() (gas: 65095)
OnRamp_applyDestChainConfigUpdates:test_ApplyDestChainConfigUpdates_WithInvalidChainSelector_Revert() (gas: 13422)
OnRamp_constructor:test_Constructor_InvalidConfigChainSelectorEqZero_Revert() (gas: 94996)
OnRamp_constructor:test_Constructor_InvalidConfigNonceManagerEqAddressZero_Revert() (gas: 92938)
OnRamp_constructor:test_Constructor_InvalidConfigRMNProxyEqAddressZero_Revert() (gas: 97971)
OnRamp_constructor:test_Constructor_InvalidConfigTokenAdminRegistryEqAddressZero_Revert() (gas: 92972)
OnRamp_constructor:test_Constructor_Success() (gas: 2736399)
OnRamp_applyAllowListUpdates:test_applyAllowListUpdates_InvalidAllowListRequestDisabledAllowListWithAdds() (gas: 18018)
OnRamp_applyAllowListUpdates:test_applyAllowListUpdates_Revert() (gas: 67797)
OnRamp_applyAllowListUpdates:test_applyAllowListUpdates_Success() (gas: 325198)
OnRamp_applyDestChainConfigUpdates:test_ApplyDestChainConfigUpdates_Success() (gas: 65878)
OnRamp_applyDestChainConfigUpdates:test_ApplyDestChainConfigUpdates_WithInvalidChainSelector_Revert() (gas: 13631)
OnRamp_constructor:test_Constructor_EnableAllowList_ForwardFromRouter_Reverts() (gas: 2673564)
OnRamp_constructor:test_Constructor_InvalidConfigChainSelectorEqZero_Revert() (gas: 95249)
OnRamp_constructor:test_Constructor_InvalidConfigNonceManagerEqAddressZero_Revert() (gas: 93191)
OnRamp_constructor:test_Constructor_InvalidConfigRMNProxyEqAddressZero_Revert() (gas: 98224)
OnRamp_constructor:test_Constructor_InvalidConfigTokenAdminRegistryEqAddressZero_Revert() (gas: 93247)
OnRamp_constructor:test_Constructor_Success() (gas: 2753286)
OnRamp_forwardFromRouter:test_ForwardFromRouterExtraArgsV2AllowOutOfOrderTrue_Success() (gas: 115307)
OnRamp_forwardFromRouter:test_ForwardFromRouterExtraArgsV2_Success() (gas: 146108)
OnRamp_forwardFromRouter:test_ForwardFromRouterSuccessCustomExtraArgs() (gas: 145705)
OnRamp_forwardFromRouter:test_ForwardFromRouterSuccessEmptyExtraArgs() (gas: 143866)
OnRamp_forwardFromRouter:test_ForwardFromRouterSuccessLegacyExtraArgs() (gas: 145902)
OnRamp_forwardFromRouter:test_ForwardFromRouter_Success() (gas: 145300)
OnRamp_forwardFromRouter:test_ForwardFromRouter_Success_ConfigurableSourceRouter() (gas: 145006)
OnRamp_forwardFromRouter:test_ForwardFromRouter_Success_ConfigurableSourceRouter() (gas: 140701)
OnRamp_forwardFromRouter:test_InvalidExtraArgsTag_Revert() (gas: 38554)
OnRamp_forwardFromRouter:test_MessageInterceptionError_Revert() (gas: 143051)
OnRamp_forwardFromRouter:test_MesssageFeeTooHigh_Revert() (gas: 36596)
Expand All @@ -781,20 +782,20 @@ OnRamp_forwardFromRouter:test_UnAllowedOriginalSender_Revert() (gas: 24010)
OnRamp_forwardFromRouter:test_UnsupportedToken_Revert() (gas: 75866)
OnRamp_forwardFromRouter:test_forwardFromRouter_UnsupportedToken_Revert() (gas: 38599)
OnRamp_forwardFromRouter:test_forwardFromRouter_WithInterception_Success() (gas: 280170)
OnRamp_getFee:test_EmptyMessage_Success() (gas: 98513)
OnRamp_getFee:test_EnforceOutOfOrder_Revert() (gas: 64645)
OnRamp_getFee:test_GetFeeOfZeroForTokenMessage_Success() (gas: 86177)
OnRamp_getFee:test_NotAFeeTokenButPricedToken_Revert() (gas: 35097)
OnRamp_getFee:test_SingleTokenMessage_Success() (gas: 113639)
OnRamp_getFee:test_Unhealthy_Revert() (gas: 17061)
OnRamp_getFee:test_EmptyMessage_Success() (gas: 98469)
OnRamp_getFee:test_EnforceOutOfOrder_Revert() (gas: 64623)
OnRamp_getFee:test_GetFeeOfZeroForTokenMessage_Success() (gas: 86133)
OnRamp_getFee:test_NotAFeeTokenButPricedToken_Revert() (gas: 35075)
OnRamp_getFee:test_SingleTokenMessage_Success() (gas: 113595)
OnRamp_getFee:test_Unhealthy_Revert() (gas: 17039)
OnRamp_getSupportedTokens:test_GetSupportedTokens_Revert() (gas: 10474)
OnRamp_getTokenPool:test_GetTokenPool_Success() (gas: 35348)
OnRamp_setDynamicConfig:test_setDynamicConfig_InvalidConfigFeeAggregatorEqAddressZero_Revert() (gas: 11536)
OnRamp_setDynamicConfig:test_setDynamicConfig_InvalidConfigFeeQuoterEqAddressZero_Revert() (gas: 13195)
OnRamp_setDynamicConfig:test_setDynamicConfig_InvalidConfigInvalidConfig_Revert() (gas: 11522)
OnRamp_setDynamicConfig:test_setDynamicConfig_InvalidConfigOnlyOwner_Revert() (gas: 16850)
OnRamp_setDynamicConfig:test_setDynamicConfig_InvalidConfigReentrancyGuardEnteredEqTrue_Revert() (gas: 13265)
OnRamp_setDynamicConfig:test_setDynamicConfig_Success() (gas: 56369)
OnRamp_setDynamicConfig:test_setDynamicConfig_Success() (gas: 56347)
OnRamp_withdrawFeeTokens:test_WithdrawFeeTokens_Success() (gas: 97302)
PingPong_ccipReceive:test_CcipReceive_Success() (gas: 151349)
PingPong_plumbing:test_OutOfOrderExecution_Success() (gas: 20310)
Expand Down
6 changes: 4 additions & 2 deletions contracts/src/v0.8/ccip/onRamp/OnRamp.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,9 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
/// can be passed in the constructor and the applyDestChainConfigUpdates function
//solhint-disable gas-struct-packing
struct DestChainConfigArgs {
uint64 destChainSelector; // Destination chain selector
IRouter router; // Source router address
uint64 destChainSelector; // ─╮ Destination chain selector
IRouter router; // │ Source router address
bool allowListEnabled; //─────╯ Boolean indicator to specify if allowList check is enabled
Copy link
Collaborator

Choose a reason for hiding this comment

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

q: since this is now part of dest chain config args - do we still need it in allowListConfigArgs? There are now 2 ways to change the allowlist - is this intended?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This just enables it when a new chain it set, to prevent unauthorised access to a private chain. The other method is still required to set the actual permitted addresses

Copy link
Collaborator

@elatoskinas elatoskinas Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For the first option it makes sense, for the second option - we can always batch 2 MCMS calls:

  1. setDestChainConfigArgs
  2. setAllowlistConfigArgs

Seems slightly redundant to have the flag twice

}

/// @dev Struct used to apply AllowList Senders for multiple destChainSelectors
Expand Down Expand Up @@ -377,6 +378,7 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {

DestChainConfig storage destChainConfig = s_destChainConfigs[destChainSelector];
destChainConfig.router = destChainConfigArg.router;
destChainConfig.allowListEnabled = destChainConfigArg.allowListEnabled;

emit DestChainConfigSet(
destChainSelector, destChainConfig.sequenceNumber, destChainConfigArg.router, destChainConfig.allowListEnabled
Expand Down
60 changes: 54 additions & 6 deletions contracts/src/v0.8/ccip/test/onRamp/OnRamp.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,39 @@ contract OnRamp_constructor is OnRampSetup {
assertEq(address(s_sourceRouter), address(s_onRamp.getRouter(DEST_CHAIN_SELECTOR)));
}

function test_Constructor_EnableAllowList_ForwardFromRouter_Reverts() public {
OnRamp.StaticConfig memory staticConfig = OnRamp.StaticConfig({
chainSelector: SOURCE_CHAIN_SELECTOR,
rmnRemote: s_mockRMNRemote,
nonceManager: address(s_outboundNonceManager),
tokenAdminRegistry: address(s_tokenAdminRegistry)
});

OnRamp.DynamicConfig memory dynamicConfig = _generateDynamicOnRampConfig(address(s_feeQuoter));

// Creating a DestChainConfig and setting allowListEnabled : true
OnRamp.DestChainConfigArgs[] memory destChainConfigs = new OnRamp.DestChainConfigArgs[](1);
destChainConfigs[0] = OnRamp.DestChainConfigArgs({
destChainSelector: DEST_CHAIN_SELECTOR,
router: s_sourceRouter,
allowListEnabled: true
});

vm.expectEmit();
emit OnRamp.ConfigSet(staticConfig, dynamicConfig);

vm.expectEmit();
emit OnRamp.DestChainConfigSet(DEST_CHAIN_SELECTOR, 0, s_sourceRouter, true);

OnRampHelper tempOnRamp = new OnRampHelper(staticConfig, dynamicConfig, destChainConfigs);

// Sending a message and expecting revert as allowList is enabled with no address in allowlist
Client.EVM2AnyMessage memory message = _generateEmptyMessage();
vm.startPrank(address(s_sourceRouter));
vm.expectRevert(abi.encodeWithSelector(OnRamp.SenderNotAllowed.selector, OWNER));
tempOnRamp.forwardFromRouter(DEST_CHAIN_SELECTOR, message, 0, OWNER);
}

function test_Constructor_InvalidConfigChainSelectorEqZero_Revert() public {
vm.expectRevert(OnRamp.InvalidConfig.selector);
new OnRampHelper(
Expand Down Expand Up @@ -844,8 +877,13 @@ contract OnRamp_applyDestChainConfigUpdates is OnRampSetup {

// supports updating and adding lanes simultaneously
configArgs = new OnRamp.DestChainConfigArgs[](2);
configArgs[0] = OnRamp.DestChainConfigArgs({destChainSelector: DEST_CHAIN_SELECTOR, router: s_sourceRouter});
configArgs[1] = OnRamp.DestChainConfigArgs({destChainSelector: 9999, router: IRouter(address(9999))});
configArgs[0] = OnRamp.DestChainConfigArgs({
destChainSelector: DEST_CHAIN_SELECTOR,
router: s_sourceRouter,
allowListEnabled: false
});
configArgs[1] =
OnRamp.DestChainConfigArgs({destChainSelector: 9999, router: IRouter(address(9999)), allowListEnabled: false});
vm.expectEmit();
emit OnRamp.DestChainConfigSet(DEST_CHAIN_SELECTOR, 0, s_sourceRouter, false);
vm.expectEmit();
Expand Down Expand Up @@ -877,8 +915,13 @@ contract OnRamp_applyAllowListUpdates is OnRampSetup {
vm.startPrank(OWNER);

OnRamp.DestChainConfigArgs[] memory configArgs = new OnRamp.DestChainConfigArgs[](2);
configArgs[0] = OnRamp.DestChainConfigArgs({destChainSelector: DEST_CHAIN_SELECTOR, router: s_sourceRouter});
configArgs[1] = OnRamp.DestChainConfigArgs({destChainSelector: 9999, router: IRouter(address(9999))});
configArgs[0] = OnRamp.DestChainConfigArgs({
destChainSelector: DEST_CHAIN_SELECTOR,
router: s_sourceRouter,
allowListEnabled: false
});
configArgs[1] =
OnRamp.DestChainConfigArgs({destChainSelector: 9999, router: IRouter(address(9999)), allowListEnabled: false});
vm.expectEmit();
emit OnRamp.DestChainConfigSet(DEST_CHAIN_SELECTOR, 0, s_sourceRouter, false);
vm.expectEmit();
Expand Down Expand Up @@ -968,8 +1011,13 @@ contract OnRamp_applyAllowListUpdates is OnRampSetup {
vm.startPrank(OWNER);

OnRamp.DestChainConfigArgs[] memory configArgs = new OnRamp.DestChainConfigArgs[](2);
configArgs[0] = OnRamp.DestChainConfigArgs({destChainSelector: DEST_CHAIN_SELECTOR, router: s_sourceRouter});
configArgs[1] = OnRamp.DestChainConfigArgs({destChainSelector: 9999, router: IRouter(address(9999))});
configArgs[0] = OnRamp.DestChainConfigArgs({
destChainSelector: DEST_CHAIN_SELECTOR,
router: s_sourceRouter,
allowListEnabled: false
});
configArgs[1] =
OnRamp.DestChainConfigArgs({destChainSelector: 9999, router: IRouter(address(9999)), allowListEnabled: false});
vm.expectEmit();
emit OnRamp.DestChainConfigSet(DEST_CHAIN_SELECTOR, 0, s_sourceRouter, false);
vm.expectEmit();
Expand Down
3 changes: 2 additions & 1 deletion contracts/src/v0.8/ccip/test/onRamp/OnRampSetup.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,8 @@ contract OnRampSetup is FeeQuoterFeeSetup {

function _generateDestChainConfigArgs(IRouter router) internal pure returns (OnRamp.DestChainConfigArgs[] memory) {
OnRamp.DestChainConfigArgs[] memory destChainConfigs = new OnRamp.DestChainConfigArgs[](1);
destChainConfigs[0] = OnRamp.DestChainConfigArgs({destChainSelector: DEST_CHAIN_SELECTOR, router: router});
destChainConfigs[0] =
OnRamp.DestChainConfigArgs({destChainSelector: DEST_CHAIN_SELECTOR, router: router, allowListEnabled: false});
return destChainConfigs;
}

Expand Down
Loading
Loading