diff --git a/.changeset/thirty-shoes-run.md b/.changeset/thirty-shoes-run.md new file mode 100644 index 0000000000..833f76aded --- /dev/null +++ b/.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). diff --git a/packages/world-modules/gas-report.json b/packages/world-modules/gas-report.json index 4da91a9a3a..daf6dd1a9b 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": 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", diff --git a/packages/world/gas-report.json b/packages/world/gas-report.json index 2caa9349ff..55acc5ca9b 100644 --- a/packages/world/gas-report.json +++ b/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", diff --git a/packages/world/src/modules/init/implementations/AccessManagementSystem.sol b/packages/world/src/modules/init/implementations/AccessManagementSystem.sol index fb5fad7a03..1c05a7f050 100644 --- a/packages/world/src/modules/init/implementations/AccessManagementSystem.sol +++ b/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); diff --git a/packages/world/src/modules/init/implementations/BalanceTransferSystem.sol b/packages/world/src/modules/init/implementations/BalanceTransferSystem.sol index 82c8a17ba1..58bfa7b174 100644 --- a/packages/world/src/modules/init/implementations/BalanceTransferSystem.sol +++ b/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); diff --git a/packages/world/src/modules/init/implementations/WorldRegistrationSystem.sol b/packages/world/src/modules/init/implementations/WorldRegistrationSystem.sol index 2af38fe3a2..5ac6bf1664 100644 --- a/packages/world/src/modules/init/implementations/WorldRegistrationSystem.sol +++ b/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); diff --git a/packages/world/src/requireNamespace.sol b/packages/world/src/requireNamespace.sol new file mode 100644 index 0000000000..64efcbe821 --- /dev/null +++ b/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()); + } +} diff --git a/packages/world/src/requireValidNamespace.sol b/packages/world/src/requireValidNamespace.sol new file mode 100644 index 0000000000..485032c869 --- /dev/null +++ b/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); + } + } + } +} diff --git a/packages/world/src/validateNamespace.sol b/packages/world/src/validateNamespace.sol deleted file mode 100644 index 78d9a67ac5..0000000000 --- a/packages/world/src/validateNamespace.sol +++ /dev/null @@ -1,31 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity >=0.8.24; - -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 2f4e5cbe02..9995d26988 100644 --- a/packages/world/test/World.t.sol +++ b/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 {