How To Secure This Remote Event

mariohead13 · August 22, 2021, 9:48pm

Hello everyone, in my game, I made a coin system, that when the player touches the coin, a certain amount gets added to his account. However, I just realized that I am calling a remoteEvent from the client to the server, and that exploiters could just spam this remoteEvent. I thought that the solution to this would be to check if the coin is touched on the server and add the coins, and then fire to client to add the UI. I just wanted to see if there was another solution, or if the remoteEvent is already secured. Here is the localscript.

local Players = game:GetService("Players")
local Sounds = workspace:WaitForChild("Sounds")
local coinSound = Sounds:WaitForChild("CoinSound")
local part = workspace:WaitForChild(script.Name)
local player = Players.LocalPlayer
local gui = script.Parent.Parent
local textLabelPoints = gui:WaitForChild("Points")
local textLabel = textLabelPoints:Clone()
textLabel.Parent = gui
textLabel.Name = "PointsClone"
local destroyed = false
local remoteEvent = game.ReplicatedStorage:WaitForChild("AddPointsNight")
local MoonCoinValue = game.ReplicatedStorage:WaitForChild("MoonCoinValue").Value

local Num = math.random(1, 4)


part.Touched:Connect(function(Hit)
	local humanoid = Hit.Parent:FindFirstChild("Humanoid")
	if humanoid then
		local char = humanoid.Parent
		local player = game.Players:GetPlayerFromCharacter(char)
		if not destroyed and player == game.Players.LocalPlayer then
			if Num == 1 then
				textLabel.Position = UDim2.new(0.639, 0, 0.761, 0)
		
			elseif Num == 2 then
				textLabel.Position = UDim2.new(0.14, 0, 0.583, 0)
	
			elseif Num == 3 then
				textLabel.Position = UDim2.new(0.205, 0, 0.219, 0)
	
			elseif Num == 4 then
				textLabel.Position = UDim2.new(0.599, 0, 0.191, 0)

			end
			textLabel.Text = "+"..MoonCoinValue
			textLabel.Visible = true
			destroyed = true 
			coinSound:Play()
			remoteEvent:FireServer()
			part:Destroy()
			wait(1.2)
			textLabel:Destroy()
		end
	end
	
end)

This is the server side script.

local remoteEvent = game.ReplicatedStorage:WaitForChild("AddPointsNight")

remoteEvent.OnServerEvent:Connect(function(player)
	local accumulatedCoins = player:WaitForChild("accumulatedCoins")
	accumulatedCoins.Value = accumulatedCoins.Value + 3
end)

Thank you

sleitnick · August 22, 2021, 9:52pm

I agree with your thought about listening for the touched event server-side. You absolutely should do that instead. Currently, you have a RemoteEvent that any client could spam to get free money. In other words, the client has full control on making money. This is not very safe for your game. It would be better for the server to listen for the part to be touched and then add money as necessary.

To keep visuals snappy, I recommend still having client-side code listen for touches and move around the UI as necessary, but the server should still have the full say in whether to reward coins or not.

mariohead13 · August 22, 2021, 10:10pm

I think I still have to use a remoteEvent, because I want the part to get deleted only in the localscript, so that the other players still see the coin. Right now, when a player touches the coin, he still gets more coins if he stands at the place it was, since it is getting deleted only on the client. So I think I will need to use a server to client remote event.

mariohead13 · August 22, 2021, 10:43pm

I just realized that even with a remoteEvent, it still has the same problem.Right now, I can’t think of any way to make it so that the server adds the Coins, yet the localscript deletes the Coin, in a secure and efficient way. How would I go about doing that?

goldenstein64 · August 22, 2021, 11:09pm

Maybe you can try keeping a list of all the coins the player has touched and use that to rule out which ones they can’t get anymore.

e.g. You can give each coin instance a CoinId attribute, and if the player has touched a coin they already collected, you don’t reward them any money.

-- client psuedocode

local CollectionService = game:GetService("CollectionService")

local coinRemote = path.to.CoinRemote

LocalPlayerHumanoid.Touched:Connect(function(hit)

	-- it would be a good idea to make a client-side check
	-- for coins they already touched too

	if CollectionService:HasTag(hit, "Coin") then
		coinRemote:FireServer(hit)
	end
end)

-- server psuedocode

local CollectionService = game:GetService("CollectionService")

local coinRemote = path.to.CoinRemote

coinRemote.OnServerEvent:Connect(function(player, coin)
	if 
		typeof(coin) ~= "Instance"
		or not CollectionService:HasTag(coin, "Coin")
	then return end

	-- dictionary of string ids to bools
	-- can be retrieved/cached from a datastore or created on join, etc.
	local allHitCoins = -- {}

	local coinId = coin:GetAttribute("CoinId")

	if allHitCoins[coinId] then return end
	allHitCoins[coinId] = true

	awardCurrency(player, coin:GetAttribute("Value"))
end)

If coins are created/destroyed over time because they naturally regenerate or something, you can just increment an id counter and give those coins the new id. If you don’t want to risk an overflow (at around 9 * 10^15), you can probably add a function for deleting all existing coins and regenerating them with new id’s starting from 1, as well as remapping hit coins if necessary.

mariohead13 · August 22, 2021, 11:16pm

I will try that solution out, however, I am confused about one line

typeof(coin) ~= "Instance"

Why are you checking to see if the coin is not an Instance?

goldenstein64 · August 22, 2021, 11:24pm

An exploiter could spoof the arguments and send something that’s not an instance, which would make the remote invocation (specifically CollectionService:HasTag) error. I’m not really sure if the error gets propagated back to the client, letting the exploiter use that as a clue to figure out what your remote is doing server-side.

mariohead13 · August 23, 2021, 2:59pm

Hello, sorry for the late response, i had some problems. now, it works though, thank you. This is the way I did it, please tell me if there is anything I should change.

--localscript

local collectionService = game:GetService("CollectionService")
local Players = game:GetService("Players")
local Sounds = workspace:WaitForChild("Sounds")
local coinSound = Sounds:WaitForChild("CoinSound")
local player = Players.LocalPlayer
local gui = script.Parent.Parent
local part = workspace:WaitForChild(script.Name)
local textLabelPoints = gui:WaitForChild("Points")

local remoteEvent = game.ReplicatedStorage:WaitForChild("AddPointsCandy")
local GummyBearCoin = game.ReplicatedStorage:WaitForChild("GummyBearCoinValue").Value

local Num = math.random(1, 4)


local humanoid = player.Character.Humanoid

local debounce = false

humanoid.Touched:Connect(function(Hit)
	if collectionService:HasTag(Hit, "GummyBearCoin") then
		if not debounce then
			debounce = true
			remoteEvent:FireServer(Hit)
			local char = humanoid.Parent
			local player = game.Players:GetPlayerFromCharacter(char)
			if player == game.Players.LocalPlayer then
				local textLabel = textLabelPoints:Clone()
				textLabel.Parent = gui
				textLabel.Name = "PointsClone"
				if Num == 1 then
					textLabel.Position = UDim2.new(0.639, 0, 0.761, 0)

				elseif Num == 2 then
					textLabel.Position = UDim2.new(0.14, 0, 0.583, 0)

				elseif Num == 3 then
					textLabel.Position = UDim2.new(0.205, 0, 0.219, 0)

				elseif Num == 4 then
					textLabel.Position = UDim2.new(0.599, 0, 0.191, 0)

				end
				
				textLabel.Text = "+"..GummyBearCoin
				textLabel.Visible = true
				coinSound:Play()
				Hit:Destroy()
				wait(0.3)
				debounce = false
				wait(1.2)
				textLabel:Destroy()
				
			end
		end
		
	end
end)

-- script
local remoteEvent = game.ReplicatedStorage:WaitForChild("AddPointsCandy")
local collectionService = game:GetService("CollectionService")


remoteEvent.OnServerEvent:Connect(function(player, coin)
	if typeof(coin) ~= "Instance" or not collectionService:HasTag(coin, "GummyBearCoin") then
		return
	end
	-- I should probably use a for loop for this, instead of doing it manually
	local allHitCoins = {
		Coin1 = false,
		Coin2 = false,
		Coin3 = false,
		Coin4 = false,
		Coin5 = false,
		Coin6 = false,
		Coin7 = false,
		Coin8 = false,
		Coin9 = false,
		Coin10 = false,
		Coin11 = false,
		Coin12 = false,
		Coin13 = false,
		Coin14 = false,
		Coin15 = false,
		Coin16 = false,
		Coin17 = false,
		Coin18 = false,
		Coin19 = false,
		Coin20 = false,
		Coin21 = false,
		Coin22 = false,
		Coin23 = false,
		Coin24 = false,
		Coin25 = false,
		Coin26 = false,
		Coin27 = false,
		Coin28 = false,
		Coin29 = false,
		Coin30 = false,
		Coin31 = false,
		Coin32 = false,
		Coin33 = false,
		Coin34 = false,
		Coin35 = false,
		Coin36 = false,
		Coin37 = false,
		Coin38 = false,
		Coin39 = false,
		Coin40 = false,
		Coin41 = false,
		Coin42 = false,
		Coin43 = false,
		Coin44 = false,
		Coin45 = false,
		Coin46 = false,
		Coin47 = false,
		Coin48 = false,
		Coin49 = false,
		Coin50 = false,
		Coin51 = false,
		Coin52 = false,
		Coin53 = false,
		Coin54 = false,
		Coin55 = false,
		Coin56 = false,
		Coin57 = false,
		Coin58 = false,
		Coin59 = false,
		Coin60 = false
	}
	
	local coinId = coin:GetAttribute("CoinId")
	
	if allHitCoins[coinId] then 
		return 
	end
	
	allHitCoins[coinId] = true
	local accumulatedCoins = player:WaitForChild("accumulatedCoins")
	
	accumulatedCoins.Value = accumulatedCoins.Value + 5
end)

goldenstein64 · August 23, 2021, 10:38pm

If you create allHitCoins from inside the function, it will get destroyed and reconstructed every time the remote is called, which means you can still get the same coin every time this remote is called.
- Instead, you can create the table when the player joins, put it in a player dictionary that holds all the allHitCoins tables, and retrieve it when the remote event is fired.
You can just have an empty table for allHitCoins, because indexing a table for an entry that doesn’t exist will give back nil, and nil is falsy, so it will interrupt the function early.

-- in your server script,

local Players = game:GetService("Players")

local hitCoinsPerPlayer = {}

Players.PlayerAdded:Connect(function(player)
	hitCoinsPerPlayer[player] = {}
end)

Players.PlayerRemoving:Connect(function(player)
	hitCoinsPerPlayer[player] = nil
end)

remoteEvent.OnServerEvent:Connect(function(player, coin)
	-- validate `coin` input

	local allHitCoins = hitCoinsPerPlayer[player]

	-- check coin id, award currency, etc.
end)

mariohead13 · August 23, 2021, 11:04pm

Ok, I will try that. I also noticed another issue, in this local script

local collectionService = game:GetService("CollectionService")
local Players = game:GetService("Players")
local Sounds = workspace:WaitForChild("Sounds")
local coinSound = Sounds:WaitForChild("CoinSound")
local player = Players.LocalPlayer
local gui = script.Parent.Parent
local part = workspace:WaitForChild(script.Name)
local textLabelPoints = gui:WaitForChild("Points")

local remoteEvent = game.ReplicatedStorage:WaitForChild("AddPointsCandy")
local GummyBearCoin = game.ReplicatedStorage:WaitForChild("GummyBearCoinValue").Value

local Num = math.random(1, 4)


local humanoid = player.Character.Humanoid

local debounce = false

humanoid.Touched:Connect(function(Hit) -- I think this line is faulty
	if collectionService:HasTag(Hit, "GummyBearCoin") then
		if not debounce then
			debounce = true
			remoteEvent:FireServer(Hit)
			local char = humanoid.Parent
			local player = game.Players:GetPlayerFromCharacter(char)
			if player == game.Players.LocalPlayer then
				local textLabel = textLabelPoints:Clone()
				textLabel.Parent = gui
				textLabel.Name = "PointsClone"
				if Num == 1 then
					textLabel.Position = UDim2.new(0.639, 0, 0.761, 0)

				elseif Num == 2 then
					textLabel.Position = UDim2.new(0.14, 0, 0.583, 0)

				elseif Num == 3 then
					textLabel.Position = UDim2.new(0.205, 0, 0.219, 0)

				elseif Num == 4 then
					textLabel.Position = UDim2.new(0.599, 0, 0.191, 0)

				end
				
				textLabel.Text = "+"..GummyBearCoin
				textLabel.Visible = true
				coinSound:Play()
				Hit:Destroy()
				wait(0.3)
				debounce = false
				wait(1.2)
				textLabel:Destroy()
				
			end
		end
		
	end
end)

The player doesn’t collect any coins after he dies once, I think the humanoid.Touched is faulty. How could I solve this?

goldenstein64 · August 24, 2021, 12:21am

I’m guessing what’s wrong is you placed the local script in StarterPlayerScripts, which means the humanoid variable and any GUIs you were using are destroyed when the character respawns, along with the humanoid.Touched event being disconnected.

I would just place the local script in StarterCharacterScripts for simplicity’s sake.

mariohead13 · August 24, 2021, 1:30am

The localscripts are in the StarterGui, and I just tested it again, and it doesn’t seem like humanoid.Touched is the issue.
My test to see if it was humanoid.touch

local Players = game:GetService("Players")
local player = Players.LocalPlayer
local collectionService = game:GetService("CollectionService")

local humanoid = player.Character:WaitForChild("Humanoid")

local debounce = false

humanoid.Touched:Connect(function(Hit)
	if collectionService:HasTag(Hit, "GummyBearCoin") then
		if not debounce then
			print("player touched GummyBearCoin")
			debounce = true
			wait(1)
			debounce = false
		end
	end
	
end)

This printed every time even when the player died, so I have no idea what could be the problem.

goldenstein64 · August 24, 2021, 7:51am

The last thing I can think of is making sure the gui this script is a descendant of has its .ResetOnSpawn property set to true. Otherwise, I have no idea what could be causing humanoid.Touched to stop working.

mariohead13 · August 24, 2021, 6:27pm

Hello, I have fixed everything now. This is the final code.

local collectionService = game:GetService("CollectionService")

local Players = game:GetService("Players")

local hitCoinsPerPlayer = {}

Players.PlayerAdded:Connect(function(player)
	hitCoinsPerPlayer[player] = {}
	for i = 1, 60 do
		local name = "Coin"..i
		hitCoinsPerPlayer[player][name] = false
	end

end)

Players.PlayerRemoving:Connect(function(player)

	hitCoinsPerPlayer[player] = nil
end)

remoteEvent.OnServerEvent:Connect(function(player, coin)
	if typeof(coin) ~= "Instance" or not collectionService:HasTag(coin, "GummyBearCoin") then
		return
	end
	


	local coinId = coin:GetAttribute("CoinId")

	if hitCoinsPerPlayer[player][coinId] then 
		return 
	end

	hitCoinsPerPlayer[player][coinId] = true
	local accumulatedCoins = player:WaitForChild("accumulatedCoins")

	accumulatedCoins.Value = accumulatedCoins.Value + 5
end)

goldenstein64 · August 24, 2021, 7:32pm

mariohead13:

Players.PlayerAdded:Connect(function(player)
	local allHitCoins = hitCoinsPerPlayer[player]
	for i = 1, 60 do
		local name = "Coin"..i
		allHitCoins[name] = false
	end
	hitCoinsPerPlayer[player] = allHitCoins

end)

By indexing hitCoinsPerPlayer[player] in Players.PlayerAdded, you’re implying the table was already created when the player joins, which is incorrect. You have to create a table first before you can use it by using { ... } syntax.

-- change this (line 2 relative to quote)
local allHitCoins = hitCoinsPerPlayer[player]

-- to this
local allHitCoins = {}

mariohead13 · August 24, 2021, 8:06pm

I think the code you saw was my old post that I erased. This is the one I have now.

local remoteEvent = game.ReplicatedStorage:WaitForChild("AddPointsCandy")
local collectionService = game:GetService("CollectionService")

local Players = game:GetService("Players")

local hitCoinsPerPlayer = {}

Players.PlayerAdded:Connect(function(player)
	hitCoinsPerPlayer[player] = {}
	for i = 1, 60 do
		local name = "Coin"..i
		hitCoinsPerPlayer[player][name] = false
	end

end)

Players.PlayerRemoving:Connect(function(player)

	hitCoinsPerPlayer[player] = nil
end)

remoteEvent.OnServerEvent:Connect(function(player, coin)
	if typeof(coin) ~= "Instance" or not collectionService:HasTag(coin, "GummyBearCoin") then
		return
	end
	


	local coinId = coin:GetAttribute("CoinId")

	if hitCoinsPerPlayer[player][coinId] then 
		return 
	end

	hitCoinsPerPlayer[player][coinId] = true
	local accumulatedCoins = player:WaitForChild("accumulatedCoins")

	accumulatedCoins.Value = accumulatedCoins.Value + 5
end)