fix: Stop treasure chest guardian spawn farming via stack splits (#2568)
### Summary Players reported an exploit: decipher a treasure map, then run a ClassicUO/Razor organizer agent that pulls the gold out of the chest in small amounts. Each pull spawned more monsters, turning one chest into an unbounded farmable spawn generator. ### Root cause `TreasureMapChest.OnItemLifted` grants a 10% guardian spawn roll per first-time-lifted item, deduplicated by the instance-keyed `_lifted` set. But a partial lift goes through `Mobile.LiftItemDupe`, which re-adds the stack remainder to the chest as a **brand-new item instance** (engine-side `AddItem`, bypassing the `CheckHold` block on refilling). Every subsequent pull lifts an instance the `_lifted` set has never seen, so each one re-rolls the 10% spawn chance: - A level 4 chest holds 4,000 gold → pulled coin by coin, ~400 spawned creatures (plus more from reagent stacks), hands-free, per chest. - Spawns use `guardian: false`, so nothing tracks or caps them. - Legit full-stack looting yields roughly 5–8 bonus spawns per chest for comparison. The code is inherited from RunUO, so descendant shards likely share the hole. ### Fix Mark every item that enters the chest **after the initial fill** as already lifted, via an `OnItemAdded` override gated by a non-serialized `_filled` flag (set at the end of the constructor and in `[AfterDeserialization]`). Ordering makes this exact: `LiftItemDupe` re-adds the remainder *before* the chest's `OnItemLifted` runs, so the lifted original still gets its one legitimate roll while the remainder is pre-marked. This also covers packing items *into* the chest (e.g., merging gold back in to lift it out again) and bounce-backs — anything not part of the original loot can never grant a spawn roll. ### Tests - `PartialLift_MarksSplitRemainderAsLifted` — drives the real `Mobile.Lift` path with a 1-coin pull and asserts the split remainder is marked (failed before the fix). - `ItemAddedAfterFill_IsMarkedLifted` — post-fill additions are marked (failed before the fix). - `OriginalFillLoot_IsNotMarkedLifted` — original loot keeps spawn-roll eligibility. Full `UOContent.Tests` suite: 701 passed.
This commit is contained in:
Kamron Batman
GitHub
parent
0628902644
commit
c1442aff3e
3 changed files with 219 additions and 13 deletions
|
|
@ -0,0 +1,111 @@
|
|||
using Server;
|
||||
using Server.Items;
|
||||
using Server.Mobiles;
|
||||
using Server.Tests;
|
||||
using Xunit;
|
||||
|
||||
namespace UOContent.Tests;
|
||||
|
||||
[Collection("Sequential UOContent Tests")]
|
||||
public class TreasureMapChestLiftTests
|
||||
{
|
||||
// Coordinates chosen to avoid overlap with Tracking (1000-4000, 1000-4000) and
|
||||
// DetectHidden (1000-2400, 500) test areas.
|
||||
|
||||
[Fact]
|
||||
public void PartialLift_MarksSplitRemainderAsLifted()
|
||||
{
|
||||
using var rng = new PredictableRandom(10); // RandomDouble() = 0.5, no spawn roll fires
|
||||
var map = Map.Felucca;
|
||||
var location = new Point3D(5000, 600, 0);
|
||||
var player = CreatePlayerMobile(map, location);
|
||||
var chest = new TreasureMapChest(1);
|
||||
|
||||
try
|
||||
{
|
||||
chest.MoveToWorld(location, map);
|
||||
chest.Locked = false;
|
||||
|
||||
var gold = FindGold(chest, null);
|
||||
Assert.NotNull(gold);
|
||||
|
||||
player.Lift(gold, 1, out var rejected, out _);
|
||||
Assert.False(rejected);
|
||||
|
||||
// The stack split re-adds the remainder as a brand-new item. It must count as
|
||||
// already lifted, otherwise every 1-coin pull grants a fresh guardian spawn roll.
|
||||
var remainder = FindGold(chest, gold);
|
||||
Assert.NotNull(remainder);
|
||||
Assert.Contains(remainder, chest.Lifted);
|
||||
Assert.Contains(gold, chest.Lifted);
|
||||
}
|
||||
finally
|
||||
{
|
||||
player.Holding?.Delete();
|
||||
player.Delete();
|
||||
chest.Delete();
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ItemAddedAfterFill_IsMarkedLifted()
|
||||
{
|
||||
using var rng = new PredictableRandom(10);
|
||||
var chest = new TreasureMapChest(1);
|
||||
var packed = new Gold(500);
|
||||
|
||||
try
|
||||
{
|
||||
// Anything entering the chest after the initial fill (packed-back gold, split
|
||||
// remainders, GM drops) was never part of the original loot and must not
|
||||
// grant spawn rolls when lifted back out.
|
||||
chest.DropItem(packed);
|
||||
|
||||
Assert.Contains(packed, chest.Lifted);
|
||||
}
|
||||
finally
|
||||
{
|
||||
chest.Delete();
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void OriginalFillLoot_IsNotMarkedLifted()
|
||||
{
|
||||
using var rng = new PredictableRandom(10);
|
||||
var chest = new TreasureMapChest(1);
|
||||
|
||||
try
|
||||
{
|
||||
// The original loot must stay roll-eligible for its first lift.
|
||||
Assert.True(chest.Lifted == null || chest.Lifted.Count == 0);
|
||||
}
|
||||
finally
|
||||
{
|
||||
chest.Delete();
|
||||
}
|
||||
}
|
||||
|
||||
private static Gold FindGold(TreasureMapChest chest, Gold except)
|
||||
{
|
||||
var items = chest.Items;
|
||||
|
||||
for (var i = 0; i < items.Count; i++)
|
||||
{
|
||||
if (items[i] is Gold gold && gold != except)
|
||||
{
|
||||
return gold;
|
||||
}
|
||||
}
|
||||
|
||||
return null;
|
||||
}
|
||||
|
||||
private static PlayerMobile CreatePlayerMobile(Map map, Point3D location)
|
||||
{
|
||||
var mobile = new PlayerMobile(World.NewMobile);
|
||||
mobile.DefaultMobileInit();
|
||||
mobile.MoveToWorld(location, map);
|
||||
return mobile;
|
||||
}
|
||||
}
|
||||
|
|
@ -9,10 +9,11 @@ using Server.Network;
|
|||
|
||||
namespace Server.Items;
|
||||
|
||||
[SerializationGenerator(2, false)]
|
||||
[SerializationGenerator(3, false)]
|
||||
public partial class TreasureMapChest : LockableContainer
|
||||
{
|
||||
[Tidy]
|
||||
[CanBeNull]
|
||||
[SerializableField(0, setter: "private")]
|
||||
private List<Mobile> _guardians;
|
||||
|
||||
|
|
@ -43,10 +44,14 @@ public partial class TreasureMapChest : LockableContainer
|
|||
}
|
||||
|
||||
[Tidy]
|
||||
[CanBeNull]
|
||||
[SerializableField(5, setter: "private")]
|
||||
[SerializedCommandProperty(AccessLevel.GameMaster)]
|
||||
private HashSet<Item> _lifted;
|
||||
|
||||
// False only while the constructor fills the chest; deserialized chests are always filled.
|
||||
private bool _filled;
|
||||
|
||||
[Constructible]
|
||||
public TreasureMapChest(int level) : this(null, level)
|
||||
{
|
||||
|
|
@ -58,10 +63,10 @@ public partial class TreasureMapChest : LockableContainer
|
|||
_level = level;
|
||||
|
||||
_temporary = temporary;
|
||||
_guardians = [];
|
||||
|
||||
_expireTimer = Timer.DelayCall(TimeSpan.FromHours(3.0), Delete);
|
||||
Fill(this, level);
|
||||
_filled = true;
|
||||
}
|
||||
|
||||
public override int LabelNumber => 3000541;
|
||||
|
|
@ -182,7 +187,6 @@ public partial class TreasureMapChest : LockableContainer
|
|||
2 => 76,
|
||||
3 => 84,
|
||||
4 => 92,
|
||||
5 => 100,
|
||||
_ => 100
|
||||
};
|
||||
|
||||
|
|
@ -300,14 +304,17 @@ public partial class TreasureMapChest : LockableContainer
|
|||
|
||||
if (_level == 0 && from.AccessLevel < AccessLevel.GameMaster)
|
||||
{
|
||||
for (var i = 0; i < _guardians.Count; i++)
|
||||
if (_guardians.Count > 0)
|
||||
{
|
||||
var m = _guardians[i];
|
||||
if (m.Alive)
|
||||
for (var i = 0; i < _guardians.Count; i++)
|
||||
{
|
||||
// You must first kill the guardians before you may open this chest.
|
||||
from.SendLocalizedMessage(1046448);
|
||||
return true;
|
||||
var m = _guardians[i];
|
||||
if (m.Alive)
|
||||
{
|
||||
// You must first kill the guardians before you may open this chest.
|
||||
from.SendLocalizedMessage(1046448);
|
||||
return true;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -361,6 +368,17 @@ public partial class TreasureMapChest : LockableContainer
|
|||
public override bool CheckLift(Mobile from, Item item, ref LRReason reject) =>
|
||||
CheckLoot(from, true) && base.CheckLift(from, item, ref reject);
|
||||
|
||||
public override void OnItemAdded(Item item)
|
||||
{
|
||||
base.OnItemAdded(item);
|
||||
|
||||
if (_filled)
|
||||
{
|
||||
_lifted ??= [];
|
||||
_lifted.Add(item);
|
||||
}
|
||||
}
|
||||
|
||||
public override void OnItemLifted(Mobile from, Item item)
|
||||
{
|
||||
var notYetLifted = _lifted?.Contains(item) != true;
|
||||
|
|
@ -393,8 +411,6 @@ public partial class TreasureMapChest : LockableContainer
|
|||
|
||||
private void Deserialize(IGenericReader reader, int version)
|
||||
{
|
||||
_guardians = [];
|
||||
|
||||
_owner = reader.ReadEntity<Mobile>();
|
||||
_level = reader.ReadInt();
|
||||
var expireTimerNext = reader.ReadDeltaTime();
|
||||
|
|
@ -402,12 +418,34 @@ public partial class TreasureMapChest : LockableContainer
|
|||
_lifted = reader.ReadEntitySet<Item>();
|
||||
}
|
||||
|
||||
[AfterDeserialization(false)]
|
||||
private void MigrateFrom(V2Content content)
|
||||
{
|
||||
_guardians = content.Guardians;
|
||||
if (_guardians.Count == 0)
|
||||
{
|
||||
_guardians = null;
|
||||
}
|
||||
_temporary = content.Temporary;
|
||||
_owner = content.Owner;
|
||||
_level = content.Level;
|
||||
_lifted = content.Lifted;
|
||||
if (_lifted.Count == 0)
|
||||
{
|
||||
_lifted = null;
|
||||
}
|
||||
|
||||
var expireTimerDelay = content.ExpireTimerDelay;
|
||||
DeserializeExpireTimer(expireTimerDelay);
|
||||
}
|
||||
|
||||
[AfterDeserialization]
|
||||
private void AfterDeserialization()
|
||||
{
|
||||
_filled = true;
|
||||
|
||||
if (_expireTimer == null)
|
||||
{
|
||||
Delete();
|
||||
Timer.DelayCall(Delete);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
57
Projects/UOContent/Migrations/Server.Items.TreasureMapChest.v3.json
generated
Normal file
57
Projects/UOContent/Migrations/Server.Items.TreasureMapChest.v3.json
generated
Normal file
|
|
@ -0,0 +1,57 @@
|
|||
{
|
||||
"version": 3,
|
||||
"type": "Server.Items.TreasureMapChest",
|
||||
"properties": [
|
||||
{
|
||||
"name": "Guardians",
|
||||
"type": "System.Collections.Generic.List\u003CServer.Mobile\u003E",
|
||||
"rule": "ListMigrationRule",
|
||||
"ruleArguments": [
|
||||
"@Tidy",
|
||||
"@CanBeNull",
|
||||
"Server.Mobile",
|
||||
"SerializableInterfaceMigrationRule"
|
||||
]
|
||||
},
|
||||
{
|
||||
"name": "Temporary",
|
||||
"type": "bool",
|
||||
"rule": "PrimitiveTypeMigrationRule",
|
||||
"ruleArguments": [
|
||||
""
|
||||
]
|
||||
},
|
||||
{
|
||||
"name": "Owner",
|
||||
"type": "Server.Mobile",
|
||||
"rule": "SerializableInterfaceMigrationRule"
|
||||
},
|
||||
{
|
||||
"name": "Level",
|
||||
"type": "int",
|
||||
"rule": "PrimitiveTypeMigrationRule",
|
||||
"ruleArguments": [
|
||||
""
|
||||
]
|
||||
},
|
||||
{
|
||||
"name": "ExpireTimer",
|
||||
"type": "Server.Timer",
|
||||
"rule": "TimerMigrationRule",
|
||||
"ruleArguments": [
|
||||
"@TimerDrift"
|
||||
]
|
||||
},
|
||||
{
|
||||
"name": "Lifted",
|
||||
"type": "System.Collections.Generic.HashSet\u003CServer.Item\u003E",
|
||||
"rule": "HashSetMigrationRule",
|
||||
"ruleArguments": [
|
||||
"@Tidy",
|
||||
"@CanBeNull",
|
||||
"Server.Item",
|
||||
"SerializableInterfaceMigrationRule"
|
||||
]
|
||||
}
|
||||
]
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue