diff --git a/packages/store-consumer/src/experimental/WithWorld.sol b/packages/store-consumer/src/experimental/WithWorld.sol index bd7e25fd9e..2875604899 100644 --- a/packages/store-consumer/src/experimental/WithWorld.sol +++ b/packages/store-consumer/src/experimental/WithWorld.sol @@ -18,6 +18,7 @@ abstract contract WithWorld is WithStore, System { error WithWorld_RootNamespaceNotAllowed(); error WithWorld_NamespaceAlreadyExists(); error WithWorld_NamespaceDoesNotExists(); + error WithWorld_CallerHasNoNamespaceAccess(); error WithWorld_CallerIsNotWorld(); modifier onlyWorld() { @@ -27,6 +28,14 @@ abstract contract WithWorld is WithStore, System { _; } + modifier onlyNamespace() { + // We use WorldContextConsumer directly as we already know the world is the caller + if (!_callerIsWorld() || !ResourceAccess.get(getNamespaceId(), WorldContextConsumer._msgSender())) { + revert WithWorld_CallerHasNoNamespaceAccess(); + } + _; + } + constructor(IBaseWorld _world, bytes14 _namespace, bool registerNamespace) WithStore(address(_world)) { if (_namespace == bytes14(0)) { revert WithWorld_RootNamespaceNotAllowed(); diff --git a/packages/store-consumer/test/StoreConsumer.t.sol b/packages/store-consumer/test/StoreConsumer.t.sol index 201b2bae47..1070654a5a 100644 --- a/packages/store-consumer/test/StoreConsumer.t.sol +++ b/packages/store-consumer/test/StoreConsumer.t.sol @@ -52,7 +52,11 @@ contract MockWithWorld is WithWorld, MockStoreConsumer { getWorld().transferOwnership(getNamespaceId(), to); } + function callableByAnyone() external view {} + function onlyCallableByWorld() external view onlyWorld {} + + function onlyCallableByNamespace() external view onlyNamespace {} } contract StoreConsumerTest is Test, GasReporter { @@ -78,7 +82,8 @@ contract StoreConsumerTest is Test, GasReporter { assertTrue(ResourceIds.getExists(namespaceId), "Namespace not registered"); } - function testOnlyWorld() public { + // Test internal MUD access control + function testAccessControl() public { IBaseWorld world = createWorld(); StoreSwitch.setStoreAddress(address(world)); @@ -89,16 +94,71 @@ contract StoreConsumerTest is Test, GasReporter { MockWithWorld mock = new MockWithWorld(world, namespace, true); mock.transferNamespaceOwnership(address(this)); - // Register the mock as a system + // Register the mock as a system with PRIVATE access world.registerSystem(systemId, mock, false); address alice = address(0x1234); vm.prank(alice); vm.expectRevert(); - mock.onlyCallableByWorld(); + world.call(systemId, abi.encodeCall(mock.callableByAnyone, ())); + // After granting access to namespace, it should work world.grantAccess(namespaceId, alice); + vm.prank(alice); + world.call(systemId, abi.encodeCall(mock.callableByAnyone, ())); + } + + function testOnlyWorld() public { + IBaseWorld world = createWorld(); + StoreSwitch.setStoreAddress(address(world)); + + bytes16 systemName = "mySystem"; + bytes14 namespace = "myNamespace"; + ResourceId systemId = WorldResourceIdLib.encode(RESOURCE_SYSTEM, namespace, systemName); + MockWithWorld mock = new MockWithWorld(world, namespace, true); + mock.transferNamespaceOwnership(address(this)); + + // Register the mock as a system with PUBLIC access + world.registerSystem(systemId, mock, true); + + address alice = address(0x1234); + + vm.prank(alice); + vm.expectRevert(); + mock.onlyCallableByWorld(); + + vm.prank(alice); world.call(systemId, abi.encodeCall(mock.onlyCallableByWorld, ())); } + + function testOnlyNamespace() public { + IBaseWorld world = createWorld(); + StoreSwitch.setStoreAddress(address(world)); + + bytes16 systemName = "mySystem"; + bytes14 namespace = "myNamespace"; + ResourceId namespaceId = WorldResourceIdLib.encodeNamespace(namespace); + ResourceId systemId = WorldResourceIdLib.encode(RESOURCE_SYSTEM, namespace, systemName); + MockWithWorld mock = new MockWithWorld(world, namespace, true); + mock.transferNamespaceOwnership(address(this)); + + // Register the mock as a system with PUBLIC access + world.registerSystem(systemId, mock, true); + + address alice = address(0x1234); + + vm.prank(alice); + vm.expectRevert(); + mock.onlyCallableByNamespace(); + + vm.prank(alice); + vm.expectRevert(); + world.call(systemId, abi.encodeCall(mock.onlyCallableByNamespace, ())); + + // After granting access to namespace, it should work + world.grantAccess(namespaceId, alice); + vm.prank(alice); + world.call(systemId, abi.encodeCall(mock.onlyCallableByNamespace, ())); + } } diff --git a/packages/world-module-erc20/src/examples/ERC20WithWorld.sol b/packages/world-module-erc20/src/examples/ERC20WithWorld.sol index 74fdc5dc6e..eae98b639e 100644 --- a/packages/world-module-erc20/src/examples/ERC20WithWorld.sol +++ b/packages/world-module-erc20/src/examples/ERC20WithWorld.sol @@ -23,22 +23,22 @@ contract ERC20WithWorld is WithWorld, MUDERC20, ERC20Pausable, ERC20Burnable { world.transferOwnership(getNamespaceId(), _msgSender()); } - function mint(address to, uint256 value) public onlyWorld { + function mint(address to, uint256 value) public onlyNamespace { _mint(to, value); } - function pause() public onlyWorld { + function pause() public onlyNamespace { _pause(); } - function unpause() public onlyWorld { + function unpause() public onlyNamespace { _unpause(); } // The following functions are overrides required by Solidity. function _msgSender() public view override(Context, WithWorld) returns (address sender) { - return super._msgSender(); + return WithWorld._msgSender(); } function _update(address from, address to, uint256 value) internal override(MUDERC20, ERC20Pausable) { diff --git a/packages/world-module-erc20/src/experimental/ERC20Module.sol b/packages/world-module-erc20/src/experimental/ERC20Module.sol index bf45a67c3b..3f35711277 100644 --- a/packages/world-module-erc20/src/experimental/ERC20Module.sol +++ b/packages/world-module-erc20/src/experimental/ERC20Module.sol @@ -44,8 +44,8 @@ contract ERC20Module is Module { // Grant access to the token so it can write to tables after transferring ownership world.grantAccess(namespaceId, address(token)); - // Register token as a system so `onlyWorld` functions can be called through the world - world.registerSystem(systemId, token, false); + // Register token as a system so its functions can be called through the world + world.registerSystem(systemId, token, true); // The token should have transferred the namespace ownership to this module in its constructor world.transferOwnership(namespaceId, _msgSender()); diff --git a/packages/world-module-erc20/test/ERC20BaseTest.t.sol b/packages/world-module-erc20/test/ERC20BaseTest.t.sol index 16bf2bf3cb..dddf7d3d96 100644 --- a/packages/world-module-erc20/test/ERC20BaseTest.t.sol +++ b/packages/world-module-erc20/test/ERC20BaseTest.t.sol @@ -55,6 +55,11 @@ abstract contract ERC20BehaviorTest is Test, GasReporter, IERC20Events, IERC20Er function createToken() internal virtual returns (MockERC20Base); + // Used for validating the addresses in fuzz tests + function validAddress(address addr) internal view returns (bool) { + return addr != address(0) && addr != StoreSwitch.getStoreAddress(); + } + // TODO: startGasReport should be marked virtual so we can override function startGasReportWithPrefix(string memory name) internal { startGasReport(reportNameWithPrefix(name)); @@ -213,7 +218,7 @@ abstract contract ERC20BehaviorTest is Test, GasReporter, IERC20Events, IERC20Er } function testMint(address to, uint256 amount) public { - vm.assume(to != address(0)); + vm.assume(validAddress(to)); vm.expectEmit(true, true, true, true); emit Transfer(address(0), to, amount); @@ -224,7 +229,7 @@ abstract contract ERC20BehaviorTest is Test, GasReporter, IERC20Events, IERC20Er } function testBurn(address from, uint256 mintAmount, uint256 burnAmount) public { - vm.assume(from != address(0)); + vm.assume(validAddress(from)); vm.assume(burnAmount <= mintAmount); token.__mint(from, mintAmount); @@ -237,7 +242,7 @@ abstract contract ERC20BehaviorTest is Test, GasReporter, IERC20Events, IERC20Er } function testApprove(address to, uint256 amount) public { - vm.assume(to != address(0)); + vm.assume(validAddress(to)); assertTrue(token.approve(to, amount)); @@ -245,7 +250,7 @@ abstract contract ERC20BehaviorTest is Test, GasReporter, IERC20Events, IERC20Er } function testTransfer(address to, uint256 amount) public { - vm.assume(to != address(0)); + vm.assume(validAddress(to)); token.__mint(address(this), amount); vm.expectEmit(true, true, true, true); @@ -262,9 +267,9 @@ abstract contract ERC20BehaviorTest is Test, GasReporter, IERC20Events, IERC20Er } function testTransferFrom(address spender, address from, address to, uint256 approval, uint256 amount) public { - vm.assume(from != address(0)); - vm.assume(to != address(0)); - vm.assume(spender != address(0)); + vm.assume(validAddress(from)); + vm.assume(validAddress(to)); + vm.assume(validAddress(spender)); vm.assume(amount <= approval); token.__mint(from, amount); @@ -294,7 +299,7 @@ abstract contract ERC20BehaviorTest is Test, GasReporter, IERC20Events, IERC20Er } function testBurnInsufficientBalanceReverts(address to, uint256 mintAmount, uint256 burnAmount) public { - vm.assume(to != address(0)); + vm.assume(validAddress(to)); vm.assume(mintAmount < type(uint256).max); vm.assume(burnAmount > mintAmount); @@ -304,7 +309,7 @@ abstract contract ERC20BehaviorTest is Test, GasReporter, IERC20Events, IERC20Er } function testTransferInsufficientBalanceReverts(address to, uint256 mintAmount, uint256 sendAmount) public { - vm.assume(to != address(0)); + vm.assume(validAddress(to)); vm.assume(mintAmount < type(uint256).max); vm.assume(sendAmount > mintAmount); @@ -314,7 +319,7 @@ abstract contract ERC20BehaviorTest is Test, GasReporter, IERC20Events, IERC20Er } function testTransferFromInsufficientAllowanceReverts(address to, uint256 approval, uint256 amount) public { - vm.assume(to != address(0)); + vm.assume(validAddress(to)); vm.assume(approval < type(uint256).max); vm.assume(amount > approval); @@ -330,7 +335,7 @@ abstract contract ERC20BehaviorTest is Test, GasReporter, IERC20Events, IERC20Er } function testTransferFromInsufficientBalanceReverts(address to, uint256 mintAmount, uint256 sendAmount) public { - vm.assume(to != address(0)); + vm.assume(validAddress(to)); vm.assume(mintAmount < type(uint256).max); vm.assume(sendAmount > mintAmount);