Skip to content

Commit

Permalink
Fix/onramp allowlist race condition (#1480)
Browse files Browse the repository at this point in the history
  • Loading branch information
0xsuryansh authored Oct 8, 2024
1 parent c4907f3 commit f772bcf
Show file tree
Hide file tree
Showing 6 changed files with 85 additions and 32 deletions.
41 changes: 21 additions & 20 deletions contracts/gas-snapshots/ccip.gas-snapshot
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
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
}

/// @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
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
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

0 comments on commit f772bcf

Please sign in to comment.