Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(world): check namespace exists before balance transfer [L-03] #2095

Merged
merged 12 commits into from
Jan 12, 2024
Merged

fix(world): check namespace exists before balance transfer [L-03] #2095

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
994fd3a
fix(world): check namespace exists before balance transfer
yonadaa Jan 9, 2024
15b28c9
fix: not exists
yonadaa Jan 9, 2024
80dcec0
test: add test for resource not found
yonadaa Jan 9, 2024
f6263b5
test: comment
yonadaa Jan 9, 2024
dffd365
test: change name
yonadaa Jan 9, 2024
c977e6b
chore: gas report
yonadaa Jan 9, 2024
64e4820
refactor: use brackets for if statement
yonadaa Jan 11, 2024
1a5321c
Merge remote-tracking branch 'origin/main' into yonadaaa/namespace-ba…
yonadaa Jan 11, 2024
e5317df
Merge remote-tracking branch 'origin/main' into yonadaaa/namespace-ba…
yonadaa Jan 11, 2024
8d201d6
refactor: use requireExistence
yonadaa Jan 11, 2024
89fc0d8
chore: changeset
yonadaa Jan 11, 2024
962af4d
Update .changeset/seven-carpets-develop.md
holic Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions packages/world-modules/gas-report.json
View file
Open in desktop
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": 1413089
"gasUsed": 1413137
},
{
"file": "test/KeysInTableModule.t.sol",
"test": "testInstallGas",
"name": "install keys in table module",
"gasUsed": 1413089
"gasUsed": 1413137
},
{
"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": 1413089
"gasUsed": 1413137
},
{
"file": "test/KeysInTableModule.t.sol",
"test": "testSetAndDeleteRecordHookCompositeGas",
"name": "install keys in table module",
"gasUsed": 1413089
"gasUsed": 1413137
},
{
"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": 1413089
"gasUsed": 1413137
},
{
"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": 653436
"gasUsed": 653478
},
{
"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": 653436
"gasUsed": 653478
},
{
"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": 653436
"gasUsed": 653478
},
{
"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": 653436
"gasUsed": 653478
},
{
"file": "test/KeysWithValueModule.t.sol",
Expand Down Expand Up @@ -303,7 +303,7 @@
"file": "test/UniqueEntityModule.t.sol",
"test": "testInstall",
"name": "install unique entity module",
"gasUsed": 681732
"gasUsed": 681786
},
{
"file": "test/UniqueEntityModule.t.sol",
Expand All @@ -315,7 +315,7 @@
"file": "test/UniqueEntityModule.t.sol",
"test": "testInstallRoot",
"name": "installRoot unique entity module",
"gasUsed": 648129
"gasUsed": 648165
},
{
"file": "test/UniqueEntityModule.t.sol",
Expand Down
6 changes: 3 additions & 3 deletions packages/world/gas-report.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@
"file": "test/World.t.sol",
"test": "testRegisterNamespace",
"name": "Register a new namespace",
"gasUsed": 120887
"gasUsed": 120905
},
{
"file": "test/World.t.sol",
Expand All @@ -111,13 +111,13 @@
"file": "test/World.t.sol",
"test": "testRegisterSystem",
"name": "register a system",
"gasUsed": 164238
"gasUsed": 164256
},
{
"file": "test/World.t.sol",
"test": "testRegisterTable",
"name": "Register a new table in the namespace",
"gasUsed": 636447
"gasUsed": 636483
},
{
"file": "test/World.t.sol",
Expand Down
4 changes: 4 additions & 0 deletions packages/world/src/modules/core/implementations/BalanceTransferSystem.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
pragma solidity >=0.8.21;

import { ResourceId, ResourceIdInstance } from "@latticexyz/store/src/ResourceId.sol";
import { ResourceIds } from "@latticexyz/store/src/codegen/tables/ResourceIds.sol";

import { System } from "../../../System.sol";
import { revertWithBytes } from "../../../revertWithBytes.sol";
Expand Down Expand Up @@ -37,6 +38,9 @@ contract BalanceTransferSystem is System, IWorldErrors {
revert World_InvalidResourceType(RESOURCE_NAMESPACE, toNamespaceId, toNamespaceId.toString());
}

// Require the namespace to exist
if (!ResourceIds.getExists(toNamespaceId)) revert World_ResourceNotFound(toNamespaceId, toNamespaceId.toString());
yonadaa marked this conversation as resolved.
Show resolved Hide resolved

// Require caller to have access to the namespace
AccessControl.requireAccess(fromNamespaceId, _msgSender());
Copy link
Member

@holic holic Jan 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how much gas we'd incur if we added this check in requireAccess instead?

edit: see #2105


Expand Down
26 changes: 26 additions & 0 deletions packages/world/test/WorldBalance.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,32 @@ contract WorldBalanceTest is Test, GasReporter {
assertEq(Balances.get(invalidNamespace), 0);
}

function testTransferBalanceToNamespaceRevertResourceNotFound() public {
bytes14 toNamespace = "not_registered";
ResourceId toNamespaceId = WorldResourceIdLib.encodeNamespace(toNamespace);

uint256 value = 1 ether;

// Expect the root namespace to have no balance
assertEq(Balances.get(ROOT_NAMESPACE_ID), 0);

// Send balance to root namespace
vm.deal(caller, value);
vm.prank(caller);
(bool success, bytes memory data) = address(world).call{ value: value }("");
assertTrue(success);
assertEq(data.length, 0);

// Expect the root namespace to have the value as balance
assertEq(Balances.get(ROOT_NAMESPACE_ID), value);

// Expect revert when attempting to transfer to a non-existent namespace
vm.expectRevert(
abi.encodeWithSelector(IWorldErrors.World_ResourceNotFound.selector, toNamespaceId, toNamespaceId.toString())
);
world.transferBalanceToNamespace(ROOT_NAMESPACE_ID, toNamespaceId, value);
}

function testTransferBalanceToAddress() public {
uint256 value = 1 ether;

Expand Down
Loading