From bebe624eff6368f35153cefbd1787b33efbaeedd Mon Sep 17 00:00:00 2001 From: Yaroslav Bolyukin Date: Tue, 19 Apr 2022 14:58:51 +0000 Subject: [PATCH] feat: prevent ouroboros creation during nest --- --- a/client/rpc/src/lib.rs +++ b/client/rpc/src/lib.rs @@ -77,11 +77,7 @@ ) -> Result>; #[rpc(name = "unique_totalSupply")] - fn total_supply( - &self, - collection: CollectionId, - at: Option, - ) -> Result; + fn total_supply(&self, collection: CollectionId, at: Option) -> Result; #[rpc(name = "unique_accountBalance")] fn account_balance( &self, --- a/pallets/common/src/lib.rs +++ b/pallets/common/src/lib.rs @@ -918,7 +918,7 @@ fn check_nesting( &self, sender: T::CrossAccountId, - from: CollectionId, + from: (CollectionId, TokenId), under: TokenId, budget: &dyn Budget, ) -> DispatchResult; --- a/pallets/fungible/src/common.rs +++ b/pallets/fungible/src/common.rs @@ -237,7 +237,7 @@ fn check_nesting( &self, _sender: ::CrossAccountId, - _from: CollectionId, + _from: (CollectionId, TokenId), _under: TokenId, _budget: &dyn Budget, ) -> sp_runtime::DispatchResult { --- a/pallets/fungible/src/lib.rs +++ b/pallets/fungible/src/lib.rs @@ -224,7 +224,12 @@ let dispatch = T::CollectionDispatch::dispatch(handle); let dispatch = dispatch.as_dyn(); - dispatch.check_nesting(from.clone(), collection.id, target.1, nesting_budget)?; + dispatch.check_nesting( + from.clone(), + (collection.id, TokenId::default()), + target.1, + nesting_budget, + )?; } // ========= @@ -293,7 +298,12 @@ let dispatch = T::CollectionDispatch::dispatch(handle); let dispatch = dispatch.as_dyn(); - dispatch.check_nesting(sender.clone(), collection.id, target.1, nesting_budget)?; + dispatch.check_nesting( + sender.clone(), + (collection.id, TokenId::default()), + target.1, + nesting_budget, + )?; } } @@ -386,10 +396,11 @@ if let Some(source) = T::CrossTokenAddressMapping::address_to_token(from) { // TODO: should collection owner be allowed to perform this transfer? ensure!( - >::indirectly_owned( + >::check_indirectly_owned( spender.clone(), source.0, source.1, + None, nesting_budget )?, >::ApprovedValueTooLow, --- a/pallets/nonfungible/src/common.rs +++ b/pallets/nonfungible/src/common.rs @@ -251,7 +251,7 @@ fn check_nesting( &self, sender: T::CrossAccountId, - from: CollectionId, + from: (CollectionId, TokenId), under: TokenId, budget: &dyn Budget, ) -> sp_runtime::DispatchResult { --- a/pallets/nonfungible/src/lib.rs +++ b/pallets/nonfungible/src/lib.rs @@ -296,7 +296,12 @@ let dispatch = T::CollectionDispatch::dispatch(handle); let dispatch = dispatch.as_dyn(); - dispatch.check_nesting(from.clone(), collection.id, target.1, nesting_budget)?; + dispatch.check_nesting( + from.clone(), + (collection.id, token), + target.1, + nesting_budget, + )?; } // ========= @@ -381,13 +386,18 @@ ); } - for (to, _) in balances.iter() { - if let Some(target) = T::CrossTokenAddressMapping::address_to_token(to) { + for (i, data) in data.iter().enumerate() { + let token = TokenId(first_token + i as u32 + 1); + if let Some(target) = T::CrossTokenAddressMapping::address_to_token(&data.owner) { let handle = >::try_get(target.0)?; let dispatch = T::CollectionDispatch::dispatch(handle); let dispatch = dispatch.as_dyn(); - - dispatch.check_nesting(sender.clone(), collection.id, target.1, nesting_budget)?; + dispatch.check_nesting( + sender.clone(), + (collection.id, token), + target.1, + nesting_budget, + )?; } } @@ -535,10 +545,11 @@ if let Some(source) = T::CrossTokenAddressMapping::address_to_token(from) { // TODO: should collection owner be allowed to perform this transfer? ensure!( - >::indirectly_owned( + >::check_indirectly_owned( spender.clone(), source.0, source.1, + None, nesting_budget )?, >::ApprovedValueTooLow, @@ -610,31 +621,40 @@ pub fn check_nesting( handle: &NonfungibleHandle, sender: T::CrossAccountId, - from: CollectionId, + from: (CollectionId, TokenId), under: TokenId, nesting_budget: &dyn Budget, ) -> DispatchResult { fn ensure_sender_allowed( collection: CollectionId, token: TokenId, + for_nest: (CollectionId, TokenId), sender: T::CrossAccountId, budget: &dyn Budget, ) -> DispatchResult { ensure!( - >::indirectly_owned(sender, collection, token, budget)?, + >::check_indirectly_owned( + sender, + collection, + token, + Some(for_nest), + budget + )?, >::OnlyOwnerAllowedToNest, ); Ok(()) } match handle.limits.nesting_rule() { NestingRule::Disabled => fail!(>::NestingIsDisabled), - NestingRule::Owner => ensure_sender_allowed::(handle.id, under, sender, nesting_budget)?, + NestingRule::Owner => { + ensure_sender_allowed::(handle.id, under, from, sender, nesting_budget)? + } NestingRule::OwnerRestricted(whitelist) => { ensure!( - whitelist.contains(&from), + whitelist.contains(&from.0), >::SourceCollectionIsNotAllowedToNest ); - ensure_sender_allowed::(handle.id, under, sender, nesting_budget)? + ensure_sender_allowed::(handle.id, under, from, sender, nesting_budget)? } } Ok(()) --- a/pallets/refungible/src/common.rs +++ b/pallets/refungible/src/common.rs @@ -260,7 +260,7 @@ fn check_nesting( &self, _sender: ::CrossAccountId, - _from: CollectionId, + _from: (CollectionId, TokenId), _under: TokenId, _budget: &dyn Budget, ) -> sp_runtime::DispatchResult { --- a/pallets/refungible/src/lib.rs +++ b/pallets/refungible/src/lib.rs @@ -352,7 +352,12 @@ let dispatch = T::CollectionDispatch::dispatch(handle); let dispatch = dispatch.as_dyn(); - dispatch.check_nesting(from.clone(), collection.id, target.1, nesting_budget)?; + dispatch.check_nesting( + from.clone(), + (collection.id, token), + target.1, + nesting_budget, + )?; } // ========= @@ -455,7 +460,8 @@ } } - for token in data.iter() { + for (i, token) in data.iter().enumerate() { + let token_id = TokenId(first_token_id + i as u32 + 1); for (to, _) in token.users.iter() { if let Some(target) = T::CrossTokenAddressMapping::address_to_token(to) { let handle = >::try_get(target.0)?; @@ -464,7 +470,7 @@ dispatch.check_nesting( sender.clone(), - collection.id, + (collection.id, token_id), target.1, nesting_budget, )?; @@ -575,10 +581,11 @@ if let Some(source) = T::CrossTokenAddressMapping::address_to_token(from) { // TODO: should collection owner be allowed to perform this transfer? ensure!( - >::indirectly_owned( + >::check_indirectly_owned( spender.clone(), source.0, source.1, + None, nesting_budget )?, >::ApprovedValueTooLow, --- a/pallets/structure/src/lib.rs +++ b/pallets/structure/src/lib.rs @@ -71,7 +71,7 @@ #[derive(PartialEq)] pub enum Parent { /// Token owned by normal account - Normal(CrossAccountId), + User(CrossAccountId), /// Passed token not found TokenNotFound, /// Token owner is another token (target token still may not exist) @@ -94,7 +94,7 @@ Ok(match handle.token_owner(token) { Some(owner) => match T::CrossTokenAddressMapping::address_to_token(&owner) { Some((collection, token)) => Parent::Token(collection, token), - None => Parent::Normal(owner), + None => Parent::User(owner), }, None => Parent::TokenNotFound, }) @@ -137,29 +137,46 @@ ) -> Result { let owner = Self::parent_chain(collection, token) .take_while(|_| budget.consume()) - .find(|p| matches!(p, Ok(Parent::Normal(_) | Parent::TokenNotFound))) + .find(|p| matches!(p, Ok(Parent::User(_) | Parent::TokenNotFound))) .ok_or(>::DepthLimit)??; Ok(match owner { - Parent::Normal(v) => v, + Parent::User(v) => v, _ => fail!(>::TokenNotFound), }) } /// Check if token indirectly owned by specified user - pub fn indirectly_owned( + pub fn check_indirectly_owned( user: T::CrossAccountId, collection: CollectionId, token: TokenId, + for_nest: Option<(CollectionId, TokenId)>, budget: &dyn Budget, ) -> Result { let target_parent = match T::CrossTokenAddressMapping::address_to_token(&user) { Some((collection, token)) => Parent::Token(collection, token), - None => Parent::Normal(user), + None => Parent::User(user), }; - Ok(Self::parent_chain(collection, token) - .take_while(|_| budget.consume()) - .any(|parent| Ok(&target_parent) == parent.as_ref())) + // Tried to nest token in itself + if Some((collection, token)) == for_nest { + return Err(>::OuroborosDetected.into()); + } + + for parent in Self::parent_chain(collection, token).take_while(|_| budget.consume()) { + match parent? { + // Tried to nest token in chain, which has this token as one of parents + Parent::Token(collection, token) if Some((collection, token)) == for_nest => { + return Err(>::OuroborosDetected.into()) + } + // Found needed parent, token is indirecty owned + v if v == target_parent => return Ok(true), + Parent::TokenNotFound => return Ok(false), + _ => {} + } + } + + Err(>::DepthLimit.into()) } } --- a/tests/src/eth/util/helpers.ts +++ b/tests/src/eth/util/helpers.ts @@ -23,7 +23,7 @@ import usingApi, {submitTransactionAsync} from '../../substrate/substrate-api'; import {IKeyringPair} from '@polkadot/types/types'; import {expect} from 'chai'; -import {getGenericResult, UNIQUE} from '../../util/helpers'; +import {CrossAccountId, getGenericResult, UNIQUE} from '../../util/helpers'; import * as solc from 'solc'; import config from '../../config'; import privateKey from '../../substrate/privateKey'; @@ -80,6 +80,11 @@ ]); return Web3.utils.toChecksumAddress('0x' + buf.toString('hex')); } +export function tokenIdToCross(collection: number, token: number): CrossAccountId { + return { + Ethereum: tokenIdToAddress(collection, token), + }; +} export function createEthAccount(web3: Web3) { const account = web3.eth.accounts.create(); --- /dev/null +++ b/tests/src/nesting/graphs.test.ts @@ -0,0 +1,51 @@ +import {ApiPromise} from '@polkadot/api'; +import {IKeyringPair} from '@polkadot/types/types'; +import {expect} from 'chai'; +import {tokenIdToCross} from '../eth/util/helpers'; +import privateKey from '../substrate/privateKey'; +import usingApi, {executeTransaction} from '../substrate/substrate-api'; +import {getCreateCollectionResult, transferExpectSuccess} from '../util/helpers'; + +/** + * ```dot + * 4 -> 3 -> 2 -> 1 + * 7 -> 6 -> 5 -> 2 + * 8 -> 5 + * ``` + */ +async function buildComplexObjectGraph(api: ApiPromise, sender: IKeyringPair): Promise { + const events = await executeTransaction(api, sender, api.tx.unique.createCollectionEx({mode: 'NFT'})); + const {collectionId} = getCreateCollectionResult(events); + + await executeTransaction(api, sender, api.tx.unique.createMultipleItemsEx(collectionId, {NFT: Array(8).fill({owner: {Substrate: sender.address}})})); + + await transferExpectSuccess(collectionId, 8, sender, tokenIdToCross(collectionId, 5)); + + await transferExpectSuccess(collectionId, 7, sender, tokenIdToCross(collectionId, 6)); + await transferExpectSuccess(collectionId, 6, sender, tokenIdToCross(collectionId, 5)); + await transferExpectSuccess(collectionId, 5, sender, tokenIdToCross(collectionId, 2)); + + await transferExpectSuccess(collectionId, 4, sender, tokenIdToCross(collectionId, 3)); + await transferExpectSuccess(collectionId, 3, sender, tokenIdToCross(collectionId, 2)); + await transferExpectSuccess(collectionId, 2, sender, tokenIdToCross(collectionId, 1)); + + return collectionId; +} + +describe('graphs', () => { + it('ouroboros can\'t be created in graph', async () => { + await usingApi(async api => { + const alice = privateKey('//Alice'); + const collection = await buildComplexObjectGraph(api, alice); + + // to self + await expect(executeTransaction(api, alice, api.tx.unique.transfer(tokenIdToCross(collection, 1), collection, 1, 1))) + .to.be.rejectedWith(/structure\.OuroborosDetected/); + // to nested part of graph + await expect(executeTransaction(api, alice, api.tx.unique.transfer(tokenIdToCross(collection, 5), collection, 1, 1))) + .to.be.rejectedWith(/structure\.OuroborosDetected/); + await expect(executeTransaction(api, alice, api.tx.unique.transfer(tokenIdToCross(collection, 8), collection, 2, 1))) + .to.be.rejectedWith(/structure\.OuroborosDetected/); + }); + }); +}); -- gitstuff