Trinitycore: [3.3.5] Eternal Water Dupe exploit (works only with Eternal Water and other elements)

[r4d1sh]

Description:

This bug allows you to clone items

Steps to reproduce the problem:

1. .add 37705 11
2. Quickly right-click on an item. Try to press a second time before the item is deleted. You will need several attempts if server running on your localhost
3. With a certain probability you will get two items (35622) instead of one

Branch(es):

3.3.5

TC rev. hash/commit:

5cdbbc29ab

[Demonid]

Confirmed on 5cdbbc29abdceab67af6f398279a81c426d0f758

I was actually going to report this a week ago but i forgot :P

[jerbookins]

Guess you have to have pretty high ping for this to occur

[jackpoz]

sounds like a nice case for a PR

[jackpoz]

With a certain probability you will get two items (35662) instead of one

is the item id correct ? http://reinosdeleyenda.org/aowow/?item=35662 seems quite unrelated

[r4d1sh]

Typo, thx. Correct id 37705 (instead of 35622).

[Jildor]

.die @xinef1
https://github.com/TrinityCore/TrinityCore/commit/464d3ad5d09d58136911f4ad09335f53049ac628

[Foereaper]

https://www.youtube.com/watch?v=f32v7ld7CR8

Video showcasing the dupe, I'd look more into trying to fix it but it's a bit over my head.

[Rochet2]

Main issue:
The spell the item casts to transform to another item has SPELL_ATTR2_IGNORE_ITEM_CHECK flag in DBC in AttributesEx2. This causes Spell::CheckItems to return before checking anything meaningful, so the reagents are not checked at all! This means that players could hack and send packets to do the transform too at any time without trying to "glitch it".

Removing the flag from the spell will make the core check the items and avoid the exploit. Not sure how to properly fix this issue though as removing the flag is probably not the right way.

Secondary issue: (not 100% sure on this)
The reagents are not checked before removing them since the spell is instant cast and cast(!m_casttime); causes skipCheck to be true in Spell::_cast. This is some kind of attempt at optimization to avoid checking reagents twice for instant spells. In Spell::prepare items are supposed to be already checked, but checking the items (Spell::prepare) and taking them from the player (Spell::_cast) are separated by a timed event, which means that the items could no longer be available when they are removed.
Here is a call stack (most recent call first)

>   worldserver.exe!Spell::_cast(bool skipCheck) Line 3164  C++
    worldserver.exe!Spell::cast(bool skipCheck) Line 3108   C++
    worldserver.exe!Spell::update(unsigned int difftime) Line 3591  C++
    worldserver.exe!SpellEvent::Execute(unsigned __int64 e_time, unsigned int p_time) Line 7075 C++
    worldserver.exe!EventProcessor::Update(unsigned int p_time) Line 56 C++
    worldserver.exe!Unit::Update(unsigned int p_time) Line 447  C++
    worldserver.exe!Player::Update(unsigned int p_time) Line 1059   C++
    worldserver.exe!Map::Update(unsigned int t_diff) Line 788   C++
    worldserver.exe!MapUpdateRequest::call() Line 43    C++

This means that any spell that requires reagents and is instant cast can possibly be casted multiple times with same reagents in an instant by just trying to do it fast (macro?)

Also when items are removed the missing items are ignored. And if the player doesnt have them they are taken from the player's bank (which is probably incorrect). You can see this in Spell::TakeReagents and p_caster->DestroyItemCount which it calls.

My suggestion is to check the items even with instant cast spells - but my knowledge of how the spell system works and how this could be done better is a bit limited.

diff --git a/src/server/game/Spells/Spell.cpp b/src/server/game/Spells/Spell.cpp
index 48dddb23ca..088ef9b626 100644
--- a/src/server/game/Spells/Spell.cpp
+++ b/src/server/game/Spells/Spell.cpp
@@ -3587,7 +3587,7 @@ void Spell::update(uint32 difftime)

             if (m_timer == 0 && !m_spellInfo->IsNextMeleeSwingSpell() && !IsAutoRepeat())
                 // don't CheckCast for instant spells - done in spell::prepare, skip duplicate checks, needed for range checks for example
-                cast(!m_casttime);
+                cast();
             break;
         }
         case SPELL_STATE_CASTING:

The code above is for f9f5a701f48bbb703658a99d689270d6bd7469c6

In case the secondary issue is unclear, here is a usecase:
player clicks to transform twice.
The first spell cast hits the prepare part and items are checked and a timed event for the _cast is scheduled.
The second spell cast hits the prepare part and items are checked and a timed event for the _cast is scheduled.
The timed events execute and _cast is executed. In _cast the items are not checked since they were already checked before. Also the _cast will remove the items and ignore if any items are missing, so both _cast calls succeed regardless of missing items.

[tje3d]

Not fixed yet, i've managed to do it several time with ~60ms

[jackpoz]

how many motes did you have in the inventory ? how many in the bank ? did you update TC to a version more recent than the PR merged ?

[Rochet2]

On 645627e652a6 still reproducible with the guide in main post.
Simply add 11 crystallized water and try to spam click it.
What may help is running debug build with debugger attached so the server runs slower maybe. /shrug

EDIT:
I tried with an automatic clicker and it could not reproduce the bug (at least so far..) by clicking a single stack. What worked easier is to have a stack of 10 water and stack of one water. Then try to click on both stacks quickly, not just the other.

[jackpoz]

I'll just change how the SPELL_ATTR2_IGNORE_ITEM_CHECK flag is handled in Spell::CheckItems(), especially with items with Reagent

[ariel-]

Can't remember why I made specifically that change, feel free to revert the attribute renaming/implement, lets see what breaks :P