Skip to content

Commit

Permalink
remove rawvs from RMNRemote (#14969)
Browse files Browse the repository at this point in the history
* remove rawvs from RMNRemote

* fix offchain code

* add changeset

* [Bot] Update changeset file with jira issues

* add 28 => 27 ECDSA sig conversion in tests

* increase f in verify() test

* add comments for v sig fligging

* remove rawVs from offramp test

* generate snapshot & wrappers

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
  • Loading branch information
1 parent b888043 commit ccd9956
Show file tree
Hide file tree
Showing 18 changed files with 128 additions and 175 deletions.
10 changes: 10 additions & 0 deletions contracts/.changeset/nervous-cherries-judge.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
'@chainlink/contracts': patch
---

remove rawVs from RMNRemote


PR issue: CCIP-4015

Solidity Review issue: CCIP-3966
74 changes: 37 additions & 37 deletions contracts/gas-snapshots/ccip.gas-snapshot
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ CCIPHome_setCandidate:test_setCandidate_ConfigDigestMismatch_reverts() (gas: 139
CCIPHome_setCandidate:test_setCandidate_success() (gas: 1365439)
DefensiveExampleTest:test_HappyPath_Success() (gas: 200473)
DefensiveExampleTest:test_Recovery() (gas: 424859)
E2E:test_E2E_3MessagesMMultiOffRampSuccess_gas() (gas: 1521230)
E2E:test_E2E_3MessagesMMultiOffRampSuccess_gas() (gas: 1520953)
EtherSenderReceiverTest_ccipReceive:test_ccipReceive_fallbackToWethTransfer() (gas: 96962)
EtherSenderReceiverTest_ccipReceive:test_ccipReceive_happyPath() (gas: 49812)
EtherSenderReceiverTest_ccipReceive:test_ccipReceive_wrongToken() (gas: 17457)
Expand Down Expand Up @@ -392,17 +392,17 @@ NonceManager_applyPreviousRampsUpdates:test_PreviousRampAlreadySet_overrideAllow
NonceManager_applyPreviousRampsUpdates:test_SingleRampUpdate_success() (gas: 66889)
NonceManager_applyPreviousRampsUpdates:test_ZeroInput_success() (gas: 12213)
NonceManager_typeAndVersion:test_typeAndVersion() (gas: 9705)
OffRamp_afterOC3ConfigSet:test_afterOCR3ConfigSet_SignatureVerificationDisabled_Revert() (gas: 5910204)
OffRamp_afterOC3ConfigSet:test_afterOCR3ConfigSet_SignatureVerificationDisabled_Revert() (gas: 5908404)
OffRamp_applySourceChainConfigUpdates:test_AddMultipleChains_Success() (gas: 626115)
OffRamp_applySourceChainConfigUpdates:test_AddNewChain_Success() (gas: 166515)
OffRamp_applySourceChainConfigUpdates:test_ApplyZeroUpdates_Success() (gas: 16763)
OffRamp_applySourceChainConfigUpdates:test_InvalidOnRampUpdate_Revert() (gas: 275075)
OffRamp_applySourceChainConfigUpdates:test_InvalidOnRampUpdate_Revert() (gas: 274803)
OffRamp_applySourceChainConfigUpdates:test_ReplaceExistingChainOnRamp_Success() (gas: 168560)
OffRamp_applySourceChainConfigUpdates:test_ReplaceExistingChain_Success() (gas: 181059)
OffRamp_applySourceChainConfigUpdates:test_RouterAddress_Revert() (gas: 13463)
OffRamp_applySourceChainConfigUpdates:test_ZeroOnRampAddress_Revert() (gas: 72746)
OffRamp_applySourceChainConfigUpdates:test_ZeroSourceChainSelector_Revert() (gas: 15476)
OffRamp_applySourceChainConfigUpdates:test_allowNonOnRampUpdateAfterLaneIsUsed_success() (gas: 285425)
OffRamp_applySourceChainConfigUpdates:test_allowNonOnRampUpdateAfterLaneIsUsed_success() (gas: 285153)
OffRamp_batchExecute:test_MultipleReportsDifferentChainsSkipCursedChain_Success() (gas: 177564)
OffRamp_batchExecute:test_MultipleReportsDifferentChains_Success() (gas: 333809)
OffRamp_batchExecute:test_MultipleReportsSameChain_Success() (gas: 277075)
Expand All @@ -412,28 +412,28 @@ OffRamp_batchExecute:test_SingleReport_Success() (gas: 156555)
OffRamp_batchExecute:test_Unhealthy_Success() (gas: 553993)
OffRamp_batchExecute:test_ZeroReports_Revert() (gas: 10600)
OffRamp_ccipReceive:test_Reverts() (gas: 15407)
OffRamp_commit:test_CommitOnRampMismatch_Revert() (gas: 93097)
OffRamp_commit:test_FailedRMNVerification_Reverts() (gas: 63763)
OffRamp_commit:test_InvalidIntervalMinLargerThanMax_Revert() (gas: 70395)
OffRamp_commit:test_InvalidInterval_Revert() (gas: 66458)
OffRamp_commit:test_InvalidRootRevert() (gas: 65545)
OffRamp_commit:test_NoConfigWithOtherConfigPresent_Revert() (gas: 6671577)
OffRamp_commit:test_NoConfig_Revert() (gas: 6254907)
OffRamp_commit:test_OnlyGasPriceUpdates_Success() (gas: 113187)
OffRamp_commit:test_OnlyPriceUpdateStaleReport_Revert() (gas: 121714)
OffRamp_commit:test_OnlyTokenPriceUpdates_Success() (gas: 113186)
OffRamp_commit:test_PriceSequenceNumberCleared_Success() (gas: 355766)
OffRamp_commit:test_ReportAndPriceUpdate_Success() (gas: 164660)
OffRamp_commit:test_ReportOnlyRootSuccess_gas() (gas: 141660)
OffRamp_commit:test_RootAlreadyCommitted_Revert() (gas: 148891)
OffRamp_commit:test_RootWithRMNDisabled_success() (gas: 154318)
OffRamp_commit:test_SourceChainNotEnabled_Revert() (gas: 62025)
OffRamp_commit:test_StaleReportWithRoot_Success() (gas: 233095)
OffRamp_commit:test_UnauthorizedTransmitter_Revert() (gas: 125579)
OffRamp_commit:test_Unhealthy_Revert() (gas: 60822)
OffRamp_commit:test_ValidPriceUpdateThenStaleReportWithRoot_Success() (gas: 207441)
OffRamp_commit:test_ZeroEpochAndRound_Revert() (gas: 53889)
OffRamp_constructor:test_Constructor_Success() (gas: 6216896)
OffRamp_commit:test_CommitOnRampMismatch_Revert() (gas: 92834)
OffRamp_commit:test_FailedRMNVerification_Reverts() (gas: 63500)
OffRamp_commit:test_InvalidIntervalMinLargerThanMax_Revert() (gas: 70146)
OffRamp_commit:test_InvalidInterval_Revert() (gas: 66209)
OffRamp_commit:test_InvalidRootRevert() (gas: 65304)
OffRamp_commit:test_NoConfigWithOtherConfigPresent_Revert() (gas: 6669536)
OffRamp_commit:test_NoConfig_Revert() (gas: 6252866)
OffRamp_commit:test_OnlyGasPriceUpdates_Success() (gas: 112980)
OffRamp_commit:test_OnlyPriceUpdateStaleReport_Revert() (gas: 121333)
OffRamp_commit:test_OnlyTokenPriceUpdates_Success() (gas: 112979)
OffRamp_commit:test_PriceSequenceNumberCleared_Success() (gas: 355372)
OffRamp_commit:test_ReportAndPriceUpdate_Success() (gas: 164388)
OffRamp_commit:test_ReportOnlyRootSuccess_gas() (gas: 141416)
OffRamp_commit:test_RootAlreadyCommitted_Revert() (gas: 148426)
OffRamp_commit:test_RootWithRMNDisabled_success() (gas: 154111)
OffRamp_commit:test_SourceChainNotEnabled_Revert() (gas: 61771)
OffRamp_commit:test_StaleReportWithRoot_Success() (gas: 232626)
OffRamp_commit:test_UnauthorizedTransmitter_Revert() (gas: 125320)
OffRamp_commit:test_Unhealthy_Revert() (gas: 60572)
OffRamp_commit:test_ValidPriceUpdateThenStaleReportWithRoot_Success() (gas: 207009)
OffRamp_commit:test_ZeroEpochAndRound_Revert() (gas: 53689)
OffRamp_constructor:test_Constructor_Success() (gas: 6215096)
OffRamp_constructor:test_SourceChainSelector_Revert() (gas: 136627)
OffRamp_constructor:test_ZeroChainSelector_Revert() (gas: 103707)
OffRamp_constructor:test_ZeroNonceManager_Revert() (gas: 101534)
Expand All @@ -444,12 +444,12 @@ OffRamp_execute:test_IncorrectArrayType_Revert() (gas: 17639)
OffRamp_execute:test_LargeBatch_Success() (gas: 3406667)
OffRamp_execute:test_MultipleReportsWithPartialValidationFailures_Success() (gas: 371505)
OffRamp_execute:test_MultipleReports_Success() (gas: 299194)
OffRamp_execute:test_NoConfigWithOtherConfigPresent_Revert() (gas: 7079421)
OffRamp_execute:test_NoConfig_Revert() (gas: 6303869)
OffRamp_execute:test_NoConfigWithOtherConfigPresent_Revert() (gas: 7077621)
OffRamp_execute:test_NoConfig_Revert() (gas: 6302069)
OffRamp_execute:test_NonArray_Revert() (gas: 27643)
OffRamp_execute:test_SingleReport_Success() (gas: 175809)
OffRamp_execute:test_UnauthorizedTransmitter_Revert() (gas: 147805)
OffRamp_execute:test_WrongConfigWithSigners_Revert() (gas: 6971078)
OffRamp_execute:test_WrongConfigWithSigners_Revert() (gas: 6969278)
OffRamp_execute:test_ZeroReports_Revert() (gas: 17317)
OffRamp_executeSingleMessage:test_MessageSender_Revert() (gas: 18537)
OffRamp_executeSingleMessage:test_NonContractWithTokens_Success() (gas: 244193)
Expand Down Expand Up @@ -606,7 +606,7 @@ RMNHome_setDynamicConfig:test_setDynamicConfig_MinObserversTooHigh_reverts() (ga
RMNHome_setDynamicConfig:test_setDynamicConfig_OnlyOwner_reverts() (gas: 14099)
RMNHome_setDynamicConfig:test_setDynamicConfig_success() (gas: 104862)
RMNRemote_constructor:test_constructor_success() (gas: 8334)
RMNRemote_constructor:test_constructor_zeroChainSelector_reverts() (gas: 59245)
RMNRemote_constructor:test_constructor_zeroChainSelector_reverts() (gas: 59238)
RMNRemote_curse:test_curse_AlreadyCursed_duplicateSubject_reverts() (gas: 154479)
RMNRemote_curse:test_curse_calledByNonOwner_reverts() (gas: 18802)
RMNRemote_curse:test_curse_success() (gas: 149431)
Expand All @@ -618,13 +618,13 @@ RMNRemote_setConfig:test_setConfig_notEnoughSigners_reverts() (gas: 54212)
RMNRemote_uncurse:test_uncurse_NotCursed_duplicatedUncurseSubject_reverts() (gas: 51993)
RMNRemote_uncurse:test_uncurse_calledByNonOwner_reverts() (gas: 18772)
RMNRemote_uncurse:test_uncurse_success() (gas: 40171)
RMNRemote_verify_withConfigNotSet:test_verify_reverts() (gas: 13650)
RMNRemote_verify_withConfigSet:test_verify_InvalidSignature_reverts() (gas: 78585)
RMNRemote_verify_withConfigSet:test_verify_OutOfOrderSignatures_duplicateSignature_reverts() (gas: 76403)
RMNRemote_verify_withConfigSet:test_verify_OutOfOrderSignatures_not_sorted_reverts() (gas: 83466)
RMNRemote_verify_withConfigSet:test_verify_ThresholdNotMet_reverts() (gas: 321808)
RMNRemote_verify_withConfigSet:test_verify_UnexpectedSigner_reverts() (gas: 387816)
RMNRemote_verify_withConfigSet:test_verify_success() (gas: 68296)
RMNRemote_verify_withConfigNotSet:test_verify_reverts() (gas: 13578)
RMNRemote_verify_withConfigSet:test_verify_InvalidSignature_reverts() (gas: 96449)
RMNRemote_verify_withConfigSet:test_verify_OutOfOrderSignatures_duplicateSignature_reverts() (gas: 94267)
RMNRemote_verify_withConfigSet:test_verify_OutOfOrderSignatures_not_sorted_reverts() (gas: 101330)
RMNRemote_verify_withConfigSet:test_verify_ThresholdNotMet_reverts() (gas: 303846)
RMNRemote_verify_withConfigSet:test_verify_UnexpectedSigner_reverts() (gas: 427512)
RMNRemote_verify_withConfigSet:test_verify_success() (gas: 86159)
RateLimiter_constructor:test_Constructor_Success() (gas: 19806)
RateLimiter_consume:test_AggregateValueMaxCapacityExceeded_Revert() (gas: 16042)
RateLimiter_consume:test_AggregateValueRateLimitReached_Revert() (gas: 22435)
Expand Down
4 changes: 1 addition & 3 deletions contracts/src/v0.8/ccip/interfaces/IRMNRemote.sol
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,11 @@ interface IRMNRemote {
/// @param offRampAddress is not inferred by msg.sender, in case the call is made through ARMProxy
/// @param merkleRoots must be well formed, and is a representation of the CommitReport received from the oracles
/// @param signatures rmnNodes ECDSA sigs, only r & s, must be sorted in ascending order by signer address
/// @param rawVs rmnNodes ECDSA sigs, part v bitmap
/// @dev Will revert if verification fails
function verify(
address offRampAddress,
Internal.MerkleRoot[] memory merkleRoots,
Signature[] memory signatures,
uint256 rawVs
Signature[] memory signatures
) external view;

/// @notice gets the current set of cursed subjects
Expand Down
3 changes: 1 addition & 2 deletions contracts/src/v0.8/ccip/offRamp/OffRamp.sol
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,6 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
Internal.PriceUpdates priceUpdates; // Collection of gas and price updates to commit.
Internal.MerkleRoot[] merkleRoots; // Collection of merkle roots per source chain to commit.
IRMNRemote.Signature[] rmnSignatures; // RMN signatures on the merkle roots.
uint256 rmnRawVs; // Raw v values of the RMN signatures.
}

/// @dev Both receiverExecutionGasLimit and tokenGasOverrides are optional. To indicate no override, set the value
Expand Down Expand Up @@ -786,7 +785,7 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
// Verify RMN signatures
if (!dynamicConfig.isRMNVerificationDisabled) {
if (commitReport.merkleRoots.length > 0) {
i_rmnRemote.verify(address(this), commitReport.merkleRoots, commitReport.rmnSignatures, commitReport.rmnRawVs);
i_rmnRemote.verify(address(this), commitReport.merkleRoots, commitReport.rmnSignatures);
}
}

Expand Down
11 changes: 7 additions & 4 deletions contracts/src/v0.8/ccip/rmn/RMNRemote.sol
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ contract RMNRemote is OwnerIsCreator, ITypeAndVersion, IRMNRemote {
Config private s_config;
uint32 private s_configCount;

/// @dev RMN nodes only generate sigs with v=27; making this constant allows us to save gas by not transmitting v
/// @dev Any valid ECDSA sig (r, s, v) can be "flipped" into (r, s*, v*) without knowing the private key (where v=27 or 28 for secp256k1)
/// https://github.com/kadenzipfel/smart-contract-vulnerabilities/blob/master/vulnerabilities/signature-malleability.md
uint8 private constant ECDSA_RECOVERY_V = 27;

EnumerableSet.Bytes16Set private s_cursedSubjects;
mapping(address signer => bool exists) private s_signers; // for more gas efficient verify

Expand All @@ -90,8 +95,7 @@ contract RMNRemote is OwnerIsCreator, ITypeAndVersion, IRMNRemote {
function verify(
address offrampAddress,
Internal.MerkleRoot[] calldata merkleRoots,
Signature[] calldata signatures,
uint256 rawVs
Signature[] calldata signatures
) external view {
if (s_configCount == 0) {
revert ConfigNotSet();
Expand All @@ -115,8 +119,7 @@ contract RMNRemote is OwnerIsCreator, ITypeAndVersion, IRMNRemote {
address prevAddress;
address signerAddress;
for (uint256 i = 0; i < signatures.length; ++i) {
// The v value is bit-encoded into rawVs
signerAddress = ecrecover(digest, 27 + uint8(rawVs & 0x01 << i), signatures[i].r, signatures[i].s);
signerAddress = ecrecover(digest, ECDSA_RECOVERY_V, signatures[i].r, signatures[i].s);
if (signerAddress == address(0)) revert InvalidSignature();
if (prevAddress >= signerAddress) revert OutOfOrderSignatures();
if (!s_signers[signerAddress]) revert UnexpectedSigner();
Expand Down
8 changes: 2 additions & 6 deletions contracts/src/v0.8/ccip/test/e2e/End2End.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -168,12 +168,8 @@ contract E2E is OnRampSetup, OffRampSetup {
merkleRoot: merkleRoots[1]
});

OffRamp.CommitReport memory report = OffRamp.CommitReport({
priceUpdates: _getEmptyPriceUpdates(),
merkleRoots: roots,
rmnSignatures: rmnSignatures,
rmnRawVs: 0
});
OffRamp.CommitReport memory report =
OffRamp.CommitReport({priceUpdates: _getEmptyPriceUpdates(), merkleRoots: roots, rmnSignatures: rmnSignatures});

vm.resumeGasMetering();
_commit(report, ++s_latestSequenceNumber);
Expand Down
Loading

0 comments on commit ccd9956

Please sign in to comment.