RMNRemote: curse admin management + blessing removal [CCIP-10871]

chains/evm/contracts/rmn/RMNRemote.sol

@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: BUSL-1.1
 pragma solidity ^0.8.24;
 
-import {IRMN} from "../interfaces/IRMN.sol";
 import {IRMNRemote} from "../interfaces/IRMNRemote.sol";
 import {ITypeAndVersion} from "@chainlink/contracts/src/v0.8/shared/interfaces/ITypeAndVersion.sol";
 
@@ -15,11 +14,9 @@ import {EnumerableSet} from "@chainlink/contracts/src/v0.8/shared/enumerable/Enu
 bytes16 constant GLOBAL_CURSE_SUBJECT = 0x01000000000000000000000000000001;
 
 /// @notice This contract supports verification of RMN reports for any Any2EVM OffRamp.
-/// @dev This contract implements both the new IRMNRemote interface and the legacy IRMN interface. This is to allow for
-/// a seamless migration from the legacy RMN contract to this one. The only function that has been dropped in the newer
-/// interface is `isBlessed`. For the `isBlessed` function, this contract relays the call to the legacy RMN contract.
-contract RMNRemote is Ownable2StepMsgSender, ITypeAndVersion, IRMNRemote, IRMN {
+contract RMNRemote is Ownable2StepMsgSender, ITypeAndVersion, IRMNRemote {
   using EnumerableSet for EnumerableSet.Bytes16Set;
+  using EnumerableSet for EnumerableSet.AddressSet;
 
   error AlreadyCursed(bytes16 subject);
   error ConfigNotSet();
@@ -28,13 +25,15 @@ contract RMNRemote is Ownable2StepMsgSender, ITypeAndVersion, IRMNRemote, IRMN {
   error InvalidSignerOrder();
   error NotEnoughSigners();
   error NotCursed(bytes16 subject);
+  error OnlyOwnerOrCurseAdmin(address caller);
   error OutOfOrderSignatures();
   error ThresholdNotMet();
   error UnexpectedSigner();
   error ZeroValueNotAllowed();
-  error IsBlessedNotAvailable();
 
   event ConfigSet(uint32 indexed version, Config config);
+  event CurseAdminAdded(address indexed curseAdmin);
+  event CurseAdminRemoved(address indexed curseAdmin);
   event Cursed(bytes16[] subjects);
   event Uncursed(bytes16[] subjects);
 
@@ -67,7 +66,6 @@ contract RMNRemote is Ownable2StepMsgSender, ITypeAndVersion, IRMNRemote, IRMN {
 
   string public constant override typeAndVersion = "RMNRemote 1.6.0";
   uint64 internal immutable i_localChainSelector;
-  IRMN internal immutable i_legacyRMN;
 
   Config private s_config;
   uint32 private s_configCount;
@@ -78,17 +76,22 @@ contract RMNRemote is Ownable2StepMsgSender, ITypeAndVersion, IRMNRemote, IRMN {
   uint8 private constant ECDSA_RECOVERY_V = 27;
 
   EnumerableSet.Bytes16Set private s_cursedSubjects;
+  EnumerableSet.AddressSet private s_curseAdmins;
   mapping(address signer => bool exists) private s_signers; // for more gas efficient verify.
 
   /// @param localChainSelector the chain selector of the chain this contract is deployed to.
   constructor(
-    uint64 localChainSelector,
-    IRMN legacyRMN
+    uint64 localChainSelector
   ) {
     if (localChainSelector == 0) revert ZeroValueNotAllowed();
     i_localChainSelector = localChainSelector;
+  }
 
-    i_legacyRMN = legacyRMN;
+  modifier onlyOwnerOrCurseAdmin() {
+    if (msg.sender != owner() && !s_curseAdmins.contains(msg.sender)) {
+      revert OnlyOwnerOrCurseAdmin(msg.sender);
+    }
+    _;
   }
 
   // ================================================================
@@ -213,7 +216,7 @@ contract RMNRemote is Ownable2StepMsgSender, ITypeAndVersion, IRMNRemote, IRMN {
   /// @dev reverts if any of the subjects are already cursed or if there is a duplicate.
   function curse(
     bytes16[] memory subjects
-  ) public onlyOwner {
+  ) public onlyOwnerOrCurseAdmin {
     for (uint256 i = 0; i < subjects.length; ++i) {
       if (!s_cursedSubjects.add(subjects[i])) {
         revert AlreadyCursed(subjects[i]);
@@ -251,8 +254,39 @@ contract RMNRemote is Ownable2StepMsgSender, ITypeAndVersion, IRMNRemote, IRMN {
     return s_cursedSubjects.values();
   }
 
+  /// @notice Add or remove curse admins.
+  /// @param removes Addresses to remove from the curse admin set.
+  /// @param adds Addresses to add to the curse admin set.
+  function applyCurseAdminUpdates(
+    address[] calldata removes,
+    address[] calldata adds
+  ) external onlyOwner {
+    for (uint256 i = 0; i < removes.length; ++i) {
+      if (s_curseAdmins.remove(removes[i])) {
+        emit CurseAdminRemoved(removes[i]);
+      }
+    }
+    for (uint256 i = 0; i < adds.length; ++i) {
+      if (s_curseAdmins.add(adds[i])) {
+        emit CurseAdminAdded(adds[i]);
+      }
+    }
+  }
+
+  /// @notice Returns all current curse admins.
+  function getCurseAdmins() external view returns (address[] memory) {
+    return s_curseAdmins.values();
+  }
+
+  /// @notice Returns true if the given address is a curse admin.
+  function isCurseAdmin(
+    address curseAdmin
+  ) external view returns (bool) {
+    return s_curseAdmins.contains(curseAdmin);
+  }
+
   /// @inheritdoc IRMNRemote
-  function isCursed() external view override(IRMN, IRMNRemote) returns (bool) {
+  function isCursed() external view override returns (bool) {
     // There are zero curses under normal circumstances, which means it's cheaper to check for the absence of curses.
     // than to check the subject list for the global curse subject.
     if (s_cursedSubjects.length() == 0) {
@@ -264,28 +298,12 @@ contract RMNRemote is Ownable2StepMsgSender, ITypeAndVersion, IRMNRemote, IRMN {
   /// @inheritdoc IRMNRemote
   function isCursed(
     bytes16 subject
-  ) external view override(IRMN, IRMNRemote) returns (bool) {
+  ) external view override returns (bool) {
     // There are zero curses under normal circumstances, which means it's cheaper to check for the absence of curses.
     // than to check the subject list twice, as we have to check for both the given and global curse subjects.
     if (s_cursedSubjects.length() == 0) {
       return false;
     }
     return s_cursedSubjects.contains(subject) || s_cursedSubjects.contains(GLOBAL_CURSE_SUBJECT);
   }
-
-  // ================================================================
-  // │                     Legacy pass through                      │
-  // ================================================================
-
-  /// @inheritdoc IRMN
-  /// @dev This function is only expected to be used for messages from CCIP versions below 1.6.
-  function isBlessed(
-    TaggedRoot calldata taggedRoot
-  ) external view returns (bool) {
-    if (i_legacyRMN == IRMN(address(0))) {
-      revert IsBlessedNotAvailable();
-    }
-
-    return i_legacyRMN.isBlessed(taggedRoot);
-  }
 }

chains/evm/contracts/test/rmn/RMNProxy/RMNProxy.isCursed.t.sol

@@ -12,7 +12,7 @@ contract RMNProxy_isCursed is RMNProxyTestSetup {
 
   function setUp() public virtual override {
     super.setUp();
-    s_mockRMNRemote = new RMNRemote(1, IRMN(address(0)));
+    s_mockRMNRemote = new RMNRemote(1);
     s_rmnProxy = new RMNProxy(address(s_mockRMNRemote));
   }
 

chains/evm/contracts/test/rmn/RMNRemote/RMNRemote.curse.t.sol

@@ -27,9 +27,117 @@ contract RMNRemote_curse is RMNRemoteSetup {
   }
 
   function test_RevertWhen_curse_calledByNonOwner() public {
+    vm.expectRevert(abi.encodeWithSelector(RMNRemote.OnlyOwnerOrCurseAdmin.selector, STRANGER));
+    vm.stopPrank();
+    vm.prank(STRANGER);
+    s_rmnRemote.curse(s_curseSubjects);
+  }
+}
+
+contract RMNRemote_curseAdmin is RMNRemoteSetup {
+  address internal s_curseAdmin = makeAddr("curseAdmin");
+
+  function setUp() public override {
+    super.setUp();
+    address[] memory adds = new address[](1);
+    adds[0] = s_curseAdmin;
+    s_rmnRemote.applyCurseAdminUpdates(new address[](0), adds);
+  }
+
+  function test_curse_byCurseAdmin_Success() public {
+    vm.expectEmit();
+    emit RMNRemote.Cursed(s_curseSubjects);
+
+    vm.stopPrank();
+    vm.prank(s_curseAdmin);
+    s_rmnRemote.curse(s_curseSubjects);
+
+    assertTrue(s_rmnRemote.isCursed(CURSE_SUBJ_1));
+    assertTrue(s_rmnRemote.isCursed(CURSE_SUBJ_2));
+  }
+
+  function test_applyCurseAdminUpdates_addsAndRemoves() public {
+    address newAdmin = makeAddr("newAdmin");
+    address[] memory adds = new address[](1);
+    adds[0] = newAdmin;
+
+    vm.expectEmit();
+    emit RMNRemote.CurseAdminAdded(newAdmin);
+    s_rmnRemote.applyCurseAdminUpdates(new address[](0), adds);
+    assertTrue(s_rmnRemote.isCurseAdmin(newAdmin));
+
+    address[] memory adminList = s_rmnRemote.getCurseAdmins();
+    assertEq(adminList.length, 2); // s_curseAdmin (from setUp) + newAdmin
+
+    // Duplicate add is idempotent (no event emitted)
+    vm.recordLogs();
+    s_rmnRemote.applyCurseAdminUpdates(new address[](0), adds);
+    assertEq(vm.getRecordedLogs().length, 0);
+
+    // Remove
+    address[] memory toRemove = new address[](1);
+    toRemove[0] = newAdmin;
+    vm.expectEmit();
+    emit RMNRemote.CurseAdminRemoved(newAdmin);
+    s_rmnRemote.applyCurseAdminUpdates(toRemove, new address[](0));
+    assertFalse(s_rmnRemote.isCurseAdmin(newAdmin));
+
+    // Remove of non-member is idempotent (no event emitted)
+    vm.recordLogs();
+    s_rmnRemote.applyCurseAdminUpdates(toRemove, new address[](0));
+    assertEq(vm.getRecordedLogs().length, 0);
+  }
+
+  function test_RevertWhen_applyCurseAdminUpdates_calledByNonOwner() public {
     vm.expectRevert(Ownable2Step.OnlyCallableByOwner.selector);
     vm.stopPrank();
     vm.prank(STRANGER);
+    s_rmnRemote.applyCurseAdminUpdates(new address[](0), new address[](0));
+  }
+
+  function test_curse_byOwner_SuccessWhenCurseAdminsExist() public {
+    vm.expectEmit();
+    emit RMNRemote.Cursed(s_curseSubjects);
+    s_rmnRemote.curse(s_curseSubjects);
+
+    assertTrue(s_rmnRemote.isCursed(CURSE_SUBJ_1));
+    assertTrue(s_rmnRemote.isCursed(CURSE_SUBJ_2));
+  }
+
+  function test_curse_byNewlyAddedCurseAdmin_Success() public {
+    address newAdmin = makeAddr("newAdmin");
+    address[] memory adds = new address[](1);
+    adds[0] = newAdmin;
+    s_rmnRemote.applyCurseAdminUpdates(new address[](0), adds);
+
+    vm.expectEmit();
+    emit RMNRemote.Cursed(s_curseSubjects);
+
+    vm.stopPrank();
+    vm.prank(newAdmin);
     s_rmnRemote.curse(s_curseSubjects);
+
+    assertTrue(s_rmnRemote.isCursed(CURSE_SUBJ_1));
+    assertTrue(s_rmnRemote.isCursed(CURSE_SUBJ_2));
+  }
+
+  function test_RevertWhen_curse_calledByRemovedCurseAdmin() public {
+    address[] memory toRemove = new address[](1);
+    toRemove[0] = s_curseAdmin;
+    s_rmnRemote.applyCurseAdminUpdates(toRemove, new address[](0));
+
+    vm.expectRevert(abi.encodeWithSelector(RMNRemote.OnlyOwnerOrCurseAdmin.selector, s_curseAdmin));
+    vm.stopPrank();
+    vm.prank(s_curseAdmin);
+    s_rmnRemote.curse(s_curseSubjects);
+  }
+
+  function test_RevertWhen_uncurse_calledByCurseAdmin() public {
+    s_rmnRemote.curse(s_curseSubjects);
+
+    vm.expectRevert(Ownable2Step.OnlyCallableByOwner.selector);
+    vm.stopPrank();
+    vm.prank(s_curseAdmin);
+    s_rmnRemote.uncurse(s_curseSubjects);
   }
 }

chains/evm/contracts/test/rmn/RMNRemote/RMNRemoteSetup.t.sol

@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: BUSL-1.1
 pragma solidity ^0.8.24;
 
-import {IRMN} from "../../../interfaces/IRMN.sol";
 import {IRMNRemote} from "../../../interfaces/IRMNRemote.sol";
 
 import {Internal} from "../../../libraries/Internal.sol";
@@ -23,11 +22,9 @@ contract RMNRemoteSetup is BaseTest {
   bytes16 internal constant CURSE_SUBJ_2 = bytes16(keccak256("subject 2"));
   bytes16[] internal s_curseSubjects;
 
-  IRMN internal s_legacyRMN = IRMN(makeAddr("legacyRMN"));
-
   function setUp() public virtual override {
     super.setUp();
-    s_rmnRemote = new RMNRemote(1, s_legacyRMN);
+    s_rmnRemote = new RMNRemote(1);
     OFF_RAMP_ADDRESS = makeAddr("OFF RAMP");
     s_curseSubjects = [CURSE_SUBJ_1, CURSE_SUBJ_2];