fix trade DoS

common/src/comp/inventory/mod.rs

@@ -2,7 +2,7 @@ use core::ops::Not;
 use hashbrown::HashMap;
 use serde::{Deserialize, Serialize};
 use specs::{Component, DerefFlaggedStorage};
-use std::{cmp::Ordering, convert::TryFrom, mem, ops::Range};
+use std::{cmp::Ordering, convert::TryFrom, mem, num::NonZeroU32, ops::Range};
 use tracing::{debug, trace, warn};
 use vek::Vec3;
 
@@ -595,6 +595,42 @@ impl Inventory {
         }
     }
 
+    /// Takes an amount of items from a slot
+    pub fn take_amount(
+        &mut self,
+        inv_slot_id: InvSlotId,
+        amount: NonZeroU32,
+        ability_map: &AbilityMap,
+        msm: &MaterialStatManifest,
+    ) -> Option<Item> {
+        if let Some(Some(item)) = self.slot_mut(inv_slot_id) {
+            if item.is_stackable() && item.amount() > 1 {
+                let mut return_item = item.duplicate(ability_map, msm);
+                let return_amount = amount.get().min(item.amount());
+                return_item
+                    .set_amount(return_amount)
+                    .expect("We know that 0 < return_amount <= item.amount()");
+
+                // Instead of setting item amount to 0 we remove it
+                if amount.get() == item.amount() {
+                    self.remove(inv_slot_id);
+                } else {
+                    let new_amount = item.amount() - return_amount;
+                    item.set_amount(new_amount).expect(
+                        "new_amount must be > 0 since return amount is != item.amount() and < \
+                         item.amount()",
+                    );
+                }
+
+                Some(return_item)
+            } else {
+                self.remove(inv_slot_id)
+            }
+        } else {
+            None
+        }
+    }
+
     /// Takes half of the items from a slot in the inventory
     #[must_use = "Returned items will be lost if not used"]
     pub fn take_half(

server/src/events/trade.rs

@@ -16,7 +16,7 @@ use common_net::{
 };
 use hashbrown::{hash_map::Entry, HashMap};
 use specs::{world::WorldExt, Entity as EcsEntity};
-use std::cmp::Ordering;
+use std::{cmp::Ordering, num::NonZeroU32};
 use tracing::{error, trace};
 use world::IndexOwned;
 
@@ -375,12 +375,11 @@ fn commit_trade(ecs: &specs::World, trade: &PendingTrade) -> TradeResult {
     let msm = ecs.read_resource::<MaterialStatManifest>();
     for who in [0, 1].iter().cloned() {
         for (slot, quantity) in trade.offers[who].iter() {
-            // Take the items one by one, to benefit from Inventory's stack handling
-            for _ in 0..*quantity {
+            if let Some(quantity) = NonZeroU32::new(*quantity) {
                 inventories
                     .get_mut(entities[who])
                     .expect(invmsg)
-                    .take(*slot, &ability_map, &msm)
+                    .take_amount(*slot, quantity, &ability_map, &msm)
                     .map(|item| items[who].push(item));
             }
         }