fix(world): prevent namespace from ending with underscore [M-05] (#2182)

Co-authored-by: Kevin Ingersoll <[email protected]>
Co-authored-by: yonada <[email protected]>

.changeset/thirty-shoes-run.md

@@ -0,0 +1,5 @@
+---
+"@latticexyz/world": patch
+---
+
+Added a check to prevent namespaces from ending with an underscore (which could cause problems with world function signatures).

packages/world-modules/gas-report.json

@@ -135,7 +135,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testGetKeysWithValueGas",
     "name": "install keys with value module",
-    "gasUsed": 676716
+    "gasUsed": 676402
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -153,7 +153,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testInstall",
     "name": "install keys with value module",
-    "gasUsed": 676716
+    "gasUsed": 676402
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -165,7 +165,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testSetAndDeleteRecordHook",
     "name": "install keys with value module",
-    "gasUsed": 676716
+    "gasUsed": 676402
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -183,7 +183,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testSetField",
     "name": "install keys with value module",
-    "gasUsed": 676716
+    "gasUsed": 676402
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -303,7 +303,7 @@
     "file": "test/UniqueEntityModule.t.sol",
     "test": "testInstall",
     "name": "install unique entity module",
-    "gasUsed": 690443
+    "gasUsed": 691509
   },
   {
     "file": "test/UniqueEntityModule.t.sol",
@@ -315,7 +315,7 @@
     "file": "test/UniqueEntityModule.t.sol",
     "test": "testInstallRoot",
     "name": "installRoot unique entity module",
-    "gasUsed": 660912
+    "gasUsed": 661978
   },
   {
     "file": "test/UniqueEntityModule.t.sol",

packages/world/gas-report.json

@@ -63,7 +63,7 @@
     "file": "test/Factories.t.sol",
     "test": "testWorldFactoryGas",
     "name": "deploy world via WorldFactory",
-    "gasUsed": 12718635
+    "gasUsed": 12716027
   },
   {
     "file": "test/World.t.sol",
@@ -111,7 +111,7 @@
     "file": "test/World.t.sol",
     "test": "testRegisterNamespace",
     "name": "Register a new namespace",
-    "gasUsed": 121348
+    "gasUsed": 122690
   },
   {
     "file": "test/World.t.sol",
@@ -135,7 +135,7 @@
     "file": "test/World.t.sol",
     "test": "testRenounceNamespace",
     "name": "Renounce namespace ownership",
-    "gasUsed": 37438
+    "gasUsed": 34830
   },
   {
     "file": "test/World.t.sol",
@@ -153,7 +153,7 @@
     "file": "test/World.t.sol",
     "test": "testUnregisterNamespaceDelegation",
     "name": "unregister a namespace delegation",
-    "gasUsed": 29012
+    "gasUsed": 26404
   },
   {
     "file": "test/World.t.sol",

packages/world/src/modules/init/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 { validateNamespace } from "../../../validateNamespace.sol";
+import { requireNamespace } from "../../../requireNamespace.sol";
 
 import { LimitedCallContext } from "../LimitedCallContext.sol";
 
@@ -53,8 +53,8 @@ contract AccessManagementSystem is System, LimitedCallContext {
    * @param newOwner The address to which ownership should be transferred.
    */
   function transferOwnership(ResourceId namespaceId, address newOwner) public virtual onlyDelegatecall {
-    // Require the namespace ID to be a valid namespace
-    validateNamespace(namespaceId);
+    // Ensure namespace resource is a namespace
+    requireNamespace(namespaceId);
 
     // Require the namespace to exist
     AccessControl.requireExistence(namespaceId);
@@ -78,8 +78,8 @@ contract AccessManagementSystem is System, LimitedCallContext {
    * @param namespaceId The ID of the namespace to transfer ownership.
    */
   function renounceOwnership(ResourceId namespaceId) public virtual onlyDelegatecall {
-    // Require the namespace ID to be a valid namespace
-    validateNamespace(namespaceId);
+    // Ensure namespace resource is a namespace
+    requireNamespace(namespaceId);
 
     // Require the namespace to exist
     AccessControl.requireExistence(namespaceId);

packages/world/src/modules/init/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 { validateNamespace } from "../../../validateNamespace.sol";
+import { requireNamespace } from "../../../requireNamespace.sol";
 
 import { LimitedCallContext } from "../LimitedCallContext.sol";
 
@@ -34,10 +34,10 @@ contract BalanceTransferSystem is System, IWorldErrors, LimitedCallContext {
     ResourceId toNamespaceId,
     uint256 amount
   ) public virtual onlyDelegatecall {
-    // Require the from namespace to be a valid namespace ID
-    validateNamespace(fromNamespaceId);
-    // Require the to namespace to be a valid namespace ID
-    validateNamespace(toNamespaceId);
+    // Ensure the "from" namespace resource is a namespace
+    requireNamespace(fromNamespaceId);
+    // Ensure the "to" namespace resource is a namespace
+    requireNamespace(toNamespaceId);
 
     // Require the namespace to exist
     AccessControl.requireExistence(toNamespaceId);

packages/world/src/modules/init/implementations/WorldRegistrationSystem.sol

@@ -27,7 +27,8 @@ 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 { validateNamespace } from "../../../validateNamespace.sol";
+import { requireNamespace } from "../../../requireNamespace.sol";
+import { requireValidNamespace } from "../../../requireValidNamespace.sol";
 
 import { LimitedCallContext } from "../LimitedCallContext.sol";
 
@@ -44,8 +45,10 @@ contract WorldRegistrationSystem is System, IWorldErrors, LimitedCallContext {
    * @param namespaceId The unique identifier for the new namespace
    */
   function registerNamespace(ResourceId namespaceId) public virtual onlyDelegatecall {
-    // Require namespace ID to be a valid namespace
-    validateNamespace(namespaceId);
+    // Ensure namespace resource is a namespace
+    requireNamespace(namespaceId);
+    // and is valid (does not include reserved characters)
+    requireValidNamespace(namespaceId);
 
     // Require namespace to not exist yet
     if (ResourceIds._getExists(namespaceId)) {
@@ -302,8 +305,8 @@ contract WorldRegistrationSystem is System, IWorldErrors, LimitedCallContext {
     ResourceId delegationControlId,
     bytes memory initCallData
   ) public onlyDelegatecall {
-    // Require namespace ID to be a valid namespace
-    validateNamespace(namespaceId);
+    // Ensure namespace resource is a namespace
+    requireNamespace(namespaceId);
 
     // Require the delegation to not be unlimited
     if (!Delegation.isLimited(delegationControlId)) {
@@ -337,8 +340,8 @@ contract WorldRegistrationSystem is System, IWorldErrors, LimitedCallContext {
    * @param namespaceId The ID of the namespace
    */
   function unregisterNamespaceDelegation(ResourceId namespaceId) public onlyDelegatecall {
-    // Require namespace ID to be a valid namespace
-    validateNamespace(namespaceId);
+    // Ensure namespace resource is a namespace
+    requireNamespace(namespaceId);
 
     // Require the namespace to exist
     AccessControl.requireExistence(namespaceId);

packages/world/src/requireNamespace.sol

@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: MIT
+pragma solidity >=0.8.21;
+
+import { ResourceId, WorldResourceIdInstance } 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 namespace.
+ * @dev Reverts with `IWorldErrors.World_InvalidResourceType` if the ID does not have the correct components.
+ * @param resourceId The resource ID to check.
+ */
+function requireNamespace(ResourceId resourceId) pure {
+  // Require the resourceId to have the namespace type and the root name
+  // (the resource ID is identical to the resource ID of its namespace)
+  if (ResourceId.unwrap(resourceId) != ResourceId.unwrap(resourceId.getNamespaceId())) {
+    revert IWorldErrors.World_InvalidResourceType(RESOURCE_NAMESPACE, resourceId, resourceId.toString());
+  }
+}

packages/world/src/requireValidNamespace.sol

@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: MIT
+pragma solidity >=0.8.21;
+
+import { Bytes } from "@latticexyz/store/src/Bytes.sol";
+
+import { ResourceId, WorldResourceIdInstance, WorldResourceIdLib } from "./WorldResourceId.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_InvalidNamespace` if the namespace includes the reserved `__` separator string or ends with `_`.
+ * @param resourceId The resource ID to validate.
+ */
+function requireValidNamespace(ResourceId resourceId) pure {
+  // Require the namespace to not include the reserved separator
+  bytes14 namespace = resourceId.getNamespace();
+  string memory trimmedNamespace = WorldResourceIdLib.toTrimmedString(namespace);
+  uint256 trimmedNamespaceLength = bytes(trimmedNamespace).length;
+
+  if (trimmedNamespaceLength > 0) {
+    if (Bytes.slice1(bytes(trimmedNamespace), trimmedNamespaceLength - 1) == "_") {
+      revert IWorldErrors.World_InvalidNamespace(namespace);
+    }
+
+    for (uint256 i; i < trimmedNamespaceLength - 1; i++) {
+      if (Bytes.slice1(bytes(trimmedNamespace), i) == "_" && Bytes.slice1(bytes(trimmedNamespace), i + 1) == "_") {
+        revert IWorldErrors.World_InvalidNamespace(namespace);
+      }
+    }
+  }
+}

packages/world/test/World.t.sol

@@ -361,6 +361,10 @@ contract WorldTest is Test, GasReporter {
     bytes14 invalidNamespace = "invld__nmsp";
     vm.expectRevert(abi.encodeWithSelector(IWorldErrors.World_InvalidNamespace.selector, invalidNamespace));
     world.registerNamespace(WorldResourceIdLib.encodeNamespace(invalidNamespace));
+
+    bytes14 invalidNamespace2 = "invldnmsp_";
+    vm.expectRevert(abi.encodeWithSelector(IWorldErrors.World_InvalidNamespace.selector, invalidNamespace2));
+    world.registerNamespace(WorldResourceIdLib.encodeNamespace(invalidNamespace2));
   }
 
   function testRegisterCoreNamespacesRevert() public {