Uniques: Reset approved account after transfer (#12145)

* reset approved account

* wrap at 100

* doc

* fmt

* Update frame/uniques/src/tests.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* new test

* Update frame/uniques/src/lib.rs

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

* fmt

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

substrate/frame/uniques/src/functions.rs

@@ -48,6 +48,12 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		Account::<T, I>::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::<T, I>::insert(&collection, &item, &details);
 		ItemPriceOf::<T, I>::remove(&collection, &item);
 

substrate/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)`

substrate/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::<Test>::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::<Test>::NoPermission);
+	});
+}
+
 #[test]
 fn cancel_approval_works() {
 	new_test_ext().execute_with(|| {