Skill Points Exploit

[trevius]

I thought there was a bug report on this somewhere already, but I can't seem to find it anywhere now. The problem is with skill points that are earned upon leveling that can be used to train skills at GM NPCs. Basically, the problem is that if you lose your level and level up again, you earn skill points again. This happens everytime you level up even from a rez. On most servers, it might not be a huge issue, but my server has a deleveling system which leaves players with hundreds or thousands of skill points to use for GM training lol.

I was thinking of many adding an alternate field to the _character table that would be for setting the max level that a character has reached. It would only track level ups exceeding it's current value and would never go down for any reason. This means that if skill points were only earned when that field was increased, this bug would be resolved. If added, I think it might be useful for my de-leveling system as well so I could expand it *wink*

I haven't really messed with anything related to the character table yet, so it may take time for me to figure out what all would need to be added and changed for it to work. But, if I get a chance, I will try to figure it out unless someone else has a better idea to resolve the bug.

[Derision]

There is a field called 'level2' in the player profile struct:

common/eq_packet_structs.h

/*0241*/ uint8 level2; //no idea why this is here, but thats how it is on live


level2 isn't referenced anywhere in the EQEmu code, so presumably it is set to 0 at character creation and never changes. You could try storing the max level reached in here, as that is possibly what it was intended for. I just looked at three of 6.2 era packet collects and the level2 field was always the same as the level field in those collects.

[trevius]

Ahh sweet! You are the man! I will see if I can figure out what to do to get that done. But again, I haven't messed with the Character table stuff yet and am not really sure what would be involved in doing it. But, it does seem like Level2 would be perfect for what I am wanting to do :)

[trevius]

I am trying to take a second look into getting this fixed. Looking at the code, there is something that I don't understand the purpose of, and if possible, maybe it could be used for level2 instead.

eq_packet_structs.h
/*
** Level Update
** Length: 12 Bytes
*/
struct LevelUpdate_Struct
{
/*00*/ uint32 level; // New level
/*04*/ uint32 level_old; // Old level
/*08*/ uint32 exp; // Current Experience
};

I am not sure what the point of level_old is. Maybe that is supposed to be level2. If so, then I think the solution to fix this bug wouldn't be too bad. I think we could change the code above to this:

eq_packet_structs.h
/*
** Level Update
** Length: 12 Bytes
*/
struct LevelUpdate_Struct
{
/*00*/ uint32 level; // New level
/*04*/ uint32 level2; // Level2 for retaining max level reached (Don't think this is used anywhere)
/*08*/ uint32 exp; // Current Experience
};

Then, in exp.cpp change this:
EQApplicationPacket* outapp = new EQApplicationPacket(OP_LevelUpdate, sizeof(LevelUpdate_Struct));
LevelUpdate_Struct* lu = (LevelUpdate_Struct*)outapp->pBuffer;
lu->level = set_level;
lu->level_old = level;
level = set_level;

if(IsRaidGrouped())
{
Raid *r = this->GetRaid();
if(r){
r->UpdateLevel(GetName(), set_level);
}
}

if(set_level > m_pp.level) { // Yes I am aware that you could delevel yourself and relevel this is just to test!
m_pp.points += 5 * (set_level - m_pp.level);

#ifdef EMBPERL
((PerlembParser*)parse)->Event(EVENT_LEVEL_UP, 0, "", (NPC*)NULL, this);
#endif
}

m_pp.level = set_level;

To this:
EQApplicationPacket* outapp = new EQApplicationPacket(OP_LevelUpdate, sizeof(LevelUpdate_Struct));
LevelUpdate_Struct* lu = (LevelUpdate_Struct*)outapp->pBuffer;
lu->level = set_level;

if(IsRaidGrouped())
{
Raid *r = this->GetRaid();
if(r){
r->UpdateLevel(GetName(), set_level);
}
}

#ifdef EMBPERL
((PerlembParser*)parse)->Event(EVENT_LEVEL_UP, 0, "", (NPC*)NULL, this);
#endif
}

m_pp.level = set_level;
if(set_level > m_pp.level2) {
m_pp.points += 5 * (set_level - m_pp.level2);
m_pp.level2 = set_level;
}

And, I think this SQL would add the level2 field to the character_ table:
ALTER TABLE `character_` ADD column `level2` mediumint(8) unsigned NOT NULL default '1';

Note that I haven't tested this yet, and am not sure if it would work or not. But, at least it should be a start to getting this bug resolved. Anyone have thoughts on it so far?

[KLS]

Why exactly are you changing the level update struct? It has nothing to do with the problem at all. The sql isn't needed either as pp is stored in character table already.

If the level we're trying to set is greater than our level2 in pp then add points in the amount of 5 for every level diff between level we're trying to set and our level2. Set level2 to our new level:


void Client::SetLevel(int8 set_level, bool command)
{
#ifdef GUILDWARS
if(set_level > SETLEVEL) {
Message(0,"You cannot exceed level %i on a GuildWars Server.",SETLEVEL);
return;
}
#endif

if (GetEXPForLevel(set_level) == 0xFFFFFFFF) {
LogFile->write(EQEMuLog::Error,"Client::SetLevel() GetEXPForLevel(%i) = 0xFFFFFFFF", set_level);
return;
}

EQApplicationPacket* outapp = new EQApplicationPacket(OP_LevelUpdate, sizeof(LevelUpdate_Struct));
LevelUpdate_Struct* lu = (LevelUpdate_Struct*)outapp->pBuffer;
lu->level = set_level;
lu->level_old = level;
level = set_level;

if(IsRaidGrouped())
{
Raid *r = this->GetRaid();
if(r){
r->UpdateLevel(GetName(), set_level);
}
}

if(set_level > m_pp.level2)
{
m_pp.points += (5 * (set_level - m_pp.level2));
m_pp.level2 = set_level;
}

[trevius]

Thanks for clearing that up KLS. I don't really know much about the BLOBs and I am guessing that is what pp stuff is. I know level is stored directly in the character_ table, which is why I thought level2 had to be added there. I am still not clear on why there is a level field in the table if it is also in the pp (blob I guess).

Either way, if level2 is already there, then it looks like this should be a simple fix. I will give it a try and see how it tests.

I also think that these 2 lines could be removed:

lu->level_old = level;
level = set_level;

Because I don't see where those are even used anywhere in the code.

I also don't see any use of level_old and can't think of any reason why it would even exist. That is why I thought the packet structure might be wrong. Once I was done posting that, I already knew it had nothing to do with the fix, but it probably doesn't hurt to have the packet structure corrected if it is wrong. I am not saying that it is wrong, but just that I don't know what a level_old would be used for and I don't see it referenced anywhere else, or used anywhere. Just something to consider, anyway.

[KLS]

No leave those in. lu->level_old = level; tells the client what their old level was. Likely the client doctors the You gained a level / you lost a level message via that but I'm not sure.

level = set_level; is what actually sets the level =p

[trevius]

Hmm, if "level = set_level;" is setting the character level, then what is "m_pp.level = set_level;" doing? Is that just for updating the BLOB? I don't get the difference. Sorry for my noobishness lol.

I know we generate the leveling message, but there is another message that shows up when you #level that tells you how many levels you gained or lost. I am not at home right now, so I can't check it, but that is the only thing I can think of that level_old could possibly be used for. Not that there is any good reason to remove it (if nothing is broken), but I am just trying to understand what is going on.

No big deal really though, as I am pretty sure we should have this bug cleared up. I will definitely try to get this in tonight and test it out.

[KLS]

You have to understand how the objects work. m_pp basically is the player profile that's what we read back when we log in to get our bearings and what we save() to. But most the time the client/npc tracks it's own data without referencing the player profile. Without that line you wouldn't be that level for things like spell and combat calculations until you zoned because the active client object still thinks your level is the one it got from pp originally.

[trevius]

Thanks again KLS. So, that is basically just sending a level update to the client. I understand much better now :)

[trevius]

Well, I got this tested, and had to adjust how it was written slightly to make sure it included the logging.

void Client::SetLevel(int8 set_level, bool command)
{
#ifdef GUILDWARS
if(set_level > SETLEVEL) {
Message(0,"You cannot exceed level %i on a GuildWars Server.",SETLEVEL);
return;
}
#endif

if (GetEXPForLevel(set_level) == 0xFFFFFFFF) {
LogFile->write(EQEMuLog::Error,"Client::SetLevel() GetEXPForLevel(%i) = 0xFFFFFFFF", set_level);
return;
}

EQApplicationPacket* outapp = new EQApplicationPacket(OP_LevelUpdate, sizeof(LevelUpdate_Struct));
LevelUpdate_Struct* lu = (LevelUpdate_Struct*)outapp->pBuffer;
lu->level = set_level;
lu->level_old = level;
level = set_level;

if(IsRaidGrouped())
{
Raid *r = this->GetRaid();
if(r){
r->UpdateLevel(GetName(), set_level);
}
}

if (set_level > m_pp.level2) {
m_pp.points += 5 * (set_level - m_pp.level2);
m_pp.level2 = set_level;

#ifdef EMBPERL
((PerlembParser*)parse)->Event(EVENT_LEVEL_UP, 0, "", (NPC*)NULL, this);
#endif
}

m_pp.level = set_level;
if (command){
m_pp.exp = GetEXPForLevel(set_level);
Message(15, "Welcome to level %i!", set_level);
lu->exp = 0;
}
else {
float tmpxp = (float) ( (float) m_pp.exp - GetEXPForLevel( GetLevel() )) /
( (float) GetEXPForLevel(GetLevel()+1) - GetEXPForLevel(GetLevel()));
lu->exp = (int32)(330.0f * tmpxp);
}
QueuePacket(outapp);
safe_delete(outapp);
this->SendAppearancePacket(AT_WhoLevel, set_level); // who level change

LogFile->write(EQEMuLog::Normal,"Setting Level for %s to %i", GetName(), set_level);

CalcBonuses();
if(!RuleB(Character, HealOnLevel))
{
int mhp = CalcMaxHP();
if(GetHP() > mhp)
SetHP(mhp);
}
else
{
SetHP(CalcMaxHP()); // Why not, lets give them a free heal
}

SendHPUpdate();
SetMana(CalcMaxMana());
UpdateWho();
Save();
}


But, it still doesn't seem to be working, so I am guessing that level2 is either being updated somewhere else to match current level, or the points are being added elsewhere.

[Derision]

You need to check if m_pp.level2 is zero, which it will be for any character created before this change, otherwise you will give existing characters lots of extra skill points. The code below works for me, however if you try and train skills after levelling, without zoning, the client will show an extra 5 points, however if you zone first, it seems to display correctly.


if(set_level > m_pp.level2)
{
printf("m_pp.level2 is %i\n", m_pp.level2); fflush(stdout);
if(m_pp.level2 == 0)
m_pp.points += 5;
else
m_pp.points += (5 * (set_level - m_pp.level2));

m_pp.level2 = set_level;
}
if(set_level > m_pp.level) { // Yes I am aware that you could delevel yourself and relevel this is just to test!

#ifdef EMBPERL
((PerlembParser*)parse)->Event(EVENT_LEVEL_UP, 0, "", (NPC*)NULL, this);
#endif
}

m_pp.level = set_level;

[Derision]

I've just had a look at the client disassembly and it is comparing lu->level with lu->level_old and adding 5 skill points if lu->level > lu_level_old, so in fact lu->level_old should be set to the max level the player has previously reached (m_pp.level2). This seems to work:


void Client::SetLevel(int8 set_level, bool command)
{
#ifdef GUILDWARS
if(set_level > SETLEVEL) {
Message(0,"You cannot exceed level %i on a GuildWars Server.",SETLEVEL);
return;
}
#endif

if (GetEXPForLevel(set_level) == 0xFFFFFFFF) {
LogFile->write(EQEMuLog::Error,"Client::SetLevel() GetEXPForLevel(%i) = 0xFFFFFFFF", set_level);
return;
}

EQApplicationPacket* outapp = new EQApplicationPacket(OP_LevelUpdate, sizeof(LevelUpdate_Struct));
LevelUpdate_Struct* lu = (LevelUpdate_Struct*)outapp->pBuffer;
lu->level = set_level;
if(m_pp.level2 != 0)
lu->level_old = m_pp.level2;
else
lu->level_old = level;

level = set_level;

if(IsRaidGrouped())
{
Raid *r = this->GetRaid();
if(r){
r->UpdateLevel(GetName(), set_level);
}
}
if(set_level > m_pp.level2)
{
if(m_pp.level2 == 0)
m_pp.points += 5;
else
m_pp.points += (5 * (set_level - m_pp.level2));

m_pp.level2 = set_level;
}
if(set_level > m_pp.level) { // Yes I am aware that you could delevel yourself and relevel this is just to test!

#ifdef EMBPERL
((PerlembParser*)parse)->Event(EVENT_LEVEL_UP, 0, "", (NPC*)NULL, this);
#endif
}

m_pp.level = set_level;
if (command){
m_pp.exp = GetEXPForLevel(set_level);
Message(15, "Welcome to level %i!", set_level);
lu->exp = 0;
}
else {
float tmpxp = (float) ( (float) m_pp.exp - GetEXPForLevel( GetLevel() )) /
( (float) GetEXPForLevel(GetLevel()+1) - GetEXPForLevel(GetLevel()));
lu->exp = (int32)(330.0f * tmpxp);
}
QueuePacket(outapp);
safe_delete(outapp);
this->SendAppearancePacket(AT_WhoLevel, set_level); // who level change

LogFile->write(EQEMuLog::Normal,"Setting Level for %s to %i", GetName(), set_level);

CalcBonuses();
if(!RuleB(Character, HealOnLevel))
{
int mhp = CalcMaxHP();
if(GetHP() > mhp)
SetHP(mhp);
}
else
{
SetHP(CalcMaxHP()); // Why not, lets give them a free heal
}

SendHPUpdate();
SetMana(CalcMaxMana());
UpdateWho();
Save();
}

[trevius]

Nice work, Derision! So, basically, if I understand correctly, the packet structure should say level2 instead of level_old? Not that it actually requires it to say that for this to work, but in reality, I am thinking level_old is really level2. I think that would make everything match up perfectly and finally make actual sense of the level2 field.

I am going to test this now.

[Derision]

Well, both old_level and m.pp_level2 should probably both be renamed to MaxLevelReached, or something like that.

[trevius]

YES! Tested and it worked perfectly! When you train skills, it will even update the current number of points showing properly thanks to your finding that level_old is actually used for the client to update points.

The only case where it doesn't update properly is if you #level more than 1 level at a time. But, this isn't really an issue. And, it is easily resolved by just zoning.

Excellent fix. If someone else doesn't get this on the SVN before tonight, I will do it later :)