Is this a good way for handling leaderstats?

Help and Feedback Code Review
#1

I’m trying to handle player’s leaderstats by using Module Scripts.

Every player will be granted a Data Module script upon joining the game, it’s stored inside ServerStorage such that player/exploiter can not access and edit it. currently it handles 3 data: Eliminations, Deaths and Streaks. It’s also referenced by game.Players.Player.leaderstats to use the in-game leaderboard stats.

Whenever the game detects a player gets an elimination, it will use a RemoteEvent to request the server to add an elimination, streak to the eliminator and add a death to the eliminated player. The server will require the eliminator’s player Module script upon OnServerEvent, and add the corresponding data to the eliminator and eliminated.
A client will send a table to the server, including the DataName, Type (Including Add, Minus, Reset), Amount, TargetPlayer (Player to modify stats to)

Client script: (This will be simplified into a function)

									-- Add 1 elimination and 1 streak to eliminator
									RemoteEvents.UpdateLeaderstats:FireServer( {DataName = "Eliminations", Type = "Add", Amount = 1, TargetPlayer = plr.Name} )
									RemoteEvents.UpdateLeaderstats:FireServer( {DataName = "Streak", Type = "Add", Amount = 1, TargetPlayer = plr.Name} )
									-- Reset eliminated streak and Add 1 Death
									RemoteEvents.UpdateLeaderstats:FireServer( {DataName = "Deaths", Type = "Add", Amount = 1, TargetPlayer = HitPlr.Name} )
									RemoteEvents.UpdateLeaderstats:FireServer( {DataName = "Streak", Type = "Reset", Amount = 1, TargetPlayer = HitPlr.Name} )

Server Script:

RemoteEvents.UpdateLeaderstats.OnServerEvent:Connect(function(FireFrom, DataTable)

print(DataTable.TargetPlayer)
-- Data is named "DATA_" PlayerName
local MS = require(ss.Data:FindFirstChild("DATA_"..DataTable.TargetPlayer))
MS.UpdateLeaderstats(FireFrom,DataTable)

end)

Module Script:

function DataModule.UpdateLeaderstats(RequestFrom,DataTable)
	local TargetPlayer = DataTable.TargetPlayer
	local DataName =  DataTable.DataName
	local Type = DataTable.Type
	print(RequestFrom," requests to change ", TargetPlayer,"stats: ", DataName ," by ", Type, DataTable)
	
	-- This will be simplified to function
	if Type == "Add" then
		game.Players:FindFirstChild(TargetPlayer).leaderstats:FindFirstChild(DataName).Value = game.Players:FindFirstChild(TargetPlayer).leaderstats:FindFirstChild(DataName).Value + 1
	elseif Type == "Minus" then
		game.Players:FindFirstChild(TargetPlayer).leaderstats:FindFirstChild(DataName).Value =  game.Players:FindFirstChild(TargetPlayer).leaderstats:FindFirstChild(DataName).Value - 1
	elseif Type == "Reset" then
		game.Players:FindFirstChild(TargetPlayer).leaderstats:FindFirstChild(DataName).Value = 0
	end
end

Despite on the code keep repeating itself, overall, In terms of game security, efficiency, is it a good method or way to handle stats? Is this way better than simply adding stats by Value = Value + amount by simply requesting from the client?

1 Like
#2

In general there’s nothing really wrong with this, though I wouldn’t say it’s a “good way” for handling leaderstats. It’s definitely not a bad way, however handling it through a single server script is a lot easier and doesn’t cause any loss in functionality, so I don’t see any real point in using this extended method. Of course I could be wrong, but to the best of my knowledge using a single server script is the better solution.

1 Like
#3

it looks like a bad event setup, there is no check on the server when the event is fired so anyone can get a script injector and fire your event and get free stats, i suggest securing your remote event more, other than that, it looks good :+1:

#4

I mean yeah It’s not really a good idea to send leaderstats trought the RemoteEvents. This is also easy for exploiters to bypass your leaderstats. They can change some valuses while sending it to the server. It’s better to create full leaderstats on the script. I would use a DataStore wrapper if you need one leaderstats right now, the best one there out is DataStore2, it’s esay to use and to learn their functions. I hope this helps. :herb:

#5

Multiple things:
Issues with your reasoning:
You can’t really use module scripts themselves to hold data. Your game looks at it and requires it once, then you can’t really store data into them themselves. What you can do is make an instance of some ModuleScript Class for managing data, but that doesn’t really make it any different from managing from an ordinary script.

Issues with your approach:
Using remotes to move and interact does not make your game secure whatsoever. It is true that ServerStorage is not viewable from the clientside. But you still have one direct link from the server side to client side. It is etremely bad that you are sending one huge table from the client to the server full of data. You just stated that your client can modify other player’s data. Which means I can run a client-side loop exploit to infinitely add/subtract the stats of the Target player through your remote.

How Exploits work:
How modern exploits work are all from the client side. Exploiters are able to view anything client-side including remotes.
Then they use a script injector to “inject” a script from the client side. These injected scripts can fire off your remotes with their own data. In addition if you use any modulescripts on the clientside, it is possible for them to replace those modulescripts with their own that utilize your remotes and modifies the data.

Easiest Solution:
In terms of data management, keep it all on the Server side. They can’t ruin/access what they can’t touch.
From the client-side, use only Getter remotes for retrieving data from the server, as that can’t be modified if you are only retrieving data instead of sending.
In terms of death detection and other sorts that can happen where you need to send data from the client, you will need to take additional measure such as distance between targets, detecting if its a legal kill, etc. Raycasting is a good option for checking distance. There are many other options and solutions, but that’s the easiest I can give you.

1 Like
#6

Yes, that’s definitely my major concern right now. Stats can be even changed by me using the command bar and FireSevrer with it, how do I secure my RemoteEvents?

#7

So for what you mean is detect the kill on the server? Currently I detect the kill by Damage - Health <= 0, will it cause performance issues such as delay or the eliminations stacking for few times because of the transition time between client and server?

All of my raycasting are all client based, I also implement a system where damage won’t be passed if a player’s ping is too high. Can you give some comments/suggestions on that?

#8

The only way you can secure Remotes are checks. Its find to send data on a kill to the server, but you still need to checks to determine if the kill is legal. A large issue indeed has to do with performance issues and overhead between the client and the server. There are ways to mediate it through other methods, but it means you would have to perform more checks. Not passing damage due to too high ping isn’t very good for the player base.
For the damage, you can double check the damage based on known stats and weapons, etc from the server-side. Distance is a bit harder. I would probably check from both sides. The kill on the player, and the death of the player to the killer before and after the kill. I’ve seen exploits where players bypass the whole distance by simply setting the CFrames of their player, or mobs in PvE environment to wherever.
Some checks may require some math. Such as monitoring movement speed no top of distance, and etc.

#9

How would I check the damage is valid or not apart from checking the ray casted distance or player based properties? Is there anything do with the player ping? I mainly want to prevent damage being valid when there’s a lag spike.

#10

I mean the ping for lag spike is fine since they probably won’t notice it.
Checking damage amount you can do with some math since you are retrieving stored stats from the server. But otherwise you have to check based on client interactions via player based properties. Exploiters can only modify their client and their version of the client. They cannot change other player’s pc’s. How Client and Server Interaction works is that you have 1 server. The client is replicated for every player and every pc. So it is possible to analyze between two different clients, the player and the target. As long as you are just passing information from the client without modifying any actual, stored data, it should be safe.

#11

This was the primary reason for the current Development Mode and FilteringEnabled additions a few years back. Prior, there wasn’t directly a separation of Client and Servers.

#12

When lag spike occurs, either on client or server, most likely everything will be freeze on the perspective of the client, the player can just walk around and damage everyone, how do I prevent that?

Plus, can exploiters modify Module Scripts? You said they can change to their own module script so I suppose they can? In conclusion, The stats of the weapon such as Damage, Fire Rate should be stored inside Server Storage?

#13

I’m saying its fine to keep your ignore damage due to high ping since it is unlikely that the players will notice it.
Exploiters can’t directly modify Module Scripts. I’ve seen on a recent game I work on that they can simply create their own, and fit it in with the same name. How the injector worked was that they create their own table that refers to certain ModuleScripts. It works like a dictionary. Then they insert theirs into it. They don’t actually look into the scripts themselves. Roblox scripts are generally Read-Only and locked. They can see remotes though and fire it from their own scripts. As I said, modern exploiters use Script injectors which, by definition inserts/injects a script in.
As I said, that is only from the Client Side. If you use ModuleScripts in the server, it should be fine for those related stuff. Although, you should keep specific data related stuff to the ServerStorage. When a Client is loaded, modulescripts are initially copied over, not necessarily changed into the ReplicatedStorage. They cannot change what is in Replicated Storage necessarily.

#14

This is SUPER unsafe as other have been telling you. NEVER DO THIS!!!

What you are doing is letting the CLIENT send vital data to the server. This breaks rule number 1:

Never trust the client

What’s stopping me from sending

RemoteEvents.UpdateLeaderstats:FireServer( {DataName = "Eliminations", Type = "Add", Amount = 9999999, TargetPlayer = plr.Name} )
RemoteEvents.UpdateLeaderstats:FireServer( {DataName = "Eliminations", Type = "Reset", Amount = 1, TargetPlayer = HitPlr.Name} )

ALWAYS handle all vital data on the server. You should be detecting/counting eliminations on the server, NEVER on the client! The only thing the client should be doing is handling UI and input. (there are a few exceptions, but in a secure game this is the general rule)

#15

Now I detect the elimination on server, everything works fine.

RemoteEvents.TakeDMG.OnServerEvent:Connect(function(EliminatorPlr , EliminatedPlr,dmg)
local HitChar = EliminatedPlr.Character
if HitChar.Humanoid.Health ~= 0 then
	HitChar.Humanoid:TakeDamage(dmg)
	
	if HitChar.Humanoid.Health < 1 then

		spawn(function()
			local Module = require(ss.Data:FindFirstChild("DATA_"..EliminatorPlr.Name))
			Module.UpdateLeaderstats( EliminatorPlr, CreateDataTable("Eliminations", "ADD", 1) )
			Module.UpdateLeaderstats( EliminatorPlr, CreateDataTable("Streak", "ADD", 1) )
		end)
		
		spawn(function()
			local Module = require(ss.Data:FindFirstChild("DATA_"..EliminatedPlr.Name))
			Module.UpdateLeaderstats( EliminatedPlr, CreateDataTable("Deaths", "ADD", 1) )
			Module.UpdateLeaderstats( EliminatedPlr, CreateDataTable("Streak", "RESET", 1) )
		end)
					
		RemoteEvents.KillMsg:FireClient(EliminatorPlr, EliminatedPlr)
		RemoteEvents.KillMsg:FireClient(EliminatedPlr, EliminatorPlr)

	end
end

end)

Is there any exceptional case where the client can change his data located on server by using RemoteEvents? For an example, a client wants to equip a new weapon, I’ll use a RemoteEvent to let the server know to update his Data Module, is it a good approach?

#16

The general rule is to never trust the client and always assume any data sent from the client to the server has been tampered with. With that in mind any data sent from the client should be checked by the server. The client should never have any control over what happens to their data.

I am going to say this is fine providing there is proper server sided checks in place and there is no way for the client to have any authority over what weapon they equipt. The server should be checking if the user is allowed to equip that weapon or even owns the weapon.

You may want to read over this tutorial about securing your game because it explains about not trusting the client well: