Skip to content

Commit

Permalink
feat: validate transmitter lengths against F values
Browse files Browse the repository at this point in the history
  • Loading branch information
elatoskinas committed Sep 12, 2024
1 parent 7ba1e75 commit 535a28e
Show file tree
Hide file tree
Showing 6 changed files with 194 additions and 22 deletions.
39 changes: 29 additions & 10 deletions contracts/src/v0.8/ccip/capability/CCIPConfig.sol
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ contract CCIPConfig is ITypeAndVersion, ICapabilityConfiguration, OwnerIsCreator
error TooManySigners();
error P2PIdsLengthNotMatching(uint256 p2pIdsLength, uint256 signersLength, uint256 transmittersLength);
error NotEnoughTransmitters(uint256 got, uint256 minimum);
error FMustBePositive();
error FChainMustBePositive();
error FTooHigh();
error FChainTooHigh();
error InvalidPluginType();
error OfframpAddressCannotBeZero();
error InvalidConfigLength(uint256 length);
Expand Down Expand Up @@ -167,6 +167,7 @@ contract CCIPConfig is ITypeAndVersion, ICapabilityConfiguration, OwnerIsCreator
}

/// @notice Called by the registry prior to the config being set for a particular DON.
/// @dev precondition Requires all chain configs to be set
function beforeCapabilityConfigSet(
bytes32[] calldata, /* nodes */
bytes calldata config,
Expand Down Expand Up @@ -352,9 +353,7 @@ contract CCIPConfig is ITypeAndVersion, ICapabilityConfiguration, OwnerIsCreator
/// @param ocr3Configs The OCR3 configurations to group.
/// @return commitConfigs The commit configurations.
/// @return execConfigs The execution configurations.
function _groupByPluginType(
CCIPConfigTypes.OCR3Config[] memory ocr3Configs
)
function _groupByPluginType(CCIPConfigTypes.OCR3Config[] memory ocr3Configs)
internal
pure
returns (CCIPConfigTypes.OCR3Config[] memory commitConfigs, CCIPConfigTypes.OCR3Config[] memory execConfigs)
Expand Down Expand Up @@ -402,18 +401,37 @@ contract CCIPConfig is ITypeAndVersion, ICapabilityConfiguration, OwnerIsCreator
}
if (!s_remoteChainSelectors.contains(cfg.chainSelector)) revert ChainSelectorNotFound(cfg.chainSelector);

// We check for chain config presence above, so fChain here must be non-zero.
uint256 minTransmittersLength = 3 * s_chainConfigurations[cfg.chainSelector].fChain + 1;
if (cfg.transmitters.length < minTransmittersLength) {
revert NotEnoughTransmitters(cfg.transmitters.length, minTransmittersLength);
// fChain cannot exceed F, since it is a subcommittee in the larger DON
uint256 F = cfg.F;
uint256 fChain = s_chainConfigurations[cfg.chainSelector].fChain;
// F != 0 check here is done implicitly, since fChain > 0 (since the config is set via the contains() check),
// and F >= fChain
if (fChain > F) {
revert FChainTooHigh();
}

// Transmitters can be set to 0 since there can be more signers than transmitters,
uint256 nonZeroTransmitters = 0;
for (uint256 i = 0; i < cfg.transmitters.length; ++i) {
if (cfg.transmitters[i].length != 0) {
nonZeroTransmitters++;
}
}

// We check for chain config presence above, so fChain here must be non-zero. fChain <= F due to the checks above.
// There can be less transmitters than signers - so they can be set to zero (which indicates that a node is a signer, but not a transmitter).
uint256 minTransmittersLength = 3 * fChain + 1;
if (nonZeroTransmitters < minTransmittersLength) {
revert NotEnoughTransmitters(nonZeroTransmitters, minTransmittersLength);
}
uint256 numberOfSigners = cfg.signers.length;
if (numberOfSigners > MAX_NUM_ORACLES) revert TooManySigners();

// Enforcing len(signers) == len(transmitters) = len(p2pIds)
if (numberOfSigners != cfg.p2pIds.length || numberOfSigners != cfg.transmitters.length) {
revert P2PIdsLengthNotMatching(cfg.p2pIds.length, cfg.signers.length, cfg.transmitters.length);
}
if (cfg.F == 0) revert FMustBePositive();
if (numberOfSigners <= 3 * cfg.F) revert FTooHigh();
if (numberOfSigners <= 3 * F) revert FTooHigh();

// Check that the readers are in the capabilities registry.
_ensureInRegistry(cfg.p2pIds);
Expand Down Expand Up @@ -459,6 +477,7 @@ contract CCIPConfig is ITypeAndVersion, ICapabilityConfiguration, OwnerIsCreator
// ================================================================

/// @notice Sets and/or removes chain configurations.
/// Does not validate that fChain <= F and relies on OCR3Configs to be changed in case fChain becomes larger than the big F value.
/// @param chainSelectorRemoves The chain configurations to remove.
/// @param chainConfigAdds The chain configurations to add.
function applyChainConfigUpdates(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,19 @@ library CCIPConfigTypes {
}

/// @notice OCR3 configuration.
/// Note that F >= fChain, since F represents the role DON, and fChain represents committees.
/// F values are typically identical across multiple OCR3 configs since the chains pertain to one role DON,
/// but F values can change across OCR3 configs to indicate role DON splits.
struct OCR3Config {
Internal.OCRPluginType pluginType; // ────────╮ The plugin that the configuration is for.
uint64 chainSelector; // | The (remote) chain that the configuration is for.
uint8 F; // | The "big F" parameter for the role DON.
uint64 offchainConfigVersion; // ─────────────╯ The version of the offchain configuration.
bytes offrampAddress; // The remote chain offramp address.
// len(p2pIds) == len(signers) == len(transmitters) == 3 * F + 1
// len(p2pIds) == len(signers) == len(transmitters) >= 3 * F + 1
// NOTE: indexes matter here! The p2p ID at index i corresponds to the signer at index i and the transmitter at index i.
// This is crucial in order to build the oracle ID <-> peer ID mapping offchain.
// Transmitters can be set to 0-bytes to represent that a node is a signer but not a transmitter (since len(signers) >= len(transmitters))
bytes32[] p2pIds; // The P2P IDs of the oracles that are part of the role DON.
bytes[] signers; // The onchain signing keys of nodes in the don.
bytes[] transmitters; // The onchain transmitter keys of nodes in the don.
Expand Down
10 changes: 9 additions & 1 deletion contracts/src/v0.8/ccip/ocr/MultiOCR3Base.sol
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ abstract contract MultiOCR3Base is ITypeAndVersion, OwnerIsCreator {
TOO_MANY_TRANSMITTERS,
TOO_MANY_SIGNERS,
F_TOO_HIGH,
REPEATED_ORACLE_ADDRESS
REPEATED_ORACLE_ADDRESS,
NO_TRANSMITTERS
}

error InvalidConfig(InvalidConfigErrorType errorType);
Expand Down Expand Up @@ -74,6 +75,7 @@ abstract contract MultiOCR3Base is ITypeAndVersion, OwnerIsCreator {
/// @notice OCR configuration for a single OCR plugin within a DON.
struct OCRConfig {
ConfigInfo configInfo; // latest OCR config
// NOTE: len(signers) can be different from len(transmitters). There is no index relationship between the two arrays
address[] signers; // addresses oracles use to sign the reports
address[] transmitters; // addresses oracles use to transmit the reports
}
Expand Down Expand Up @@ -123,6 +125,8 @@ abstract contract MultiOCR3Base is ITypeAndVersion, OwnerIsCreator {
/// NOTE: The OCR3 config must be sanity-checked against the home-chain registry configuration, to ensure
/// home-chain and remote-chain parity!
/// @param ocrConfigArgs OCR config update args.
/// @dev precondition number of transmitters should match the expected F/fChain relationship.
/// For transmitters, the function only validates that len(transmitters) > 0 && len(transmitters) <= MAX_NUM_ORACLES
function setOCR3Configs(OCRConfigArgs[] memory ocrConfigArgs) external onlyOwner {
for (uint256 i; i < ocrConfigArgs.length; ++i) {
_setOCR3Config(ocrConfigArgs[i]);
Expand All @@ -147,6 +151,7 @@ abstract contract MultiOCR3Base is ITypeAndVersion, OwnerIsCreator {

address[] memory transmitters = ocrConfigArgs.transmitters;
if (transmitters.length > MAX_NUM_ORACLES) revert InvalidConfig(InvalidConfigErrorType.TOO_MANY_TRANSMITTERS);
if (transmitters.length == 0) revert InvalidConfig(InvalidConfigErrorType.NO_TRANSMITTERS);

_clearOracleRoles(ocrPluginType, ocrConfig.transmitters);

Expand All @@ -157,6 +162,9 @@ abstract contract MultiOCR3Base is ITypeAndVersion, OwnerIsCreator {

if (signers.length > MAX_NUM_ORACLES) revert InvalidConfig(InvalidConfigErrorType.TOO_MANY_SIGNERS);
if (signers.length <= 3 * ocrConfigArgs.F) revert InvalidConfig(InvalidConfigErrorType.F_TOO_HIGH);
// NOTE: Transmitters cannot exceed signers. Transmitters do not have to be >= 3F + 1 because they can match >= 3fChain + 1, where fChain <= F.
// fChain is not represented in MultiOCR3Base - so we skip this check.
if (signers.length < transmitters.length) revert InvalidConfig(InvalidConfigErrorType.TOO_MANY_TRANSMITTERS);

configInfo.n = uint8(signers.length);
ocrConfig.signers = signers;
Expand Down
68 changes: 62 additions & 6 deletions contracts/src/v0.8/ccip/test/capability/CCIPConfig.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,16 @@ contract CCIPConfigSetup is Test {
if (i < right) _sort(arr, i, right);
}

function _addChainConfig(uint256 numNodes)
internal
returns (bytes32[] memory p2pIds, bytes[] memory signers, bytes[] memory transmitters)
{
return _addChainConfig(numNodes, 1);
}

function _addChainConfig(
uint256 numNodes
uint256 numNodes,
uint8 fChain
) internal returns (bytes32[] memory p2pIds, bytes[] memory signers, bytes[] memory transmitters) {
p2pIds = _makeBytes32Array(numNodes, 0);
_sort(p2pIds, 0, int256(numNodes - 1));
Expand All @@ -90,7 +98,7 @@ contract CCIPConfigSetup is Test {
CCIPConfigTypes.ChainConfigInfo[] memory adds = new CCIPConfigTypes.ChainConfigInfo[](1);
adds[0] = CCIPConfigTypes.ChainConfigInfo({
chainSelector: 1,
chainConfig: CCIPConfigTypes.ChainConfig({readers: p2pIds, fChain: 1, config: bytes("config1")})
chainConfig: CCIPConfigTypes.ChainConfig({readers: p2pIds, fChain: fChain, config: bytes("config1")})
});

vm.expectEmit();
Expand Down Expand Up @@ -373,8 +381,8 @@ contract CCIPConfig_chainConfig is CCIPConfigSetup {
}

contract CCIPConfig_validateConfig is CCIPConfigSetup {
function _getCorrectOCR3Config() internal returns (CCIPConfigTypes.OCR3Config memory) {
(bytes32[] memory p2pIds, bytes[] memory signers, bytes[] memory transmitters) = _addChainConfig(4);
function _getCorrectOCR3Config(uint8 numNodes, uint8 F) internal returns (CCIPConfigTypes.OCR3Config memory) {
(bytes32[] memory p2pIds, bytes[] memory signers, bytes[] memory transmitters) = _addChainConfig(numNodes);

return CCIPConfigTypes.OCR3Config({
pluginType: Internal.OCRPluginType.Commit,
Expand All @@ -383,18 +391,39 @@ contract CCIPConfig_validateConfig is CCIPConfigSetup {
p2pIds: p2pIds,
signers: signers,
transmitters: transmitters,
F: 1,
F: F,
offchainConfigVersion: 30,
offchainConfig: bytes("offchainConfig")
});
}

function _getCorrectOCR3Config() internal returns (CCIPConfigTypes.OCR3Config memory) {
return _getCorrectOCR3Config(4, 1);
}

// Successes.

function test__validateConfig_Success() public {
s_ccipCC.validateConfig(_getCorrectOCR3Config());
}

function test__validateConfigLessTransmittersThanSigners_Success() public {
// fChain is 1, so there should be at least 4 transmitters.
CCIPConfigTypes.OCR3Config memory config = _getCorrectOCR3Config(5, 1);
config.transmitters[1] = bytes("");

s_ccipCC.validateConfig(config);
}

function test__validateConfigSmallerFChain_Success() public {
CCIPConfigTypes.OCR3Config memory config = _getCorrectOCR3Config(11, 3);

// Set fChain to 2
_addChainConfig(4, 2);

s_ccipCC.validateConfig(config);
}

// Reverts.

function test__validateConfig_ChainSelectorNotSet_Reverts() public {
Expand Down Expand Up @@ -450,6 +479,22 @@ contract CCIPConfig_validateConfig is CCIPConfigSetup {
s_ccipCC.validateConfig(config);
}

function test__validateConfig_NotEnoughTransmittersEmptyAddresses_Reverts() public {
CCIPConfigTypes.OCR3Config memory config = _getCorrectOCR3Config();
config.transmitters[0] = bytes("");

vm.expectRevert(abi.encodeWithSelector(CCIPConfig.NotEnoughTransmitters.selector, 3, 4));
s_ccipCC.validateConfig(config);

// Zero out remaining transmitters to verify error changes
for (uint256 i = 1; i < config.transmitters.length; ++i) {
config.transmitters[i] = bytes("");
}

vm.expectRevert(abi.encodeWithSelector(CCIPConfig.NotEnoughTransmitters.selector, 0, 4));
s_ccipCC.validateConfig(config);
}

function test__validateConfig_TooManySigners_Reverts() public {
CCIPConfigTypes.OCR3Config memory config = _getCorrectOCR3Config();
config.signers = new bytes[](257);
Expand All @@ -458,11 +503,22 @@ contract CCIPConfig_validateConfig is CCIPConfigSetup {
s_ccipCC.validateConfig(config);
}

function test__validateConfig_FChainTooHigh_Reverts() public {
CCIPConfigTypes.OCR3Config memory config = _getCorrectOCR3Config();
config.F = 2; // too low

// Set fChain to 3
_addChainConfig(4, 3);

vm.expectRevert(CCIPConfig.FChainTooHigh.selector);
s_ccipCC.validateConfig(config);
}

function test__validateConfig_FMustBePositive_Reverts() public {
CCIPConfigTypes.OCR3Config memory config = _getCorrectOCR3Config();
config.F = 0; // not positive

vm.expectRevert(CCIPConfig.FMustBePositive.selector);
vm.expectRevert(CCIPConfig.FChainTooHigh.selector);
s_ccipCC.validateConfig(config);
}

Expand Down
2 changes: 1 addition & 1 deletion contracts/src/v0.8/ccip/test/e2e/MultiRampsEnd2End.sol
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ contract MultiRampsE2E is OnRampSetup, OffRampSetup {
router.ccipSend(DEST_CHAIN_SELECTOR, message);
vm.pauseGasMetering();

uint256 gasLimit = s_feeQuoter.parseEVMExtraArgsFromBytes(msgEvent.extraArgs, DEST_CHAIN_SELECTOR).gasLimit;
s_feeQuoter.parseEVMExtraArgsFromBytes(msgEvent.extraArgs, DEST_CHAIN_SELECTOR).gasLimit;
for (uint256 i = 0; i < msgEvent.tokenAmounts.length; ++i) {
msgEvent.tokenAmounts[i].destExecData = abi.encode(MAX_TOKEN_POOL_RELEASE_OR_MINT_GAS);
}
Expand Down
Loading

0 comments on commit 535a28e

Please sign in to comment.