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

world: make world events only emitable by world #1092

Closed
wants to merge 2 commits into from
Closed

world: make world events only emitable by world #1092

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
660da6a
world: make world events only emitable by world
notV4l Oct 24, 2023
ef50de7
update manifest testdata
notV4l Oct 24, 2023
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
13 changes: 13 additions & 0 deletions crates/dojo-core/src/world.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ mod world {
models: LegacyMap::<felt252, ClassHash>,
owners: LegacyMap::<(felt252, ContractAddress), bool>,
writers: LegacyMap::<(felt252, ContractAddress), bool>,
reserved_events: LegacyMap::<felt252, bool>,
}

#[constructor]
Expand All @@ -174,6 +175,17 @@ mod world {
self.contract_base.write(contract_base);
self.owners.write((WORLD, creator), true);

self.reserved_events.write(selector!("WorldSpawned"), true);
self.reserved_events.write(selector!("ContractDeployed"), true);
self.reserved_events.write(selector!("ContractUpgraded"), true);
self.reserved_events.write(selector!("MetadataUpdate"), true);
self.reserved_events.write(selector!("ModelRegistered"), true);
self.reserved_events.write(selector!("StoreSetRecord"), true);
self.reserved_events.write(selector!("StoreDelRecord"), true);
self.reserved_events.write(selector!("WriterUpdated"), true);
self.reserved_events.write(selector!("OwnerUpdated"), true);
self.reserved_events.write(selector!("ExecutorUpdated"), true);

EventEmitter::emit(ref self, WorldSpawned { address: get_contract_address(), creator });
}

Expand Down Expand Up @@ -455,6 +467,7 @@ mod world {
/// * `keys` - The keys of the event.
/// * `values` - The data to be logged by the event.
fn emit(self: @ContractState, mut keys: Array<felt252>, values: Span<felt252>) {
assert(keys.len() > 0 && !self.reserved_events.read(*keys.at(0)), 'reserved event name');
Copy link
Contributor

Choose a reason for hiding this comment

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

i would prefer not to add this additional overhead to every emit.

instead, consumers should properly validate events and ensure that the caller address isn't added (see below) for world events

Copy link
Contributor

Choose a reason for hiding this comment

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

so the change here should really be in torii server, where we must verify the event data length is what we expect

Copy link
Collaborator Author

@notV4l notV4l Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

i would prefer not to add this additional overhead to every emit.

instead, consumers should properly validate events and ensure that the caller address isn't added (see below) for world events

still possible to emit fake event for any event where last field is ContractAddress type or even Span / bool

Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think so, since the event data will be incorrectly serialized

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes indeed mb, caller_address is added to keys not values

let system = get_caller_address();
system.serialize(ref keys);
emit_event_syscall(keys.span(), values).unwrap_syscall();
Expand Down
87 changes: 87 additions & 0 deletions crates/dojo-core/src/world_test.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,3 +399,90 @@ fn test_execute_multiple_worlds() {
assert(data2.a == 7331, 'data2 not stored');
}



#[starknet::interface]
trait IMalicious<TContractState> {
fn emit_worldspawned(self: @TContractState);
fn emit_modelregistered(self: @TContractState);
}

#[dojo::contract]
mod malicious {
use starknet::get_caller_address;
use super::ContractAddress;
use super::ClassHash;

#[event]
#[derive(Drop, starknet::Event)]
enum Event {
WorldSpawned: WorldSpawned,
ModelRegistered: ModelRegistered,
}

#[derive(Drop, starknet::Event)]
struct WorldSpawned {
address: ContractAddress,
creator: ContractAddress
}

#[derive(Drop, starknet::Event)]
struct ModelRegistered {
name: felt252,
class_hash: ClassHash,
prev_class_hash: ClassHash
}


#[external(v0)]
impl IMaliciousImpl of super::IMalicious<ContractState> {
fn emit_worldspawned(self: @ContractState) {
let address = get_caller_address();
emit!(self.world_dispatcher.read(), WorldSpawned {
address: address,
creator: address,
})
}

fn emit_modelregistered(self: @ContractState) {
let address = get_caller_address();
emit!(self.world_dispatcher.read(), ModelRegistered {
name: 'Malicious',
class_hash: 0.try_into().unwrap(),
prev_class_hash: 0.try_into().unwrap(),
});
}
}
}

#[test]
#[available_gas(60000000)]
#[should_panic(expected:('reserved event name','ENTRYPOINT_FAILED', 'ENTRYPOINT_FAILED'))]
fn test_reserved_event_worldspawned() {
let world = spawn_test_world(array![foo::TEST_CLASS_HASH],);
let malicious_contract = IMaliciousDispatcher {
contract_address: world.deploy_contract('peper', malicious::TEST_CLASS_HASH.try_into().unwrap())
};
malicious_contract.emit_worldspawned();
}

#[test]
#[available_gas(60000000)]
#[should_panic(expected:('reserved event name','ENTRYPOINT_FAILED', 'ENTRYPOINT_FAILED'))]
fn test_reserved_event_modelregistered() {
let world = spawn_test_world(array![foo::TEST_CLASS_HASH],);
let malicious_contract = IMaliciousDispatcher {
contract_address: world.deploy_contract('tree', malicious::TEST_CLASS_HASH.try_into().unwrap())
};
malicious_contract.emit_modelregistered();
}

#[test]
#[available_gas(60000000)]
#[should_panic(expected:('reserved event name','ENTRYPOINT_FAILED'))]
fn test_reserved_event_storesetrecord() {
let world = spawn_test_world(array![foo::TEST_CLASS_HASH],);
let mut keys = array![selector!("StoreSetRecord")];
let mut values = array![];
world.emit(keys, values.span());
}
2 changes: 1 addition & 1 deletion crates/dojo-lang/src/manifest_test_data/manifest
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ test_manifest_file
"world": {
"name": "world",
"address": null,
"class_hash": "0x5179c281a8d3cca6cfb820201fa7a81150b56f9db492405a29e9564222370b8",
"class_hash": "0x83ec4c151ba2dc7c4debc96430272afc6f36f2f750b83e072cd1da88ed19cb",
"abi": [
{
"type": "impl",
Expand Down
Loading