fix(world): proper resource ID validation [L-09] (#2142)

Co-authored-by: Kevin Ingersoll <[email protected]>
latticexyz · Jan 18, 2024 · 158c9af · 158c9af
1 parent 37c228c
commit 158c9af
Show file tree

Hide file tree

Showing 7 changed files with 79 additions and 33 deletions.
diff --git a/packages/world-modules/gas-report.json b/packages/world-modules/gas-report.json
@@ -75,13 +75,13 @@
     "file": "test/KeysInTableModule.t.sol",
     "test": "testInstallComposite",
     "name": "install keys in table module",
-    "gasUsed": 1434753
+    "gasUsed": 1438673
   },
   {
     "file": "test/KeysInTableModule.t.sol",
     "test": "testInstallGas",
     "name": "install keys in table module",
-    "gasUsed": 1434753
+    "gasUsed": 1438673
   },
   {
     "file": "test/KeysInTableModule.t.sol",
@@ -93,13 +93,13 @@
     "file": "test/KeysInTableModule.t.sol",
     "test": "testInstallSingleton",
     "name": "install keys in table module",
-    "gasUsed": 1434753
+    "gasUsed": 1438673
   },
   {
     "file": "test/KeysInTableModule.t.sol",
     "test": "testSetAndDeleteRecordHookCompositeGas",
     "name": "install keys in table module",
-    "gasUsed": 1434753
+    "gasUsed": 1438673
   },
   {
     "file": "test/KeysInTableModule.t.sol",
@@ -117,7 +117,7 @@
     "file": "test/KeysInTableModule.t.sol",
     "test": "testSetAndDeleteRecordHookGas",
     "name": "install keys in table module",
-    "gasUsed": 1434753
+    "gasUsed": 1438673
   },
   {
     "file": "test/KeysInTableModule.t.sol",
@@ -135,7 +135,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testGetKeysWithValueGas",
     "name": "install keys with value module",
-    "gasUsed": 681477
+    "gasUsed": 684127
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -153,7 +153,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testInstall",
     "name": "install keys with value module",
-    "gasUsed": 681477
+    "gasUsed": 684127
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -165,7 +165,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testSetAndDeleteRecordHook",
     "name": "install keys with value module",
-    "gasUsed": 681477
+    "gasUsed": 684127
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -183,7 +183,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testSetField",
     "name": "install keys with value module",
-    "gasUsed": 681477
+    "gasUsed": 684127
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -267,7 +267,7 @@
     "file": "test/StandardDelegationsModule.t.sol",
     "test": "testCallFromCallboundDelegation",
     "name": "register a callbound delegation",
-    "gasUsed": 118105
+    "gasUsed": 118111
   },
   {
     "file": "test/StandardDelegationsModule.t.sol",
@@ -279,7 +279,7 @@
     "file": "test/StandardDelegationsModule.t.sol",
     "test": "testCallFromSystemDelegation",
     "name": "register a systembound delegation",
-    "gasUsed": 115658
+    "gasUsed": 115664
   },
   {
     "file": "test/StandardDelegationsModule.t.sol",
@@ -291,7 +291,7 @@
     "file": "test/StandardDelegationsModule.t.sol",
     "test": "testCallFromTimeboundDelegation",
     "name": "register a timebound delegation",
-    "gasUsed": 112581
+    "gasUsed": 112587
   },
   {
     "file": "test/StandardDelegationsModule.t.sol",
@@ -303,7 +303,7 @@
     "file": "test/UniqueEntityModule.t.sol",
     "test": "testInstall",
     "name": "install unique entity module",
-    "gasUsed": 702511
+    "gasUsed": 703920
   },
   {
     "file": "test/UniqueEntityModule.t.sol",
@@ -315,7 +315,7 @@
     "file": "test/UniqueEntityModule.t.sol",
     "test": "testInstallRoot",
     "name": "installRoot unique entity module",
-    "gasUsed": 671555
+    "gasUsed": 672952
   },
   {
     "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": 12509892
+    "gasUsed": 12511434
   },
   {
     "file": "test/World.t.sol",
@@ -81,7 +81,7 @@
     "file": "test/World.t.sol",
     "test": "testCallFromUnlimitedDelegation",
     "name": "register an unlimited delegation",
-    "gasUsed": 47514
+    "gasUsed": 47520
   },
   {
     "file": "test/World.t.sol",
@@ -105,31 +105,31 @@
     "file": "test/World.t.sol",
     "test": "testRegisterFunctionSelector",
     "name": "Register a function selector",
-    "gasUsed": 83102
+    "gasUsed": 84455
   },
   {
     "file": "test/World.t.sol",
     "test": "testRegisterNamespace",
     "name": "Register a new namespace",
-    "gasUsed": 120879
+    "gasUsed": 120905
   },
   {
     "file": "test/World.t.sol",
     "test": "testRegisterRootFunctionSelector",
     "name": "Register a root function selector",
-    "gasUsed": 80403
+    "gasUsed": 80409
   },
   {
     "file": "test/World.t.sol",
     "test": "testRegisterSystem",
     "name": "register a system",
-    "gasUsed": 164303
+    "gasUsed": 164321
   },
   {
     "file": "test/World.t.sol",
     "test": "testRegisterTable",
     "name": "Register a new table in the namespace",
-    "gasUsed": 536805
+    "gasUsed": 536817
   },
   {
     "file": "test/World.t.sol",

diff --git a/packages/world/src/modules/core/implementations/AccessManagementSystem.sol b/packages/world/src/modules/core/implementations/AccessManagementSystem.sol
@@ -6,6 +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";
 
 /**
  * @title Access Management System
@@ -19,6 +20,9 @@ contract AccessManagementSystem is System {
    * @param grantee The address to which access should be granted.
    */
   function grantAccess(ResourceId resourceId, address grantee) public virtual {
+    // Require the resource to exist
+    AccessControl.requireExistence(resourceId);
+
     // Require the caller to own the namespace
     AccessControl.requireOwner(resourceId, _msgSender());
 
@@ -47,6 +51,12 @@ contract AccessManagementSystem is System {
    * @param newOwner The address to which ownership should be transferred.
    */
   function transferOwnership(ResourceId namespaceId, address newOwner) public virtual {
+    // Require the namespace to be a valid namespace ID
+    requireNamespace(namespaceId);
+
+    // Require the namespace to exist
+    AccessControl.requireExistence(namespaceId);
+
     // Require the caller to own the namespace
     AccessControl.requireOwner(namespaceId, _msgSender());
 

diff --git a/packages/world/src/modules/core/implementations/BalanceTransferSystem.sol b/packages/world/src/modules/core/implementations/BalanceTransferSystem.sol
@@ -11,6 +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";
 
 /**
  * @title Balance Transfer System
@@ -31,10 +32,10 @@ contract BalanceTransferSystem is System, IWorldErrors {
     ResourceId toNamespaceId,
     uint256 amount
   ) public virtual {
-    // Require the target ID to be a namespace ID
-    if (toNamespaceId.getType() != RESOURCE_NAMESPACE) {
-      revert World_InvalidResourceType(RESOURCE_NAMESPACE, toNamespaceId, toNamespaceId.toString());
-    }
+    // Require the from namespace to be a valid namespace ID
+    requireNamespace(fromNamespaceId);
+    // Require the to namespace to be a valid namespace ID
+    requireNamespace(toNamespaceId);
 
     // Require the namespace to exist
     AccessControl.requireExistence(toNamespaceId);

diff --git a/packages/world/src/modules/core/implementations/StoreRegistrationSystem.sol b/packages/world/src/modules/core/implementations/StoreRegistrationSystem.sol
@@ -73,6 +73,9 @@ contract StoreRegistrationSystem is System, IWorldErrors {
     // Require the hook to implement the store hook interface
     requireInterface(address(hookAddress), type(IStoreHook).interfaceId);
 
+    // Require the table's namespace to exist
+    AccessControl.requireExistence(tableId.getNamespaceId());
+
     // Require caller to own the namespace
     AccessControl.requireOwner(tableId, _msgSender());
 

diff --git a/packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol b/packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol
@@ -27,6 +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";
 
 /**
  * @title WorldRegistrationSystem
@@ -41,10 +42,8 @@ contract WorldRegistrationSystem is System, IWorldErrors {
    * @param namespaceId The unique identifier for the new namespace
    */
   function registerNamespace(ResourceId namespaceId) public virtual {
-    // Require the provided namespace ID to have type RESOURCE_NAMESPACE
-    if (namespaceId.getType() != RESOURCE_NAMESPACE) {
-      revert World_InvalidResourceType(RESOURCE_NAMESPACE, namespaceId, namespaceId.toString());
-    }
+    // Require namespace to be a valid namespace ID
+    requireNamespace(namespaceId);
 
     // Require namespace to not exist yet
     if (ResourceIds._getExists(namespaceId)) {
@@ -69,9 +68,17 @@ contract WorldRegistrationSystem is System, IWorldErrors {
    * @param enabledHooksBitmap Bitmap indicating which hooks are enabled
    */
   function registerSystemHook(ResourceId systemId, ISystemHook hookAddress, uint8 enabledHooksBitmap) public virtual {
+    // Require the provided system ID to have type RESOURCE_SYSTEM
+    if (systemId.getType() != RESOURCE_SYSTEM) {
+      revert World_InvalidResourceType(RESOURCE_SYSTEM, systemId, systemId.toString());
+    }
+
     // Require the provided address to implement the ISystemHook interface
     requireInterface(address(hookAddress), type(ISystemHook).interfaceId);
 
+    // Require the system's namespace to exist
+    AccessControl.requireExistence(systemId.getNamespaceId());
+
     // Require caller to own the namespace
     AccessControl.requireOwner(systemId, _msgSender());
 
@@ -169,6 +176,14 @@ contract WorldRegistrationSystem is System, IWorldErrors {
     ResourceId systemId,
     string memory systemFunctionSignature
   ) public returns (bytes4 worldFunctionSelector) {
+    // Require the provided system ID to have type RESOURCE_SYSTEM
+    if (systemId.getType() != RESOURCE_SYSTEM) {
+      revert World_InvalidResourceType(RESOURCE_SYSTEM, systemId, systemId.toString());
+    }
+
+    // Require the resource to exist
+    AccessControl.requireExistence(systemId);
+
     // Require the caller to own the namespace
     AccessControl.requireOwner(systemId, _msgSender());
 
@@ -276,10 +291,8 @@ contract WorldRegistrationSystem is System, IWorldErrors {
     ResourceId delegationControlId,
     bytes memory initCallData
   ) public {
-    // Require the namespaceId to be a valid namespace ID
-    if (namespaceId.getType() != RESOURCE_NAMESPACE) {
-      revert World_InvalidResourceType(RESOURCE_NAMESPACE, namespaceId, namespaceId.toString());
-    }
+    // Require namespace to be a valid namespace ID
+    requireNamespace(namespaceId);
 
     // Require the delegation to not be unlimited
     if (!Delegation.isLimited(delegationControlId)) {

diff --git a/packages/world/src/requireNamespace.sol b/packages/world/src/requireNamespace.sol
@@ -0,0 +1,19 @@
+// 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());
+  }
+}