fix: check a trade offer against the items the player holds

A trade used the LOT and the count from the offer as the player sent
it. The offer is now resolved when it arrives and again when the trade
completes: the LOT comes from the held item, the count stops at the
held count, and an offer that names an unknown item, the same item
twice or a count of 0 is refused. The other player only receives an
item that was taken from the offering player.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>

dGame/TradingManager.cpp

@@ -13,7 +13,9 @@
 #include "CharacterComponent.h"
 #include "MissionComponent.h"
 #include "eMissionTaskType.h"
+#include <algorithm>
 #include <ranges>
+#include <set>
 
 namespace {
 	std::unique_ptr<Trade> g_EmptyTrade;
@@ -27,6 +29,26 @@ Trade::Trade(LWOOBJID tradeId, LWOOBJID participantA, LWOOBJID participantB) {
 	m_ParticipantB = participantB;
 }
 
+std::optional<std::vector<TradeItem>> Trade::ResolveOffer(
+	const std::vector<TradeItem>& offered,
+	const std::function<std::optional<HeldTradeItem>(LWOOBJID)>& findHeld) {
+	std::vector<TradeItem> resolved;
+	resolved.reserve(offered.size());
+	std::set<LWOOBJID> seen;
+
+	for (const auto& tradeItem : offered) {
+		// The client keeps its offer by item ID, so an item is only ever in it once
+		if (tradeItem.itemCount == 0 || !seen.insert(tradeItem.itemId).second) return std::nullopt;
+
+		const auto held = findHeld(tradeItem.itemId);
+		if (!held || held->count == 0) return std::nullopt;
+
+		resolved.push_back({ tradeItem.itemId, held->lot, std::min(tradeItem.itemCount, held->count) });
+	}
+
+	return resolved;
+}
+
 Trade::~Trade() {
 
 }
@@ -149,6 +171,20 @@ void Trade::Complete() {
 		return;
 	}
 
+	// The inventories may have changed since the offers were made, so check the offers against them again
+	const auto holds = [](const InventoryComponent* const inventory) {
+		return [inventory](const LWOOBJID itemId) -> std::optional<HeldTradeItem> {
+			const auto* const item = inventory->FindItemById(itemId);
+			if (!item) return std::nullopt;
+			return HeldTradeItem{ item->GetLot(), item->GetCount() };
+		};
+	};
+
+	if (ResolveOffer(m_ItemsA, holds(inventoryA)) != m_ItemsA || ResolveOffer(m_ItemsB, holds(inventoryB)) != m_ItemsB) {
+		LOG("The items offered by %s and %s no longer match what they hold, aborting trade", characterA->GetName().c_str(), characterB->GetName().c_str());
+		return;
+	}
+
 	for (const auto& tradeItem : m_ItemsA) {
 		auto* itemToRemove = inventoryA->FindItemById(tradeItem.itemId);
 		if (itemToRemove) {
@@ -194,7 +230,7 @@ void Trade::Complete() {
 		InventoryComponent* to, const LWOOBJID fromCharacter, const LWOOBJID toCharacter) {
 		for (const auto& tradeItem : items) {
 			auto* itemToRemove = from->FindItemById(tradeItem.itemId);
-			if (!itemToRemove) continue;
+			if (!itemToRemove || itemToRemove->GetCount() < tradeItem.itemCount) continue;
 
 			// A whole item keeps its data (subkey, config) but gets a new object id, as upstream: reusing the id would
 			// leave two objects with one id if the server stopped between the two players' saves. A stack merges into
@@ -203,17 +239,19 @@ void Trade::Complete() {
 			const auto config = itemToRemove->GetConfig();
 			const auto subKey = itemToRemove->GetSubKey();
 			const auto bound = itemToRemove->GetBound();
+			// Taking the whole stack removes the item, so read its LOT first
+			const LOT lot = itemToRemove->GetLot();
 			itemToRemove->SetCount(itemToRemove->GetCount() - tradeItem.itemCount);
-			fromMissions->Progress(eMissionTaskType::GATHER, tradeItem.itemLot, LWOOBJID_EMPTY, "", -static_cast<int32_t>(tradeItem.itemCount));
+			fromMissions->Progress(eMissionTaskType::GATHER, lot, LWOOBJID_EMPTY, "", -static_cast<int32_t>(tradeItem.itemCount));
 			// Live sent the trade's ID with the traded items
 			ReceiveItemOptions options{};
 			options.metrics.transactionId = tradeId;
 			const auto received = whole
-				? to->ReceiveItem(LWOOBJID_EMPTY, tradeItem.itemLot, tradeItem.itemCount, eLootSourceType::TRADE, config, subKey, bound, options)
-				: to->ReceiveItem(LWOOBJID_EMPTY, tradeItem.itemLot, tradeItem.itemCount, eLootSourceType::TRADE, {}, LWOOBJID_EMPTY, false, options);
+				? to->ReceiveItem(LWOOBJID_EMPTY, lot, tradeItem.itemCount, eLootSourceType::TRADE, config, subKey, bound, options)
+				: to->ReceiveItem(LWOOBJID_EMPTY, lot, tradeItem.itemCount, eLootSourceType::TRADE, {}, LWOOBJID_EMPTY, false, options);
 
 			EconomyLedger::RecordTransfer({ .method = IEconomyLedger::eTransferMethod::TRADE, .itemId = tradeItem.itemId, .newItemId = received.id,
-				.lot = tradeItem.itemLot, .count = tradeItem.itemCount, .fromCharacter = fromCharacter, .toCharacter = toCharacter, .zone = zone,
+				.lot = lot, .count = tradeItem.itemCount, .fromCharacter = fromCharacter, .toCharacter = toCharacter, .zone = zone,
 				.merged = received.merged });
 		}
 	};

dGame/TradingManager.h

@@ -2,17 +2,39 @@
 
 #include "Entity.h"
 
+#include <functional>
 #include <map>
 #include <memory>
+#include <optional>
+#include <vector>
 
 struct TradeItem {
 	LWOOBJID itemId;
 	LOT itemLot;
 	uint32_t itemCount;
+
+	bool operator==(const TradeItem& other) const = default;
+};
+
+// What a player holds of an item they offered
+struct HeldTradeItem {
+	LOT lot;
+	uint32_t count;
 };
 
 class Trade {
 public:
+	/**
+	 * Checks the items a player offered against the items they hold.
+	 * @param offered the items as the player sent them
+	 * @param findHeld gives the held item with an ID, or nothing if the player has no such item
+	 * @return the offer with every LOT taken from the held item and every count limited to the held count,
+	 * or nothing if the offer names an item the player does not hold, the same item twice or a count of 0
+	 */
+	static std::optional<std::vector<TradeItem>> ResolveOffer(
+		const std::vector<TradeItem>& offered,
+		const std::function<std::optional<HeldTradeItem>(LWOOBJID)>& findHeld);
+
 	explicit Trade(LWOOBJID tradeId, LWOOBJID participantA, LWOOBJID participantB);
 	~Trade();
 

dGame/dGameMessages/TradeMessages.cpp

@@ -9,6 +9,8 @@
 #include "ePermissionMap.h"
 #include "Game.h"
 #include "GeneralUtils.h"
+#include "InventoryComponent.h"
+#include "Item.h"
 #include "Logger.h"
 #include "TradingManager.h"
 #include "User.h"
@@ -233,8 +235,23 @@ namespace GameMessages {
 
 		if (trade == nullptr) return;
 
+		const auto* const inventoryComponent = entity.GetComponent<InventoryComponent>();
+		if (!inventoryComponent) return;
+
+		// The LOT and the count the trade works with come from the items the player holds
+		const auto resolvedItems = Trade::ResolveOffer(items, [inventoryComponent](const LWOOBJID itemId) -> std::optional<HeldTradeItem> {
+			const auto* const item = inventoryComponent->FindItemById(itemId);
+			if (!item) return std::nullopt;
+			return HeldTradeItem{ item->GetLot(), item->GetCount() };
+			});
+
+		if (!resolvedItems) {
+			LOG("Trade update from (%llu) does not match the items they hold, ignoring it", entity.GetObjectID());
+			return;
+		}
+
 		trade->SetCoins(entity.GetObjectID(), i64Currency);
-		trade->SetItems(entity.GetObjectID(), items);
+		trade->SetItems(entity.GetObjectID(), *resolvedItems);
 		trade->SendUpdateToOther(entity.GetObjectID());
 	}
 

tests/dGameTests/dGameMessagesTests/CMakeLists.txt

@@ -19,7 +19,8 @@ SET(DGAMEMESSAGES_TESTS
 	"HotPropertyMessageTests.cpp"
 	"ClientMessagesTests.cpp"
 	"LoadSequenceTests.cpp"
-	"ChatRoutingTests.cpp")
+	"ChatRoutingTests.cpp"
+	"TradeOfferTests.cpp")
 
 # Get the folder name and prepend it to the files above
 get_filename_component(thisFolderName ${CMAKE_CURRENT_SOURCE_DIR} NAME)

tests/dGameTests/dGameMessagesTests/TradeOfferTests.cpp

@@ -0,0 +1,58 @@
+#include <gtest/gtest.h>
+
+#include <map>
+
+#include "TradingManager.h"
+
+namespace {
+	const std::map<LWOOBJID, HeldTradeItem> g_Held = {
+		{ 100, { 1727, 5 } },
+		{ 200, { 3040, 1 } },
+	};
+
+	std::optional<HeldTradeItem> FindHeld(const LWOOBJID itemId) {
+		const auto it = g_Held.find(itemId);
+		if (it == g_Held.end()) return std::nullopt;
+		return it->second;
+	}
+}
+
+TEST(TradeOfferTest, AnOfferOfHeldItemsIsKept) {
+	const std::vector<TradeItem> offered = { { 100, 1727, 3 }, { 200, 3040, 1 } };
+	const auto resolved = Trade::ResolveOffer(offered, FindHeld);
+	ASSERT_TRUE(resolved.has_value());
+	EXPECT_EQ(*resolved, offered);
+}
+
+TEST(TradeOfferTest, AnEmptyOfferIsKept) {
+	const auto resolved = Trade::ResolveOffer({}, FindHeld);
+	ASSERT_TRUE(resolved.has_value());
+	EXPECT_TRUE(resolved->empty());
+}
+
+TEST(TradeOfferTest, TheLotComesFromTheHeldItem) {
+	const auto resolved = Trade::ResolveOffer({ { 100, 7777, 2 } }, FindHeld);
+	ASSERT_TRUE(resolved.has_value());
+	ASSERT_EQ(resolved->size(), 1);
+	EXPECT_EQ(resolved->front().itemLot, 1727);
+	EXPECT_EQ(resolved->front().itemCount, 2);
+}
+
+TEST(TradeOfferTest, TheCountStopsAtTheHeldCount) {
+	const auto resolved = Trade::ResolveOffer({ { 100, 1727, 4000000000U } }, FindHeld);
+	ASSERT_TRUE(resolved.has_value());
+	ASSERT_EQ(resolved->size(), 1);
+	EXPECT_EQ(resolved->front().itemCount, 5);
+}
+
+TEST(TradeOfferTest, AnItemOfferedTwiceIsRefused) {
+	EXPECT_FALSE(Trade::ResolveOffer({ { 100, 1727, 3 }, { 100, 1727, 3 } }, FindHeld).has_value());
+}
+
+TEST(TradeOfferTest, AnItemThePlayerDoesNotHoldIsRefused) {
+	EXPECT_FALSE(Trade::ResolveOffer({ { 300, 1727, 1 } }, FindHeld).has_value());
+}
+
+TEST(TradeOfferTest, ACountOfZeroIsRefused) {
+	EXPECT_FALSE(Trade::ResolveOffer({ { 100, 1727, 0 } }, FindHeld).has_value());
+}