feat(world): add Balance table and BalanceTransferSystem #1425

alvrs · 2023-09-08T11:56:04Z

Fixes #1283
TODO

fix WorldContextConsumer
add test for WorldContextConsumer / WorldContextProvider with msgSender and msgValue
add logic for updating root namespace balance when sending to world without calling a system
add test for calling root systems with balance
add test for calling non-root systems with balance
add test for sending balance to world without calling a system
add test for transferring balance between namespaces
add test for transferring balance to address

changeset-bot · 2023-09-08T11:56:18Z

🦋 Changeset detected

Latest commit: 7ffde62

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 28 packages

Name	Type
@latticexyz/world	Major
@latticexyz/cli	Major
@latticexyz/dev-tools	Major
@latticexyz/store-sync	Major
@latticexyz/store-indexer	Major
@latticexyz/abi-ts	Major
@latticexyz/block-logs-stream	Major
@latticexyz/common	Major
@latticexyz/config	Major
create-mud	Major
@latticexyz/ecs-browser	Major
@latticexyz/gas-report	Major
@latticexyz/network	Major
@latticexyz/noise	Major
@latticexyz/phaserx	Major
@latticexyz/protocol-parser	Major
@latticexyz/react	Major
@latticexyz/recs	Major
@latticexyz/schema-type	Major
@latticexyz/services	Major
@latticexyz/solecs	Major
solhint-config-mud	Major
solhint-plugin-mud	Major
@latticexyz/std-client	Major
@latticexyz/std-contracts	Major
@latticexyz/store-cache	Major
@latticexyz/store	Major
@latticexyz/utils	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

holic · 2023-09-08T12:00:56Z

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

+
+    // Update the balances
+    Balances.set(fromNamespace, balance - amount);
+    payable(toAddress).transfer(amount);


I don't know the "why" but I've seen folks prefer this approach over the above:

(bool success, ) = toAddress.call{value: amount}("");

good catch! probably because of the 2300 gas limit of transfer (see https://twitter.com/obatirou/status/1697167795077698038 and https://medium.com/coinmonks/solidity-transfer-vs-send-vs-call-function-64c92cfc878a)

holic · 2023-09-08T12:01:40Z

packages/world/src/SystemCall.sol

+      bytes16 namespace = resourceSelector.getNamespace();
+      uint256 currentBalance = Balances.get(namespace);
+      Balances.set(namespace, currentBalance + value);
+    }


anything to be worried about here wrt reentry?

yes this smells like a re-entry attack

Can you elaborate how a reentry attack would work here?

This is a utility that is used by call, callFrom and the World's fallback function, in each case just passing the msg.value to the SystemCall util. If the contract that's called in here would call one of these entry points again, it would still only pass that new call's msg.value to the function.

I think we need to update the balance table value before calling the system, because the system might want to transfer the balance somewhere else in this call.

I'm not an expert on this, reentry + managing balances/funds just makes me nervous and so wanted to make sure we've considered it.

I think the important thing is that the balance transfer happens before any code that can be called/overridden/injected by malicious actors? In this case, requireAccess is the only thing I can see but it doesn't make any external calls, so I think it's fine?

Yes, requireAccess only checks an internal table.

Agree this is definitely a critical part of the system that we should be extra careful with and should also point this out as a focus area in the audit.

holic · 2023-09-08T17:02:37Z

do we need to protect against systems receiving value if not called through the world?
- similarly but not related to this PR, should we prevent systems being called without the world entry point?
does the world/root namespace have a way to withdraw all? if so, how should it update its accounting of balances? or will you have to specify the namespace to withdraw from?
what if I just send value to the world? do we need to account for that or prevent it?

ludns · 2023-09-08T17:40:56Z

you can never prevent someone from sending eth to a contract, a contract can always self destruct and send value and not trigger your fallback

alvrs · 2023-09-10T19:46:16Z

do we need to protect against systems receiving value if not called through the world?

similarly but not related to this PR, should we prevent systems being called without the world entry point?

Calling systems directly is not intended, but I don't think we should protect against it. As long as systems don't hold state it doesn't matter by who they're called since they will read/write on their caller by default. And there might be some advanced custom use cases where systems are limited to one World or do hold some internal state and the developers have some method for updating the internal state by calling them directly.

does the world/root namespace have a way to withdraw all? if so, how should it update its accounting of balances? or will you have to specify the namespace to withdraw from?

Whoever has root access can theoretically withdraw everything by deploying a new root system to transfer all the world contract's balance out, but it's basically a rug pull and not intended (we can't prevent it though). In the ideal world namespaces can only manage their own balances, which is the case once root namespace access is burned.

what if I just send value to the world? do we need to account for that or prevent it?

Right now the world can receive ETH without being attributed to any namespace via its receive function. We could update the root namespace's balance in that case?

.changeset/pink-tips-give.md

packages/world/src/World.sol

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

holic

Are there ways for funds to get into the world without triggering receive?

If so, do we want some way for the root namespace owner to withdraw address(world).balance - totalOfCurrentBalances as a safety/backstop?

ludns · 2023-09-12T11:52:39Z

Yes it is possible to not trigger receive (see my comment on selfdestruct)

…

On Tue, Sep 12, 2023 at 12:51 PM Kevin Ingersoll ***@***.***> wrote: ***@***.**** approved this pull request. Are there ways for funds to get into the world without triggering receive? If so, do we want some way for the root namespace owner to withdraw address(world).balance - totalOfCurrentBalances? — Reply to this email directly, view it on GitHub <#1425 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFBYUK4RYXLHZLD2QBF6KVDX2BEFRANCNFSM6AAAAAA4QIWAQQ> . You are receiving this because your review was requested.Message ID: ***@***.***>

alvrs · 2023-09-12T12:41:05Z

If so, do we want some way for the root namespace owner to withdraw address(world).balance - totalOfCurrentBalances as a safety/backstop?

I'd say because it requires real effort to send balance to the world without triggering receive we don't need to solve for this case (maintaining a totalOfCurrentBalances would increase the gas cost of every system call with value).

For anyone with root access to the World it's still possible to withdraw all balance from the World (with or without adjusting the Balances table) by registering a new root system.

holic · 2023-09-12T13:51:34Z

For anyone with root access to the World it's still possible to withdraw all balance from the World (with or without adjusting the Balances table by registering a new root system.

How so?

alvrs · 2023-09-12T15:24:25Z

How so?

Root systems are delegatecalled and therefore can access the world state directly (including the World contract's balance)

alvrs added 2 commits September 8, 2023 13:33

feat(world): store balances per namespace in Balances table

ced7f57

feat(world): add Balance table and BalanceTransferSystem

248aa88

vercel bot deployed to Preview September 8, 2023 11:56 View deployment

holic reviewed Sep 8, 2023

View reviewed changes

use call{value: amount}('') instead of transfer

ef1c784

vercel bot deployed to Preview September 11, 2023 11:19 View deployment

add test for WorldContext

4098d31

vercel bot deployed to Preview September 11, 2023 13:19 View deployment

add World Balance tests

da52884

vercel bot deployed to Preview September 12, 2023 09:49 View deployment

fix remaining world tests

975c9f0

vercel bot deployed to Preview September 12, 2023 10:09 View deployment

add changeset

4b67c44

vercel bot deployed to Preview September 12, 2023 10:26 View deployment

gas report

0f2fb9c

vercel bot deployed to Preview September 12, 2023 10:30 View deployment

alvrs commented Sep 12, 2023

View reviewed changes

.changeset/pink-tips-give.md Outdated Show resolved Hide resolved

alvrs commented Sep 12, 2023

View reviewed changes

.changeset/pink-tips-give.md Outdated Show resolved Hide resolved

alvrs commented Sep 12, 2023

View reviewed changes

packages/world/src/World.sol Outdated Show resolved Hide resolved

alvrs commented Sep 12, 2023

View reviewed changes

packages/world/src/modules/core/CoreModule.sol Outdated Show resolved Hide resolved

update changeset and comment

0b96621

vercel bot deployed to Preview September 12, 2023 10:45 View deployment

remove unrelated change

a660480

vercel bot deployed to Preview September 12, 2023 10:56 View deployment

Merge branch 'main' into alvrs/world-balance

7ffde62

alvrs marked this pull request as ready for review September 12, 2023 11:04

alvrs requested review from holic, dk1a and ludns September 12, 2023 11:05

vercel bot deployed to Preview September 12, 2023 11:05 View deployment

holic approved these changes Sep 12, 2023

View reviewed changes

alvrs merged commit 2ca75f9 into main Sep 12, 2023

alvrs deleted the alvrs/world-balance branch September 12, 2023 15:24

github-actions bot mentioned this pull request Sep 12, 2023

Version Packages (next) #1444

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(world): add Balance table and BalanceTransferSystem #1425

feat(world): add Balance table and BalanceTransferSystem #1425

alvrs commented Sep 8, 2023 •

edited

Loading

changeset-bot bot commented Sep 8, 2023 •

edited

Loading

holic Sep 8, 2023

alvrs Sep 10, 2023

holic Sep 8, 2023

ludns Sep 8, 2023

alvrs Sep 10, 2023

holic Sep 11, 2023

alvrs Sep 11, 2023

holic commented Sep 8, 2023

ludns commented Sep 8, 2023

alvrs commented Sep 10, 2023

holic left a comment •

edited

Loading

ludns commented Sep 12, 2023 via email

alvrs commented Sep 12, 2023 •

edited

Loading

holic commented Sep 12, 2023

alvrs commented Sep 12, 2023

feat(world): add Balance table and BalanceTransferSystem #1425

feat(world): add Balance table and BalanceTransferSystem #1425

Conversation

alvrs commented Sep 8, 2023 • edited Loading

changeset-bot bot commented Sep 8, 2023 • edited Loading

🦋 Changeset detected

holic Sep 8, 2023

Choose a reason for hiding this comment

alvrs Sep 10, 2023

Choose a reason for hiding this comment

holic Sep 8, 2023

Choose a reason for hiding this comment

ludns Sep 8, 2023

Choose a reason for hiding this comment

alvrs Sep 10, 2023

Choose a reason for hiding this comment

holic Sep 11, 2023

Choose a reason for hiding this comment

alvrs Sep 11, 2023

Choose a reason for hiding this comment

holic commented Sep 8, 2023

ludns commented Sep 8, 2023

alvrs commented Sep 10, 2023

holic left a comment • edited Loading

Choose a reason for hiding this comment

ludns commented Sep 12, 2023 via email

alvrs commented Sep 12, 2023 • edited Loading

holic commented Sep 12, 2023

alvrs commented Sep 12, 2023

alvrs commented Sep 8, 2023 •

edited

Loading

changeset-bot bot commented Sep 8, 2023 •

edited

Loading

holic left a comment •

edited

Loading

alvrs commented Sep 12, 2023 •

edited

Loading