Skip to content

Commit

Permalink
extract caller validation into implementations
Browse files Browse the repository at this point in the history
  • Loading branch information
RensR committed Sep 24, 2024
1 parent 5309aa2 commit ce86ed6
Show file tree
Hide file tree
Showing 7 changed files with 73 additions and 55 deletions.
50 changes: 25 additions & 25 deletions contracts/gas-snapshots/ccip.gas-snapshot
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
11 changes: 10 additions & 1 deletion contracts/src/v0.8/ccip/capability/CCIPHome.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
}
}
}

Expand Down Expand Up @@ -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 │
// ================================================================
Expand Down
31 changes: 14 additions & 17 deletions contracts/src/v0.8/ccip/capability/HomeBase.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);

// ================================================================
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -226,11 +230,4 @@ abstract contract HomeBase is OwnerIsCreator, ITypeAndVersion {
)
);
}

modifier OnlyOwnerOrSelfCall() {
if (msg.sender != owner() && msg.sender != address(this)) {
revert OnlyOwnerOrSelfCallAllowed();
}
_;
}
}
4 changes: 4 additions & 0 deletions contracts/src/v0.8/ccip/capability/RMNHome.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
16 changes: 8 additions & 8 deletions contracts/src/v0.8/ccip/test/capability/HomeBaseTest.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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"));
}
}
Expand Down Expand Up @@ -149,21 +149,21 @@ 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"));
}
}

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"));
}
}
Expand Down
8 changes: 4 additions & 4 deletions contracts/src/v0.8/ccip/test/capability/RMNHomeTest.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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"));
}
}
Expand Down Expand Up @@ -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"));
}
}
Expand All @@ -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"));
}
}
8 changes: 8 additions & 0 deletions contracts/src/v0.8/ccip/test/helpers/HomeBaseHelper.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
Expand Down

0 comments on commit ce86ed6

Please sign in to comment.