In order to check if a player can pick up a weapon within a region. I had checked if the object was nearby in a local script. If they were within range and had clicked on an invisible hitbox of the weapon, it would fire a remote event where it would check if there is space in the players backpack on the server.
As I am checking if the weapon is nearby through the client, and (as far as I know) I have no sanity checks on my server script, I’m worried that this could be abused by exploiters. Are there any possible improvements?
local script
local function NearbyPickUp() -- Check if dropped wep is within range
local min = character.HumanoidRootPart.Position - (3 * character.HumanoidRootPart.Size)
local max = character.HumanoidRootPart.Position + (3 * character.HumanoidRootPart.Size)
local region = Region3.new(min, max)
for _,Part in pairs(game.Workspace:FindPartsInRegion3WithIgnoreList(region, ignoreList,math.huge)) do
if Part.Name == "PickUp" then
PickupType = Part.Parent.Parent
PickingUp = Part.Parent
local Target = Part.Parent
PickUpRM:FireServer(PickingUp, PickupType, Target)
end
end
end
Server Script
PickUpRM.OnServerEvent:Connect(function(player, PickingUp, PickupType, Target)
local count = 0
for i,v in pairs(player.Backpack:GetChildren()) do -- Check if bag is full
count += 1
end
if count == 0 then
Target:Destroy()
local ItemClass = ReplicatedStorage.Items:FindFirstChild(PickupType.Name)
local Item = ItemClass:FindFirstChild(PickingUp.Name):Clone()
print("Putting in primary")
Item.Parent = player.Backpack
end
if count == 1 then
Target:Destroy()
local ItemClass = ReplicatedStorage.Items:FindFirstChild(PickupType.Name)
local Item = ItemClass:FindFirstChild(PickingUp.Name):Clone()
print("Putting in secondary")
Item.Parent = player.Backpack
end
if count == 2 then
print("Bag Full")
end
end)
3 Likes
Yes, an exploiter can run the pickup function.
1 Like
And do you have any suggestions how I could improve the code? I’m stuck here.
1 Like
Try to avoid doing things via remotes or functions, also you could stop unexperienced script makers by encoding your outputs.
1 Like
Add a sanity check on the server to check if the player is close enough. That’s all you need to do.
What about the function checking if the player is within reach? Should I move that to a server script?
And how would I apply sanity checks in this scenario?
I meant that you need to do the same check for distance on both the client and server. I should’ve been more clear, sorry.
Take the code that you used for checking the distance on the client and copy it to the server, and don’t give the player the item if they’re too far away.
2 Likes
@amadeupworld2
I just noticed another potential vulnerability, making a separate reply so you get notified about it. You should get rid of all the values the client sends to the server except for PickingUp, and let the server set the define the values you removed from the client. (except for Target, correct me if i’m wrong but that variable was useless and was set to the same thing as PickingUp?)
The reason why is that the exploiter could set those 3 values to different things (and stuff from other tools or other things in your game) and potentially cause problems.
2 Likes
This code is trivially exploitable. Assume an exploiter can fire the remote with anything.
if count == 0 then
Target:Destroy()
If the player’s inventory is not full, then an exploiter can destroy anything. Other players, blocks, scripts that aren’t in ServerScriptService, anything. There is no checks up to this point other than the amount of items in the backpack.
local ItemClass = ReplicatedStorage.Items:FindFirstChild(PickupType.Name)
local Item = ItemClass:FindFirstChild(PickingUp.Name):Clone()
An exploiter can put anything that is in ReplicatedStorage.Items and one level deep (e.g. RS.Items.Foo.Bar, but not RS.Items.Foo or RS.Items.Foo.Bar.Handle) into their inventory, as long as their inventory is not full and they supply a Target that can be Destroyed.
FireRemote({Name = "Weapons"}, {Name = "Sword"}, workspace.Camera)
This will give the player RS.Items.Weapons.Sword and destroy workspace.Camera (which will regenerate immediately)
A small nitpick:
for i,v in pairs(player.Backpack:GetChildren()) do -- Check if bag is full
count += 1
end
You can get the amount of things in a table with the # operator.
count = #player.Backpack:GetChildren()
It would’ve made sense to do it your way if you wanted to exclude or include only certain things from the count, though.
3 Likes
Good eye. I missed a lot of things…
This isn’t always true. It won’t count dictionary keys, it starts from one and doesn’t work well with sparse arrays. (this also applies to ipairs)
local Table =
{
[0] = "zero";
[1] = "a"; -- counted
[2] = "b"; -- counted
[4] = "d";
abc = "z";
}
print(#Table) -- prints 2, instead of 5
If the player’s inventory is not full, then an exploiter can destroy anything. Other players, blocks, scripts that aren’t in ServerScriptService, anything. There is no checks up to this point other than the amount of items in the backpack.
How could I prevent this? Something along the lines of if count == 0 and PickingUp:IsA("Model") then?
An exploiter can put anything that is in ReplicatedStorage.Items and one level deep (e.g. RS.Items.Foo.Bar, but not RS.Items.Foo or RS.Items.Foo.Bar.Handle) into their inventory, as long as their inventory is not full and they supply a Target that can be Destroyed.
So to prevent this, anything more than RS.Items would be inaccessible to the client?
FireRemote({Name = "Weapons"}, {Name = "Sword"}, workspace.Camera)
This will give the player RS.Items.Weapons.Sword and destroy workspace.Camera (which will regenerate immediately)
If I was to move the variables to the server, it would prevent the exploiter from giving them items correct?
Correct, but the use case here was counting the children of Backpack, and that’s always a neat array 
You’re still thinking of it the wrong way!
All of these arguments passed to that function can be ANYTHING, ANYWHERE!
The only thing that the exploiter needs to change to bypass that is make PickingUp a model — any model, be it a character or any other model in workspace — and they can continue destroying everything, because Target does not need to be correlated with PickingUp.
PickUpRM.OnServerEvent:Connect(function(player, PickingUp, PickupType, Target)
Throw all that extra stuff away. It serves no purpose but to make your life harder and the exploiter’s life easier.
-- localscript
PickUpRM:FireServer(Target)
-- script
PickUpRM.OnServerEvent:Connect(function(player, Target)
Now make the script work, with only Target being known.
This is in the original localscript:
local Target = Part.Parent
PickupType = Part.Parent.Parent
PickingUp = Part.Parent
PickingUp is the same as Target, and PickupType is the same as Target.Parent.
1 Like
So to prevent this and this:
FireRemote({Name = "Weapons"}, {Name = "Sword"}, workspace.Camera)
This will give the player RS.Items.Weapons.Sword and destroy workspace.Camera (which will regenerate immediately)
I should move the variables into the server script instead?
Oh, and thanks for the help.