How to prevent client from abusing RemoteEvent to get infinite cash

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.

In the specification, he asked for a system where you can claim cash based on clicking on a button, thus this would be required. Sure, in a automated system where you earn it automatically, this would work, but he needs a system that works with events.

Additionally, the code is to prevent abuse of the remote, rather than rule out his method entirely.

2 Likes

Are you sure this is how it works? nil isn’t something that is necessary to be stored in memory, since it’s literally nothing, so even if lua would make t[3] = 3 into {nil,nil,3}, the nils shouldn’t occupy the memory, as they are only the mark that there is no value under the keys 1 and 2, not that their value is an actual nil.

1 Like

Forgive me - I misread. :smiley:

I could be wrong, and yes you are right about how nil values obviously don’t occupy memory. It wasn’t really that it’d occupy memory, it was more-so that I remember t[1000] = 1 being effectively the same as for i = 1,999 do t[i] = nil end t[1000] = 1 internally on the C-side, making it a more CPU-intensive calculation. It was something that I distinctly remember being prevalent back in the day. You could even crash games back then by setting a high index number. But I can’t find anything on it in the documentation. I’m going to do some more digging on the subject as I’m interested now heh, perhaps I made an oopsy there. It doesn’t really make sense either way, I just somehow distinctly remember that being a thing.

At any rate, for the moment, I will say I still think it’d be better to use the Player object as that’s just a memory reference to something already in the memory. And in this case, he’d need to use game.Players:GetPlayerFromUserId which is just another calculation, although not intensive by any means, it’s still not necessary either in my opinion.

1 Like

I don’t think it does that. I just asked my friend (who reads lua source on a daily basis) and they said the same.

Moreover this code prints 0:

local t = {}

local a = os.time()
t[9e9] = 1
print(os.time() - a)
1 Like

I started thinking about this question a little more and I figured; why not just change the approach here? I find that this is backwards. Instead of the client telling the server that it’s ready to claim a reward, have the server tell the client that it’s been given a reward.

Over having the client click a button to claim a cash reward, change the logic. The server will update the cash on it’s side and then tell the client, via a remote, that they received a cash reward. The client will then display a Gui showing what cash was awarded and they can click claim. When claim is clicked, the client animates their cash value up until it reaches the appropriate goal (current + reward).

This way, it’s impossible to abuse a remote because the server isn’t taking any requests from the client. You also keep the functionality you wanted. The backend may differ but players won’t see that.

cc @NinjoOnline

3 Likes