KS-391: Capabilities Registry Fixes (#13937)

* prevent malicious a node operator from taking over another node belonging to another node operator * prevent malicious node operator from becoming the admin for another node operator
smartcontractkit · Jul 29, 2024 · 27d9c71 · 27d9c71
1 parent 683a12e
commit 27d9c71
Show file tree

Hide file tree

Showing 7 changed files with 36 additions and 10 deletions.
diff --git a/.changeset/bright-readers-dress.md b/.changeset/bright-readers-dress.md
@@ -0,0 +1,5 @@
+---
+"chainlink": patch
+---
+
+#internal address security vulnerabilities around updating nodes and node operators on capabilities registry
diff --git a/contracts/.changeset/chatty-feet-clean.md b/contracts/.changeset/chatty-feet-clean.md
@@ -0,0 +1,5 @@
+---
+'@chainlink/contracts': patch
+---
+
+#internal address security vulnerabilities around updating nodes and node operators on capabilities registry
diff --git a/contracts/src/v0.8/keystone/CapabilitiesRegistry.sol b/contracts/src/v0.8/keystone/CapabilitiesRegistry.sol
@@ -502,7 +502,7 @@ contract CapabilitiesRegistry is OwnerIsCreator, TypeAndVersionInterface {
 
       NodeOperator memory nodeOperator = nodeOperators[i];
       if (nodeOperator.admin == address(0)) revert InvalidNodeOperatorAdmin();
-      if (msg.sender != nodeOperator.admin && msg.sender != owner) revert AccessForbidden(msg.sender);
+      if (msg.sender != currentNodeOperator.admin && msg.sender != owner) revert AccessForbidden(msg.sender);
 
       if (
         currentNodeOperator.admin != nodeOperator.admin ||
@@ -611,11 +611,12 @@ contract CapabilitiesRegistry is OwnerIsCreator, TypeAndVersionInterface {
     bool isOwner = msg.sender == owner();
     for (uint256 i; i < nodes.length; ++i) {
       NodeParams memory node = nodes[i];
-      NodeOperator memory nodeOperator = s_nodeOperators[node.nodeOperatorId];
-      if (!isOwner && msg.sender != nodeOperator.admin) revert AccessForbidden(msg.sender);
-
       Node storage storedNode = s_nodes[node.p2pId];
+      NodeOperator memory nodeOperator = s_nodeOperators[storedNode.nodeOperatorId];
+
       if (storedNode.signer == bytes32("")) revert NodeDoesNotExist(node.p2pId);
+      if (!isOwner && msg.sender != nodeOperator.admin) revert AccessForbidden(msg.sender);
+
       if (node.signer == bytes32("")) revert InvalidNodeSigner();
 
       bytes32 previousSigner = storedNode.signer;

diff --git a/contracts/src/v0.8/keystone/test/CapabilitiesRegistry_UpdateNodeOperatorsTest.t.sol b/contracts/src/v0.8/keystone/test/CapabilitiesRegistry_UpdateNodeOperatorsTest.t.sol
@@ -19,10 +19,7 @@ contract CapabilitiesRegistry_UpdateNodeOperatorTest is BaseTest {
     changePrank(STRANGER);
 
     CapabilitiesRegistry.NodeOperator[] memory nodeOperators = new CapabilitiesRegistry.NodeOperator[](1);
-    nodeOperators[0] = CapabilitiesRegistry.NodeOperator({
-      admin: NEW_NODE_OPERATOR_ADMIN,
-      name: NEW_NODE_OPERATOR_NAME
-    });
+    nodeOperators[0] = CapabilitiesRegistry.NodeOperator({admin: ADMIN, name: NEW_NODE_OPERATOR_NAME});
 
     uint32[] memory nodeOperatorIds = new uint32[](1);
     nodeOperatorIds[0] = TEST_NODE_OPERATOR_ID;

diff --git a/contracts/src/v0.8/keystone/test/CapabilitiesRegistry_UpdateNodesTest.t.sol b/contracts/src/v0.8/keystone/test/CapabilitiesRegistry_UpdateNodesTest.t.sol
@@ -59,6 +59,24 @@ contract CapabilitiesRegistry_UpdateNodesTest is BaseTest {
     s_CapabilitiesRegistry.updateNodes(nodes);
   }
 
+  function test_RevertWhen_CalledByAnotherNodeOperatorAdmin() public {
+    changePrank(NODE_OPERATOR_TWO_ADMIN);
+    CapabilitiesRegistry.NodeParams[] memory nodes = new CapabilitiesRegistry.NodeParams[](1);
+
+    bytes32[] memory hashedCapabilityIds = new bytes32[](1);
+    hashedCapabilityIds[0] = s_basicHashedCapabilityId;
+
+    nodes[0] = CapabilitiesRegistry.NodeParams({
+      nodeOperatorId: TEST_NODE_OPERATOR_TWO_ID,
+      p2pId: P2P_ID,
+      signer: NEW_NODE_SIGNER,
+      hashedCapabilityIds: hashedCapabilityIds
+    });
+
+    vm.expectRevert(abi.encodeWithSelector(CapabilitiesRegistry.AccessForbidden.selector, NODE_OPERATOR_TWO_ADMIN));
+    s_CapabilitiesRegistry.updateNodes(nodes);
+  }
+
   function test_RevertWhen_NodeDoesNotExist() public {
     changePrank(NODE_OPERATOR_ONE_ADMIN);
     CapabilitiesRegistry.NodeParams[] memory nodes = new CapabilitiesRegistry.NodeParams[](1);

diff --git a/core/gethwrappers/keystone/generated/capabilities_registry/capabilities_registry.go b/core/gethwrappers/keystone/generated/capabilities_registry/capabilities_registry.go
diff --git a/core/gethwrappers/keystone/generation/generated-wrapper-dependency-versions-do-not-edit.txt b/core/gethwrappers/keystone/generation/generated-wrapper-dependency-versions-do-not-edit.txt
@@ -1,5 +1,5 @@
 GETH_VERSION: 1.13.8
-capabilities_registry: ../../../contracts/solc/v0.8.24/CapabilitiesRegistry/CapabilitiesRegistry.abi ../../../contracts/solc/v0.8.24/CapabilitiesRegistry/CapabilitiesRegistry.bin 3a082f0307411f41c30db26e61d59adcd5b003141a5aa8fe79d7779619028e26
+capabilities_registry: ../../../contracts/solc/v0.8.24/CapabilitiesRegistry/CapabilitiesRegistry.abi ../../../contracts/solc/v0.8.24/CapabilitiesRegistry/CapabilitiesRegistry.bin 6d2e3aa3a6f3aed2cf24b613743bb9ae4b9558f48a6864dc03b8b0ebb37235e3
 feeds_consumer: ../../../contracts/solc/v0.8.24/KeystoneFeedsConsumer/KeystoneFeedsConsumer.abi ../../../contracts/solc/v0.8.24/KeystoneFeedsConsumer/KeystoneFeedsConsumer.bin f098e25df6afc100425fcad7f5107aec0844cc98315117e49da139a179d0eead
 forwarder: ../../../contracts/solc/v0.8.24/KeystoneForwarder/KeystoneForwarder.abi ../../../contracts/solc/v0.8.24/KeystoneForwarder/KeystoneForwarder.bin dc98a86a3775ead987b79d5b6079ee0e26f31c0626032bdd6508f986e2423227
 ocr3_capability: ../../../contracts/solc/v0.8.24/OCR3Capability/OCR3Capability.abi ../../../contracts/solc/v0.8.24/OCR3Capability/OCR3Capability.bin 8bf0f53f222efce7143dea6134552eb26ea1eef845407b4475a0d79b7d7ba9f8