diff --git a/contracts/gas-snapshots/ccip.gas-snapshot b/contracts/gas-snapshots/ccip.gas-snapshot index b3c9e70cf6..722b3920f8 100644 --- a/contracts/gas-snapshots/ccip.gas-snapshot +++ b/contracts/gas-snapshots/ccip.gas-snapshot @@ -809,32 +809,32 @@ PingPong_plumbing:test_OutOfOrderExecution_Success() (gas: 20310) PingPong_plumbing:test_Pausing_Success() (gas: 17810) PingPong_startPingPong:test_StartPingPong_With_OOO_Success() (gas: 162091) PingPong_startPingPong:test_StartPingPong_With_Sequenced_Ordered_Success() (gas: 181509) -RMNHome_promoteSecondaryAndRevokePrimary:test_promoteSecondaryAndRevokePrimary_OnlyOwner_reverts() (gas: 10885) -RMNHome_promoteSecondaryAndRevokePrimary:test_promoteSecondaryAndRevokePrimary_OnlyOwner_reverts() (gas: 10997) +RMNHome_promoteSecondaryAndRevokePrimary:test_promoteSecondaryAndRevokePrimary_InvalidCaller_reverts() (gas: 10972) +RMNHome_promoteSecondaryAndRevokePrimary:test_promoteSecondaryAndRevokePrimary_OnlyOwner_reverts() (gas: 10967) +RMNHome_promoteSecondaryAndRevokePrimary:test_promoteSecondaryAndRevokePrimary_success() (gas: 187) RMNHome_promoteSecondaryAndRevokePrimary:test_promoteSecondaryAndRevokePrimary_success() (gas: 209) -RMNHome_promoteSecondaryAndRevokePrimary:test_promoteSecondaryAndRevokePrimary_success() (gas: 209) -RMNHome_revokeSecondary:test_revokeSecondary_ConfigDigestMismatch_reverts() (gas: 19344) -RMNHome_revokeSecondary:test_revokeSecondary_ConfigDigestMismatch_reverts() (gas: 19344) -RMNHome_revokeSecondary:test_revokeSecondary_OnlyOwner_reverts() (gas: 10890) -RMNHome_revokeSecondary:test_revokeSecondary_OnlyOwner_reverts() (gas: 10912) -RMNHome_revokeSecondary:test_revokeSecondary_success() (gas: 27088) -RMNHome_revokeSecondary:test_revokeSecondary_success() (gas: 28943) -RMNHome_setDynamicConfig:test_setDynamicConfig_DigestNotFound_reverts() (gas: 33356) -RMNHome_setDynamicConfig:test_setDynamicConfig_MinObserversTooHigh_reverts() (gas: 20467) -RMNHome_setDynamicConfig:test_setDynamicConfig_OnlyOwner_reverts() (gas: 11812) -RMNHome_setDynamicConfig:test_setDynamicConfig_OnlyOwner_reverts() (gas: 15502) -RMNHome_setDynamicConfig:test_setDynamicConfig_success() (gas: 133259) -RMNHome_setDynamicConfig:test_setDynamicConfig_success() (gas: 63157) -RMNHome_setSecondary:test_setSecondary_DuplicateOffchainPublicKey_reverts() (gas: 20872) -RMNHome_setSecondary:test_setSecondary_DuplicatePeerId_reverts() (gas: 20666) -RMNHome_setSecondary:test_setSecondary_DuplicateSourceChain_reverts() (gas: 24457) -RMNHome_setSecondary:test_setSecondary_MinObserversTooHigh_reverts() (gas: 24896) -RMNHome_setSecondary:test_setSecondary_OnlyOwner_reverts() (gas: 12661) -RMNHome_setSecondary:test_setSecondary_OnlyOwner_reverts() (gas: 18048) -RMNHome_setSecondary:test_setSecondary_OutOfBoundsNodesLength_reverts() (gas: 186804) -RMNHome_setSecondary:test_setSecondary_OutOfBoundsObserverNodeIndex_reverts() (gas: 24601) -RMNHome_setSecondary:test_setSecondary_success() (gas: 282455) -RMNHome_setSecondary:test_setSecondary_success() (gas: 820695) +RMNHome_revokeSecondary:test_revokeSecondary_ConfigDigestMismatch_reverts() (gas: 19366) +RMNHome_revokeSecondary:test_revokeSecondary_ConfigDigestMismatch_reverts() (gas: 19408) +RMNHome_revokeSecondary:test_revokeSecondary_InvalidCaller_reverts() (gas: 10866) +RMNHome_revokeSecondary:test_revokeSecondary_OnlyOwner_reverts() (gas: 10997) +RMNHome_revokeSecondary:test_revokeSecondary_success() (gas: 27257) +RMNHome_revokeSecondary:test_revokeSecondary_success() (gas: 29136) +RMNHome_setDynamicConfig:test_setDynamicConfig_DigestNotFound_reverts() (gas: 33400) +RMNHome_setDynamicConfig:test_setDynamicConfig_InvalidCaller_reverts() (gas: 11766) +RMNHome_setDynamicConfig:test_setDynamicConfig_MinObserversTooHigh_reverts() (gas: 20489) +RMNHome_setDynamicConfig:test_setDynamicConfig_OnlyOwner_reverts() (gas: 15584) +RMNHome_setDynamicConfig:test_setDynamicConfig_success() (gas: 133281) +RMNHome_setDynamicConfig:test_setDynamicConfig_success() (gas: 63155) +RMNHome_setSecondary:test_setSecondary_DuplicateOffchainPublicKey_reverts() (gas: 20894) +RMNHome_setSecondary:test_setSecondary_DuplicatePeerId_reverts() (gas: 20688) +RMNHome_setSecondary:test_setSecondary_DuplicateSourceChain_reverts() (gas: 24479) +RMNHome_setSecondary:test_setSecondary_InvalidCaller_reverts() (gas: 12615) +RMNHome_setSecondary:test_setSecondary_MinObserversTooHigh_reverts() (gas: 24918) +RMNHome_setSecondary:test_setSecondary_OnlyOwner_reverts() (gas: 18130) +RMNHome_setSecondary:test_setSecondary_OutOfBoundsNodesLength_reverts() (gas: 186826) +RMNHome_setSecondary:test_setSecondary_OutOfBoundsObserverNodeIndex_reverts() (gas: 24623) +RMNHome_setSecondary:test_setSecondary_success() (gas: 282453) +RMNHome_setSecondary:test_setSecondary_success() (gas: 820717) RMNRemote_constructor:test_constructor_success() (gas: 8334) RMNRemote_constructor:test_constructor_zeroChainSelector_reverts() (gas: 59165) RMNRemote_curse:test_curse_AlreadyCursed_duplicateSubject_reverts() (gas: 154457) diff --git a/contracts/src/v0.8/ccip/capability/CCIPHome.sol b/contracts/src/v0.8/ccip/capability/CCIPHome.sol index 2d8bd28c75..ef02fd169c 100644 --- a/contracts/src/v0.8/ccip/capability/CCIPHome.sol +++ b/contracts/src/v0.8/ccip/capability/CCIPHome.sol @@ -38,6 +38,7 @@ contract CCIPHome is HomeBase, ICapabilityConfiguration, IERC165 { error FTooHigh(); error InvalidNode(OCR3Node node); error NotEnoughTransmitters(uint256 got, uint256 minimum); + error OnlySelfCallAllowed(); /// @notice Represents an oracle node in OCR3 configs part of the role DON. /// Every configured node should be a signer, but does not have to be a transmitter. @@ -132,7 +133,9 @@ contract CCIPHome is HomeBase, ICapabilityConfiguration, IERC165 { } (bool success, bytes memory errorData) = address(this).call(update); if (!success) { - revert(string(errorData)); + assembly { + revert(add(errorData, 32), errorData) + } } } @@ -269,6 +272,12 @@ contract CCIPHome is HomeBase, ICapabilityConfiguration, IERC165 { return PREFIX; } + function _validateCaller() internal view override { + if (msg.sender != address(this)) { + revert OnlySelfCallAllowed(); + } + } + // ================================================================ // │ Chain Configuration │ // ================================================================ diff --git a/contracts/src/v0.8/ccip/capability/HomeBase.sol b/contracts/src/v0.8/ccip/capability/HomeBase.sol index 53787d1582..0f57ed29a2 100644 --- a/contracts/src/v0.8/ccip/capability/HomeBase.sol +++ b/contracts/src/v0.8/ccip/capability/HomeBase.sol @@ -15,7 +15,6 @@ abstract contract HomeBase is OwnerIsCreator, ITypeAndVersion { error DigestNotFound(bytes32 configDigest); error ZeroAddressNotAllowed(); error OnlyCapabilitiesRegistryCanCall(); - error OnlyOwnerOrSelfCallAllowed(); /// @notice Used for encoding the config digest prefix uint256 private constant PREFIX_MASK = type(uint256).max << (256 - 16); // 0xFFFF00..00 @@ -44,6 +43,8 @@ abstract contract HomeBase is OwnerIsCreator, ITypeAndVersion { function _validateDynamicConfig(bytes memory staticConfig, bytes memory dynamicConfig) internal view virtual; + function _validateCaller() internal view virtual; + function _getConfigDigestPrefix() internal pure virtual returns (uint256); // ================================================================ @@ -84,7 +85,7 @@ abstract contract HomeBase is OwnerIsCreator, ITypeAndVersion { return (s_configs[pluginKey][i], true); } } - return (storedConfig, false); + return (StoredConfig(ZERO_DIGEST, 0, "", ""), false); } function _getPrimaryStoredConfig( @@ -119,7 +120,8 @@ abstract contract HomeBase is OwnerIsCreator, ITypeAndVersion { bytes calldata encodedStaticConfig, bytes calldata encodedDynamicConfig, bytes32 digestToOverwrite - ) external OnlyOwnerOrSelfCall returns (bytes32 newConfigDigest) { + ) external returns (bytes32 newConfigDigest) { + _validateCaller(); _validateStaticAndDynamicConfig(encodedStaticConfig, encodedDynamicConfig); bytes32 existingDigest = getSecondaryDigest(pluginKey); @@ -152,7 +154,9 @@ abstract contract HomeBase is OwnerIsCreator, ITypeAndVersion { /// @notice Revokes a specific config by digest. /// @param configDigest The digest of the config to revoke. This is done to prevent accidental revokes. - function revokeSecondary(bytes32 pluginKey, bytes32 configDigest) external OnlyOwnerOrSelfCall { + function revokeSecondary(bytes32 pluginKey, bytes32 configDigest) external { + _validateCaller(); + uint256 secondaryConfigIndex = s_primaryConfigIndex ^ 1; if (s_configs[pluginKey][secondaryConfigIndex].configDigest != configDigest) { revert ConfigDigestMismatch(s_configs[pluginKey][secondaryConfigIndex].configDigest, configDigest); @@ -170,7 +174,9 @@ abstract contract HomeBase is OwnerIsCreator, ITypeAndVersion { bytes32 pluginKey, bytes32 digestToPromote, bytes32 digestToRevoke - ) external OnlyOwnerOrSelfCall { + ) external { + _validateCaller(); + uint256 secondaryConfigIndex = s_primaryConfigIndex ^ 1; if (s_configs[pluginKey][secondaryConfigIndex].configDigest != digestToPromote) { revert ConfigDigestMismatch(s_configs[pluginKey][secondaryConfigIndex].configDigest, digestToPromote); @@ -190,11 +196,9 @@ abstract contract HomeBase is OwnerIsCreator, ITypeAndVersion { emit ConfigPromoted(digestToPromote); } - function setDynamicConfig( - bytes32 pluginKey, - bytes calldata newDynamicConfig, - bytes32 currentDigest - ) external OnlyOwnerOrSelfCall { + function setDynamicConfig(bytes32 pluginKey, bytes calldata newDynamicConfig, bytes32 currentDigest) external { + _validateCaller(); + for (uint256 i = 0; i < MAX_CONCURRENT_CONFIGS; ++i) { if (s_configs[pluginKey][i].configDigest == currentDigest && currentDigest != ZERO_DIGEST) { _validateDynamicConfig(s_configs[pluginKey][i].staticConfig, newDynamicConfig); @@ -226,11 +230,4 @@ abstract contract HomeBase is OwnerIsCreator, ITypeAndVersion { ) ); } - - modifier OnlyOwnerOrSelfCall() { - if (msg.sender != owner() && msg.sender != address(this)) { - revert OnlyOwnerOrSelfCallAllowed(); - } - _; - } } diff --git a/contracts/src/v0.8/ccip/capability/RMNHome.sol b/contracts/src/v0.8/ccip/capability/RMNHome.sol index 951c749af1..dda5a92fab 100644 --- a/contracts/src/v0.8/ccip/capability/RMNHome.sol +++ b/contracts/src/v0.8/ccip/capability/RMNHome.sol @@ -166,6 +166,10 @@ contract RMNHome is HomeBase { _validateDynamicConfigParsed(dynamicConfig, numberOfNodes); } + function _validateCaller() internal view override { + _validateOwnership(); + } + function _getConfigDigestPrefix() internal pure override returns (uint256) { return PREFIX; } diff --git a/contracts/src/v0.8/ccip/test/capability/HomeBaseTest.t.sol b/contracts/src/v0.8/ccip/test/capability/HomeBaseTest.t.sol index fd9f91de34..87abad68b0 100644 --- a/contracts/src/v0.8/ccip/test/capability/HomeBaseTest.t.sol +++ b/contracts/src/v0.8/ccip/test/capability/HomeBaseTest.t.sol @@ -67,10 +67,10 @@ contract RMNHome_setSecondary is HomeBaseTest { assertEq(storedConfig.dynamicConfig, encodedConfig.dynamicConfig); } - function test_setSecondary_OnlyOwner_reverts() public { + function test_setSecondary_InvalidCaller_reverts() public { vm.startPrank(address(0)); - vm.expectRevert(HomeBase.OnlyOwnerOrSelfCallAllowed.selector); + vm.expectRevert(HomeBaseHelper.InvalidCaller.selector); s_homeBase.setSecondary(DON_ID, _getStaticConfig(), _getDynamicConfig(), ZERO_DIGEST); } } @@ -101,10 +101,10 @@ contract RMNHome_setDynamicConfig is HomeBaseTest { assertEq(secondaryDigest, secondaryConfigDigest); } - function test_setDynamicConfig_OnlyOwner_reverts() public { + function test_setDynamicConfig_InvalidCaller_reverts() public { vm.startPrank(address(0)); - vm.expectRevert(HomeBase.OnlyOwnerOrSelfCallAllowed.selector); + vm.expectRevert(HomeBaseHelper.InvalidCaller.selector); s_homeBase.setDynamicConfig(DON_ID, _getDynamicConfig(), keccak256("configDigest")); } } @@ -149,10 +149,10 @@ contract RMNHome_revokeSecondary is HomeBaseTest { s_homeBase.revokeSecondary(DON_ID, wrongDigest); } - function test_revokeSecondary_OnlyOwner_reverts() public { + function test_revokeSecondary_InvalidCaller_reverts() public { vm.startPrank(address(0)); - vm.expectRevert(HomeBase.OnlyOwnerOrSelfCallAllowed.selector); + vm.expectRevert(HomeBaseHelper.InvalidCaller.selector); s_homeBase.revokeSecondary(DON_ID, keccak256("configDigest")); } } @@ -160,10 +160,10 @@ contract RMNHome_revokeSecondary is HomeBaseTest { contract RMNHome_promoteSecondaryAndRevokePrimary is HomeBaseTest { function test_promoteSecondaryAndRevokePrimary_success() public {} - function test_promoteSecondaryAndRevokePrimary_OnlyOwner_reverts() public { + function test_promoteSecondaryAndRevokePrimary_InvalidCaller_reverts() public { vm.startPrank(address(0)); - vm.expectRevert(HomeBase.OnlyOwnerOrSelfCallAllowed.selector); + vm.expectRevert(HomeBaseHelper.InvalidCaller.selector); s_homeBase.promoteSecondaryAndRevokePrimary(DON_ID, keccak256("toPromote"), keccak256("ToRevoke")); } } diff --git a/contracts/src/v0.8/ccip/test/capability/RMNHomeTest.t.sol b/contracts/src/v0.8/ccip/test/capability/RMNHomeTest.t.sol index 63b4e5dffc..84a2fc7727 100644 --- a/contracts/src/v0.8/ccip/test/capability/RMNHomeTest.t.sol +++ b/contracts/src/v0.8/ccip/test/capability/RMNHomeTest.t.sol @@ -162,7 +162,7 @@ contract RMNHome_setSecondary is RMNHomeTest { vm.startPrank(address(0)); - vm.expectRevert(HomeBase.OnlyOwnerOrSelfCallAllowed.selector); + vm.expectRevert("Only callable by owner"); s_rmnHome.setSecondary(RMN_DON_ID, abi.encode(config.staticConfig), abi.encode(config.dynamicConfig), ZERO_DIGEST); } } @@ -227,7 +227,7 @@ contract RMNHome_setDynamicConfig is RMNHomeTest { vm.startPrank(address(0)); - vm.expectRevert(HomeBase.OnlyOwnerOrSelfCallAllowed.selector); + vm.expectRevert("Only callable by owner"); s_rmnHome.setDynamicConfig(RMN_DON_ID, abi.encode(config.dynamicConfig), keccak256("configDigest")); } } @@ -279,7 +279,7 @@ contract RMNHome_revokeSecondary is RMNHomeTest { function test_revokeSecondary_OnlyOwner_reverts() public { vm.startPrank(address(0)); - vm.expectRevert(HomeBase.OnlyOwnerOrSelfCallAllowed.selector); + vm.expectRevert("Only callable by owner"); s_rmnHome.revokeSecondary(RMN_DON_ID, keccak256("configDigest")); } } @@ -290,7 +290,7 @@ contract RMNHome_promoteSecondaryAndRevokePrimary is RMNHomeTest { function test_promoteSecondaryAndRevokePrimary_OnlyOwner_reverts() public { vm.startPrank(address(0)); - vm.expectRevert(HomeBase.OnlyOwnerOrSelfCallAllowed.selector); + vm.expectRevert("Only callable by owner"); s_rmnHome.promoteSecondaryAndRevokePrimary(RMN_DON_ID, keccak256("toPromote"), keccak256("ToRevoke")); } } diff --git a/contracts/src/v0.8/ccip/test/helpers/HomeBaseHelper.sol b/contracts/src/v0.8/ccip/test/helpers/HomeBaseHelper.sol index 70633ed3e8..0ff0df1ae4 100644 --- a/contracts/src/v0.8/ccip/test/helpers/HomeBaseHelper.sol +++ b/contracts/src/v0.8/ccip/test/helpers/HomeBaseHelper.sol @@ -4,6 +4,8 @@ pragma solidity 0.8.24; import {HomeBase} from "../../capability/HomeBase.sol"; contract HomeBaseHelper is HomeBase { + error InvalidCaller(); + string public constant override typeAndVersion = "HomeBaseHelper 1.6.0-dev"; uint256 public constant PREFIX = 0x0c0c << (256 - 16); @@ -16,6 +18,12 @@ contract HomeBaseHelper is HomeBase { return PREFIX; } + function _validateCaller() internal view override { + if (msg.sender != owner()) { + revert InvalidCaller(); + } + } + function getStoredConfig( bytes32 pluginKey, bytes32 configDigest