feat(world): prevent invalid namespace strings [M-05] (#2169)

latticexyz · Jan 22, 2024 · c642ff3 · c642ff3
1 parent 865253d
commit c642ff3
Show file tree

Hide file tree

Showing 11 changed files with 85 additions and 40 deletions.
diff --git a/.changeset/tidy-stingrays-think.md b/.changeset/tidy-stingrays-think.md
@@ -0,0 +1,9 @@
+---
+"@latticexyz/world": major
+---
+
+Namespaces are not allowed to contain double underscores ("\_\_") anymore, as this sequence of characters is used to [separate the namespace and function selector](https://github.com/latticexyz/mud/pull/2168) in namespaced systems.
+This is to prevent signature clashes of functions in different namespaces.
+
+(Example: If namespaces were allowed to contain this separator string, a function "function" in namespace "namespace\_\_my" would result in the namespaced function selector "namespace\_\_my\_\_function",
+and would clash with a function "my\_\_function" in namespace "namespace".)
diff --git a/packages/world-modules/gas-report.json b/packages/world-modules/gas-report.json
@@ -135,7 +135,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testGetKeysWithValueGas",
     "name": "install keys with value module",
-    "gasUsed": 683899
+    "gasUsed": 687430
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -153,7 +153,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testInstall",
     "name": "install keys with value module",
-    "gasUsed": 683899
+    "gasUsed": 687430
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -165,7 +165,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testSetAndDeleteRecordHook",
     "name": "install keys with value module",
-    "gasUsed": 683899
+    "gasUsed": 687430
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -183,7 +183,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testSetField",
     "name": "install keys with value module",
-    "gasUsed": 683899
+    "gasUsed": 687430
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -303,7 +303,7 @@
     "file": "test/UniqueEntityModule.t.sol",
     "test": "testInstall",
     "name": "install unique entity module",
-    "gasUsed": 701756
+    "gasUsed": 705287
   },
   {
     "file": "test/UniqueEntityModule.t.sol",
@@ -315,7 +315,7 @@
     "file": "test/UniqueEntityModule.t.sol",
     "test": "testInstallRoot",
     "name": "installRoot unique entity module",
-    "gasUsed": 670623
+    "gasUsed": 674154
   },
   {
     "file": "test/UniqueEntityModule.t.sol",

diff --git a/packages/world/gas-report.json b/packages/world/gas-report.json
@@ -63,7 +63,7 @@
     "file": "test/Factories.t.sol",
     "test": "testWorldFactory",
     "name": "deploy world via WorldFactory",
-    "gasUsed": 13041829
+    "gasUsed": 13045360
   },
   {
     "file": "test/World.t.sol",
@@ -111,7 +111,7 @@
     "file": "test/World.t.sol",
     "test": "testRegisterNamespace",
     "name": "Register a new namespace",
-    "gasUsed": 120951
+    "gasUsed": 124482
   },
   {
     "file": "test/World.t.sol",
@@ -129,13 +129,13 @@
     "file": "test/World.t.sol",
     "test": "testRegisterTable",
     "name": "Register a new table in the namespace",
-    "gasUsed": 536881
+    "gasUsed": 536849
   },
   {
     "file": "test/World.t.sol",
     "test": "testRenounceNamespace",
     "name": "Renounce namespace ownership",
-    "gasUsed": 36773
+    "gasUsed": 40304
   },
   {
     "file": "test/World.t.sol",
@@ -153,7 +153,7 @@
     "file": "test/World.t.sol",
     "test": "testUnregisterNamespaceDelegation",
     "name": "unregister a namespace delegation",
-    "gasUsed": 28360
+    "gasUsed": 31891
   },
   {
     "file": "test/World.t.sol",

diff --git a/packages/world/src/IWorldErrors.sol b/packages/world/src/IWorldErrors.sol
@@ -42,6 +42,12 @@ interface IWorldErrors {
    */
   error World_InvalidResourceId(ResourceId resourceId, string resourceIdString);
 
+  /**
+   * @notice Raised when an namespace contains an invalid sequence of characters ("__").
+   * @param namespace The invalid namespace.
+   */
+  error World_InvalidNamespace(bytes14 namespace);
+
   /**
    * @notice Raised when trying to register a system that already exists.
    * @param system The address of the system.

diff --git a/packages/world/src/WorldResourceId.sol b/packages/world/src/WorldResourceId.sol
@@ -7,6 +7,7 @@ import { ResourceId, ResourceIdInstance, TYPE_BITS } from "@latticexyz/store/src
 import { ROOT_NAMESPACE, ROOT_NAME } from "./constants.sol";
 import { RESOURCE_NAMESPACE, MASK_RESOURCE_NAMESPACE } from "./worldResourceTypes.sol";
 
+uint256 constant NAMESPACE_BYTES = 14;
 uint256 constant NAMESPACE_BITS = 14 * 8; // 14 bytes * 8 bits per byte
 uint256 constant NAME_BITS = 16 * 8; // 16 bytes * 8 bits per byte
 

diff --git a/packages/world/src/modules/core/implementations/AccessManagementSystem.sol b/packages/world/src/modules/core/implementations/AccessManagementSystem.sol
@@ -6,7 +6,7 @@ import { AccessControl } from "../../../AccessControl.sol";
 import { ResourceId } from "../../../WorldResourceId.sol";
 import { ResourceAccess } from "../../../codegen/tables/ResourceAccess.sol";
 import { NamespaceOwner } from "../../../codegen/tables/NamespaceOwner.sol";
-import { requireNamespace } from "../../../requireNamespace.sol";
+import { validateNamespace } from "../../../validateNamespace.sol";
 
 import { LimitedCallContext } from "../LimitedCallContext.sol";
 
@@ -54,7 +54,7 @@ contract AccessManagementSystem is System, LimitedCallContext {
    */
   function transferOwnership(ResourceId namespaceId, address newOwner) public virtual onlyDelegatecall {
     // Require the namespace ID to be a valid namespace
-    requireNamespace(namespaceId);
+    validateNamespace(namespaceId);
 
     // Require the namespace to exist
     AccessControl.requireExistence(namespaceId);
@@ -79,7 +79,7 @@ contract AccessManagementSystem is System, LimitedCallContext {
    */
   function renounceOwnership(ResourceId namespaceId) public virtual onlyDelegatecall {
     // Require the namespace ID to be a valid namespace
-    requireNamespace(namespaceId);
+    validateNamespace(namespaceId);
 
     // Require the namespace to exist
     AccessControl.requireExistence(namespaceId);

diff --git a/packages/world/src/modules/core/implementations/BalanceTransferSystem.sol b/packages/world/src/modules/core/implementations/BalanceTransferSystem.sol
@@ -11,7 +11,7 @@ import { RESOURCE_NAMESPACE } from "../../../worldResourceTypes.sol";
 import { IWorldErrors } from "../../../IWorldErrors.sol";
 
 import { Balances } from "../../../codegen/tables/Balances.sol";
-import { requireNamespace } from "../../../requireNamespace.sol";
+import { validateNamespace } from "../../../validateNamespace.sol";
 
 import { LimitedCallContext } from "../LimitedCallContext.sol";
 
@@ -35,9 +35,9 @@ contract BalanceTransferSystem is System, IWorldErrors, LimitedCallContext {
     uint256 amount
   ) public virtual onlyDelegatecall {
     // Require the from namespace to be a valid namespace ID
-    requireNamespace(fromNamespaceId);
+    validateNamespace(fromNamespaceId);
     // Require the to namespace to be a valid namespace ID
-    requireNamespace(toNamespaceId);
+    validateNamespace(toNamespaceId);
 
     // Require the namespace to exist
     AccessControl.requireExistence(toNamespaceId);

diff --git a/packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol b/packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol
@@ -27,7 +27,7 @@ import { SystemRegistry } from "../../../codegen/tables/SystemRegistry.sol";
 import { Systems } from "../../../codegen/tables/Systems.sol";
 import { FunctionSelectors } from "../../../codegen/tables/FunctionSelectors.sol";
 import { FunctionSignatures } from "../../../codegen/tables/FunctionSignatures.sol";
-import { requireNamespace } from "../../../requireNamespace.sol";
+import { validateNamespace } from "../../../validateNamespace.sol";
 
 import { LimitedCallContext } from "../LimitedCallContext.sol";
 
@@ -45,7 +45,7 @@ contract WorldRegistrationSystem is System, IWorldErrors, LimitedCallContext {
    */
   function registerNamespace(ResourceId namespaceId) public virtual onlyDelegatecall {
     // Require namespace ID to be a valid namespace
-    requireNamespace(namespaceId);
+    validateNamespace(namespaceId);
 
     // Require namespace to not exist yet
     if (ResourceIds._getExists(namespaceId)) {
@@ -300,7 +300,7 @@ contract WorldRegistrationSystem is System, IWorldErrors, LimitedCallContext {
     bytes memory initCallData
   ) public onlyDelegatecall {
     // Require namespace ID to be a valid namespace
-    requireNamespace(namespaceId);
+    validateNamespace(namespaceId);
 
     // Require the delegation to not be unlimited
     if (!Delegation.isLimited(delegationControlId)) {
@@ -335,7 +335,7 @@ contract WorldRegistrationSystem is System, IWorldErrors, LimitedCallContext {
    */
   function unregisterNamespaceDelegation(ResourceId namespaceId) public onlyDelegatecall {
     // Require namespace ID to be a valid namespace
-    requireNamespace(namespaceId);
+    validateNamespace(namespaceId);
 
     // Require the namespace to exist
     AccessControl.requireExistence(namespaceId);

diff --git a/packages/world/src/requireNamespace.sol b/packages/world/src/requireNamespace.sol
diff --git a/packages/world/src/validateNamespace.sol b/packages/world/src/validateNamespace.sol
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: MIT
+pragma solidity >=0.8.21;
+
+import { Bytes } from "@latticexyz/store/src/Bytes.sol";
+
+import { ResourceId, WorldResourceIdInstance, NAMESPACE_BYTES } from "./WorldResourceId.sol";
+import { RESOURCE_NAMESPACE } from "./worldResourceTypes.sol";
+import { IWorldErrors } from "./IWorldErrors.sol";
+
+using WorldResourceIdInstance for ResourceId;
+
+/**
+ * @notice Checks if a given `resourceId` is a valid namespace.
+ * @dev Reverts with IWorldErrors.World_InvalidResourceType if the ID does not have the correct components.
+ * @dev Reverts with IWorldErrors.World_InvalidNamespace if the namespace includes the reserved separator string ("__").
+ * @param resourceId The resource ID to verify.
+ */
+function validateNamespace(ResourceId resourceId) pure {
+  // Require the resourceId to have the namespace type
+  if (ResourceId.unwrap(resourceId) != ResourceId.unwrap(resourceId.getNamespaceId())) {
+    revert IWorldErrors.World_InvalidResourceType(RESOURCE_NAMESPACE, resourceId, resourceId.toString());
+  }
+
+  // Require the namespace to not include the reserved separator
+  bytes14 namespace = resourceId.getNamespace();
+  for (uint256 i; i < NAMESPACE_BYTES - 1; i++) {
+    if (Bytes.slice1(namespace, i) == bytes1("_") && Bytes.slice1(namespace, i + 1) == bytes1("_")) {
+      revert IWorldErrors.World_InvalidNamespace(namespace);
+    }
+  }
+}
diff --git a/packages/world/test/World.t.sol b/packages/world/test/World.t.sol
@@ -348,6 +348,23 @@ contract WorldTest is Test, GasReporter {
     world.registerNamespace(namespaceId);
   }
 
+  function testRegisterInvalidNamespace() public {
+    ResourceId invalidNamespaceId = WorldResourceIdLib.encode(RESOURCE_SYSTEM, "namespace", "system");
+    vm.expectRevert(
+      abi.encodeWithSelector(
+        IWorldErrors.World_InvalidResourceType.selector,
+        RESOURCE_NAMESPACE,
+        invalidNamespaceId,
+        invalidNamespaceId.toString()
+      )
+    );
+    world.registerNamespace(invalidNamespaceId);
+
+    bytes14 invalidNamespace = "invld__nmsp";
+    vm.expectRevert(abi.encodeWithSelector(IWorldErrors.World_InvalidNamespace.selector, invalidNamespace));
+    world.registerNamespace(WorldResourceIdLib.encodeNamespace(invalidNamespace));
+  }
+
   function testRegisterCoreNamespacesRevert() public {
     vm.expectRevert(
       abi.encodeWithSelector(