From c642ff3a0ad0d6f47d53d7c381ad6d3fffe52bbf Mon Sep 17 00:00:00 2001 From: alvarius Date: Mon, 22 Jan 2024 21:10:31 +0000 Subject: [PATCH] feat(world): prevent invalid namespace strings [M-05] (#2169) --- .changeset/tidy-stingrays-think.md | 9 ++++++ packages/world-modules/gas-report.json | 12 +++---- packages/world/gas-report.json | 10 +++--- packages/world/src/IWorldErrors.sol | 6 ++++ packages/world/src/WorldResourceId.sol | 1 + .../AccessManagementSystem.sol | 6 ++-- .../implementations/BalanceTransferSystem.sol | 6 ++-- .../WorldRegistrationSystem.sol | 8 ++--- packages/world/src/requireNamespace.sol | 19 ------------ packages/world/src/validateNamespace.sol | 31 +++++++++++++++++++ packages/world/test/World.t.sol | 17 ++++++++++ 11 files changed, 85 insertions(+), 40 deletions(-) create mode 100644 .changeset/tidy-stingrays-think.md delete mode 100644 packages/world/src/requireNamespace.sol create mode 100644 packages/world/src/validateNamespace.sol diff --git a/.changeset/tidy-stingrays-think.md b/.changeset/tidy-stingrays-think.md new file mode 100644 index 0000000000..2bbee42817 --- /dev/null +++ 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 index a330ba3498..310266e60e 100644 --- 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 index 3d135987cd..f180dbc31b 100644 --- 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 index 29aed90045..7308db3d23 100644 --- 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 index 4c6b340103..eb91ddd245 100644 --- 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 index 51816fd70a..41e58ad6af 100644 --- 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 index fa7a30a566..cb703f8bdc 100644 --- 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 index f88a6faf68..a0f7dc70dc 100644 --- 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 deleted file mode 100644 index 7de2c8088c..0000000000 --- a/packages/world/src/requireNamespace.sol +++ /dev/null @@ -1,19 +0,0 @@ -// 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 verify. - */ -function requireNamespace(ResourceId resourceId) pure { - if (ResourceId.unwrap(resourceId) != ResourceId.unwrap(resourceId.getNamespaceId())) { - revert IWorldErrors.World_InvalidResourceType(RESOURCE_NAMESPACE, resourceId, resourceId.toString()); - } -} diff --git a/packages/world/src/validateNamespace.sol b/packages/world/src/validateNamespace.sol new file mode 100644 index 0000000000..bb6f4627e0 --- /dev/null +++ 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 index 9078a01e79..278733ed46 100644 --- 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(