diff --git a/frame/uniques/src/functions.rs b/frame/uniques/src/functions.rs index 107214558307f..4e68f1139420d 100644 --- a/frame/uniques/src/functions.rs +++ b/frame/uniques/src/functions.rs @@ -48,6 +48,12 @@ 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..3b0cf6dc673c9 100644 --- a/frame/uniques/src/lib.rs +++ b/frame/uniques/src/lib.rs @@ -606,6 +606,8 @@ pub mod pallet { /// 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`; /// - the Owner of the `item`; @@ -914,6 +916,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 reset 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..85d1bec574cf0 100644 --- a/frame/uniques/src/tests.rs +++ b/frame/uniques/src/tests.rs @@ -557,6 +557,44 @@ 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); + // The new owner can transfer fine: + assert_ok!(Uniques::transfer(Origin::signed(5), 0, 42, 6)); + }); +} + +#[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(|| {