Feedback for my first anti-cheat script

[Aerodymier]

Hello everyone, I actually made this script for someone but they have never returned to me and I didn’t give either so I wanted to post it in here and have your feedback.

Module:

-- Services
local dataStoreService = game:GetService("DataStoreService")
local banStore = dataStoreService:GetDataStore("BanList")
local data

local succ, err = pcall(function()
	local dataStoreData = banStore:GetAsync("a$%324k2}*124.£#1$")
	if dataStoreData then
		dataStoreData = data
		print("Loaded the data.")
	else
		data = {}
		print("No data - default one loaded.")
	end
end)

if succ then
	print("Banned list successfully received to server.")
else
	print("Fail for receiving banned list.")
	warn(err)
end

local module = {}

module.getBanned = function()
	return data
end

module.ban = function()
	local banCmds = {}
	-- Event for banning
	local event = Instance.new("BindableEvent")
	banCmds.Changed = event.Event
	
	banCmds.updateBanned = function()
		local succ0, err0 = pcall(function()
			banStore:SetAsync(data, "a$%324k2}*124.£#1$")
		end)
		if succ0 then
			print("Successfully updated ban list.")
		else
			print("An error was occuired while trying to update ban list.")
			warn(err0)
		end
	end
	
	banCmds.add = function(plr)
		local lastIndex
		for i, v in pairs (data) do
			lastIndex = i	
		end
		print(lastIndex)
		if lastIndex == nil then lastIndex = 0 end
		table.insert(data, lastIndex+1, plr.UserId)
		banCmds.updateBanned()
		event:Fire(0)
		plr:Kick("Banned for exploiting. Contact game adminsration if you think this is wrong.")
	end
	
	banCmds.remove = function(plr)
		local lastIndex
		for i, v in pairs (data) do
			if v == plr.UserId then
				lastIndex = i
			end
		end
		table.remove(data, i)
		banCmds.updateBanned()
		event:Fire(1)
	end
	
	return banCmds
end

module.humanoidJumpCheck = function(plr)
	local char = plr.Character
	local torso = char:FindFirstChild("Torso")
	local hum = char:FindFirstChild("Humanoid")
	
	local torsoPos = torso.Position
	local trig = os.time()
	repeat wait() until hum:GetState() == Enum.HumanoidStateType.Freefall
	repeat wait() until hum:GetState() == Enum.HumanoidStateType.Landed
	local newPos = torso.Position
	local rate = (newPos - torsoPos).Magnitude * (os.time() - trig)
	if rate > 150 then
		plr:Kick("Suspicious client activity has been detected.")
	end
end

module.CheckChar = function(char)
	local HRP = char:FindFirstChild("HumanoidRootPart")
	local Torso = char:FindFirstChild("Torso")
	local Humanoid = char:FindFirstChild("Humanoid")
	local plr = game.Players:GetPlayerFromCharacter(char)
	if HRP and Torso and Humanoid then
		return
	else
		plr:Kick("An unexpected error has occuired.")
	end
end

module.HumStateCheck = function(plr)
	local char = plr.Character
	local hum = char:FindFirstChild("Humanoid")
	
	local state = hum:GetState()
	if state == Enum.HumanoidStateType.StrafingNoPhysics then
		local ban = module.ban()
		ban.add(plr)
	end
end

module.checkTPandFly = function(plr)
	while wait(1) do
		local char = plr.Character
		if char then
			local hrp = char:FindFirstChild("HumanoidRootPart")
			if hrp then
				local pos = hrp.Position
				pos = Vector3.new(pos.X, pos.Y, 0)
				
				wait(1)
				local newPos = hrp.Position
				newPos = Vector3.new(newPos.X, newPos.Y, 0)
				if (pos - newPos).Magnitude > 30 then
					plr:Kick("Suspicious client activity has been detected.")
				end
			end
		end
	end
end

module.checkGodMode = function(plr, health)
	local hum = plr.Character:FindFirstChild("Humanoid")
	
	if health > 100 then
		local ban = module.ban()
		ban.add(plr)
	end
end


return module

ServerScript:

-- Services
local players = game:GetService("Players")
-- Event
local banned
game.Players.PlayerAdded:Connect(function(plr)
	local module = require(script.AntiCheatModule)
	repeat wait() until banned
	local function banCheck(plr)
		local id = plr.UserId
		for _, v in pairs (banned) do
			if v == id then
				plr:Kick("Permanently banned, can't connect the game.")
				return
			end
		end
		print("Player is not banned.")
	end
	banCheck(plr)
	plr.CharacterAdded:Connect(function(char)
		local function checkCharParts(plr)
			module.CheckChar(plr.Character)
		end 
		local function humStateCheck(plr)
			module.HumStateCheck(plr)
		end
		
		local function checkTPandFly(plr)
			module.checkTPandFly(plr)
		end
		
		local function humanoidJumpCheck(plr)
			module.humanoidJumpCheck(plr)
		end
		
		local function checkGodMode(plr,health)
			module.checkGodMode(plr, health)
		end
		
		local function charAdded(plr, char)
			local hum = char:WaitForChild("Humanoid")
			local hrp = char:WaitForChild("HumanoidRootPart")
			
			char.DescendantRemoving:Connect(function()
				checkCharParts(plr)
			end)
			
			hum.StateChanged:Connect(function(old, new)
				humStateCheck(plr)
				if new == Enum.HumanoidStateType.Jumping then
					humanoidJumpCheck(plr)
				end
			end)
			
			hrp:GetPropertyChangedSignal("CFrame"):Connect(function() --May cause overload
				wait(2)
				checkTPandFly(plr)
			end)
			
			hum.HealthChanged:Connect(function(health)
				if health > 100 then
					checkGodMode(plr, health)
				end
			end)
		end
		charAdded(plr, char)
	end)
	plr:LoadCharacter()
end)
local module = require(script.AntiCheatModule)
wait()
banned = module.getBanned()
if banned then
	print("ServerScript has received the banned list.")
else
	print("Banned list couldn't be loaded to the server.")
end

game:BindToClose(function()
	local ban = module.ban()
	ban.updateBanned()
end)

Known issues:

• Module breaks some other scripts in game.
• Bans don’t save.

[Aerodymier]

Updated and topic still up. (30 chars)

[Syclya (zzz)]

1. HealthChanged is actually a client-sided event.

2. I assume ‘banned’ is an array, use ipairs over pairs as ipairs is designed (and is faster) when looping through arrays.

3. WaitForChild on the server is underrated, is it mainly used on the client.

4. You shouldn’t ban players, kick them instead, as this might have false outcomes at times, caused by lag etc.

5. You don’t need to check if character part’s gets deleted, it does not replicate: nor does its properties.

6. You define local players = GetService('Players'), yet you still use game.Players when creating the PlayerAdded event.

7. You only need to require() the module once, not everytime they join.

8. repeat wait() until banned - this is a very bad way of checking.

9. Overall: very messy, and can definitely be improved A LOT.

10. I recommend using datastore2 as it prevents data loss and caches its data when possible.

[Aerodymier]

Actually, it could detect player’s character parts.

[Syclya (zzz)]

Noticed that before posting it, however never got to test it as studio didn’t let me.
That’s the only thing that does replicate; colors and other properties does not.

[PysephDEV (Pyseph)]

You skimmed over a lot of other issues.

1. hrp:GetPropertyChangedSignal("CFrame") does not actually call unless set manually from the server. Some client anticheats actually have these connections on client to detect when a client manually sets a part’s cframe (assuming you never set the cframe manually through other scripts).
   I’ve seen exploiters use body movers or other exploit functions to bypass these client checks, though.
2. You can easily spoof or lock humanoid states (such as locking Landed to yield forever, which can also cause memory leaks).
3. The noclip check method is actually pretty great, as the StrafingNoPhysics state is replicated to server even when spoofed on client. Most exploits use CanCollide off for noclip now, though.
4. Your god mode check will not work as setting a humanoids health on client will not replicate to server. This would’ve caused huge issues to a lot of games otherwise.
5. Please use UpdateAsync for your modules, SetAsync is usually a bad alternative.
6. Your code,

		for _, v in pairs (banned) do
			if v == id then
				plr:Kick("Permanently banned, can't connect the game.")
				return
			end
		end

can be improved by simply doing

local isBanned = table.find(banned, id)
print('Player is', isBanned and 'banned' or 'not banned.')

7. Please do not kick a player the moment they go over a speed threshold, so many things can occur that aren’t exploit-related - getting flung by whacky physics, another exploiter flinging an innocent player, etc. You should also take into account for ping.

For Syclya:
2: ipairs is barely faster than pairs and next, and unless you want to cut down microseconds off your 20-index-long array, then be my guest.
I used my benchmark script for this, and the results are very alike at low numbers; you’d only see a minor difference in absurdly huge arrays. And by then, you wouldn’t even use ipairs for micro-optimizations anyway, you’d probably use for i = 1, #t loops (or find a better solution like a linked list).

10: DataStore2 has it’s own issues, you shouldn’t really concern yourself with data loss as it’s never occured to me in over my 4 years of scripting on Roblox. Either way, DS2 is limited in forms that it’s bound to player lifetime. If data is not bound to a player, DS2 ‘disintegrates’ and you’d have to fall back to DS. There are other arguments and counter-arguments, but I’d rather not start a discussion about DS2 on this thread.

[Aerodymier]

Marked yours as solution instead of the other post. I’m no longer continuing this project but it still taught me a lot, thanks.

[Syclya (zzz)]

1. ipairs() will only work on numerical indexes, it’s mainly built for arrays (ex, his banned table) though however I’d recommend using a numerical for loop over a generic due to the micro-optomisation.
   What I am saying: ipairs > pairs.
   pairs is used for dictionaries.

2. Data loss is not common, however DataStore2 offers a lot to avoid it even when it’s about to happen. If you hate it that much, inform the developer instead.

[PysephDEV (Pyseph)]

1. I… Never said about that?
2. I’d rather not start a discussion about DS2 on this thread.

[Syclya (zzz)]

I never mentioned numeric for loops, I’m not sure why you’re bringing it up when we’re talking about generic ones.
Obviously, numeric is superior in this situation (IF I ever was going to mention numeric ones).

[TheEdgyDev]

Luau’s VM actually optimizes ipairs and pairs over numerical for loops. Refer to my previous post here displaying the benchmark:

How efficient is this raycast hit module script?

Please keep in mind that #help-and-feedback:code-review should not be used to discuss such negligible optimizations and that the review should be relevant to the topic at hand. For the given code, pairs should be used, as premature optimizations are “the root of all evil”. This falls under said case. Using next, table is by no means a representation of “good” production code. Something many gloss over is that code must be maintained, reviewed, and augmented at a later date. “Good” code is so…

As for the technical rationale as to why this is the case with the new VM, refer to the proper documentation:

https://roblox.github.io/luau/performance.html

The performance of iteration using pairs and
ipairs is comparable, so it’s recommended to pick
the iteration style based on readability instead of
performance.Iterating through array-like tables using for i=1,#t tends to be slightly slower because of extra cost incurred when reading elements from the table

There’s my daily “micro-optimizations are anything but optimizations” digression.

[PysephDEV (Pyseph)]

You talked about ipairs being faster than other alternatives, to which I replied ipairs has an Infinitesimal performance difference than other methods (next, pairs). I don’t know where you got your information from, but pairs & next don’t have to be used for only dictionaries, in fact I’ve barely even seen people use ipairs these days due to how much Luau has optimized all iteration methods (see @TheEdgyDev 's reply).

I mentioned numeric for loops because you were on the topic of micro-optimization (ipairs > pairs).