diff --git a/.changeset/lazy-foxes-applaud.md b/.changeset/lazy-foxes-applaud.md new file mode 100644 index 0000000000..9713f4a8bb --- /dev/null +++ b/.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. diff --git a/packages/world-modules/gas-report.json b/packages/world-modules/gas-report.json index de6104f5e9..d399d9fb7f 100644 --- 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": 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", diff --git a/packages/world/gas-report.json b/packages/world/gas-report.json index 1cff0fe8d0..4ea750e7ae 100644 --- a/packages/world/gas-report.json +++ b/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", diff --git a/packages/world/src/modules/core/LimitedCallContext.sol b/packages/world/src/modules/core/LimitedCallContext.sol new file mode 100644 index 0000000000..30603834df --- /dev/null +++ b/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(); + } + } +} diff --git a/packages/world/src/modules/core/implementations/AccessManagementSystem.sol b/packages/world/src/modules/core/implementations/AccessManagementSystem.sol index 0c9a35dbf0..6d9c5c6d99 100644 --- a/packages/world/src/modules/core/implementations/AccessManagementSystem.sol +++ b/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); diff --git a/packages/world/src/modules/core/implementations/BalanceTransferSystem.sol b/packages/world/src/modules/core/implementations/BalanceTransferSystem.sol index 87eac76733..fa7a30a566 100644 --- a/packages/world/src/modules/core/implementations/BalanceTransferSystem.sol +++ b/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()); diff --git a/packages/world/src/modules/core/implementations/BatchCallSystem.sol b/packages/world/src/modules/core/implementations/BatchCallSystem.sol index 3378714eab..b1d8e278ad 100644 --- a/packages/world/src/modules/core/implementations/BatchCallSystem.sol +++ b/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); diff --git a/packages/world/src/modules/core/implementations/ModuleInstallationSystem.sol b/packages/world/src/modules/core/implementations/ModuleInstallationSystem.sol index 62a1eb77fc..ccd340d26f 100644 --- a/packages/world/src/modules/core/implementations/ModuleInstallationSystem.sol +++ b/packages/world/src/modules/core/implementations/ModuleInstallationSystem.sol @@ -7,11 +7,13 @@ 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. @@ -19,7 +21,7 @@ contract ModuleInstallationSystem is System { * @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); diff --git a/packages/world/src/modules/core/implementations/StoreRegistrationSystem.sol b/packages/world/src/modules/core/implementations/StoreRegistrationSystem.sol index d0789eb170..40aadb7ea2 100644 --- a/packages/world/src/modules/core/implementations/StoreRegistrationSystem.sol +++ b/packages/world/src/modules/core/implementations/StoreRegistrationSystem.sol @@ -16,6 +16,7 @@ import { revertWithBytes } from "../../../revertWithBytes.sol"; import { IWorldErrors } from "../../../IWorldErrors.sol"; import { CORE_REGISTRATION_SYSTEM_ID } from "../constants.sol"; +import { LimitedCallContext } from "../LimitedCallContext.sol"; import { WorldRegistrationSystem } from "./WorldRegistrationSystem.sol"; @@ -23,7 +24,7 @@ import { WorldRegistrationSystem } from "./WorldRegistrationSystem.sol"; * @title Store Registration System * @dev This contract provides functionality for the registration of store-related resources within the World framework. */ -contract StoreRegistrationSystem is System, IWorldErrors { +contract StoreRegistrationSystem is System, IWorldErrors, LimitedCallContext { using WorldResourceIdInstance for ResourceId; /** @@ -44,7 +45,7 @@ contract StoreRegistrationSystem is System, IWorldErrors { Schema valueSchema, string[] calldata keyNames, string[] calldata fieldNames - ) public virtual { + ) public virtual onlyDelegatecall { // Require the name to not be the namespace's root name if (tableId.getName() == ROOT_NAME) { revert World_InvalidResourceId(tableId, tableId.toString()); @@ -69,7 +70,11 @@ contract StoreRegistrationSystem is System, IWorldErrors { * @param enabledHooksBitmap A bitmap indicating which hook functionalities are enabled. */ - function registerStoreHook(ResourceId tableId, IStoreHook hookAddress, uint8 enabledHooksBitmap) public virtual { + function registerStoreHook( + ResourceId tableId, + IStoreHook hookAddress, + uint8 enabledHooksBitmap + ) public virtual onlyDelegatecall { // Require the hook to implement the store hook interface requireInterface(address(hookAddress), type(IStoreHook).interfaceId); @@ -89,7 +94,7 @@ contract StoreRegistrationSystem is System, IWorldErrors { * @param tableId The resource ID of the table from which the hook is being unregistered. * @param hookAddress The address of the storage hook contract. */ - function unregisterStoreHook(ResourceId tableId, IStoreHook hookAddress) public virtual { + function unregisterStoreHook(ResourceId tableId, IStoreHook hookAddress) public virtual onlyDelegatecall { // 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 index e8df09a6ee..5b3fc1c526 100644 --- a/packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol +++ b/packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol @@ -29,11 +29,13 @@ import { FunctionSelectors } from "../../../codegen/tables/FunctionSelectors.sol import { FunctionSignatures } from "../../../codegen/tables/FunctionSignatures.sol"; import { requireNamespace } from "../../../requireNamespace.sol"; +import { LimitedCallContext } from "../LimitedCallContext.sol"; + /** * @title WorldRegistrationSystem * @dev This contract provides functions related to registering resources other than tables in the World. */ -contract WorldRegistrationSystem is System, IWorldErrors { +contract WorldRegistrationSystem is System, IWorldErrors, LimitedCallContext { using WorldResourceIdInstance for ResourceId; /** @@ -41,7 +43,7 @@ contract WorldRegistrationSystem is System, IWorldErrors { * @dev Creates a new namespace resource with the given ID * @param namespaceId The unique identifier for the new namespace */ - function registerNamespace(ResourceId namespaceId) public virtual { + function registerNamespace(ResourceId namespaceId) public virtual onlyDelegatecall { // Require namespace to be a valid namespace ID requireNamespace(namespaceId); @@ -67,7 +69,11 @@ contract WorldRegistrationSystem is System, IWorldErrors { * @param hookAddress The address of the hook being registered * @param enabledHooksBitmap Bitmap indicating which hooks are enabled */ - function registerSystemHook(ResourceId systemId, ISystemHook hookAddress, uint8 enabledHooksBitmap) public virtual { + function registerSystemHook( + ResourceId systemId, + ISystemHook hookAddress, + uint8 enabledHooksBitmap + ) public virtual onlyDelegatecall { // Require the provided system ID to have type RESOURCE_SYSTEM if (systemId.getType() != RESOURCE_SYSTEM) { revert World_InvalidResourceType(RESOURCE_SYSTEM, systemId, systemId.toString()); @@ -92,7 +98,7 @@ contract WorldRegistrationSystem is System, IWorldErrors { * @param systemId The ID of the system * @param hookAddress The address of the hook being unregistered */ - function unregisterSystemHook(ResourceId systemId, ISystemHook hookAddress) public virtual { + function unregisterSystemHook(ResourceId systemId, ISystemHook hookAddress) public virtual onlyDelegatecall { // Require caller to own the namespace AccessControl.requireOwner(systemId, _msgSender()); @@ -113,7 +119,7 @@ contract WorldRegistrationSystem is System, IWorldErrors { * @param system The system being registered * @param publicAccess Flag indicating if access control check is bypassed */ - function registerSystem(ResourceId systemId, System system, bool publicAccess) public virtual { + function registerSystem(ResourceId systemId, System system, bool publicAccess) public virtual onlyDelegatecall { // Require the provided system ID to have type RESOURCE_SYSTEM if (systemId.getType() != RESOURCE_SYSTEM) { revert World_InvalidResourceType(RESOURCE_SYSTEM, systemId, systemId.toString()); @@ -175,7 +181,7 @@ contract WorldRegistrationSystem is System, IWorldErrors { function registerFunctionSelector( ResourceId systemId, string memory systemFunctionSignature - ) public returns (bytes4 worldFunctionSelector) { + ) public onlyDelegatecall 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()); @@ -224,7 +230,7 @@ contract WorldRegistrationSystem is System, IWorldErrors { ResourceId systemId, string memory worldFunctionSignature, bytes4 systemFunctionSelector - ) public returns (bytes4 worldFunctionSelector) { + ) public onlyDelegatecall returns (bytes4 worldFunctionSelector) { // Require the caller to own the root namespace AccessControl.requireOwner(ROOT_NAMESPACE_ID, _msgSender()); @@ -250,7 +256,11 @@ contract WorldRegistrationSystem is System, IWorldErrors { * @param delegationControlId The ID controlling the delegation * @param initCallData The initialization data for the delegation */ - function registerDelegation(address delegatee, ResourceId delegationControlId, bytes memory initCallData) public { + function registerDelegation( + address delegatee, + ResourceId delegationControlId, + bytes memory initCallData + ) public onlyDelegatecall { // Store the delegation control contract address UserDelegationControl._set({ delegator: _msgSender(), @@ -290,7 +300,7 @@ contract WorldRegistrationSystem is System, IWorldErrors { ResourceId namespaceId, ResourceId delegationControlId, bytes memory initCallData - ) public { + ) public onlyDelegatecall { // Require namespace to be a valid namespace ID requireNamespace(namespaceId);