Skip to content

Commit

Permalink
Enforce signature verification for commit plugin in OffRamp (#1416)
Browse files Browse the repository at this point in the history
## Motivation

The goal of this PR is to implement a check to ensure that the OCR3
signature verification is enabled for the commit plugin on config. This
reduces misconfig risks and impact because previously an accidental
setting of `isSignatureVerificationEnabled=false` for the commit plugin
in the `OffRamp` would have required a redeploy to be fixed.

## Solution

Add a check enforcing `isSignatureVerificationEnabled=false` for the
commit plugin in `_afterOC3Config`.
  • Loading branch information
RayXpub authored Sep 9, 2024
1 parent 95ffd86 commit 53a8491
Show file tree
Hide file tree
Showing 9 changed files with 922 additions and 911 deletions.
2 changes: 1 addition & 1 deletion contracts/foundry.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ single_line_statement_blocks = "preserve"
solc_version = '0.8.24'
src = 'src/v0.8/ccip'
test = 'src/v0.8/ccip/test'
optimizer_runs = 3_600
optimizer_runs = 1_925
evm_version = 'paris'

[profile.functions]
Expand Down
1,758 changes: 879 additions & 879 deletions contracts/gas-snapshots/ccip.gas-snapshot

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion contracts/scripts/native_solc_compile_all_ccip
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ SOLC_VERSION="0.8.24"
OPTIMIZE_RUNS=26000
OPTIMIZE_RUNS_OFFRAMP=18000
OPTIMIZE_RUNS_ONRAMP=4100
OPTIMIZE_RUNS_MULTI_OFFRAMP=1999
OPTIMIZE_RUNS_MULTI_OFFRAMP=1925


SCRIPTPATH="$( cd "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )"
Expand Down
5 changes: 5 additions & 0 deletions contracts/src/v0.8/ccip/offRamp/OffRamp.sol
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
error ZeroAddressNotAllowed();
error InvalidMessageDestChainSelector(uint64 messageDestChainSelector);
error SourceChainSelectorMismatch(uint64 reportSourceChainSelector, uint64 messageSourceChainSelector);
error SignatureVerificationDisabled();

/// @dev Atlas depends on this event, if changing, please notify Atlas.
event StaticConfigSet(StaticConfig staticConfig);
Expand Down Expand Up @@ -673,6 +674,10 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
/// @inheritdoc MultiOCR3Base
function _afterOCR3ConfigSet(uint8 ocrPluginType) internal override {
if (ocrPluginType == uint8(Internal.OCRPluginType.Commit)) {
// Signature verification must be enabled for commit plugin
if (!s_ocrConfigs[ocrPluginType].configInfo.isSignatureVerificationEnabled) {
revert SignatureVerificationDisabled();
}
// When the OCR config changes, we reset the sequence number
// since it is scoped per config digest.
// Note that s_minSeqNr/roots do not need to be reset as the roots persist
Expand Down
2 changes: 2 additions & 0 deletions contracts/src/v0.8/ccip/test/e2e/MultiRampsEnd2End.sol
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// SPDX-License-Identifier: BUSL-1.1
pragma solidity 0.8.24;

import {IRMN} from "../../interfaces/IRMN.sol";

import {AuthorizedCallers} from "../../../shared/access/AuthorizedCallers.sol";
import {NonceManager} from "../../NonceManager.sol";
import {IRMNV2} from "../../interfaces/IRMNV2.sol";
Expand Down
56 changes: 31 additions & 25 deletions contracts/src/v0.8/ccip/test/offRamp/OffRamp.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ pragma solidity 0.8.24;

import {IFeeQuoter} from "../../interfaces/IFeeQuoter.sol";
import {IMessageInterceptor} from "../../interfaces/IMessageInterceptor.sol";
import {IPriceRegistry} from "../../interfaces/IPriceRegistry.sol";
import {IRMNV2} from "../../interfaces/IRMNV2.sol";
import {IRouter} from "../../interfaces/IRouter.sol";
import {ITokenAdminRegistry} from "../../interfaces/ITokenAdminRegistry.sol";
Expand All @@ -13,7 +12,6 @@ import {FeeQuoter} from "../../FeeQuoter.sol";
import {NonceManager} from "../../NonceManager.sol";
import {Client} from "../../libraries/Client.sol";
import {Internal} from "../../libraries/Internal.sol";
import {MerkleMultiProof} from "../../libraries/MerkleMultiProof.sol";
import {Pool} from "../../libraries/Pool.sol";
import {RateLimiter} from "../../libraries/RateLimiter.sol";
import {MultiOCR3Base} from "../../ocr/MultiOCR3Base.sol";
Expand Down Expand Up @@ -2113,8 +2111,8 @@ contract OffRamp_execute is OffRampSetup {
ocrPluginType: uint8(Internal.OCRPluginType.Commit),
configDigest: s_configDigestCommit,
F: s_F,
isSignatureVerificationEnabled: false,
signers: s_emptySigners,
isSignatureVerificationEnabled: true,
signers: s_validSigners,
transmitters: s_validTransmitters
});
s_offRamp.setOCR3Configs(ocrConfigs);
Expand Down Expand Up @@ -2457,7 +2455,7 @@ contract OffRamp_trialExecute is OffRampSetup {
}
}

contract OffRamp__releaseOrMintSingleToken is OffRampSetup {
contract OffRamp_releaseOrMintSingleToken is OffRampSetup {
function setUp() public virtual override {
super.setUp();
_setupMultipleOffRamps();
Expand Down Expand Up @@ -3395,26 +3393,6 @@ contract OffRamp_commit is OffRampSetup {
s_offRamp.commit(reportContext, abi.encode(commitReport), rs, ss, rawVs);
}

function test_WrongConfigWithoutSigners_Revert() public {
_redeployOffRampWithNoOCRConfigs();

OffRamp.CommitReport memory commitReport = _constructCommitReport();

MultiOCR3Base.OCRConfigArgs[] memory ocrConfigs = new MultiOCR3Base.OCRConfigArgs[](1);
ocrConfigs[0] = MultiOCR3Base.OCRConfigArgs({
ocrPluginType: uint8(Internal.OCRPluginType.Commit),
configDigest: s_configDigestCommit,
F: s_F,
isSignatureVerificationEnabled: false,
signers: s_emptySigners,
transmitters: s_validTransmitters
});
s_offRamp.setOCR3Configs(ocrConfigs);

vm.expectRevert();
_commit(commitReport, s_latestSequenceNumber);
}

function test_FailedRMNVerification_Reverts() public {
// force RMN verification to fail
vm.mockCallRevert(address(s_mockRMNRemote), abi.encodeWithSelector(IRMNV2.verify.selector), bytes(""));
Expand Down Expand Up @@ -3583,3 +3561,31 @@ contract OffRamp_commit is OffRampSetup {
});
}
}

contract OffRamp_afterOC3ConfigSet is OffRampSetup {
function test_afterOCR3ConfigSet_SignatureVerificationDisabled_Revert() public {
s_offRamp = new OffRampHelper(
OffRamp.StaticConfig({
chainSelector: DEST_CHAIN_SELECTOR,
rmn: s_mockRMNRemote,
tokenAdminRegistry: address(s_tokenAdminRegistry),
nonceManager: address(s_inboundNonceManager)
}),
_generateDynamicOffRampConfig(address(s_feeQuoter)),
new OffRamp.SourceChainConfigArgs[](0)
);

MultiOCR3Base.OCRConfigArgs[] memory ocrConfigs = new MultiOCR3Base.OCRConfigArgs[](1);
ocrConfigs[0] = MultiOCR3Base.OCRConfigArgs({
ocrPluginType: uint8(Internal.OCRPluginType.Commit),
configDigest: s_configDigestCommit,
F: s_F,
isSignatureVerificationEnabled: false,
signers: s_validSigners,
transmitters: s_validTransmitters
});

vm.expectRevert(OffRamp.SignatureVerificationDisabled.selector);
s_offRamp.setOCR3Configs(ocrConfigs);
}
}
2 changes: 0 additions & 2 deletions contracts/src/v0.8/ccip/test/offRamp/OffRampSetup.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ pragma solidity 0.8.24;

import {IAny2EVMMessageReceiver} from "../../interfaces/IAny2EVMMessageReceiver.sol";
import {ICommitStore} from "../../interfaces/ICommitStore.sol";
import {IRMN} from "../../interfaces/IRMN.sol";
import {IRMNV2} from "../../interfaces/IRMNV2.sol";

import {AuthorizedCallers} from "../../../shared/access/AuthorizedCallers.sol";
Expand All @@ -16,7 +15,6 @@ import {MultiOCR3Base} from "../../ocr/MultiOCR3Base.sol";
import {EVM2EVMOffRamp} from "../../offRamp/EVM2EVMOffRamp.sol";
import {OffRamp} from "../../offRamp/OffRamp.sol";
import {TokenPool} from "../../pools/TokenPool.sol";

import {FeeQuoterSetup} from "../feeQuoter/FeeQuoterSetup.t.sol";
import {EVM2EVMOffRampHelper} from "../helpers/EVM2EVMOffRampHelper.sol";
import {MaybeRevertingBurnMintTokenPool} from "../helpers/MaybeRevertingBurnMintTokenPool.sol";
Expand Down
4 changes: 2 additions & 2 deletions core/gethwrappers/ccip/generated/offramp/offramp.go

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ multi_aggregate_rate_limiter: ../../../contracts/solc/v0.8.24/MultiAggregateRate
multi_ocr3_helper: ../../../contracts/solc/v0.8.24/MultiOCR3Helper/MultiOCR3Helper.abi ../../../contracts/solc/v0.8.24/MultiOCR3Helper/MultiOCR3Helper.bin 6b56e0114a4d50797d30a34aecc2641ef340451d0c3fcb9d729bba4df2435122
nonce_manager: ../../../contracts/solc/v0.8.24/NonceManager/NonceManager.abi ../../../contracts/solc/v0.8.24/NonceManager/NonceManager.bin 6f64e1083b356c06ee66b9138e398b9c97a4cd3e8c9ec38cf3010cebc79af536
ocr3_config_encoder: ../../../contracts/solc/v0.8.24/IOCR3ConfigEncoder/IOCR3ConfigEncoder.abi ../../../contracts/solc/v0.8.24/IOCR3ConfigEncoder/IOCR3ConfigEncoder.bin 9254b35a86f00fde7b7193a033ca58f6521a66e87b9cf9da6ce5660082e79f5d
offramp: ../../../contracts/solc/v0.8.24/OffRamp/OffRamp.abi ../../../contracts/solc/v0.8.24/OffRamp/OffRamp.bin 2d1cdfd810e2fde409610f4d188889e62a41c8baceb8948849c9a78623a252b6
offramp: ../../../contracts/solc/v0.8.24/OffRamp/OffRamp.abi ../../../contracts/solc/v0.8.24/OffRamp/OffRamp.bin f8b00a000ceaadcd9d0c2a9d78f6d2aedbff1e3dcdb66e4fd23333ed6daf26e7
onramp: ../../../contracts/solc/v0.8.24/OnRamp/OnRamp.abi ../../../contracts/solc/v0.8.24/OnRamp/OnRamp.bin c37096aaa0369ad988e94c300ba62917e17fcc71a3c1aa3e9b8420f21c0591d2
ping_pong_demo: ../../../contracts/solc/v0.8.24/PingPongDemo/PingPongDemo.abi ../../../contracts/solc/v0.8.24/PingPongDemo/PingPongDemo.bin c1c2f8a65c7ffd971899cae7fe62f2da57d09e936151e2b92163c4bebe699d6b
price_registry: ../../../contracts/solc/v0.8.24/PriceRegistry/PriceRegistry.abi ../../../contracts/solc/v0.8.24/PriceRegistry/PriceRegistry.bin e7781d600c1bb7aa4620106af7f6e146a109b97f4cb6a7d06c9e15773340ecb2
Expand Down

0 comments on commit 53a8491

Please sign in to comment.