Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate RMNRemote and OffRamp #1360

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
f5d823a
remove cursing from OffRamp
RyanRHall Aug 21, 2024
178f28e
flatten MerkleRoot struct
RyanRHall Aug 22, 2024
fa27937
add onrampAddress to MerkleRoot struct
RyanRHall Aug 22, 2024
b90c9d4
write IRMNRemote, move MerkleRoot definition
RyanRHall Aug 23, 2024
93b0189
swap IRMN for IRMNRemote in OffRamp
RyanRHall Aug 23, 2024
4546f02
implement call to RMNemote#verify() from OffRamp
RyanRHall Aug 23, 2024
81d9075
rename onrampAddress => onRampAddress
RyanRHall Aug 25, 2024
da570c5
reorder fields in MerkleRoot struct
RyanRHall Aug 26, 2024
336513c
update plugin with new report structure
RyanRHall Aug 30, 2024
5369bb1
solhint tidy
RyanRHall Aug 27, 2024
c61c35e
solhint ignore struct packing issue
RyanRHall Aug 27, 2024
3315c1a
move RMNHome and RMNRemote into rmn/; integrate IRMNRemote
RyanRHall Aug 27, 2024
ecab50b
add RMNRemote to gethwrapper generation
RyanRHall Aug 27, 2024
c39bfc3
remove RMN from OnRamp
RyanRHall Aug 27, 2024
9201354
add backwards-compatible cursing to RMNRemote contract
RyanRHall Aug 28, 2024
29fc6e9
update solhint ignore
RyanRHall Aug 28, 2024
5557b1e
Revert "remove cursing from OffRamp"
RyanRHall Aug 29, 2024
d286c8b
Revert "remove RMN from OnRamp"
RyanRHall Aug 29, 2024
8a79cbf
update OnRamp to use RMNRemote
RyanRHall Aug 29, 2024
207e166
make PR suggested changes
RyanRHall Aug 29, 2024
3597ed5
Revert "upgrade solhint"
RyanRHall Aug 29, 2024
60ecdc0
move MerkleRoot to Internal lib
RyanRHall Aug 30, 2024
24ab8e3
Merge branch 'ccip-develop' of github.com:smartcontractkit/ccip into …
RyanRHall Sep 3, 2024
522d0e1
forge fmt
RyanRHall Sep 3, 2024
796523a
Merge branch 'ccip-develop' of github.com:smartcontractkit/ccip into …
RyanRHall Sep 4, 2024
8b2ff48
remove bootstrapP2PIds (#1388)
makramkd Sep 4, 2024
aaed373
rename IRMNRemote -> IRMNV2
RyanRHall Sep 4, 2024
afec448
Merge branch 'ccip-develop' of github.com:smartcontractkit/ccip into …
RyanRHall Sep 4, 2024
0e7f614
update wrappers & snapshot
RyanRHall Sep 4, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions contracts/.solhintignore
Original file line number Diff line number Diff line change
Expand Up @@ -43,5 +43,4 @@
./node_modules/

# Ignore RMN contracts temporarily
./src/v0.8/ccip/RMNRemote.sol
./src/v0.8/ccip/RMNHome.sol
./src/v0.8/ccip/rmn
659 changes: 327 additions & 332 deletions contracts/gas-snapshots/ccip.gas-snapshot

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion contracts/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@
"moment": "^2.30.1",
"prettier": "^3.3.3",
"prettier-plugin-solidity": "^1.3.1",
"solhint": "^5.0.3",
"solhint": "^5.0.1",
"solhint-plugin-chainlink-solidity": "git+https://github.com/smartcontractkit/chainlink-solhint-rules.git#v1.2.1",
"solhint-plugin-prettier": "^0.1.0",
"ts-node": "^10.9.2",
Expand Down
22 changes: 14 additions & 8 deletions contracts/pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions contracts/scripts/native_solc_compile_all_ccip
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ compileContract () {
# Contracts should be ordered in reverse-import-complexity-order to minimize overwrite risks.
compileContract ccip/offRamp/EVM2EVMOffRamp.sol
compileContract ccip/offRamp/OffRamp.sol
compileContract ccip/rmn/RMNRemote.sol
RyanRHall marked this conversation as resolved.
Show resolved Hide resolved
compileContract ccip/applications/PingPongDemo.sol
compileContract ccip/applications/SelfFundedPingPong.sol
compileContract ccip/applications/EtherSenderReceiver.sol
Expand Down
22 changes: 22 additions & 0 deletions contracts/src/v0.8/ccip/interfaces/IRMNV2.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import {Internal} from "../libraries/Internal.sol";

/// @notice This interface contains the only RMN-related functions that might be used on-chain by other CCIP contracts.
interface IRMNV2 {
/// @notice signature components from RMN nodes
struct Signature {
bytes32 r;
bytes32 s;
}

function verify(Internal.MerkleRoot[] memory merkleRoots, Signature[] memory signatures) external view;
makramkd marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A basic docstring on the expected behavior of this function might be useful (e.g maybe for implementing mocks), i.e "verifies RMN signatures against provided MerkleRoot structs and reverts on invalid signatures"


/// @notice If there is an active global or legacy curse, this function returns true.
function isCursed() external view returns (bool);

/// @notice If there is an active global curse, or an active curse for `subject`, this function returns true.
/// @param subject To check whether a particular chain is cursed, set to bytes16(uint128(chainSelector)).
function isCursed(bytes16 subject) external view returns (bool);
}
10 changes: 10 additions & 0 deletions contracts/src/v0.8/ccip/libraries/Internal.sol
Original file line number Diff line number Diff line change
Expand Up @@ -338,4 +338,14 @@ library Internal {

// bytes4(keccak256("CCIP ChainFamilySelector EVM"))
bytes4 public constant CHAIN_FAMILY_SELECTOR_EVM = 0x2812d52c;

/// @dev Struct to hold a merkle root and an interval for a source chain so that an array of these can be passed in the CommitReport.
/// @dev RMN depends on this struct, if changing, please notify the RMN maintainers.
struct MerkleRoot {
uint64 sourceChainSelector; // ──╮ Remote source chain selector that the Merkle Root is scoped to
uint64 minSeqNr; // | Minimum sequence number, inclusive
uint64 maxSeqNr; // ─────────────╯ Maximum sequence number, inclusive
bytes32 merkleRoot; // Merkle root covering the interval & source chain messages
bytes onRampAddress; // Generic onramp address, to support arbitrary sources; for EVM, use abi.encode
}
}
87 changes: 24 additions & 63 deletions contracts/src/v0.8/ccip/offRamp/OffRamp.sol
RyanRHall marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {IFeeQuoter} from "../interfaces/IFeeQuoter.sol";
import {IMessageInterceptor} from "../interfaces/IMessageInterceptor.sol";
import {INonceManager} from "../interfaces/INonceManager.sol";
import {IPoolV1} from "../interfaces/IPool.sol";
import {IRMN} from "../interfaces/IRMN.sol";
import {IRMNV2} from "../interfaces/IRMNV2.sol";
import {IRouter} from "../interfaces/IRouter.sol";
import {ITokenAdminRegistry} from "../interfaces/ITokenAdminRegistry.sol";

Expand Down Expand Up @@ -55,7 +55,7 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
error InvalidNewState(uint64 sourceChainSelector, uint64 sequenceNumber, Internal.MessageExecutionState newState);
error InvalidStaticConfig(uint64 sourceChainSelector);
error StaleCommitReport();
error InvalidInterval(uint64 sourceChainSelector, Interval interval);
error InvalidInterval(uint64 sourceChainSelector, uint64 min, uint64 max);
error ZeroAddressNotAllowed();
error InvalidMessageDestChainSelector(uint64 messageDestChainSelector);

Expand Down Expand Up @@ -83,9 +83,12 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {

/// @notice Struct that contains the static configuration
/// @dev RMN depends on this struct, if changing, please notify the RMN maintainers.
/// @dev not sure why solhint complains about this, seems like a buggy detector
/// https://github.com/protofire/solhint/issues/597
// solhint-disable-next-line gas-struct-packing
RyanRHall marked this conversation as resolved.
Show resolved Hide resolved
struct StaticConfig {
uint64 chainSelector; // ───╮ Destination chainSelector
address rmnProxy; // ───────╯ RMN proxy address
IRMNV2 rmn; // ─────────────╯ RMN Verification Contract
address tokenAdminRegistry; // Token admin registry address
address nonceManager; // Nonce manager address
}
Expand Down Expand Up @@ -117,40 +120,20 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
address messageValidator; // Optional message validator to validate incoming messages (zero address = no validator)
}

/// @notice a sequenceNumber interval
/// @dev RMN depends on this struct, if changing, please notify the RMN maintainers.
struct Interval {
uint64 min; // ───╮ Minimum sequence number, inclusive
uint64 max; // ───╯ Maximum sequence number, inclusive
}

/// @dev Struct to hold a merkle root and an interval for a source chain so that an array of these can be passed in the CommitReport.
struct MerkleRoot {
uint64 sourceChainSelector; // Remote source chain selector that the Merkle Root is scoped to
Interval interval; // Report interval of the merkle root
bytes32 merkleRoot; // Merkle root covering the interval & source chain messages
}

/// @notice Report that is committed by the observing DON at the committing phase
/// @dev RMN depends on this struct, if changing, please notify the RMN maintainers.
struct CommitReport {
Internal.PriceUpdates priceUpdates; // Collection of gas and price updates to commit
MerkleRoot[] merkleRoots; // Collection of merkle roots per source chain to commit
}

/// @dev Struct to hold a merkle root for a source chain so that an array of these can be passed in the
/// resetUnblessedRoots function.
struct UnblessedRoot {
uint64 sourceChainSelector; // Remote source chain selector that the Merkle Root is scoped to
bytes32 merkleRoot; // Merkle root of a single remote source chain
Internal.MerkleRoot[] merkleRoots; // Collection of merkle roots per source chain to commit
IRMNV2.Signature[] rmnSignatures; // RMN signatures on the merkle roots
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to be accurate these aren't signatures on the merkle roots exactly. They're signatures on a particular RMN report that consists of "lane updates" that match the updates in the merkleRoots array above. Maybe reference RMNRemote.verify() for folks who want to see the exact message being signed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right. If I make additional changes, I'll fix the comment :)

}

// STATIC CONFIG
string public constant override typeAndVersion = "OffRamp 1.6.0-dev";
/// @dev ChainSelector of this chain
uint64 internal immutable i_chainSelector;
/// @dev The address of the RMN proxy
address internal immutable i_rmnProxy;
/// @dev The RMN verification contract
IRMNV2 internal immutable i_rmn;
/// @dev The address of the token admin registry
address internal immutable i_tokenAdminRegistry;
/// @dev The address of the nonce manager
Expand Down Expand Up @@ -181,7 +164,7 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
SourceChainConfigArgs[] memory sourceChainConfigs
) MultiOCR3Base() {
if (
staticConfig.rmnProxy == address(0) || staticConfig.tokenAdminRegistry == address(0)
address(staticConfig.rmn) == address(0) || staticConfig.tokenAdminRegistry == address(0)
|| staticConfig.nonceManager == address(0)
) {
revert ZeroAddressNotAllowed();
Expand All @@ -192,7 +175,7 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
}

i_chainSelector = staticConfig.chainSelector;
i_rmnProxy = staticConfig.rmnProxy;
i_rmn = staticConfig.rmn;
i_tokenAdminRegistry = staticConfig.tokenAdminRegistry;
i_nonceManager = staticConfig.nonceManager;
emit StaticConfigSet(staticConfig);
Expand Down Expand Up @@ -344,7 +327,7 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
) internal {
uint64 sourceChainSelector = report.sourceChainSelector;
bool manualExecution = manualExecGasLimits.length != 0;
if (IRMN(i_rmnProxy).isCursed(bytes16(uint128(sourceChainSelector)))) {
if (i_rmn.isCursed(bytes16(uint128(sourceChainSelector)))) {
if (manualExecution) {
// For manual execution we don't want to silently fail so we revert
revert CursedByRMN(sourceChainSelector);
Expand Down Expand Up @@ -593,6 +576,11 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
) external {
CommitReport memory commitReport = abi.decode(report, (CommitReport));

// Verify RMN signatures
if (commitReport.merkleRoots.length > 0) {
i_rmn.verify(commitReport.merkleRoots, commitReport.rmnSignatures);
}

// Check if the report contains price updates
if (commitReport.priceUpdates.tokenPriceUpdates.length > 0 || commitReport.priceUpdates.gasPriceUpdates.length > 0)
{
Expand All @@ -613,20 +601,19 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
}

for (uint256 i = 0; i < commitReport.merkleRoots.length; ++i) {
MerkleRoot memory root = commitReport.merkleRoots[i];
Internal.MerkleRoot memory root = commitReport.merkleRoots[i];
uint64 sourceChainSelector = root.sourceChainSelector;

if (IRMN(i_rmnProxy).isCursed(bytes16(uint128(sourceChainSelector)))) {
if (i_rmn.isCursed(bytes16(uint128(sourceChainSelector)))) {
revert CursedByRMN(sourceChainSelector);
}

SourceChainConfig storage sourceChainConfig = _getEnabledSourceChainConfig(sourceChainSelector);

if (sourceChainConfig.minSeqNr != root.interval.min || root.interval.min > root.interval.max) {
revert InvalidInterval(root.sourceChainSelector, root.interval);
if (sourceChainConfig.minSeqNr != root.minSeqNr || root.minSeqNr > root.maxSeqNr) {
revert InvalidInterval(root.sourceChainSelector, root.minSeqNr, root.maxSeqNr);
}

// TODO: confirm how RMN offchain blessing impacts commit report
bytes32 merkleRoot = root.merkleRoot;
if (merkleRoot == bytes32(0)) revert InvalidRoot();
// If we reached this section, the report should contain a valid root
Expand All @@ -636,7 +623,7 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
revert RootAlreadyCommitted(root.sourceChainSelector, merkleRoot);
}

sourceChainConfig.minSeqNr = root.interval.max + 1;
sourceChainConfig.minSeqNr = root.maxSeqNr + 1;
s_roots[root.sourceChainSelector][merkleRoot] = block.timestamp;
}

Expand All @@ -661,28 +648,6 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
return s_roots[sourceChainSelector][root];
}

/// @notice Returns if a root is blessed or not.
/// @param root The merkle root to check the blessing status for.
/// @return blessed Whether the root is blessed or not.
function isBlessed(bytes32 root) public view returns (bool) {
// TODO: update RMN to also consider the source chain selector for blessing
return IRMN(i_rmnProxy).isBlessed(IRMN.TaggedRoot({commitStore: address(this), root: root}));
}

/// @notice Used by the owner in case an invalid sequence of roots has been
/// posted and needs to be removed. The interval in the report is trusted.
/// @param rootToReset The roots that will be reset. This function will only
/// reset roots that are not blessed.
function resetUnblessedRoots(UnblessedRoot[] calldata rootToReset) external onlyOwner {
for (uint256 i = 0; i < rootToReset.length; ++i) {
UnblessedRoot memory root = rootToReset[i];
if (!isBlessed(root.merkleRoot)) {
delete s_roots[root.sourceChainSelector][root.merkleRoot];
emit RootRemoved(root.merkleRoot);
}
}
}

/// @notice Returns timestamp of when root was accepted or 0 if verification fails.
/// @dev This method uses a merkle tree within a merkle tree, with the hashedLeaves,
/// proofs and proofFlagBits being used to get the root of the inner tree.
Expand All @@ -695,10 +660,6 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
uint256 proofFlagBits
) internal view virtual returns (uint256 timestamp) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: do we want to keep this internal function? feels like it can be inlined now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 this is a single line

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the _verify() function is overwritten in tests to support mocking verification results. Changing this would be a big reafactor of the test suite and would probably increase code complexity there. Opting to leave this as is unless you guys feel strongly.

Copy link
Collaborator

@RayXpub RayXpub Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels a bit wrong to have a one liner internal function just because of tests. We can punt to another PR but I'd be in favor of removing it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels a bit wrong to have a one liner internal function just because of tests. We can punt to another PR but I'd be in favor of removing it.

I'll spend a little more time on it today and see how annoying it will be in tests. I was admittedly being a touch lazy yesterday 🙃.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI easy solve: add a function in the offRampHelper that just sets s_roots[sourceChainSelector][root] to whatever you want. Should make the testing trivial

Copy link
Collaborator Author

@RyanRHall RyanRHall Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to push back again here. I don't think the juice is worth the squeeze. Here is an example of what these changes would look like. It's a super marginal contract size/gas savings in the offramp but a more annoying test suite with a fair amount of code duplication b/t the tests and the implementation. IMHO this increases the overall code complexity.

I think maintaining some modularity for the sake of easier testing should be seen as an intentional feature in this case!

bytes32 root = MerkleMultiProof.merkleRoot(hashedLeaves, proofs, proofFlagBits);
// Only return non-zero if present and blessed.
if (!isBlessed(root)) {
return 0;
}
return s_roots[sourceChainSelector][root];
}

Expand All @@ -724,7 +685,7 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
function getStaticConfig() external view returns (StaticConfig memory) {
return StaticConfig({
chainSelector: i_chainSelector,
rmnProxy: i_rmnProxy,
rmn: i_rmn,
tokenAdminRegistry: i_tokenAdminRegistry,
nonceManager: i_nonceManager
});
Expand Down
Loading
Loading