Skip to content

Commit

Permalink
feat(world): require namespace to exist before registering systems/ta…
Browse files Browse the repository at this point in the history
…bles in it [C-01] (#2007)

Co-authored-by: Kevin Ingersoll <[email protected]>
  • Loading branch information
alvrs and holic authored Jan 11, 2024
1 parent f3d1ac8 commit 063daf8
Show file tree
Hide file tree
Showing 26 changed files with 337 additions and 120 deletions.
25 changes: 25 additions & 0 deletions .changeset/modern-brooms-rule.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
---
"@latticexyz/cli": patch
"@latticexyz/world-modules": patch
"@latticexyz/world": major
---

Previously `registerSystem` and `registerTable` had a side effect of registering namespaces if the system or table's namespace didn't exist yet.
This caused a possible frontrunning issue, where an attacker could detect a `registerSystem`/`registerTable` transaction in the mempool,
insert a `registerNamespace` transaction before it, grant themselves access to the namespace, transfer ownership of the namespace to the victim,
so that the `registerSystem`/`registerTable` transactions still went through successfully.
To mitigate this issue, the side effect of registering a namespace in `registerSystem` and `registerTable` has been removed.
Calls to these functions now expect the respective namespace to exist and the caller to own the namespace, otherwise they revert.

Changes in consuming projects are only necessary if tables or systems are registered manually.
If only the MUD deployer is used to register tables and systems, no changes are necessary, as the MUD deployer has been updated accordingly.

```diff
+ world.registerNamespace(namespaceId);
world.registerSystem(systemId, system, true);
```

```diff
+ world.registerNamespace(namespaceId);
MyTable.register();
```
6 changes: 5 additions & 1 deletion examples/minimal/packages/contracts/script/PostDeploy.s.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,16 @@ pragma solidity >=0.8.21;
import { Script } from "forge-std/Script.sol";
import { console } from "forge-std/console.sol";
import { StoreSwitch } from "@latticexyz/store/src/StoreSwitch.sol";
import { ResourceId, WorldResourceIdLib } from "@latticexyz/world/src/WorldResourceId.sol";
import { ResourceId, WorldResourceIdLib, WorldResourceIdInstance } from "@latticexyz/world/src/WorldResourceId.sol";
import { RESOURCE_SYSTEM } from "@latticexyz/world/src/worldResourceTypes.sol";

import { MessageTable, MessageTableTableId } from "../src/codegen/index.sol";
import { IWorld } from "../src/codegen/world/IWorld.sol";
import { ChatNamespacedSystem } from "../src/systems/ChatNamespacedSystem.sol";

contract PostDeploy is Script {
using WorldResourceIdInstance for ResourceId;

function run(address worldAddress) external {
// Specify a store so that you can use tables directly in PostDeploy
StoreSwitch.setStoreAddress(worldAddress);
Expand All @@ -29,8 +31,10 @@ contract PostDeploy is Script {
namespace: "namespace",
name: "ChatNamespaced"
});
IWorld(worldAddress).registerNamespace(systemId.getNamespaceId());
IWorld(worldAddress).registerSystem(systemId, chatNamespacedSystem, true);
IWorld(worldAddress).registerFunctionSelector(systemId, "sendMessage(string)");

// Grant this system access to MessageTable
IWorld(worldAddress).grantAccess(MessageTableTableId, address(chatNamespacedSystem));

Expand Down
42 changes: 0 additions & 42 deletions packages/cli/src/deploy/assertNamespaceOwner.ts

This file was deleted.

11 changes: 8 additions & 3 deletions packages/cli/src/deploy/deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ import { getWorldDeploy } from "./getWorldDeploy";
import { ensureFunctions } from "./ensureFunctions";
import { ensureModules } from "./ensureModules";
import { Table } from "./configToTables";
import { assertNamespaceOwner } from "./assertNamespaceOwner";
import { ensureNamespaceOwner } from "./ensureNamespaceOwner";
import { debug } from "./debug";
import { resourceLabel } from "./resourceLabel";
import { uniqueBy } from "@latticexyz/common/utils";
import { ensureContractsDeployed } from "./ensureContractsDeployed";
import { coreModuleBytecode, worldFactoryBytecode, worldFactoryContracts } from "./ensureWorldFactory";
import { worldFactoryContracts } from "./ensureWorldFactory";

type DeployOptions<configInput extends ConfigInput> = {
client: Client<Transport, Chain | undefined, Account>;
Expand Down Expand Up @@ -67,12 +67,17 @@ export async function deploy<configInput extends ConfigInput>({
throw new Error(`Unsupported World version: ${worldDeploy.worldVersion}`);
}

await assertNamespaceOwner({
const namespaceTxs = await ensureNamespaceOwner({
client,
worldDeploy,
resourceIds: [...tables.map((table) => table.tableId), ...systems.map((system) => system.systemId)],
});

debug("waiting for all namespace registration transactions to confirm");
for (const tx of namespaceTxs) {
await waitForTransactionReceipt(client, { hash: tx });
}

const tableTxs = await ensureTables({
client,
worldDeploy,
Expand Down
71 changes: 71 additions & 0 deletions packages/cli/src/deploy/ensureNamespaceOwner.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import { Account, Chain, Client, Hex, Transport, getAddress } from "viem";
import { WorldDeploy, worldAbi, worldTables } from "./common";
import { hexToResource, resourceToHex, writeContract } from "@latticexyz/common";
import { getResourceIds } from "./getResourceIds";
import { getTableValue } from "./getTableValue";
import { debug } from "./debug";

export async function ensureNamespaceOwner({
client,
worldDeploy,
resourceIds,
}: {
readonly client: Client<Transport, Chain | undefined, Account>;
readonly worldDeploy: WorldDeploy;
readonly resourceIds: readonly Hex[];
}): Promise<readonly Hex[]> {
const desiredNamespaces = Array.from(new Set(resourceIds.map((resourceId) => hexToResource(resourceId).namespace)));
const existingResourceIds = await getResourceIds({ client, worldDeploy });
const existingNamespaces = new Set(existingResourceIds.map((resourceId) => hexToResource(resourceId).namespace));
if (existingNamespaces.size) {
debug(
"found",
existingNamespaces.size,
"existing namespaces:",
Array.from(existingNamespaces)
.map((namespace) => (namespace === "" ? "<root>" : namespace))
.join(", ")
);
}

// Assert ownership of existing namespaces
const existingDesiredNamespaces = desiredNamespaces.filter((namespace) => existingNamespaces.has(namespace));
const namespaceOwners = await Promise.all(
existingDesiredNamespaces.map(async (namespace) => {
const { owner } = await getTableValue({
client,
worldDeploy,
table: worldTables.world_NamespaceOwner,
key: { namespaceId: resourceToHex({ type: "namespace", namespace, name: "" }) },
});
return [namespace, owner];
})
);

const unauthorizedNamespaces = namespaceOwners
.filter(([, owner]) => getAddress(owner) !== getAddress(client.account.address))
.map(([namespace]) => namespace);

if (unauthorizedNamespaces.length) {
throw new Error(`You are attempting to deploy to namespaces you do not own: ${unauthorizedNamespaces.join(", ")}`);
}

// Register missing namespaces
const missingNamespaces = desiredNamespaces.filter((namespace) => !existingNamespaces.has(namespace));
if (missingNamespaces.length > 0) {
debug("registering namespaces", Array.from(missingNamespaces).join(", "));
}
const registrationTxs = Promise.all(
missingNamespaces.map((namespace) =>
writeContract(client, {
chain: client.chain ?? null,
address: worldDeploy.address,
abi: worldAbi,
functionName: "registerNamespace",
args: [resourceToHex({ namespace, type: "namespace", name: "" })],
})
)
);

return registrationTxs;
}
28 changes: 14 additions & 14 deletions packages/world-modules/gas-report.json
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,13 @@
"file": "test/KeysInTableModule.t.sol",
"test": "testInstallComposite",
"name": "install keys in table module",
"gasUsed": 1413179
"gasUsed": 1413282
},
{
"file": "test/KeysInTableModule.t.sol",
"test": "testInstallGas",
"name": "install keys in table module",
"gasUsed": 1413179
"gasUsed": 1413282
},
{
"file": "test/KeysInTableModule.t.sol",
Expand All @@ -93,13 +93,13 @@
"file": "test/KeysInTableModule.t.sol",
"test": "testInstallSingleton",
"name": "install keys in table module",
"gasUsed": 1413179
"gasUsed": 1413282
},
{
"file": "test/KeysInTableModule.t.sol",
"test": "testSetAndDeleteRecordHookCompositeGas",
"name": "install keys in table module",
"gasUsed": 1413179
"gasUsed": 1413282
},
{
"file": "test/KeysInTableModule.t.sol",
Expand All @@ -117,7 +117,7 @@
"file": "test/KeysInTableModule.t.sol",
"test": "testSetAndDeleteRecordHookGas",
"name": "install keys in table module",
"gasUsed": 1413179
"gasUsed": 1413282
},
{
"file": "test/KeysInTableModule.t.sol",
Expand All @@ -135,7 +135,7 @@
"file": "test/KeysWithValueModule.t.sol",
"test": "testGetKeysWithValueGas",
"name": "install keys with value module",
"gasUsed": 653541
"gasUsed": 668083
},
{
"file": "test/KeysWithValueModule.t.sol",
Expand All @@ -153,7 +153,7 @@
"file": "test/KeysWithValueModule.t.sol",
"test": "testInstall",
"name": "install keys with value module",
"gasUsed": 653541
"gasUsed": 668083
},
{
"file": "test/KeysWithValueModule.t.sol",
Expand All @@ -165,7 +165,7 @@
"file": "test/KeysWithValueModule.t.sol",
"test": "testSetAndDeleteRecordHook",
"name": "install keys with value module",
"gasUsed": 653541
"gasUsed": 668083
},
{
"file": "test/KeysWithValueModule.t.sol",
Expand All @@ -183,7 +183,7 @@
"file": "test/KeysWithValueModule.t.sol",
"test": "testSetField",
"name": "install keys with value module",
"gasUsed": 653541
"gasUsed": 668083
},
{
"file": "test/KeysWithValueModule.t.sol",
Expand Down Expand Up @@ -267,7 +267,7 @@
"file": "test/StandardDelegationsModule.t.sol",
"test": "testCallFromCallboundDelegation",
"name": "register a callbound delegation",
"gasUsed": 118325
"gasUsed": 118319
},
{
"file": "test/StandardDelegationsModule.t.sol",
Expand All @@ -279,7 +279,7 @@
"file": "test/StandardDelegationsModule.t.sol",
"test": "testCallFromSystemDelegation",
"name": "register a systembound delegation",
"gasUsed": 115881
"gasUsed": 115875
},
{
"file": "test/StandardDelegationsModule.t.sol",
Expand All @@ -291,7 +291,7 @@
"file": "test/StandardDelegationsModule.t.sol",
"test": "testCallFromTimeboundDelegation",
"name": "register a timebound delegation",
"gasUsed": 112819
"gasUsed": 112813
},
{
"file": "test/StandardDelegationsModule.t.sol",
Expand All @@ -303,7 +303,7 @@
"file": "test/UniqueEntityModule.t.sol",
"test": "testInstall",
"name": "install unique entity module",
"gasUsed": 681852
"gasUsed": 694780
},
{
"file": "test/UniqueEntityModule.t.sol",
Expand All @@ -315,7 +315,7 @@
"file": "test/UniqueEntityModule.t.sol",
"test": "testInstallRoot",
"name": "installRoot unique entity module",
"gasUsed": 648249
"gasUsed": 663774
},
{
"file": "test/UniqueEntityModule.t.sol",
Expand Down
13 changes: 12 additions & 1 deletion packages/world-modules/src/modules/erc20-puppet/ERC20Module.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { ResourceId } from "@latticexyz/store/src/ResourceId.sol";
import { Module } from "@latticexyz/world/src/Module.sol";
import { WorldResourceIdLib } from "@latticexyz/world/src/WorldResourceId.sol";
import { IBaseWorld } from "@latticexyz/world/src/codegen/interfaces/IBaseWorld.sol";
import { revertWithBytes } from "@latticexyz/world/src/revertWithBytes.sol";

import { Puppet } from "../puppet/Puppet.sol";
import { createPuppet } from "../puppet/createPuppet.sol";
Expand Down Expand Up @@ -53,7 +54,10 @@ contract ERC20Module is Module {

// Register the ERC20 tables and system
IBaseWorld world = IBaseWorld(_world());
registrationLibrary.delegatecall(abi.encodeCall(ERC20ModuleRegistrationLibrary.register, (world, namespace)));
(bool success, bytes memory returnData) = registrationLibrary.delegatecall(
abi.encodeCall(ERC20ModuleRegistrationLibrary.register, (world, namespace))
);
if (!success) revertWithBytes(returnData);

// Initialize the Metadata
ERC20Metadata.set(_metadataTableId(namespace), metadata);
Expand All @@ -68,6 +72,7 @@ contract ERC20Module is Module {

// Register the ERC20 in the ERC20Registry
if (!ResourceIds.getExists(ERC20_REGISTRY_TABLE_ID)) {
world.registerNamespace(MODULE_NAMESPACE_ID);
ERC20Registry.register(ERC20_REGISTRY_TABLE_ID);
}
ERC20Registry.set(ERC20_REGISTRY_TABLE_ID, namespaceId, puppet);
Expand All @@ -83,6 +88,12 @@ contract ERC20ModuleRegistrationLibrary {
* Register systems and tables for a new ERC20 token in a given namespace
*/
function register(IBaseWorld world, bytes14 namespace) public {
// Register the namespace if it doesn't exist yet
ResourceId tokenNamespace = WorldResourceIdLib.encodeNamespace(namespace);
if (!ResourceIds.getExists(tokenNamespace)) {
world.registerNamespace(tokenNamespace);
}

// Register the tables
Allowances.register(_allowancesTableId(namespace));
Balances.register(_balancesTableId(namespace));
Expand Down
Loading

0 comments on commit 063daf8

Please sign in to comment.