Inventory system code review

[TheBrainy06 (Brain)]

Hello people of devforum,

I made a script a few months ago for a commission which handles the following things:

• Inventory System
• Item placing
• Item selling
• Item buying

Once I was done with the commission, I was told that my backend code was apparently not secure and needed to undergo some major changes. I personally believe that this code is very secure and does not need any major changes.

I would greatly appreciate if you guys could review and give some feedback on this code:

local ServerScriptService = game:GetService("ServerScriptService")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local RunService = game:GetService("RunService")
local HttpService = game:GetService("HttpService")

local Remotes = ReplicatedStorage.Remotes

local dataHandler = require(ServerScriptService.DataHandler)
local tableHandler = require(ServerScriptService.TableHandler)
local itemConfig = require(ReplicatedStorage.Modules.ItemConfig)

function UpdateInventory(plr, newData)
	plr.Data.Value = HttpService:JSONEncode(newData)
end

function GetPlayerData(plr)
	return HttpService:JSONDecode(plr.Data.Value)
end

function AddItemToInventory(plr, itemName)
	local plrData = HttpService:JSONDecode(plr.Data.Value)
	
	plrData.Inventory[HttpService:GenerateGUID(false)] = itemName
	
	plr.Data.Value = HttpService:JSONEncode(plrData)
	
	Remotes.UpdateInventoryUI:InvokeClient(plr, plrData)
end

function DoesPlayerHaveItem(plr, itemName, itemId)
	for currentItemId, currentItemName in pairs(GetPlayerData(plr).Inventory) do
		if itemName == currentItemName and itemId == currentItemId then
			return true
		end
	end
	
	return false
end

function RemoveItemFromInventory(plr, itemId, itemName, isSelling)
	local plrData = GetPlayerData(plr)

	if plrData.Inventory[itemId] == itemName then
		plrData.Inventory[itemId] = nil
		
		if isSelling then
			plr.leaderstats.Money.Value += itemConfig[itemName].Cost
		end

		UpdateInventory(plr, plrData)
	end
	
	Remotes.UpdateInventoryUI:InvokeClient(plr, plrData)
end

function IsItemPlaced(plr, itemName, itemId)
	local plrTable = workspace.Tables:FindFirstChild(("%sTable"):format(plr.Name))
	local decodedData = HttpService:JSONDecode(plrTable.PlacedObjects.Value)
	
	for currentItemId, currentItemName in pairs(decodedData) do
		if itemName == currentItemName and currentItemId == itemId then
			return true
		end
	end
	
	return false
end

function SavePlacedItems(plr, itemName, itemId)
	local plrTable = workspace.Tables:FindFirstChild(("%sTable"):format(plr.Name))
	local decodedData = HttpService:JSONDecode(plrTable.PlacedObjects.Value)
	
	decodedData[itemId] = itemName
	
	plrTable.PlacedObjects.Value = HttpService:JSONEncode(decodedData)
	
	Remotes.UpdatePlacedUI:InvokeClient(plr, decodedData)
	
	RemoveItemFromInventory(plr, itemId, itemName, false)
end

function UnplaceItem(plr, itemName, itemId)
	if not IsItemPlaced(plr, itemName, itemId) then return end
	
	local plrTable = workspace.Tables:FindFirstChild(("%sTable"):format(plr.Name))
	local decodedData = HttpService:JSONDecode(plrTable.PlacedObjects.Value)
	local decodedPlrData = HttpService:JSONDecode(plr.Data.Value)
	
	decodedData[itemId] = nil
	
	plrTable.PlacedObjects.Value = HttpService:JSONEncode(decodedData)
	decodedPlrData.Inventory[itemId] = itemName
	
	plr.Data.Value = HttpService:JSONEncode(decodedPlrData)
	
	Remotes.UpdateInventoryUI:InvokeClient(plr, decodedPlrData)
	Remotes.UpdatePlacedUI:InvokeClient(plr, decodedData)
	
	for _, v in pairs(plrTable.Placed:GetChildren()) do
		if v.Name == itemName and v.ItemId.Value == itemId then
			v:Destroy()
		end
	end
end

function PlaceItem(plr, itemName, itemId)
	local plrTable = workspace.Tables:FindFirstChild(("%sTable"):format(plr.Name))
	local clonedItem = ReplicatedStorage.Items:FindFirstChild(itemName):Clone()

	if not plrTable then return end
	if IsItemPlaced(plr, itemName, itemId) then return end
	if plrTable.Placed:FindFirstChild(itemName) then return end

	clonedItem.CFrame = plrTable.PrimaryPart.CFrame * itemConfig[itemName].Position
	clonedItem.ItemId.Value = itemId
	clonedItem.Parent = plrTable.Placed

	SavePlacedItems(plr, itemName, itemId)
end

game.Players.PlayerAdded:Connect(function(plr)
	dataHandler.Load(plr)
	tableHandler.AssignTable(plr)
	dataHandler.LoadTable(plr)
end)

game.Players.PlayerRemoving:Connect(function(plr)
	dataHandler.Save(plr)
	dataHandler.SaveTable(plr)
	tableHandler.UnassignTable(plr)
end)

Remotes.SellItem.OnServerEvent:Connect(function(plr, itemInfo)
	if not DoesPlayerHaveItem(plr, itemInfo.ItemName, itemInfo.ItemId) then return end
	if not itemConfig[itemInfo.ItemName] then return end
	
	RemoveItemFromInventory(plr, itemInfo.ItemId, itemInfo.ItemName, true)
end)

Remotes.PlaceItem.OnServerEvent:Connect(function(plr, itemInfo)
	if not DoesPlayerHaveItem(plr, itemInfo.ItemName, itemInfo.ItemId) then return end
	if not itemConfig[itemInfo.ItemName] then return end
	
	PlaceItem(plr, itemInfo.ItemName, itemInfo.ItemId)
end)

Remotes.UnplaceItem.OnServerEvent:Connect(function(plr, itemInfo)
	UnplaceItem(plr, itemInfo.ItemName, itemInfo.ItemId)
end)

game:BindToClose(function()
	if RunService:IsStudio() then return end
	
	for _, plr in pairs(game:GetService("Players"):GetChildren()) do
		dataHandler.Save(plr)
		dataHandler.SaveTable(plr)
		tableHandler.UnassignTable(plr)
	end
end)

workspace.BuyWood.ClickDetector.MouseClick:Connect(function(plr)
	if plr.leaderstats.Money.Value < itemConfig["Wood"].Cost then return end
	
	plr.leaderstats.Money.Value -= itemConfig["Wood"].Cost
	
	AddItemToInventory(plr, "Wood")
end)

workspace.BuyIron.ClickDetector.MouseClick:Connect(function(plr)
	if plr.leaderstats.Money.Value < itemConfig["Iron"].Cost then return end
	
	plr.leaderstats.Money.Value -= itemConfig["Iron"].Cost
	
	AddItemToInventory(plr, "Iron")
end)

workspace.BuyGold.ClickDetector.MouseClick:Connect(function(plr)
	if plr.leaderstats.Money.Value < itemConfig["Gold"].Cost then return end
	
	plr.leaderstats.Money.Value -= itemConfig["Gold"].Cost
	
	AddItemToInventory(plr, "Gold")
end)

Thank you in advance!

[Eestlane771]

(Live @Eestlane771 reaction)

That’s some clean-looking code, I like it.

I don’t have any obvious architectural gripes at the moment, only nitpicks.

It’s odd that you store player inventories (later: actually, placed objects) in StringValues and have to use FindFirstChild to get to them:

local plrTable = workspace.Tables:FindFirstChild(("%sTable"):format(plr.Name))

Maybe you do want to announce other players’ lists of placed items (and inventories/data) to other players. But I imagine there would be less chances for unusual errors when you keep tables as tables instead of having to convert between JSON and tables and when you use, say, table.find on a table of player inventories instead of FindFirstChild.
It would be even better to set up some sort of player object/wrapper system and just use GetPlayerWrapper(player).Inventory or the like.
Tables is in Workspace - why not ReplicatedStorage? (If you put it in ServerStorage, then you’re 100% clearly better off storing them in a module instead of StringValues)

(in retrospect, player data is in a StringValue and you keep it updated at all times, so it’s definitely for announcing public data to other clients)
(I suggest being able to distinguish private data from public data, and only pushing public data to a publicly visible StringValue.)

Then you do this:

Remotes.UpdateInventoryUI:InvokeClient(plr, plrData)

If the player data is exposed like that, why not just listen to player.Data.Changed and remove that remote? A GUI for looking at someone else’s inventory live would have to do that anyway.

Why do you have a GetPlayerData(plr) function when you don’t use it everywhere?

function GetPlayerData(plr)
	return HttpService:JSONDecode(plr.Data.Value)
end

function AddItemToInventory(plr, itemName)
	local plrData = HttpService:JSONDecode(plr.Data.Value)
	-- [...]

Why is selling integrated to RemoveItemFromInventory when buying an item isn’t?

RemoveItemFromInventory() has a check for whether the item is in the inventory (or rather, matches the item name). Would that function ever be called for untrusted/unverified actions anyway?

	if IsItemPlaced(plr, itemName, itemId) then return end
	if plrTable.Placed:FindFirstChild(itemName) then return end

What does it take to be fully confident that an item hasn’t already been placed?

(I think) it’s possible that this stuff:

	dataHandler.Save(plr)
	dataHandler.SaveTable(plr)
	tableHandler.UnassignTable(plr)

could run twice if the server closed and the player left. I don’t know how well equipped these functions are for that.

	if not DoesPlayerHaveItem(plr, itemInfo.ItemName, itemInfo.ItemId) then return end
	if not itemConfig[itemInfo.ItemName] then return end

DoesPlayerHaveItem() would obviously fail if it weren’t a valid item according to itemConfig. Is it really possible for such an invalid item to be added to the inventory?

You ought to make a function to buy a resource, otherwise you might forget to change one or two of these strings/names.

function TryBuyResource(plr, type)
	if plr.leaderstats.Money.Value < itemConfig[type].Cost then return end
	
	plr.leaderstats.Money.Value -= itemConfig[type].Cost
	
	AddItemToInventory(plr, type)
	
	return true	
end

workspace.BuyWood.ClickDetector.MouseClick:Connect(function(plr)
	TryBuyResource(plr, "Wood")
end)

workspace.BuyIron.ClickDetector.MouseClick:Connect(function(plr)
	TryBuyResource(plr, "Iron")
end)

workspace.BuyGold.ClickDetector.MouseClick:Connect(function(plr)
	TryBuyResource(plr, "Gold")
end)

Not much else to say about the code.

It’s extremely sus for sure, but I didn’t immediately notice any obvious ways to dupe or exploit anything or to place someone else’s item or anything.
It might be secure, but it’s scary.

[TheBrainy06 (Brain)]

Hey, thank you for the detailed criticism on the code. I didn’t quite get what you meant by this line, would you mind elaborating?

@Eestlane771

It’s extremely sus for sure, but I didn’t immediately notice any obvious ways to dupe or exploit anything or to place someone else’s item or anything.
It might be secure, but it’s scary.

(The last bit)

[Downrest (Down)]

Is the code inside a local script or a server script?

[TheBrainy06 (Brain)]

This code is server sided as I mention in the post

@TheBrainy06

I was told that my backend code

[Eestlane771]

@TheBrainy06

I didn’t quite get what you meant by this line, would you mind elaborating?

One thing I meant is that, to borrow from a saying: it’s so complicated that there are no obvious flaws, when it should be so clear that there are obviously no flaws.

The other is that there are some very ominous double-checks.
Like your code is made of leaky pipes that have been sealed water-tight so that they don’t leak any more.
If you knew what each function should return and under what condition, then there would not have been stuff similar to this:

@Eestlane771

RemoveItemFromInventory() has a check for whether the item is in the inventory (or rather, matches the item name). Would that function ever be called for untrusted/unverified actions anyway?

	if IsItemPlaced(plr, itemName, itemId) then return end
	if plrTable.Placed:FindFirstChild(itemName) then return end

What does it take to be fully confident that an item hasn’t already been placed?

I felt like you didn’t understand the code you were writing.

Generally, critical code like this has comments that describe what the functions should accomplish, any subtleties/gotchas/failure modes of the function the next person who uses the function should know about etc… (This requires thinking about how it might go wrong)

And it’s not like each of these functions need any of that! It’s kinda good even without any comments (as it is now). Nothing seems to have any subtleties, the systems all look so simple.
But it’s not reassuring, kind of like how you don’t need a light to walk around in your own room if it’s dark, but you really could appreciate it so that you don’t step onto a misplaced pile of laundry and think you stepped on your cat or something, I don’t know, this analogy is far gone.

[TheBrainy06 (Brain)]

I see, thank you for the clarification!