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.

Sign up for GitHub

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

Jump to bottom

Make onRamp mutable in OffRamp #1422

Merged
merged 24 commits into from
Sep 12, 2024
Merged

Make onRamp mutable in OffRamp #1422

merged 24 commits into from
Sep 12, 2024

Conversation

RayXpub
Copy link
Collaborator

@RayXpub RayXpub commented Sep 10, 2024 •
edited
Loading

Motivation

The goal of this PR is to reduce the misconfig risk on the OffRamp with the onRamp address. Currently, if we accidentally configure the wrong onRamp address on an existing OffRamp, the fix would involve a full redeployment + reconfig.

Solution

  • Make the onRamp mutable in the OffRamp.
  • Add onRamp address verification in the commit() function
  • Only allow onRamp address modification when minSeqNr == 1

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 10, 2024 •
edited
Loading

LCOV of commit 8495bb5 during Solidity Foundry #8125

Summary coverage rate:
  lines......: 97.7% (2195 of 2247 lines)
  functions..: 94.9% (411 of 433 functions)
  branches...: 93.4% (524 of 561 branches)

Files changed coverage rate: n/a

@RayXpub RayXpub marked this pull request as ready for review September 11, 2024 10:55
@RayXpub RayXpub requested review from makramkd, elatoskinas and a team as code owners September 11, 2024 10:55
RyanRHall
RyanRHall approved these changes Sep 11, 2024
View reviewed changes
Copy link
Contributor

@RyanRHall RyanRHall left a comment

Choose a reason for hiding this comment

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

LGTM, couple nits

contracts/foundry.toml
@@ -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 = 1_925
optimizer_runs = 1_650
Copy link
Contributor

Choose a reason for hiding this comment

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

😅

contracts/src/v0.8/ccip/offRamp/OffRamp.sol Outdated
Comment on lines 624 to 628
bytes memory onRamp = sourceChainConfig.onRamp;

if (keccak256(root.onRampAddress) != keccak256(onRamp)) {
revert InvalidOnRamp(root.onRampAddress, onRamp);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: reading onRamp into memory seems like an optimization for the error path, yeah? I think we could save on the happy path if we just read from storage again when doing the revert.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, updated.

elatoskinas
elatoskinas reviewed Sep 12, 2024
View reviewed changes
contracts/src/v0.8/ccip/offRamp/OffRamp.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/ccip/offRamp/OffRamp.sol Show resolved Hide resolved
contracts/src/v0.8/ccip/offRamp/OffRamp.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/ccip/offRamp/OffRamp.sol Outdated
@@ -64,6 +64,8 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
error InvalidMessageDestChainSelector(uint64 messageDestChainSelector);
error SourceChainSelectorMismatch(uint64 reportSourceChainSelector, uint64 messageSourceChainSelector);
error SignatureVerificationDisabled();
error InvalidOnRamp(bytes reportOnRamp, bytes configOnRamp);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: naming - should we name this InvalidCommitOnRamp?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think here the most likely scenario is that the wrong onRamp would be the configured one, not the one from the commit report.

Copy link
Collaborator

@elatoskinas elatoskinas Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Main confusion with the current naming is that this is easily confused with InvalidOnRampUpdate - would help if this error included that it is in the commit context (maybe CommitRampMismatch would be more accurate)

Copy link
Collaborator Author

@RayXpub RayXpub Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Updated to CommitOnRampMismatch

@RayXpub RayXpub enabled auto-merge (squash) September 12, 2024 16:44
@RayXpub RayXpub merged commit f880d4e into ccip-develop Sep 12, 2024
123 checks passed
@RayXpub RayXpub deleted the feat/make-onramp-dynamic-var-in-offramp branch September 12, 2024 16:49
@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elatoskinas elatoskinas elatoskinas approved these changes

@RyanRHall RyanRHall RyanRHall approved these changes

@makramkd makramkd Awaiting requested review from makramkd makramkd is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RayXpub @RyanRHall @elatoskinas