fix(world): limit call context of CoreSystem to delegatecall [C-02] (…

…#2111)

.changeset/lazy-foxes-applaud.md

@@ -0,0 +1,9 @@
+---
+"@latticexyz/world": patch
+---
+
+Systems are expected to be always called via the central World contract.
+Depending on whether it is a root or non-root system, the call is performed via `delegatecall` or `call`.
+Since Systems are expected to be stateless and only interact with the World state, it is not necessary to prevent direct calls to the systems.
+However, since the `CoreSystem` is known to always be registered as a root system in the World, it is always expected to be delegatecalled,
+so we made this expectation explicit by reverting if it is not delegatecalled.

packages/world-modules/gas-report.json

@@ -75,13 +75,13 @@
     "file": "test/KeysInTableModule.t.sol",
     "test": "testInstallComposite",
     "name": "install keys in table module",
-    "gasUsed": 1438673
+    "gasUsed": 1438969
   },
   {
     "file": "test/KeysInTableModule.t.sol",
     "test": "testInstallGas",
     "name": "install keys in table module",
-    "gasUsed": 1438673
+    "gasUsed": 1438969
   },
   {
     "file": "test/KeysInTableModule.t.sol",
@@ -93,13 +93,13 @@
     "file": "test/KeysInTableModule.t.sol",
     "test": "testInstallSingleton",
     "name": "install keys in table module",
-    "gasUsed": 1438673
+    "gasUsed": 1438969
   },
   {
     "file": "test/KeysInTableModule.t.sol",
     "test": "testSetAndDeleteRecordHookCompositeGas",
     "name": "install keys in table module",
-    "gasUsed": 1438673
+    "gasUsed": 1438969
   },
   {
     "file": "test/KeysInTableModule.t.sol",
@@ -117,7 +117,7 @@
     "file": "test/KeysInTableModule.t.sol",
     "test": "testSetAndDeleteRecordHookGas",
     "name": "install keys in table module",
-    "gasUsed": 1438673
+    "gasUsed": 1438969
   },
   {
     "file": "test/KeysInTableModule.t.sol",
@@ -135,7 +135,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testGetKeysWithValueGas",
     "name": "install keys with value module",
-    "gasUsed": 684127
+    "gasUsed": 684371
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -153,7 +153,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testInstall",
     "name": "install keys with value module",
-    "gasUsed": 684127
+    "gasUsed": 684371
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -165,7 +165,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testSetAndDeleteRecordHook",
     "name": "install keys with value module",
-    "gasUsed": 684127
+    "gasUsed": 684371
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -183,7 +183,7 @@
     "file": "test/KeysWithValueModule.t.sol",
     "test": "testSetField",
     "name": "install keys with value module",
-    "gasUsed": 684127
+    "gasUsed": 684371
   },
   {
     "file": "test/KeysWithValueModule.t.sol",
@@ -267,7 +267,7 @@
     "file": "test/StandardDelegationsModule.t.sol",
     "test": "testCallFromCallboundDelegation",
     "name": "register a callbound delegation",
-    "gasUsed": 118111
+    "gasUsed": 118175
   },
   {
     "file": "test/StandardDelegationsModule.t.sol",
@@ -279,7 +279,7 @@
     "file": "test/StandardDelegationsModule.t.sol",
     "test": "testCallFromSystemDelegation",
     "name": "register a systembound delegation",
-    "gasUsed": 115664
+    "gasUsed": 115728
   },
   {
     "file": "test/StandardDelegationsModule.t.sol",
@@ -291,7 +291,7 @@
     "file": "test/StandardDelegationsModule.t.sol",
     "test": "testCallFromTimeboundDelegation",
     "name": "register a timebound delegation",
-    "gasUsed": 112587
+    "gasUsed": 112651
   },
   {
     "file": "test/StandardDelegationsModule.t.sol",
@@ -303,7 +303,7 @@
     "file": "test/UniqueEntityModule.t.sol",
     "test": "testInstall",
     "name": "install unique entity module",
-    "gasUsed": 703920
+    "gasUsed": 704240
   },
   {
     "file": "test/UniqueEntityModule.t.sol",
@@ -315,7 +315,7 @@
     "file": "test/UniqueEntityModule.t.sol",
     "test": "testInstallRoot",
     "name": "installRoot unique entity module",
-    "gasUsed": 672952
+    "gasUsed": 673144
   },
   {
     "file": "test/UniqueEntityModule.t.sol",

packages/world/gas-report.json

@@ -45,13 +45,13 @@
     "file": "test/BatchCall.t.sol",
     "test": "testBatchCallFromReturnData",
     "name": "call systems with batchCallFrom",
-    "gasUsed": 52559
+    "gasUsed": 52611
   },
   {
     "file": "test/BatchCall.t.sol",
     "test": "testBatchCallReturnData",
     "name": "call systems with batchCall",
-    "gasUsed": 51405
+    "gasUsed": 51457
   },
   {
     "file": "test/Factories.t.sol",
@@ -63,7 +63,7 @@
     "file": "test/Factories.t.sol",
     "test": "testWorldFactory",
     "name": "deploy world via WorldFactory",
-    "gasUsed": 12511434
+    "gasUsed": 12512902
   },
   {
     "file": "test/World.t.sol",
@@ -81,7 +81,7 @@
     "file": "test/World.t.sol",
     "test": "testCallFromUnlimitedDelegation",
     "name": "register an unlimited delegation",
-    "gasUsed": 47520
+    "gasUsed": 47584
   },
   {
     "file": "test/World.t.sol",
@@ -105,31 +105,31 @@
     "file": "test/World.t.sol",
     "test": "testRegisterFunctionSelector",
     "name": "Register a function selector",
-    "gasUsed": 84455
+    "gasUsed": 84519
   },
   {
     "file": "test/World.t.sol",
     "test": "testRegisterNamespace",
     "name": "Register a new namespace",
-    "gasUsed": 120905
+    "gasUsed": 120969
   },
   {
     "file": "test/World.t.sol",
     "test": "testRegisterRootFunctionSelector",
     "name": "Register a root function selector",
-    "gasUsed": 80409
+    "gasUsed": 80467
   },
   {
     "file": "test/World.t.sol",
     "test": "testRegisterSystem",
     "name": "register a system",
-    "gasUsed": 164321
+    "gasUsed": 164385
   },
   {
     "file": "test/World.t.sol",
     "test": "testRegisterTable",
     "name": "Register a new table in the namespace",
-    "gasUsed": 536817
+    "gasUsed": 536881
   },
   {
     "file": "test/World.t.sol",

packages/world/src/modules/core/LimitedCallContext.sol

@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: MIT
+pragma solidity >=0.8.21;
+
+/**
+ * @title LimitedCallContext
+ * @dev Systems are expected to be always called via the central World contract.
+ * Depending on whether it is a root or non-root system, the call is performed via `delegatecall` or `call`.
+ * Since Systems are expected to be stateless and only interact with the World state,
+ * it is normally not necessary to prevent direct calls to the systems.
+ * However, since the `CoreSystem` is known to always be registered as a root system in the World,
+ * it is always expected to be delegatecalled, so we made this expectation explicit by reverting if it is not delegatecalled.
+ *
+ * @dev Based on OpenZeppelin's UUPSUpgradeable.sol
+ * https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v5.0.0/contracts/proxy/utils/UUPSUpgradeable.sol#L50
+ */
+contract LimitedCallContext {
+  address private immutable __self = address(this);
+
+  error UnauthorizedCallContext();
+
+  modifier onlyDelegatecall() {
+    _checkDelegatecall();
+    _;
+  }
+
+  /**
+   * @dev Reverts if the execution is not performed via delegatecall.
+   */
+  function _checkDelegatecall() internal view virtual {
+    if (
+      address(this) == __self // Must be called through delegatecall
+    ) {
+      revert UnauthorizedCallContext();
+    }
+  }
+}

packages/world/src/modules/core/implementations/AccessManagementSystem.sol

@@ -8,18 +8,20 @@ import { ResourceAccess } from "../../../codegen/tables/ResourceAccess.sol";
 import { NamespaceOwner } from "../../../codegen/tables/NamespaceOwner.sol";
 import { requireNamespace } from "../../../requireNamespace.sol";
 
+import { LimitedCallContext } from "../LimitedCallContext.sol";
+
 /**
  * @title Access Management System
  * @dev This contract manages the granting and revoking of access from/to resources.
  */
-contract AccessManagementSystem is System {
+contract AccessManagementSystem is System, LimitedCallContext {
   /**
    * @notice Grant access to the resource at the given resource ID.
    * @dev Requires the caller to own the namespace.
    * @param resourceId The ID of the resource to grant access to.
    * @param grantee The address to which access should be granted.
    */
-  function grantAccess(ResourceId resourceId, address grantee) public virtual {
+  function grantAccess(ResourceId resourceId, address grantee) public virtual onlyDelegatecall {
     // Require the resource to exist
     AccessControl.requireExistence(resourceId);
 
@@ -36,7 +38,7 @@ contract AccessManagementSystem is System {
    * @param resourceId The ID of the resource to revoke access from.
    * @param grantee The address from which access should be revoked.
    */
-  function revokeAccess(ResourceId resourceId, address grantee) public virtual {
+  function revokeAccess(ResourceId resourceId, address grantee) public virtual onlyDelegatecall {
     // Require the caller to own the namespace
     AccessControl.requireOwner(resourceId, _msgSender());
 
@@ -50,7 +52,7 @@ contract AccessManagementSystem is System {
    * @param namespaceId The ID of the namespace to transfer ownership.
    * @param newOwner The address to which ownership should be transferred.
    */
-  function transferOwnership(ResourceId namespaceId, address newOwner) public virtual {
+  function transferOwnership(ResourceId namespaceId, address newOwner) public virtual onlyDelegatecall {
     // Require the namespace to be a valid namespace ID
     requireNamespace(namespaceId);
 

packages/world/src/modules/core/implementations/BalanceTransferSystem.sol

@@ -13,11 +13,13 @@ import { IWorldErrors } from "../../../IWorldErrors.sol";
 import { Balances } from "../../../codegen/tables/Balances.sol";
 import { requireNamespace } from "../../../requireNamespace.sol";
 
+import { LimitedCallContext } from "../LimitedCallContext.sol";
+
 /**
  * @title Balance Transfer System
  * @dev A system contract that facilitates balance transfers in the World and outside of the World.
  */
-contract BalanceTransferSystem is System, IWorldErrors {
+contract BalanceTransferSystem is System, IWorldErrors, LimitedCallContext {
   using WorldResourceIdInstance for ResourceId;
 
   /**
@@ -31,7 +33,7 @@ contract BalanceTransferSystem is System, IWorldErrors {
     ResourceId fromNamespaceId,
     ResourceId toNamespaceId,
     uint256 amount
-  ) public virtual {
+  ) public virtual onlyDelegatecall {
     // Require the from namespace to be a valid namespace ID
     requireNamespace(fromNamespaceId);
     // Require the to namespace to be a valid namespace ID
@@ -61,7 +63,11 @@ contract BalanceTransferSystem is System, IWorldErrors {
    * @param toAddress The target address where the balance will be sent.
    * @param amount The amount to transfer.
    */
-  function transferBalanceToAddress(ResourceId fromNamespaceId, address toAddress, uint256 amount) public virtual {
+  function transferBalanceToAddress(
+    ResourceId fromNamespaceId,
+    address toAddress,
+    uint256 amount
+  ) public virtual onlyDelegatecall {
     // Require caller to have access to the namespace
     AccessControl.requireAccess(fromNamespaceId, _msgSender());
 

packages/world/src/modules/core/implementations/BatchCallSystem.sol

@@ -2,23 +2,27 @@
 pragma solidity >=0.8.0;
 
 import { System } from "../../../System.sol";
+import { LimitedCallContext } from "../LimitedCallContext.sol";
 import { IBaseWorld } from "../../../codegen/interfaces/IBaseWorld.sol";
 import { revertWithBytes } from "../../../revertWithBytes.sol";
 
 import { SystemCallData, SystemCallFromData } from "../types.sol";
+import { LimitedCallContext } from "../LimitedCallContext.sol";
 
 /**
  * @title Batch Call System
  * @dev A system contract that facilitates batching of calls to various systems in a single transaction.
  */
-contract BatchCallSystem is System {
+contract BatchCallSystem is System, LimitedCallContext {
   /**
    * @notice Make batch calls to multiple systems into a single transaction.
    * @dev Iterates through an array of system calls, executes them, and returns an array of return data.
    * @param systemCalls An array of SystemCallData that contains systemId and callData for each call.
    * @return returnDatas An array of bytes containing the return data for each system call.
    */
-  function batchCall(SystemCallData[] calldata systemCalls) public returns (bytes[] memory returnDatas) {
+  function batchCall(
+    SystemCallData[] calldata systemCalls
+  ) public onlyDelegatecall returns (bytes[] memory returnDatas) {
     IBaseWorld world = IBaseWorld(_world());
     returnDatas = new bytes[](systemCalls.length);
 
@@ -38,7 +42,9 @@ contract BatchCallSystem is System {
    * @param systemCalls An array of SystemCallFromData that contains from, systemId, and callData for each call.
    * @return returnDatas An array of bytes containing the return data for each system call.
    */
-  function batchCallFrom(SystemCallFromData[] calldata systemCalls) public returns (bytes[] memory returnDatas) {
+  function batchCallFrom(
+    SystemCallFromData[] calldata systemCalls
+  ) public onlyDelegatecall returns (bytes[] memory returnDatas) {
     IBaseWorld world = IBaseWorld(_world());
     returnDatas = new bytes[](systemCalls.length);
 

packages/world/src/modules/core/implementations/ModuleInstallationSystem.sol

@@ -7,19 +7,21 @@ import { WorldContextProviderLib } from "../../../WorldContext.sol";
 import { InstalledModules } from "../../../codegen/tables/InstalledModules.sol";
 import { requireInterface } from "../../../requireInterface.sol";
 
+import { LimitedCallContext } from "../LimitedCallContext.sol";
+
 /**
  * @title Module Installation System
  * @dev A system contract to handle the installation of (non-root) modules in the World.
  */
-contract ModuleInstallationSystem is System {
+contract ModuleInstallationSystem is System, LimitedCallContext {
   /**
    * @notice Installs a module into the World under a specified namespace.
    * @dev Validates the given module against the IModule interface and delegates the installation process.
    * The module is then registered in the InstalledModules table.
    * @param module The module to be installed.
    * @param args Arguments for the module installation.
    */
-  function installModule(IModule module, bytes memory args) public {
+  function installModule(IModule module, bytes memory args) public onlyDelegatecall {
     // Require the provided address to implement the IModule interface
     requireInterface(address(module), type(IModule).interfaceId);