From 8a71c62c3569241a4dfd443b21cc1e930e2984d6 Mon Sep 17 00:00:00 2001 From: Thomas Marchand Date: Mon, 8 Jan 2024 16:27:11 +0000 Subject: [PATCH 1/3] fix: assert_is_owner --- src/naming/asserts.cairo | 50 +++++++++++++------------- src/tests/naming/test_abuses.cairo | 58 +++++++++++++++++++++++++++++- 2 files changed, 83 insertions(+), 25 deletions(-) diff --git a/src/naming/asserts.cairo b/src/naming/asserts.cairo index 5971cb7..aecb939 100644 --- a/src/naming/asserts.cairo +++ b/src/naming/asserts.cairo @@ -1,3 +1,4 @@ +use core::array::SpanTrait; use naming::{ interface::{ naming::{INaming, INamingDispatcher, INamingDispatcherTrait}, @@ -27,7 +28,7 @@ use openzeppelin::token::erc20::interface::{ }; use integer::{u256_safe_divmod, u256_as_non_zero}; use naming::naming::utils::UtilsTrait; - +use debug::PrintTrait; #[generate_trait] impl AssertionsImpl of AssertionsTrait { @@ -61,36 +62,37 @@ impl AssertionsImpl of AssertionsTrait { assert(get_block_timestamp() <= root_domain_data.expiry, 'this domain has expired'); } + // ensures you own a domain or one of its parents fn assert_is_owner( self: @Naming::ContractState, domain: Span, account: ContractAddress - ) -> u32 { - let hashed_domain = self.hash_domain(domain); - let data = self._domain_data.read(hashed_domain); + ) { + let mut i = 1; + let stop = domain.len() + 1; + let mut parent_key = 0; + loop { + assert(i != stop, 'you don\'t own this domain'); + let active_domain = domain.slice(domain.len() - i, i); + let hashed_domain = self.hash_domain(active_domain); + let data = self._domain_data.read(hashed_domain); - // because erc721 crashes on zero - let owner = if data.owner == 0 { - ContractAddressZeroable::zero() - } else { - IIdentityDispatcher { contract_address: self.starknetid_contract.read() } - .owner_from_id(data.owner) - }; + assert(data.parent_key == parent_key, 'a parent domain was reset'); - // if caller owns the starknet id, he owns the domain, we return the key - if owner == account { - return data.key; - }; + // because erc721 crashes on zero + let owner = if data.owner == 0 { + ContractAddressZeroable::zero() + } else { + IIdentityDispatcher { contract_address: self.starknetid_contract.read() } + .owner_from_id(data.owner) + }; - // otherwise, if it is a root domain, he doesn't own it - assert(domain.len() != 1 && domain.len() != 0, 'you don\'t own this domain'); + // if caller owns the identity, he controls the domain and its children + if owner == account { + break; + }; - // if he doesn't own the starknet id, and doesn't own the domain, he might own the parent domain - let parent_key = self.assert_is_owner(domain.slice(1, domain.len() - 1), account); - // we ensure that the key is the same as the parent key - // this is to allow to revoke all subdomains in o(1) writes, by juste updating the key of the parent - if (data.parent_key != 0) { - assert(parent_key == data.parent_key, 'you no longer own this domain'); + parent_key = data.key; + i += 1; }; - data.key } // this ensures a non expired domain is not already written on this identity diff --git a/src/tests/naming/test_abuses.cairo b/src/tests/naming/test_abuses.cairo index 3422483..806190f 100644 --- a/src/tests/naming/test_abuses.cairo +++ b/src/tests/naming/test_abuses.cairo @@ -22,7 +22,6 @@ use naming::naming::main::Naming; use naming::pricing::Pricing; use super::common::deploy; - #[test] #[available_gas(2000000000)] #[should_panic(expected: ('u256_sub Overflow', 'ENTRYPOINT_FAILED', 'ENTRYPOINT_FAILED'))] @@ -243,3 +242,60 @@ fn test_non_admin_cannot_claim_balance() { naming.claim_balance(eth.contract_address); } +#[test] +#[available_gas(2000000000)] +#[should_panic(expected: ('a parent domain was reset', 'ENTRYPOINT_FAILED'))] +fn test_use_reset_subdomains() { + // setup + let (eth, pricing, identity, naming) = deploy(); + let alpha = contract_address_const::<0x123>(); + let bravo = contract_address_const::<0x456>(); + + // we mint the ids + + set_contract_address(alpha); + identity.mint(1); + set_contract_address(bravo); + identity.mint(2); + + set_contract_address(alpha); + let aller: felt252 = 35683102; + + // we check how much a domain costs + let (_, price) = pricing.compute_buy_price(5, 365); + + // we allow the naming to take our money + eth.approve(naming.contract_address, price); + + // we buy with no resolver, no sponsor, no discount and empty metadata + naming + .buy(1, aller, 365, ContractAddressZeroable::zero(), ContractAddressZeroable::zero(), 0, 0); + + let root_domain = array![aller].span(); + let subdomain = array![aller, aller].span(); + + // we transfer aller.aller.stark to id2 + naming.transfer_domain(subdomain, 2); + + // and make sure the owner has been updated + assert(naming.domain_to_id(subdomain) == 2, 'owner not updated correctly'); + + // now bravo should be able to create a subsubdomain (charlie.aller.aller.stark): + set_contract_address(bravo); + let subsubdomain = array!['charlie', aller, aller].span(); + naming.transfer_domain(subsubdomain, 3); + + // alpha resets subdomains of aller.stark + set_contract_address(alpha); + naming.reset_subdomains(root_domain); + + // ensure aller.stark still resolves + assert(naming.domain_to_id(root_domain) == 1, 'owner not updated correctly'); + // ensure the subdomain was reset + assert(naming.domain_to_id(subdomain) == 0, 'owner not updated correctly'); + + set_contract_address(bravo); + let subsubdomain2 = array!['delta', aller, aller].span(); + naming.transfer_domain(subsubdomain2, 4); +} + From 8d63a6df4ca17c6e18c488217372f98a4f4b3390 Mon Sep 17 00:00:00 2001 From: Thomas Marchand Date: Mon, 8 Jan 2024 16:29:29 +0000 Subject: [PATCH 2/3] chore: improve gas usage --- src/naming/asserts.cairo | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/naming/asserts.cairo b/src/naming/asserts.cairo index aecb939..5e344b2 100644 --- a/src/naming/asserts.cairo +++ b/src/naming/asserts.cairo @@ -1,3 +1,4 @@ +use core::traits::TryInto; use core::array::SpanTrait; use naming::{ interface::{ @@ -66,12 +67,13 @@ impl AssertionsImpl of AssertionsTrait { fn assert_is_owner( self: @Naming::ContractState, domain: Span, account: ContractAddress ) { - let mut i = 1; - let stop = domain.len() + 1; + let mut i: felt252 = 1; + let stop = (domain.len() + 1).into(); let mut parent_key = 0; loop { assert(i != stop, 'you don\'t own this domain'); - let active_domain = domain.slice(domain.len() - i, i); + let i_gas_saver = i.try_into().unwrap(); + let active_domain = domain.slice(domain.len() - i_gas_saver, i_gas_saver); let hashed_domain = self.hash_domain(active_domain); let data = self._domain_data.read(hashed_domain); From cfa4acccfa0296cabb914011bf37b524b64060d5 Mon Sep 17 00:00:00 2001 From: Thomas Marchand Date: Wed, 10 Jan 2024 11:41:47 +0000 Subject: [PATCH 3/3] fix: remove unused imports to debug traits --- src/naming/asserts.cairo | 1 - src/tests/naming/common.cairo | 1 - src/tests/naming/test_abuses.cairo | 1 - src/tests/naming/test_custom_resolver.cairo | 2 -- src/tests/naming/test_features.cairo | 1 - src/tests/naming/test_usecases.cairo | 1 - src/tests/test_pricing.cairo | 1 - 7 files changed, 8 deletions(-) diff --git a/src/naming/asserts.cairo b/src/naming/asserts.cairo index 5e344b2..e6b40de 100644 --- a/src/naming/asserts.cairo +++ b/src/naming/asserts.cairo @@ -29,7 +29,6 @@ use openzeppelin::token::erc20::interface::{ }; use integer::{u256_safe_divmod, u256_as_non_zero}; use naming::naming::utils::UtilsTrait; -use debug::PrintTrait; #[generate_trait] impl AssertionsImpl of AssertionsTrait { diff --git a/src/tests/naming/common.cairo b/src/tests/naming/common.cairo index 285737d..4816d62 100644 --- a/src/tests/naming/common.cairo +++ b/src/tests/naming/common.cairo @@ -1,6 +1,5 @@ use array::ArrayTrait; use array::SpanTrait; -use debug::PrintTrait; use option::OptionTrait; use zeroable::Zeroable; use traits::Into; diff --git a/src/tests/naming/test_abuses.cairo b/src/tests/naming/test_abuses.cairo index 806190f..14597c7 100644 --- a/src/tests/naming/test_abuses.cairo +++ b/src/tests/naming/test_abuses.cairo @@ -1,6 +1,5 @@ use array::ArrayTrait; use array::SpanTrait; -use debug::PrintTrait; use option::OptionTrait; use zeroable::Zeroable; use traits::Into; diff --git a/src/tests/naming/test_custom_resolver.cairo b/src/tests/naming/test_custom_resolver.cairo index 6f758d7..a47f87b 100644 --- a/src/tests/naming/test_custom_resolver.cairo +++ b/src/tests/naming/test_custom_resolver.cairo @@ -1,6 +1,5 @@ use array::ArrayTrait; use array::SpanTrait; -use debug::PrintTrait; use option::OptionTrait; use zeroable::Zeroable; use traits::Into; @@ -28,7 +27,6 @@ use naming::interface::resolver::IResolver; mod CustomResolver { use core::array::SpanTrait; use naming::interface::resolver::IResolver; - use debug::PrintTrait; #[storage] struct Storage {} diff --git a/src/tests/naming/test_features.cairo b/src/tests/naming/test_features.cairo index bec6f67..9809059 100644 --- a/src/tests/naming/test_features.cairo +++ b/src/tests/naming/test_features.cairo @@ -1,6 +1,5 @@ use array::ArrayTrait; use array::SpanTrait; -use debug::PrintTrait; use option::OptionTrait; use zeroable::Zeroable; use traits::Into; diff --git a/src/tests/naming/test_usecases.cairo b/src/tests/naming/test_usecases.cairo index cf1cf8b..f08ad75 100644 --- a/src/tests/naming/test_usecases.cairo +++ b/src/tests/naming/test_usecases.cairo @@ -1,6 +1,5 @@ use array::ArrayTrait; use array::SpanTrait; -use debug::PrintTrait; use option::OptionTrait; use zeroable::Zeroable; use traits::Into; diff --git a/src/tests/test_pricing.cairo b/src/tests/test_pricing.cairo index 5cd7fa9..f24026d 100644 --- a/src/tests/test_pricing.cairo +++ b/src/tests/test_pricing.cairo @@ -1,5 +1,4 @@ use array::ArrayTrait; -use debug::PrintTrait; use zeroable::Zeroable; use traits::Into;