multitheftauto / mtasa-blue

Multi Theft Auto is a game engine that incorporates an extendable network play element into a proprietary commercial single-player game.
https://multitheftauto.com
GNU General Public License v3.0
1.4k stars 435 forks source link

setPedArmor and setElementHealth synchronization problems from Client to Server #3791

Open bastianmarin opened 1 week ago

bastianmarin commented 1 week ago

Describe the bug

Creating anti-cheat checks for a server, we realized something curious.

Although the MTA wiki indicates that the vest that can be placed on a player is from 0-100, this is not real, you can set the number you want (example 1000) from the clientside, from the serverside this is not possible.

Testing this problem, I reached several conclusions.

Note:

Steps to reproduce

  1. In clientside use setPedArmor

Version

Client: Multi Theft Auto v1.6-release-22763

Additional context

Image

Relevant log output

No response

Security Policy

PlatinMTA commented 6 days ago

Health seems to be capped but yeah you can set any amount you want for armor clientside. So I can confirm this for the Armor.

To be honest I think that via script you should be able to set any value for health and armor, it shouldn't break anything. We already have this for vehicles, why not peds?

TracerDS commented 6 days ago

Health seems to be capped but yeah you can set any amount you want for armor clientside. So I can confirm this for the Armor.

To be honest I think that via script you should be able to set any value for health and armor, it shouldn't break anything. We already have this for vehicles, why not peds?

Tbh it should be fixed for all elements. You shouldnt have more max health just because.

Fernando-A-Rocha commented 6 days ago

Yeh max health exists for a reason, it shouldn't be allowed to exceed it.

PlatinMTA commented 6 days ago

Ok so, what's the actual reason? It works without issue with vehicles.

bastianmarin commented 6 days ago

My real problem is with server security, meaning preventing cheaters from adding more life than possible. It's 100 times safer to verify on serverside that the player has +100 armor* than on client and prevent it from being intercepted and cancelled.

PlatinMTA commented 6 days ago

My real problem is with server security, meaning preventing cheaters from adding more life than possible. It's 100 times safer to verify on serverside that the player has +100 armor* than on client and prevent it from being intercepted and cancelled.

addEventHandler("onClientPlayerDamage", localPlayer, function()
    cancelEvent()
end)

You should really be doing serversided checks for this stuff by now (never trust the client, even if it has capped values). Also, what I am asking is to be able to set any health serverside, not even just clientside. Right now this is a bug, but it could be a feature and it would make it easier for scripters so they don't need to make their own health system when there is already one. Then again is up to the rest of us to decide if its worth it or not.

bastianmarin commented 6 days ago

Mi verdadero problema es la seguridad del servidor, es decir, evitar que los tramposos agreguen más vida de la posible. Es 100 veces más seguro verificar en el servidor que el jugador tiene +100 de armadura* que en el cliente y evitar que sea interceptado y cancelado.

addEventHandler("onClientPlayerDamage", localPlayer, function() cancelEvent() end)

A estas alturas ya deberías estar haciendo comprobaciones en el servidor para estas cosas. Además, lo que estoy pidiendo es poder configurar cualquier lado del servidor en estado de salud, ni siquiera solo el lado del cliente. En este momento esto es un error, pero podría ser una característica y facilitaría que los programadores no tengan que crear su propio sistema de salud cuando ya existe uno. Por otra parte, depende del resto de nosotros decidir si vale la pena o no.

It seems that he really doesn't know how to read what I wrote. The problem is the desynchronization between the serverside and clientside values. Of course I want to check the value on Serverside but this is not possible because the values ​​do not match. If you want to make an improvement or a change to the health of the elements in MTA. Open a pull request and follow your idea. This post is to fix a desynchronization problem between items. He also seems to not know how the game works internally. Changing this may bring problems for the game's performance and the current AC of MTA. The game is not practically designed to exceed these values ​​and probably has a problem with it. MTA will automatically correct the health if it is exceeded to unrealistic values ​​in memory.

PlatinMTA commented 6 days ago

Kinda rude but sure, since we are talking about capping values I think it's related. I will not take into account the unnecessary comments you made and I will just ask you what's the request? To cap the values clientside? If that's the case, then of course I would argue that maybe we should consider uncapping those values or making the cap higher. Why? So we don't have to rely on custom resources to handle another health system.


Anyways the issue here is related to puresync most likely. What I think its happening for the server to correct their own values is that when the puresync packet arrives it already arrives with the values shorten down due to its maximum size, thats why the values are capped at 127.5 and 2047.5. So that's were the fix should be put in place, plus a cap in setArmor clientside (which is non-existent right now). Since we are already doing that we might as well add a cap for the other elemens in setElementHealth too.


Now my argument,

runcode run local veh = getPedOccupiedVehicle(me) setElementHealth(veh, 9995) iprint("new:", getElementHealth(veh)) setTimer(function() iprint(getTickCount(), getElementHealth(veh)) end, 0, 10) This will return for a tiny bit the set up value (9995) and then it will go back to 2047.5, but only serverside. All clients (or at least the syncer, since i tried this only in a local server) will still have the health value at 9995.

So, if the server can set values higher than 2047.5 and it can also sync those values without much issue, why should we cap them low?

Changing this may bring problems for the game's performance and the current AC of MTA.

Could be, but why? The cap is not being done internally (there is a clamp in setPedArmor serverside but not clientside, but there is no clamp with setElementHealth in either client nor server except for ped/players). This is a side effect of puresync. If that's the case (game performing poorly) of course we don't want it, but if it isn't this could be a nice opportunity to fix this issue + raise the current limits of MTA.

Or we can make our own implementation with lua and deal with it like we have been doing for years at this point...

TracerDS commented 6 days ago

Cancelling event on the client is dangerous as the cheater can just not execute that code

derxgbb commented 4 days ago

I'd like to point out something here because it has some connections with this issue. (kind of)

A few years ago I was testing someting with clientside setElementData and things of like that and I also noticed this desynchronization thing. I wanted to make a custom hp+armor script and firstly I wanted to make it with setElementData (I dropped the whole idea after some days) I just mentioned this that on clientside there will always be some desynchronization.

What I want to ask is, why the maximum hp is 200 and not 150?

In the singleplayer you can have a maximum of 150 hp. To get them you either need to do the paramedic side mission, or you'd need to exercise CJ's body to have more stamina+muslces to earn that slowly.

This is a problem because the game HUD is designed for 150 hp and I'm surprised that no one caught this yet in these years. I'm not sure if I can desrcibe it well, so I encourage everyone to set up a local server then try the following:

setPedStat(ped, 24, 1000)

This makes your charachter to have MAX_HEALTH of 200. After this set your current HP to 100.

setElementHealth(ped, 100)

Watch the HPBAR on the HUD. It'll look like its in the middle (almost) as it should. Now set it to 150, then check the HPBAR again. It'll look like its almost full, meanwhile there is still 50 hp left till 200. As i tested the HPBAR on the HUD visually maximises at 172 hp and all HP after that won't be seen. This can mislead players.

The other problem which also applies to this is the setPedStat itself which mislead the scripters.

setPedStat(ped, 24, 1000) -- maxhp: 200  (so if 1000points means 200maxhp)
setPedStat(ped, 24, 500) -- maxhp: 84 (why 500points is not 100maxhp?)
setPedStat(ped, 24, 565) -- maxhp: 100 (in fact 565points is 100maxhp...) -- getPedStat returns 569 by default

The best would be:

setPedStat(ped, 24, 100) -- maxhp: 100 -- making this as default
setPedStat(ped, 24, 150) -- maxhp: 150
setPedStat(ped, 24, 200) -- error: maxhp can only be between 1-150

I'm only guessing but the reason that we can have 200max hp is because of the armor can not be set to 150.

setPedStat(ped, 164, 1000) -- maxarmor: makes nothing, setting this stat doesn't do anything.

In singleplayer we have max armor at 100, but if we do the vigilante mission then it will be increased to 150. In PS2 AFAIK it works, but the hud is misleading in all ports (ps2 and on pc as well) because the same width of amorbar applies to both 100armor and 150armor. (AFAIK in definitive edition they fixed these, if you do vigilante the armorbar will be wider)

In PC if I recall this feature is bugged and never worked in singleplayer as well. You couldn't have 150 armor even if you did the vigilante mission. (maybe a silentpatch could fix this but MTA doesn't (use/let use players to use) silentpatch. The same goes for fast sprint with cj skin if you tap space and the fireproof cj as well.

I understand that we couldn't have these unless we can use silentpatch, so at least the HUD bugs on hpbar should be fixed in the future and also the setPedStat should be reworked in my opinion.

bastianmarin commented 2 days ago

I'd like to point out something here because it has some connections with this issue. (kind of)

A few years ago I was testing someting with clientside setElementData and things of like that and I also noticed this desynchronization thing. I wanted to make a custom hp+armor script and firstly I wanted to make it with setElementData (I dropped the whole idea after some days) I just mentioned this that on clientside there will always be some desynchronization.

What I want to ask is, why the maximum hp is 200 and not 150?

In the singleplayer you can have a maximum of 150 hp. To get them you either need to do the paramedic side mission, or you'd need to exercise CJ's body to have more stamina+muslces to earn that slowly.

This is a problem because the game HUD is designed for 150 hp and I'm surprised that no one caught this yet in these years. I'm not sure if I can desrcibe it well, so I encourage everyone to set up a local server then try the following:

setPedStat(ped, 24, 1000)

This makes your charachter to have MAX_HEALTH of 200. After this set your current HP to 100.

setElementHealth(ped, 100)

Watch the HPBAR on the HUD. It'll look like its in the middle (almost) as it should. Now set it to 150, then check the HPBAR again. It'll look like its almost full, meanwhile there is still 50 hp left till 200. As i tested the HPBAR on the HUD visually maximises at 172 hp and all HP after that won't be seen. This can mislead players.

The other problem which also applies to this is the setPedStat itself which mislead the scripters.

setPedStat(ped, 24, 1000) -- maxhp: 200 (so if 1000points means 200maxhp) setPedStat(ped, 24, 500) -- maxhp: 84 (why 500points is not 100maxhp?) setPedStat(ped, 24, 565) -- maxhp: 100 (in fact 565points is 100maxhp...) -- getPedStat returns 569 by default

The best would be:

setPedStat(ped, 24, 100) -- maxhp: 100 -- making this as default setPedStat(ped, 24, 150) -- maxhp: 150 setPedStat(ped, 24, 200) -- error: maxhp can only be between 1-150

I'm only guessing but the reason that we can have 200max hp is because of the armor can not be set to 150.

setPedStat(ped, 164, 1000) -- maxarmor: makes nothing, setting this stat doesn't do anything.

In singleplayer we have max armor at 100, but if we do the vigilante mission then it will be increased to 150. In PS2 AFAIK it works, but the hud is misleading in all ports (ps2 and on pc as well) because the same width of amorbar applies to both 100armor and 150armor. (AFAIK in definitive edition they fixed these, if you do vigilante the armorbar will be wider)

In PC if I recall this feature is bugged and never worked in singleplayer as well. You couldn't have 150 armor even if you did the vigilante mission. (maybe a silentpatch could fix this but MTA doesn't (use/let use players to use) silentpatch. The same goes for fast sprint with cj skin if you tap space and the fireproof cj as well.

I understand that we couldn't have these unless we can use silentpatch, so at least the HUD bugs on hpbar should be fixed in the future and also the setPedStat should be reworked in my opinion.

Who would have thought that after so many years I read something new about the difference in the GTA:SA ports. Good research. I guess the 84 life is done on purpose for when you don't take care of your character in the game. So that probably has to do with the 500 being 84 min health. Remember that in the game you can get "malnutrition" for not eating. I think that has something to do with this value.

derxgbb commented 1 day ago

I think you misunderstood, setPedStat 24 is for setting MAX_HEALTH, that has nothing to do with starvation in the game. It's just the maximum health you can have, your current health is getting lowered by starvation and from damage. A fresh game starts with a maxhp of 100 and it slowly increases to 150 by running, swimming, exercising or by doing the paramedic side mission. (I opened an issue about it, check it)

if you use setPedStat 24 with 500 value then your maxhp will be 84 which is lower than the default. It is a good thing if you want to make players to have way less maxhp by default. My problem is (thinking of opening an issue for it) that its misleading scripters.

setPedStat(ped, 24, 1000) -- maxhp: 200 -- if 1000 for 200 then
setPedStat(ped, 24, 500) -- maxhp: 84 -- 500 should be for 100 not for 84

setPedStat(ped, 24, 569) -- maxhp: 100 -- this is the default now.