Potential Improvements for Safety and Efficiency in My Remote Event

Introduction:
I am relatively new to the field of security and exploitation in remote event management. However, I have implemented several safety measures, such as a rate check to prevent DOS attacks and verification of all arguments provided by clients.

My Remote Event Explained:
I have a remote event that, when fired on the server, takes the arguments of an instance’s name, such as “Ford Pinta”, and a boolean that tells me if the car is owned or not, for example, True.

At the start, I check if the arguments are of the right class. Then, I check if the InstanceName is a correct name that can be saved. For that, I have a validation table that I will show later in my module on the server. After that, I check if the DataKey and the ItemCategory are not nil along with the ItemName (this will make sense later, I promise). Then, I check if the remote event is fired too heavily. If it is, I impose a 5-second cooldown (if you don’t find this efficient, please give me feedback).

After that, I check if the Ford Pinta is already owned. If it is, then it returns. Then, I check if I want to buy the Ford Pinta. If I do, I first check if I can even own it with my current money. If I can, then I will buy it, subtract the money, and save it to the profile.

After all these checks, I will finally save the Ford Pinta as owned.

Code Of the Remote

UpdateOwnership.OnServerEvent:Connect(function(player, InstanceName: string, InstanceState: boolean)
    local DataKey, ItemCategory = MainSaving.getDataKeyAndCategory(InstanceName)
    print(DataKey, ItemCategory)

    if type(InstanceName) ~= "string" or type(InstanceState) ~= "boolean" or not InstanceState then
        warn("Invalid input types for InstanceName or InstanceState")
        return
    end
    -- Validate car and category using the provided validation table
    if not MainSaving.validCars(InstanceName, ItemCategory) then return end
    if not DataKey or not ItemCategory then return end

    -- Check rate limits to prevent abuse
    if not Rate:Check(player) then 
        task.wait(5)
    end

    -- Check if the player already owns the item (excluding GarageParts)
    if InstanceState and MainSaving.checkOwnership(player, InstanceName, DataKey) and DataKey ~= "GarageParts" then
        print("Player already owns:", InstanceName)
        return
    end

    -- Handle the purchasing logic
    if InstanceState then
        if not MainSaving.canOwnInstance(player, InstanceName, ItemCategory) then
            warn("Player cannot afford the car:", InstanceName)
            return
        else
            -- Deduct the cost and mark the item as purchased
            MainSaving.buyInstance(player, InstanceName, ItemCategory)
        end
    end

    -- Update the player's data
    local Data = PlayerDataHandler:Get(player, DataKey)
    if Data then
        Data[InstanceName] = InstanceState
        PlayerDataHandler:Set(player, DataKey, Data)
    end
end)

I will appreciate any feedback I receive and am excited for your responses. If you need more information about my module, please let me know, and I will provide it!

UpdateOwnership.OnServerEvent:Connect(function(player, InstanceName: string, InstanceState: boolean)
    local DataKey, ItemCategory = MainSaving.getDataKeyAndCategory(InstanceName)
    print(DataKey, ItemCategory)

    if type(InstanceName) ~= "string" or type(InstanceState) ~= "boolean" or not InstanceState then
        warn("Invalid input types for InstanceName or InstanceState")
        return
    end
local DataKey, ItemCategory = MainSaving.getDataKeyAndCategory(InstanceName)
    print(DataKey, ItemCategory)

this can be moved to after the type checks. also i don’t know if this is safe or not, we don’t have the function. it looks like its just a table.unpack(string.split(InstanceName, ’ '))

if not MainSaving.validCars(InstanceName, ItemCategory) then return end

good

if not Rate:Check(player) then 
        task.wait(5)
    end

this is not a rate check. say the check returns false, then the task.wait runs for 5 seconds. but the client can still fire the remote during that 5 seconds. alternatively you can kick the player if they fire the remote too fast, and add a smaller rate check on the client.

1 Like

also move this post to #help-and-feedback:code-review

1 Like

My preferred order goes something like:

Check Argument Types
Check Ratelimit
Get DataKey and ItemCategory
Assert that DataKey or ItemCategory exists
Validate Car
Ownership Clause
Check if player has sufficient funds
If so, run the buy function
Else just return
Update player's data
1 Like

Thank you so much for the suggestions. I am sorry for the late reply havent really looked at this post

I’ll definitely take a look at this and see what I can tweak to make it better.

1 Like