From 0268ee10ccc3499f6d7c31cc3e9e7c0132cdd628 Mon Sep 17 00:00:00 2001 From: Szegoo Date: Tue, 30 Aug 2022 14:43:13 +0200 Subject: [PATCH 1/8] reset approved account --- frame/uniques/src/functions.rs | 7 +++++++ frame/uniques/src/lib.rs | 2 ++ frame/uniques/src/tests.rs | 14 ++++++++++++++ 3 files changed, 23 insertions(+) diff --git a/frame/uniques/src/functions.rs b/frame/uniques/src/functions.rs index 107214558307f..7f360f787793b 100644 --- a/frame/uniques/src/functions.rs +++ b/frame/uniques/src/functions.rs @@ -48,6 +48,13 @@ impl, I: 'static> Pallet { Account::::insert((&dest, &collection, &item), ()); let origin = details.owner; details.owner = dest; + + // The approved account has to be reset to None, because otherwise + // pre-approve attack would be possible, where the owner can approve his + // second account before making the transaction and then claiming the + // item back. + details.approved = None; + Item::::insert(&collection, &item, &details); ItemPriceOf::::remove(&collection, &item); diff --git a/frame/uniques/src/lib.rs b/frame/uniques/src/lib.rs index 70f10ca4f8b39..481dcccd77017 100644 --- a/frame/uniques/src/lib.rs +++ b/frame/uniques/src/lib.rs @@ -914,6 +914,8 @@ pub mod pallet { /// - `item`: The item of the item to be approved for delegated transfer. /// - `delegate`: The account to delegate permission to transfer the item. /// + /// Important NOTE: The `approved` account gets resest after each transfer. + /// /// Emits `ApprovedTransfer` on success. /// /// Weight: `O(1)` diff --git a/frame/uniques/src/tests.rs b/frame/uniques/src/tests.rs index 8b1d00d7ba0c7..b124175da2314 100644 --- a/frame/uniques/src/tests.rs +++ b/frame/uniques/src/tests.rs @@ -557,6 +557,20 @@ fn approval_lifecycle_works() { }); } +#[test] +fn approved_account_gets_reset_after_transfer() { + new_test_ext().execute_with(|| { + assert_ok!(Uniques::force_create(Origin::root(), 0, 1, true)); + assert_ok!(Uniques::mint(Origin::signed(1), 0, 42, 2)); + + assert_ok!(Uniques::approve_transfer(Origin::signed(2), 0, 42, 3)); + assert_ok!(Uniques::transfer(Origin::signed(2), 0, 42, 5)); + + // this shouldn't work because we have just transfered the item to another account. + assert_noop!(Uniques::transfer(Origin::signed(3), 0, 42, 4), Error::::NoPermission); + }); +} + #[test] fn cancel_approval_works() { new_test_ext().execute_with(|| { From cc2545318a79fc7c5a642352b8b47d771a30d487 Mon Sep 17 00:00:00 2001 From: Szegoo Date: Tue, 30 Aug 2022 14:55:37 +0200 Subject: [PATCH 2/8] wrap at 100 --- frame/uniques/src/functions.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/frame/uniques/src/functions.rs b/frame/uniques/src/functions.rs index 7f360f787793b..4e68f1139420d 100644 --- a/frame/uniques/src/functions.rs +++ b/frame/uniques/src/functions.rs @@ -49,10 +49,9 @@ impl, I: 'static> Pallet { let origin = details.owner; details.owner = dest; - // The approved account has to be reset to None, because otherwise - // pre-approve attack would be possible, where the owner can approve his - // second account before making the transaction and then claiming the - // item back. + // The approved account has to be reset to None, because otherwise pre-approve attack would + // be possible, where the owner can approve his second account before making the transaction + // and then claiming the item back. details.approved = None; Item::::insert(&collection, &item, &details); From a6bf6de4f2c2700bc2db9d0fabba600a2bce956a Mon Sep 17 00:00:00 2001 From: Szegoo Date: Tue, 30 Aug 2022 14:57:44 +0200 Subject: [PATCH 3/8] doc --- frame/uniques/src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frame/uniques/src/lib.rs b/frame/uniques/src/lib.rs index 481dcccd77017..210711306524d 100644 --- a/frame/uniques/src/lib.rs +++ b/frame/uniques/src/lib.rs @@ -604,7 +604,9 @@ pub mod pallet { }) } - /// Move an item from the sender account to another. + /// Move an item from the sender account to another. + /// + /// This resets the approved account of the item. /// /// Origin must be Signed and the signing account must be either: /// - the Admin of the `collection`; From d438127debfb742bf753d13c1407f824e20ddf2e Mon Sep 17 00:00:00 2001 From: Szegoo Date: Tue, 30 Aug 2022 15:12:17 +0200 Subject: [PATCH 4/8] fmt --- frame/uniques/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/uniques/src/lib.rs b/frame/uniques/src/lib.rs index 210711306524d..de69102e21d2e 100644 --- a/frame/uniques/src/lib.rs +++ b/frame/uniques/src/lib.rs @@ -604,7 +604,7 @@ pub mod pallet { }) } - /// Move an item from the sender account to another. + /// Move an item from the sender account to another. /// /// This resets the approved account of the item. /// From f537593b68bb095e5c9b7ebc0b0e58274d61fa8a Mon Sep 17 00:00:00 2001 From: Sergej Sakac <73715684+Szegoo@users.noreply.github.com> Date: Tue, 30 Aug 2022 15:38:27 +0200 Subject: [PATCH 5/8] Update frame/uniques/src/tests.rs Co-authored-by: Oliver Tale-Yazdi --- frame/uniques/src/tests.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frame/uniques/src/tests.rs b/frame/uniques/src/tests.rs index b124175da2314..3eaaa6020f104 100644 --- a/frame/uniques/src/tests.rs +++ b/frame/uniques/src/tests.rs @@ -568,6 +568,8 @@ fn approved_account_gets_reset_after_transfer() { // this shouldn't work because we have just transfered the item to another account. assert_noop!(Uniques::transfer(Origin::signed(3), 0, 42, 4), Error::::NoPermission); + // The new owner can transfer fine: + assert_ok!(Uniques::transfer(Origin::signed(5), 0, 42, 6)); }); } From 5891115c72719162a04dc1d20a61e8d257d0cff2 Mon Sep 17 00:00:00 2001 From: Szegoo Date: Tue, 30 Aug 2022 15:57:02 +0200 Subject: [PATCH 6/8] new test --- frame/uniques/src/tests.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/frame/uniques/src/tests.rs b/frame/uniques/src/tests.rs index 3eaaa6020f104..c5b6c0c753adf 100644 --- a/frame/uniques/src/tests.rs +++ b/frame/uniques/src/tests.rs @@ -573,6 +573,28 @@ fn approved_account_gets_reset_after_transfer() { }); } +#[test] +fn approved_account_gets_reset_after_buy_item() { + new_test_ext().execute_with(|| { + let item = 1; + let price = 15; + + Balances::make_free_balance_be(&2, 100); + + assert_ok!(Uniques::force_create(Origin::root(), 0, 1, true)); + assert_ok!(Uniques::mint(Origin::signed(1), 0, item, 1)); + assert_ok!(Uniques::approve_transfer(Origin::signed(1), 0, item, 5)); + + assert_ok!(Uniques::set_price(Origin::signed(1), 0, item, Some(price), None)); + + assert_ok!(Uniques::buy_item(Origin::signed(2), 0, item, price)); + + // this shouldn't work because the item has been bough and the approved account should be + // reset. + assert_noop!(Uniques::transfer(Origin::signed(5), 0, item, 4), Error::::NoPermission); + }); +} + #[test] fn cancel_approval_works() { new_test_ext().execute_with(|| { From 550978812d1c31a6a1bc93fa88c31e29275c94db Mon Sep 17 00:00:00 2001 From: Sergej Sakac <73715684+Szegoo@users.noreply.github.com> Date: Tue, 30 Aug 2022 15:58:00 +0200 Subject: [PATCH 7/8] Update frame/uniques/src/lib.rs Co-authored-by: Keith Yeung --- frame/uniques/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/uniques/src/lib.rs b/frame/uniques/src/lib.rs index de69102e21d2e..3b0cf6dc673c9 100644 --- a/frame/uniques/src/lib.rs +++ b/frame/uniques/src/lib.rs @@ -916,7 +916,7 @@ pub mod pallet { /// - `item`: The item of the item to be approved for delegated transfer. /// - `delegate`: The account to delegate permission to transfer the item. /// - /// Important NOTE: The `approved` account gets resest after each transfer. + /// Important NOTE: The `approved` account gets reset after each transfer. /// /// Emits `ApprovedTransfer` on success. /// From 83f8121d95fde06e7b5a06a2654fb99e310b8648 Mon Sep 17 00:00:00 2001 From: Szegoo Date: Tue, 30 Aug 2022 16:02:45 +0200 Subject: [PATCH 8/8] fmt --- frame/uniques/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/uniques/src/tests.rs b/frame/uniques/src/tests.rs index c5b6c0c753adf..85d1bec574cf0 100644 --- a/frame/uniques/src/tests.rs +++ b/frame/uniques/src/tests.rs @@ -588,7 +588,7 @@ fn approved_account_gets_reset_after_buy_item() { assert_ok!(Uniques::set_price(Origin::signed(1), 0, item, Some(price), None)); assert_ok!(Uniques::buy_item(Origin::signed(2), 0, item, price)); - + // this shouldn't work because the item has been bough and the approved account should be // reset. assert_noop!(Uniques::transfer(Origin::signed(5), 0, item, 4), Error::::NoPermission);