How to prevent client from abusing RemoteEvent to get infinite cash

local function giveCash(player)
	UpdateCash(player, 100)
end

function RewardService:SetUp(player)
	spawn(function()
-- Give player $100 every 10 minutes
		while wait(600) do
			TimePay:FireClient(player, 100)
		end
	end)
end

TimePay.OnServerEvent:Connect(giveCash)

This basically sends an event to the client, a UI pops up, they click a button on the UI which then fires the event back to the server for them to get the cash. The problem is, exploiters could fire this event infinitely and get the cash. How can I prevent this?

I was thinking of using something like os.time() to check if the 10 minutes has passed, but don’t know how to implement it appropriately here.

4 Likes

You could implement a table debounce, it’s like a normal debounce, except that it works per player.

local Debounce = {}

local function OnServerEvent(Client)
    if Debounce[tostring(Client.UserId)] == nil then
        Debounce[tostring(Client.UserId)] = false
        --code
        wait(60*10) --cooldown
        Debounce[tostring(Client.UserId)] = nil
    end
end

RemoteEvent.OnServerEvent:Connect(OnServerEvent)

I hope this is what you were looking for? :grimacing:

3 Likes

Kinda, problem I see with this though is since the loop keeps running after the event has been fired

spawn(function()
	while wait(600) do
		TimePay:FireClient(player, 50)
    end
end)

By the time the serverevent has come back, it’s lost a few seconds, so the next time the event fire to the client, the debounce will still be set to false, as it will still have a few seconds left.

Don’t let the client naturally decide when he/she should get money. That way you won’t need the client to tell the server to give it money. Let the server do any sort of detection and reward the clients by itself.

1 Like

I want the UI to have a claim button on it. Not fun just saying ‘you earnt money’

All it requires is some tricks, I personally usually keep a cached version of the client data. What you can do is give the client it’s money straight away from the server side and then when you click the “claim” show that visually to the client.

I think I’m getting what @ScripterGo is saying, instead of letting the player tell the server to claim their money, let the server tell the client when the money is ready to be claimed.

If the player clicks the button, the event will fire and the server will check if the 10 minutes has passed, if not, the server will return an error message to the client “Your money is not yet ready to be claimed”, otherwise the player will receive their money.

Yes but I’d like to add that in most scenarios the server won’t need to tell the client anything. It can do it’s own detection and act accordingly.

1 Like

How to Avoid Exploiting


You simply have to deal with server sanity checks. The button on the claim will update the money only and the server is giving them the money, not through client request.

A yielding on server will be made to wait for the client to press the button and then the money is given. The server will use fixed payments and not any number from client’s input, else you’ll face exploiters who abuse the system by trying to trick the system.

Attempting to fire the remote while it is in cooldown(where the money is already claimed once before under a certain time), will result in no events; if else statements will block it.

TL;DR
Do not trust the client’s inputs. attempt always rely on server’s.

2 Likes

You could do this debouncing with collectionservice by adding a tag

I think you can just use a RemoteFunction for this. Sending it out invokes the client, and on the return (clicking the button) the server can award the cash

This is a method I recently looked into, I think it was also mentioned by you in another post about dealing with debounces as well, however I kind of doubt this method as it could be easier done with tables in my opinion. You also don’t need to reference any objects, only the user id by using tables.

1 Like

Edited to prevent memory leaks.

local cashReady = {}

local function giveCash(player)
    if not cashReady[ player ] then
        -- here you could record the number of times they do this and maybe kick for too many - possibly trying to exploit.
        return
    end
    cashReady[ player ] = false
    UpdateCash(player, 100)
end

function RewardService:SetUp(player)
	spawn(function()
-- Give player $100 every 10 minutes
		while wait(600) and player:IsDescendantOf( game.Players ) do
                        cashReady[ player ] = true
			TimePay:FireClient(player, 100)
		end
	end)
end

game.Players.PlayerRemoving:Connect( function ( player )
    if cashReady[ player ] ~= nil then
        cashReady[ player ] = nil
    end
end )

TimePay.OnServerEvent:Connect(giveCash)

Memory leak.

You need to remove the value after they leave, otherwise the table will keep growing as you’re holding onto a reference to the player in the cashReady table.

game.Players.PlayerRemoving:connect(function(player)
    cashReady[player] = nil
end)

Here’s how I would do this:

local nextCashReward = {}
game.Players.PlayerAdded:connect(function(Player)
	nextCashReward[Player] = os.time() + 600 -- they get their first reward in 10 minutes.
end)

game.Players.PlayerRemoving:connect(function(Player)
	nextCashReward[Player] = nil -- clean up after they leave
end)

spawn(function()
	while true do -- check every second, 
		for i,v in pairs(nextCashReward) do
			if v <= os.time() then -- time for their next reward
				nextCashReward[i] = os.time() + 600 -- give next reward in 10 minutes.
				UpdateCash(i, 100) -- give them the cash
			end
		end

		wait(1)
	end
end)

On the server, of course. The client does not need to be involved in this. If you need any of the cod explained let me know.

Also, not sure about your coding style, but if this is its own script, I’d recommend not using spawn. You don’t need to use it in events (as I’ve seen above) because it’s already it’s own thread. You only use spawn when you want to run code after the spawn, and you want the code in the spawn to run separately. Also remember that spawn will wait until the next tick to start running as well.

You could just use the player’s UserId instead. You’re still going to have to clear it when the player leaves but at least you won’t have a strong reference to the player anymore.

Also, using while wait is bad code.

If you do this, though, it’d just be messier in my opinion.

local example = {}
example[1000] = 1

This would create 999 indexes with the value “nil”, before creating the index “1000”.
This is how lua handles tables with numerical indexes.

local example = {}
example[Player.userId] = 1

That can become a hefty calculation.
Not to mention, now you also have to use game:GetPlayerByUserId() to find the player object, when it’s not necessary. The index can be the player object which is just a reference in memory to the player instance in C. Whereas, the UserID would theoretically use more memory having to store separate numbers in the memory rather than a memory reference.

If it were my code, it’d be an event oriented system, but I’m not going to write some overly complex code for what he needs, as he wouldn’t understand it. It’s a decent, simple solution that he can easily interpret and understand. I’ll edit the while loop though lol.

Edit: Lua will clear the reference when you set the value to nil on next garbage collection cycle.
Edit 2: im bad, this is false information <3 <3

2 Likes

Uwohhhh, I actually didn’t know that. I had a feeling that table slots were allocated but I didn’t know this happened. I typically create arrays with large numerical indexes like this. I am now worried…

You’ve taught me something new and now I’m going to have to go back and edit quite a lot.

:pensive:

1 Like

Glad you could learn something new!

When you must use a UserID as a index, always tostring then tonumber back if using pairs. :slight_smile:

2 Likes

On the server side, introduce a sanity check that checks when the player last fired the event. You can perform these types of checks by utilising tick(), and check whether the latest attempt to fire is bigger or equal to the tick() + seconds between each event interaction. Additionally, you shouldn’t be sending any sort of value that’ll translate into money from the client. If you are always going to recieve 50 each time, make it controllable on the server, not on the client. It means that the client can’t fire the server to give more than 50.

Example:

local players = {}
local rate = 600 -- time between each
Event.OnServerEvent:connect(player)
local attemptTick = tick()
if not players[player.UserId] or players[player.UserId] - tick() <= 0 then
     players[player.UserId] = attemptTick + rate
     -- reward 50 coins, don't reward anything else
end
end)

On the client, you’d only fire the event, you wouldn’t attach anything else because you don’t need too.
Hope this helps!

My question is: Why?
The example I gave is 100% server-side.
Why have unnecessary network traffic via firing remotes when the client should not have anything to do with rewarding cash. That should be handled server-side, and all these solutions with remotes are introducing unnecessary network traffic and over-complicates the system, splitting it to the client and the server when that’s not necessary.