diff --git a/contracts/src/v0.8/ccip/interfaces/IRMNV2.sol b/contracts/src/v0.8/ccip/interfaces/IRMNV2.sol index 213ac6de49d..0a42beb13b6 100644 --- a/contracts/src/v0.8/ccip/interfaces/IRMNV2.sol +++ b/contracts/src/v0.8/ccip/interfaces/IRMNV2.sol @@ -19,7 +19,8 @@ interface IRMNV2 { function verify( address offRampAddress, Internal.MerkleRoot[] memory merkleRoots, - Signature[] memory signatures + Signature[] memory signatures, + uint256 rawVs ) external view; /// @notice gets the current set of cursed subjects @@ -32,6 +33,6 @@ interface IRMNV2 { /// @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)). - /// @return bool true if the profived subject is cured *or* if there is an active global curse + /// @return bool true if the provided subject is cured *or* if there is an active global curse function isCursed(bytes16 subject) external view returns (bool); } diff --git a/contracts/src/v0.8/ccip/offRamp/OffRamp.sol b/contracts/src/v0.8/ccip/offRamp/OffRamp.sol index 478259975a2..5fcddb486e9 100644 --- a/contracts/src/v0.8/ccip/offRamp/OffRamp.sol +++ b/contracts/src/v0.8/ccip/offRamp/OffRamp.sol @@ -131,6 +131,7 @@ 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 IRMNV2.Signature[] rmnSignatures; // RMN signatures on the merkle roots + uint256 rmnRawVs; // Raw v values of the RMN signatures } struct GasLimitOverride { @@ -778,13 +779,13 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base { bytes calldata report, bytes32[] calldata rs, bytes32[] calldata ss, - bytes32 rawVs // signatures + bytes32 rawVs ) external { CommitReport memory commitReport = abi.decode(report, (CommitReport)); // Verify RMN signatures if (commitReport.merkleRoots.length > 0) { - i_rmn.verify(address(this), commitReport.merkleRoots, commitReport.rmnSignatures); + i_rmn.verify(address(this), commitReport.merkleRoots, commitReport.rmnSignatures, commitReport.rmnRawVs); } // Check if the report contains price updates diff --git a/contracts/src/v0.8/ccip/rmn/RMNRemote.sol b/contracts/src/v0.8/ccip/rmn/RMNRemote.sol index 6666f836721..3f610014ea1 100644 --- a/contracts/src/v0.8/ccip/rmn/RMNRemote.sol +++ b/contracts/src/v0.8/ccip/rmn/RMNRemote.sol @@ -34,7 +34,7 @@ contract RMNRemote is OwnerIsCreator, ITypeAndVersion, IRMNV2 { error UnexpectedSigner(); error ZeroValueNotAllowed(); - event ConfigSet(VersionedConfig versionedConfig); + event ConfigSet(uint32 indexed version, Config config); event Cursed(bytes16[] subjects); event Uncursed(bytes16[] subjects); @@ -52,12 +52,6 @@ contract RMNRemote is OwnerIsCreator, ITypeAndVersion, IRMNV2 { uint64 minSigners; // Threshold for the number of signers required to verify a report } - /// @dev the contract config + a version number - struct VersionedConfig { - uint32 version; // For tracking the version of the config - Config config; // The config - } - /// @dev part of the payload that RMN nodes sign: keccak256(abi.encode(RMN_V1_6_ANY2EVM_REPORT, report)) /// @dev this struct is only ever abi-encoded and hashed; it is never stored struct Report { @@ -69,15 +63,16 @@ contract RMNRemote is OwnerIsCreator, ITypeAndVersion, IRMNV2 { Internal.MerkleRoot[] merkleRoots; // The dest lane updates } - Config s_config; - uint32 s_configCount; - string public constant override typeAndVersion = "RMNRemote 1.6.0-dev"; uint64 internal immutable i_localChainSelector; /// @dev the set of cursed subjects bytes16[] private s_cursedSubjects; - /// @dev the index+1 is stored to easily distinguish b/t noncursed and cursed at the 0 index + + Config private s_config; + uint32 private s_configCount; + + /// @dev the index+1 is stored to easily distinguish b/t non-cursed and cursed at the 0 index mapping(bytes16 subject => uint256 indexPlusOne) private s_cursedSubjectsIndexPlusOne; mapping(address signer => bool exists) private s_signers; // for more gas efficient verify @@ -95,11 +90,13 @@ contract RMNRemote is OwnerIsCreator, ITypeAndVersion, IRMNV2 { function verify( address offrampAddress, Internal.MerkleRoot[] memory merkleRoots, - Signature[] memory signatures + Signature[] memory signatures, + uint256 rawVs ) external view { if (s_configCount == 0) { revert ConfigNotSet(); } + if (signatures.length < s_config.minSigners) revert ThresholdNotMet(); if (signatures.length < s_config.minSigners) revert ThresholdNotMet(); @@ -121,7 +118,8 @@ contract RMNRemote is OwnerIsCreator, ITypeAndVersion, IRMNV2 { address signerAddress; for (uint256 i = 0; i < signatures.length; ++i) { Signature memory sig = signatures[i]; - signerAddress = ecrecover(digest, 27, sig.r, sig.s); + // The v value is bit-encoded into rawVs + signerAddress = ecrecover(digest, 27 + uint8(rawVs & 0x01 << i), sig.r, sig.s); if (signerAddress == address(0)) revert InvalidSignature(); if (prevAddress >= signerAddress) revert OutOfOrderSignatures(); if (!s_signers[signerAddress]) revert UnexpectedSigner(); @@ -173,13 +171,14 @@ contract RMNRemote is OwnerIsCreator, ITypeAndVersion, IRMNV2 { s_config = newConfig; uint32 newConfigCount = ++s_configCount; - emit ConfigSet(VersionedConfig({version: newConfigCount, config: newConfig})); + emit ConfigSet(newConfigCount, newConfig); } - /// @notice Returns the current configuration of the contract + a version number - /// @return versionedConfig the current configuration + version - function getVersionedConfig() external view returns (VersionedConfig memory) { - return VersionedConfig({version: s_configCount, config: s_config}); + /// @notice Returns the current configuration of the contract and a version number + /// @return version the current configs version + /// @return config the current config + function getVersionedConfig() external view returns (uint32 version, Config memory config) { + return (s_configCount, s_config); } /// @notice Returns the chain selector configured at deployment time diff --git a/contracts/src/v0.8/ccip/test/e2e/MultiRampsEnd2End.t.sol b/contracts/src/v0.8/ccip/test/e2e/MultiRampsEnd2End.t.sol index 12967558cee..5f06a37e13a 100644 --- a/contracts/src/v0.8/ccip/test/e2e/MultiRampsEnd2End.t.sol +++ b/contracts/src/v0.8/ccip/test/e2e/MultiRampsEnd2End.t.sol @@ -168,8 +168,12 @@ contract MultiRampsE2E is OnRampSetup, OffRampSetup { merkleRoot: merkleRoots[1] }); - OffRamp.CommitReport memory report = - OffRamp.CommitReport({priceUpdates: _getEmptyPriceUpdates(), merkleRoots: roots, rmnSignatures: rmnSignatures}); + OffRamp.CommitReport memory report = OffRamp.CommitReport({ + priceUpdates: _getEmptyPriceUpdates(), + merkleRoots: roots, + rmnSignatures: rmnSignatures, + rmnRawVs: 0 + }); vm.resumeGasMetering(); _commit(report, ++s_latestSequenceNumber); @@ -269,6 +273,7 @@ contract MultiRampsE2E is OnRampSetup, OffRampSetup { Internal.Any2EVMTokenTransfer[] memory any2EVMTokenTransfer = new Internal.Any2EVMTokenTransfer[](message.tokenAmounts.length); + for (uint256 i = 0; i < msgEvent.tokenAmounts.length; ++i) { any2EVMTokenTransfer[i] = Internal.Any2EVMTokenTransfer({ sourcePoolAddress: abi.encode(msgEvent.tokenAmounts[i].sourcePoolAddress), diff --git a/contracts/src/v0.8/ccip/test/offRamp/OffRamp.t.sol b/contracts/src/v0.8/ccip/test/offRamp/OffRamp.t.sol index 1a31691c224..73adbb12c0d 100644 --- a/contracts/src/v0.8/ccip/test/offRamp/OffRamp.t.sol +++ b/contracts/src/v0.8/ccip/test/offRamp/OffRamp.t.sol @@ -995,7 +995,8 @@ contract OffRamp_executeSingleReport is OffRampSetup { return OffRamp.CommitReport({ priceUpdates: _getSingleTokenPriceUpdateStruct(s_sourceFeeToken, 4e18), merkleRoots: roots, - rmnSignatures: s_rmnSignatures + rmnSignatures: s_rmnSignatures, + rmnRawVs: 0 }); } } @@ -3243,7 +3244,8 @@ contract OffRamp_applySourceChainConfigUpdates is OffRampSetup { OffRamp.CommitReport({ priceUpdates: _getSingleTokenPriceUpdateStruct(s_sourceFeeToken, 4e18), merkleRoots: roots, - rmnSignatures: s_rmnSignatures + rmnSignatures: s_rmnSignatures, + rmnRawVs: 0 }), s_latestSequenceNumber ); @@ -3296,8 +3298,12 @@ contract OffRamp_commit is OffRampSetup { merkleRoot: root }); - OffRamp.CommitReport memory commitReport = - OffRamp.CommitReport({priceUpdates: _getEmptyPriceUpdates(), merkleRoots: roots, rmnSignatures: s_rmnSignatures}); + OffRamp.CommitReport memory commitReport = OffRamp.CommitReport({ + priceUpdates: _getEmptyPriceUpdates(), + merkleRoots: roots, + rmnSignatures: s_rmnSignatures, + rmnRawVs: 0 + }); vm.expectEmit(); emit OffRamp.CommitReportAccepted(commitReport.merkleRoots, commitReport.priceUpdates); @@ -3324,8 +3330,12 @@ contract OffRamp_commit is OffRampSetup { maxSeqNr: maxSeq, merkleRoot: "stale report 1" }); - OffRamp.CommitReport memory commitReport = - OffRamp.CommitReport({priceUpdates: _getEmptyPriceUpdates(), merkleRoots: roots, rmnSignatures: s_rmnSignatures}); + OffRamp.CommitReport memory commitReport = OffRamp.CommitReport({ + priceUpdates: _getEmptyPriceUpdates(), + merkleRoots: roots, + rmnSignatures: s_rmnSignatures, + rmnRawVs: 0 + }); vm.expectEmit(); emit OffRamp.CommitReportAccepted(commitReport.merkleRoots, commitReport.priceUpdates); @@ -3363,7 +3373,8 @@ contract OffRamp_commit is OffRampSetup { OffRamp.CommitReport memory commitReport = OffRamp.CommitReport({ priceUpdates: _getSingleTokenPriceUpdateStruct(s_sourceFeeToken, 4e18), merkleRoots: roots, - rmnSignatures: s_rmnSignatures + rmnSignatures: s_rmnSignatures, + rmnRawVs: 0 }); vm.expectEmit(); @@ -3385,7 +3396,8 @@ contract OffRamp_commit is OffRampSetup { OffRamp.CommitReport memory commitReport = OffRamp.CommitReport({ priceUpdates: _getSingleTokenPriceUpdateStruct(s_sourceFeeToken, 4e18), merkleRoots: roots, - rmnSignatures: s_rmnSignatures + rmnSignatures: s_rmnSignatures, + rmnRawVs: 0 }); vm.expectEmit(); @@ -3403,7 +3415,8 @@ contract OffRamp_commit is OffRampSetup { OffRamp.CommitReport memory commitReport = OffRamp.CommitReport({ priceUpdates: _getSingleTokenPriceUpdateStruct(s_sourceFeeToken, 4e18), merkleRoots: roots, - rmnSignatures: s_rmnSignatures + rmnSignatures: s_rmnSignatures, + rmnRawVs: 0 }); vm.expectEmit(); @@ -3455,7 +3468,8 @@ contract OffRamp_commit is OffRampSetup { OffRamp.CommitReport memory commitReport = OffRamp.CommitReport({ priceUpdates: _getSingleTokenPriceUpdateStruct(s_sourceFeeToken, tokenPrice1), merkleRoots: roots, - rmnSignatures: s_rmnSignatures + rmnSignatures: s_rmnSignatures, + rmnRawVs: 0 }); vm.expectEmit(); @@ -3565,8 +3579,12 @@ contract OffRamp_commit is OffRampSetup { onRampAddress: abi.encode(ON_RAMP_ADDRESS_1) }); - OffRamp.CommitReport memory commitReport = - OffRamp.CommitReport({priceUpdates: _getEmptyPriceUpdates(), merkleRoots: roots, rmnSignatures: s_rmnSignatures}); + OffRamp.CommitReport memory commitReport = OffRamp.CommitReport({ + priceUpdates: _getEmptyPriceUpdates(), + merkleRoots: roots, + rmnSignatures: s_rmnSignatures, + rmnRawVs: 0 + }); vm.expectRevert(abi.encodeWithSelector(OffRamp.CursedByRMN.selector, roots[0].sourceChainSelector)); _commit(commitReport, s_latestSequenceNumber); @@ -3581,8 +3599,12 @@ contract OffRamp_commit is OffRampSetup { maxSeqNr: 4, merkleRoot: bytes32(0) }); - OffRamp.CommitReport memory commitReport = - OffRamp.CommitReport({priceUpdates: _getEmptyPriceUpdates(), merkleRoots: roots, rmnSignatures: s_rmnSignatures}); + OffRamp.CommitReport memory commitReport = OffRamp.CommitReport({ + priceUpdates: _getEmptyPriceUpdates(), + merkleRoots: roots, + rmnSignatures: s_rmnSignatures, + rmnRawVs: 0 + }); vm.expectRevert(OffRamp.InvalidRoot.selector); _commit(commitReport, s_latestSequenceNumber); @@ -3597,8 +3619,12 @@ contract OffRamp_commit is OffRampSetup { maxSeqNr: 2, merkleRoot: bytes32(0) }); - OffRamp.CommitReport memory commitReport = - OffRamp.CommitReport({priceUpdates: _getEmptyPriceUpdates(), merkleRoots: roots, rmnSignatures: s_rmnSignatures}); + OffRamp.CommitReport memory commitReport = OffRamp.CommitReport({ + priceUpdates: _getEmptyPriceUpdates(), + merkleRoots: roots, + rmnSignatures: s_rmnSignatures, + rmnRawVs: 0 + }); vm.expectRevert( abi.encodeWithSelector( @@ -3618,8 +3644,12 @@ contract OffRamp_commit is OffRampSetup { maxSeqNr: 0, merkleRoot: bytes32(0) }); - OffRamp.CommitReport memory commitReport = - OffRamp.CommitReport({priceUpdates: _getEmptyPriceUpdates(), merkleRoots: roots, rmnSignatures: s_rmnSignatures}); + OffRamp.CommitReport memory commitReport = OffRamp.CommitReport({ + priceUpdates: _getEmptyPriceUpdates(), + merkleRoots: roots, + rmnSignatures: s_rmnSignatures, + rmnRawVs: 0 + }); vm.expectRevert( abi.encodeWithSelector( @@ -3634,7 +3664,8 @@ contract OffRamp_commit is OffRampSetup { OffRamp.CommitReport memory commitReport = OffRamp.CommitReport({ priceUpdates: _getSingleTokenPriceUpdateStruct(s_sourceFeeToken, 4e18), merkleRoots: roots, - rmnSignatures: s_rmnSignatures + rmnSignatures: s_rmnSignatures, + rmnRawVs: 0 }); vm.expectRevert(OffRamp.StaleCommitReport.selector); @@ -3646,7 +3677,8 @@ contract OffRamp_commit is OffRampSetup { OffRamp.CommitReport memory commitReport = OffRamp.CommitReport({ priceUpdates: _getSingleTokenPriceUpdateStruct(s_sourceFeeToken, 4e18), merkleRoots: roots, - rmnSignatures: s_rmnSignatures + rmnSignatures: s_rmnSignatures, + rmnRawVs: 0 }); vm.expectEmit(); @@ -3667,8 +3699,12 @@ contract OffRamp_commit is OffRampSetup { merkleRoot: "Only a single root" }); - OffRamp.CommitReport memory commitReport = - OffRamp.CommitReport({priceUpdates: _getEmptyPriceUpdates(), merkleRoots: roots, rmnSignatures: s_rmnSignatures}); + OffRamp.CommitReport memory commitReport = OffRamp.CommitReport({ + priceUpdates: _getEmptyPriceUpdates(), + merkleRoots: roots, + rmnSignatures: s_rmnSignatures, + rmnRawVs: 0 + }); vm.expectRevert(abi.encodeWithSelector(OffRamp.SourceChainNotEnabled.selector, 0)); _commit(commitReport, s_latestSequenceNumber); @@ -3683,8 +3719,12 @@ contract OffRamp_commit is OffRampSetup { maxSeqNr: 2, merkleRoot: "Only a single root" }); - OffRamp.CommitReport memory commitReport = - OffRamp.CommitReport({priceUpdates: _getEmptyPriceUpdates(), merkleRoots: roots, rmnSignatures: s_rmnSignatures}); + OffRamp.CommitReport memory commitReport = OffRamp.CommitReport({ + priceUpdates: _getEmptyPriceUpdates(), + merkleRoots: roots, + rmnSignatures: s_rmnSignatures, + rmnRawVs: 0 + }); _commit(commitReport, s_latestSequenceNumber); commitReport.merkleRoots[0].minSeqNr = 3; @@ -3718,7 +3758,8 @@ contract OffRamp_commit is OffRampSetup { return OffRamp.CommitReport({ priceUpdates: _getSingleTokenPriceUpdateStruct(s_sourceFeeToken, 4e18), merkleRoots: roots, - rmnSignatures: s_rmnSignatures + rmnSignatures: s_rmnSignatures, + rmnRawVs: 0 }); } } diff --git a/contracts/src/v0.8/ccip/test/rmn/RMNRemote.t.sol b/contracts/src/v0.8/ccip/test/rmn/RMNRemote.t.sol index e978e67a918..a9b5ae22bfb 100644 --- a/contracts/src/v0.8/ccip/test/rmn/RMNRemote.t.sol +++ b/contracts/src/v0.8/ccip/test/rmn/RMNRemote.t.sol @@ -19,54 +19,67 @@ contract RMNRemote_constructor is RMNRemoteSetup { contract RMNRemote_setConfig is RMNRemoteSetup { function test_setConfig_minSignersIs0_success() public { - RMNRemote.Config memory config = - RMNRemote.Config({rmnHomeContractConfigDigest: _randomBytes32(), signers: s_signers, minSigners: 0}); - // TODO event test - s_rmnRemote.setConfig(config); - RMNRemote.VersionedConfig memory versionedConfig = s_rmnRemote.getVersionedConfig(); - assertEq(versionedConfig.config.minSigners, 0); - } - - function test_setConfig_versionIncreases_success() public { + // Initially there is no config, the version is 0 + uint32 currentConfigVersion = 0; RMNRemote.Config memory config = RMNRemote.Config({rmnHomeContractConfigDigest: _randomBytes32(), signers: s_signers, minSigners: 0}); vm.expectEmit(); - emit RMNRemote.ConfigSet(RMNRemote.VersionedConfig({version: 1, config: config})); - s_rmnRemote.setConfig(config); - assertEq(s_rmnRemote.getVersionedConfig().version, 1); + emit RMNRemote.ConfigSet(++currentConfigVersion, config); - vm.expectEmit(); - emit RMNRemote.ConfigSet(RMNRemote.VersionedConfig({version: 2, config: config})); s_rmnRemote.setConfig(config); - assertEq(s_rmnRemote.getVersionedConfig().version, 2); + (uint32 version, RMNRemote.Config memory gotConfig) = s_rmnRemote.getVersionedConfig(); + assertEq(gotConfig.minSigners, 0); + assertEq(version, currentConfigVersion); + + // A new config should increment the version vm.expectEmit(); - emit RMNRemote.ConfigSet(RMNRemote.VersionedConfig({version: 3, config: config})); + emit RMNRemote.ConfigSet(++currentConfigVersion, config); + s_rmnRemote.setConfig(config); - assertEq(s_rmnRemote.getVersionedConfig().version, 3); } function test_setConfig_addSigner_removeSigner_success() public { + uint32 currentConfigVersion = 0; uint256 numSigners = s_signers.length; RMNRemote.Config memory config = RMNRemote.Config({rmnHomeContractConfigDigest: _randomBytes32(), signers: s_signers, minSigners: 0}); + + vm.expectEmit(); + emit RMNRemote.ConfigSet(++currentConfigVersion, config); + s_rmnRemote.setConfig(config); - RMNRemote.VersionedConfig memory versionedConfig = s_rmnRemote.getVersionedConfig(); + // add a signer - s_signers.push(RMNRemote.Signer({onchainPublicKey: address(1), nodeIndex: uint64(numSigners)})); + address newSigner = makeAddr("new signer"); + s_signers.push(RMNRemote.Signer({onchainPublicKey: newSigner, nodeIndex: uint64(numSigners)})); config = RMNRemote.Config({rmnHomeContractConfigDigest: _randomBytes32(), signers: s_signers, minSigners: 0}); + + vm.expectEmit(); + emit RMNRemote.ConfigSet(++currentConfigVersion, config); + s_rmnRemote.setConfig(config); - versionedConfig = s_rmnRemote.getVersionedConfig(); - assertEq(versionedConfig.config.signers.length, numSigners + 1); - assertEq(versionedConfig.config.signers[numSigners].onchainPublicKey, address(1)); - // remove signers + + (uint32 version, RMNRemote.Config memory gotConfig) = s_rmnRemote.getVersionedConfig(); + assertEq(gotConfig.signers.length, s_signers.length); + assertEq(gotConfig.signers[numSigners].onchainPublicKey, newSigner); + assertEq(gotConfig.signers[numSigners].nodeIndex, uint64(numSigners)); + assertEq(version, currentConfigVersion); + + // remove two signers s_signers.pop(); s_signers.pop(); config = RMNRemote.Config({rmnHomeContractConfigDigest: _randomBytes32(), signers: s_signers, minSigners: 0}); + + vm.expectEmit(); + emit RMNRemote.ConfigSet(++currentConfigVersion, config); + s_rmnRemote.setConfig(config); - versionedConfig = s_rmnRemote.getVersionedConfig(); - assertEq(versionedConfig.config.signers.length, numSigners - 1); + + (version, gotConfig) = s_rmnRemote.getVersionedConfig(); + assertEq(gotConfig.signers.length, s_signers.length); + assertEq(version, currentConfigVersion); } function test_setConfig_invalidSignerOrder_reverts() public { @@ -105,24 +118,25 @@ contract RMNRemote_verify_withConfigNotSet is RMNRemoteSetup { IRMNV2.Signature[] memory signatures = new IRMNV2.Signature[](0); vm.expectRevert(RMNRemote.ConfigNotSet.selector); - s_rmnRemote.verify(OFF_RAMP_ADDRESS, merkleRoots, signatures); + s_rmnRemote.verify(OFF_RAMP_ADDRESS, merkleRoots, signatures, 0); } } contract RMNRemote_verify_withConfigSet is RMNRemoteSetup { Internal.MerkleRoot[] s_merkleRoots; IRMNV2.Signature[] s_signatures; + uint256 internal s_v; function setUp() public override { super.setUp(); RMNRemote.Config memory config = RMNRemote.Config({rmnHomeContractConfigDigest: _randomBytes32(), signers: s_signers, minSigners: 2}); s_rmnRemote.setConfig(config); - _generatePayloadAndSigs(2, 2, s_merkleRoots, s_signatures); + s_v = _generatePayloadAndSigs(2, 2, s_merkleRoots, s_signatures); } function test_verify_success() public view { - s_rmnRemote.verify(OFF_RAMP_ADDRESS, s_merkleRoots, s_signatures); + s_rmnRemote.verify(OFF_RAMP_ADDRESS, s_merkleRoots, s_signatures, s_v); } function test_verify_minSignersIsZero_success() public { @@ -134,20 +148,20 @@ contract RMNRemote_verify_withConfigSet is RMNRemoteSetup { vm.stopPrank(); vm.prank(OFF_RAMP_ADDRESS); - s_rmnRemote.verify(OFF_RAMP_ADDRESS, s_merkleRoots, new IRMNV2.Signature[](0)); + s_rmnRemote.verify(OFF_RAMP_ADDRESS, s_merkleRoots, new IRMNV2.Signature[](0), s_v); } - function test_verify_invalidSig_reverts() public { + function test_verify_InvalidSignature_reverts() public { IRMNV2.Signature memory sig = s_signatures[s_signatures.length - 1]; sig.r = _randomBytes32(); s_signatures.pop(); s_signatures.push(sig); vm.expectRevert(RMNRemote.InvalidSignature.selector); - s_rmnRemote.verify(OFF_RAMP_ADDRESS, s_merkleRoots, s_signatures); + s_rmnRemote.verify(OFF_RAMP_ADDRESS, s_merkleRoots, s_signatures, s_v); } - function test_verify_outOfOrderSig_reverts() public { + function test_verify_OutOfOrderSignatures_not_sorted_reverts() public { IRMNV2.Signature memory sig1 = s_signatures[s_signatures.length - 1]; s_signatures.pop(); IRMNV2.Signature memory sig2 = s_signatures[s_signatures.length - 1]; @@ -156,105 +170,108 @@ contract RMNRemote_verify_withConfigSet is RMNRemoteSetup { s_signatures.push(sig2); vm.expectRevert(RMNRemote.OutOfOrderSignatures.selector); - s_rmnRemote.verify(OFF_RAMP_ADDRESS, s_merkleRoots, s_signatures); + s_rmnRemote.verify(OFF_RAMP_ADDRESS, s_merkleRoots, s_signatures, s_v); } - function test_verify_duplicateSignature_reverts() public { + function test_verify_OutOfOrderSignatures_duplicateSignature_reverts() public { IRMNV2.Signature memory sig = s_signatures[s_signatures.length - 2]; s_signatures.pop(); s_signatures.push(sig); vm.expectRevert(RMNRemote.OutOfOrderSignatures.selector); - s_rmnRemote.verify(OFF_RAMP_ADDRESS, s_merkleRoots, s_signatures); + s_rmnRemote.verify(OFF_RAMP_ADDRESS, s_merkleRoots, s_signatures, s_v); } - function test_verify_unknownSigner_reverts() public { + function test_verify_UnexpectedSigner_reverts() public { _setupSigners(2); // create 2 new signers that aren't configured on RMNRemote - _generatePayloadAndSigs(2, 2, s_merkleRoots, s_signatures); + uint256 v = _generatePayloadAndSigs(2, 2, s_merkleRoots, s_signatures); vm.expectRevert(RMNRemote.UnexpectedSigner.selector); - s_rmnRemote.verify(OFF_RAMP_ADDRESS, s_merkleRoots, s_signatures); + s_rmnRemote.verify(OFF_RAMP_ADDRESS, s_merkleRoots, s_signatures, v); } - function test_verify_insufficientSignatures_reverts() public { - _generatePayloadAndSigs(2, 1, s_merkleRoots, s_signatures); // 1 sig requested, but 2 required + function test_verify_ThresholdNotMet_reverts() public { + uint256 v = _generatePayloadAndSigs(2, 1, s_merkleRoots, s_signatures); // 1 sig requested, but 2 required vm.expectRevert(RMNRemote.ThresholdNotMet.selector); - s_rmnRemote.verify(OFF_RAMP_ADDRESS, s_merkleRoots, s_signatures); + s_rmnRemote.verify(OFF_RAMP_ADDRESS, s_merkleRoots, s_signatures, v); } } contract RMNRemote_curse is RMNRemoteSetup { - bytes16 constant subj1 = bytes16(keccak256("subject 1")); - bytes16 constant subj2 = bytes16(keccak256("subject 2")); - bytes16 constant subj3 = bytes16(keccak256("subject 3")); - bytes16[] public s_subjects; + bytes16 internal constant curseSubj1 = bytes16(keccak256("subject 1")); + bytes16 internal constant curseSubj2 = bytes16(keccak256("subject 2")); + bytes16[] public s_curseSubjects; function setUp() public override { super.setUp(); - s_subjects.push(subj1); - s_subjects.push(subj2); + s_curseSubjects.push(curseSubj1); + s_curseSubjects.push(curseSubj2); } function test_curse_success() public { vm.expectEmit(); - emit RMNRemote.Cursed(s_subjects); - s_rmnRemote.curse(s_subjects); - assertEq(abi.encode(s_rmnRemote.getCursedSubjects()), abi.encode(s_subjects)); - assertTrue(s_rmnRemote.isCursed(subj1)); - assertTrue(s_rmnRemote.isCursed(subj2)); - assertFalse(s_rmnRemote.isCursed(subj3)); + emit RMNRemote.Cursed(s_curseSubjects); + + s_rmnRemote.curse(s_curseSubjects); + + assertEq(abi.encode(s_rmnRemote.getCursedSubjects()), abi.encode(s_curseSubjects)); + assertTrue(s_rmnRemote.isCursed(curseSubj1)); + assertTrue(s_rmnRemote.isCursed(curseSubj2)); + // Should not have cursed a random subject + assertFalse(s_rmnRemote.isCursed(bytes16(keccak256("subject 3")))); } - function test_curse_duplicateSubject_reverts() public { - s_subjects.push(subj1); + function test_curse_AlreadyCursed_duplicateSubject_reverts() public { + s_curseSubjects.push(curseSubj1); - vm.expectRevert(abi.encodeWithSelector(RMNRemote.AlreadyCursed.selector, subj1)); - s_rmnRemote.curse(s_subjects); + vm.expectRevert(abi.encodeWithSelector(RMNRemote.AlreadyCursed.selector, curseSubj1)); + s_rmnRemote.curse(s_curseSubjects); } function test_curse_calledByNonOwner_reverts() public { vm.expectRevert("Only callable by owner"); vm.stopPrank(); vm.prank(STRANGER); - s_rmnRemote.curse(s_subjects); + s_rmnRemote.curse(s_curseSubjects); } } contract RMNRemote_uncurse is RMNRemoteSetup { - bytes16 constant subj1 = bytes16(keccak256("subject 1")); - bytes16 constant subj2 = bytes16(keccak256("subject 2")); - bytes16 constant subj3 = bytes16(keccak256("subject 3")); - bytes16[] public s_subjects; + bytes16 private constant curseSubj1 = bytes16(keccak256("subject 1")); + bytes16 private constant curseSubj2 = bytes16(keccak256("subject 2")); + bytes16[] public s_curseSubjects; function setUp() public override { super.setUp(); - s_subjects.push(subj1); - s_subjects.push(subj2); - s_rmnRemote.curse(s_subjects); + s_curseSubjects.push(curseSubj1); + s_curseSubjects.push(curseSubj2); + s_rmnRemote.curse(s_curseSubjects); } function test_uncurse_success() public { vm.expectEmit(); - emit RMNRemote.Uncursed(s_subjects); - s_rmnRemote.uncurse(s_subjects); + emit RMNRemote.Uncursed(s_curseSubjects); + + s_rmnRemote.uncurse(s_curseSubjects); + assertEq(s_rmnRemote.getCursedSubjects().length, 0); - assertFalse(s_rmnRemote.isCursed(subj1)); - assertFalse(s_rmnRemote.isCursed(subj2)); + assertFalse(s_rmnRemote.isCursed(curseSubj1)); + assertFalse(s_rmnRemote.isCursed(curseSubj2)); } - function test_uncurse_duplicateSubject_reverts() public { - s_subjects.push(subj1); + function test_uncurse_NotCursed_duplicatedUncurseSubject_reverts() public { + s_curseSubjects.push(curseSubj1); - vm.expectRevert(abi.encodeWithSelector(RMNRemote.NotCursed.selector, subj1)); - s_rmnRemote.uncurse(s_subjects); + vm.expectRevert(abi.encodeWithSelector(RMNRemote.NotCursed.selector, curseSubj1)); + s_rmnRemote.uncurse(s_curseSubjects); } function test_uncurse_calledByNonOwner_reverts() public { vm.expectRevert("Only callable by owner"); vm.stopPrank(); vm.prank(STRANGER); - s_rmnRemote.uncurse(s_subjects); + s_rmnRemote.uncurse(s_curseSubjects); } } diff --git a/contracts/src/v0.8/ccip/test/rmn/RMNRemoteSetup.t.sol b/contracts/src/v0.8/ccip/test/rmn/RMNRemoteSetup.t.sol index 394bc9425e4..47e2b1ccdfc 100644 --- a/contracts/src/v0.8/ccip/test/rmn/RMNRemoteSetup.t.sol +++ b/contracts/src/v0.8/ccip/test/rmn/RMNRemoteSetup.t.sol @@ -28,6 +28,14 @@ contract RMNRemoteSetup is BaseTest { /// @dev signers do not have to be in order when configured, but they do when generating signatures /// rather than sort signers every time, we do it once here and store the sorted list function _setupSigners(uint256 numSigners) internal { + // remove any existing config + while (s_signerWallets.length > 0) { + s_signerWallets.pop(); + } + while (s_signers.length > 0) { + s_signers.pop(); + } + for (uint256 i = 0; i < numSigners; i++) { s_signerWallets.push(vm.createWallet(_randomNum())); } @@ -41,18 +49,12 @@ contract RMNRemoteSetup is BaseTest { /// @notice generates n merkleRoots and matching valid signatures and populates them into /// the provided storage arrays - /// @dev if tests are running out of gas, try reducing the number of sigs generated - /// @dev important note here that ONLY v=27 sigs are valid in the RMN contract. Because there is - /// very little control over how these sigs are generated in foundry, we have to "get lucky" with the - /// payload / signature combination. Therefore, we generate a payload and sigs together here in 1 function. - /// If we can't generate valid (v=27 for all signers) sigs we re-generate the payload and try again. - /// Warning: this is very annoying and clunky code. Tweak at your own risk. function _generatePayloadAndSigs( uint256 numUpdates, uint256 numSigs, Internal.MerkleRoot[] storage merkleRoots, IRMNV2.Signature[] storage signatures - ) internal { + ) internal returns (uint256 aggV) { require(numUpdates > 0, "need at least 1 dest lane update"); require(numSigs <= s_signerWallets.length, "cannot generate more sigs than signers"); @@ -60,36 +62,25 @@ contract RMNRemoteSetup is BaseTest { for (uint256 i = 0; i < merkleRoots.length; i++) { merkleRoots.pop(); } - for (uint256 i = 0; i < signatures.length; i++) { - signatures.pop(); - } for (uint256 i = 0; i < numUpdates; i++) { merkleRoots.push(_generateRandomDestLaneUpdate()); } - while (true) { - bool allSigsValid = true; - for (uint256 i = 0; i < numSigs; i++) { - (bool isValid, IRMNV2.Signature memory sig) = _signDestLaneUpdate(merkleRoots, s_signerWallets[i]); - signatures.push(sig); - allSigsValid = allSigsValid && isValid; - if (!allSigsValid) { - break; - } - } - // if all sigs are valid, don't change anything!! - if (allSigsValid) { - break; - } - // try again with a different payload if not all sigs are valid - merkleRoots.pop(); - merkleRoots.push(_generateRandomDestLaneUpdate()); - // clear existing sigs - while (signatures.length > 0) { - signatures.pop(); + uint256 sigLength = signatures.length; + for (uint256 i = 0; i < sigLength; i++) { + signatures.pop(); + } + + for (uint256 i = 0; i < numSigs; i++) { + (uint8 v, IRMNV2.Signature memory sig) = _signDestLaneUpdate(merkleRoots, s_signerWallets[i]); + signatures.push(sig); + if (v == 28) { + aggV += 1 << i; } } + + return aggV; } /// @notice generates a random dest lane update @@ -106,12 +97,13 @@ contract RMNRemoteSetup is BaseTest { } /// @notice signs the provided payload with the provided wallet - /// @return valid true only if the v component of the signature == 27 + /// @return sigV v, either 27 of 28 /// @return sig the signature function _signDestLaneUpdate( Internal.MerkleRoot[] memory merkleRoots, Vm.Wallet memory wallet - ) private returns (bool valid, IRMNV2.Signature memory) { + ) private returns (uint8 sigV, IRMNV2.Signature memory) { + (, RMNRemote.Config memory config) = s_rmnRemote.getVersionedConfig(); bytes32 digest = keccak256( abi.encode( RMN_V1_6_ANY2EVM_REPORT, @@ -120,13 +112,13 @@ contract RMNRemoteSetup is BaseTest { destChainSelector: s_rmnRemote.getLocalChainSelector(), rmnRemoteContractAddress: address(s_rmnRemote), offrampAddress: OFF_RAMP_ADDRESS, - rmnHomeContractConfigDigest: s_rmnRemote.getVersionedConfig().config.rmnHomeContractConfigDigest, + rmnHomeContractConfigDigest: config.rmnHomeContractConfigDigest, merkleRoots: merkleRoots }) ) ); (uint8 v, bytes32 r, bytes32 s) = vm.sign(wallet, digest); - return (v == 27, IRMNV2.Signature({r: r, s: s})); // only v==27 sigs are valid in RMN contract + return (v, IRMNV2.Signature({r: r, s: s})); } /// @notice bubble sort on a storage array of wallets