CASE STUDY
Reviewer: Kelly Nwanji
How It Started
When I and my team was handed project, the brief was very much clear, all I had to do was basically to dig into the C# scripts from the dev teams repo and then go through them accordingly in other to fish out any the weak spots and actually fix what we could. This was not a theoretical exercise at all, it was more of a practical exercise, where i:
• Read real source code (in your case, the C# scripts from the Naija Kitchen Chaos repo)
• Use tools on a real environment (my Kali Linux setup terminal, Semgrep, SonarLint, gitleaks, Unity Play Mode)
• Confirm issues by tracing logic
• Rank severity by actual impact (crash on launch, missing consent, silent wrong rebind and many more)
• Recommend or apply real fixes in the code
Findings
The reviews turned up a mix of few crash paths, logic bugs, privacy gaps and a few robustness issues that might eventually be an issue later on.
Input bindings loaded with no safety net
GameInput pulled a JSON string straight out of PlayerPrefs and fed it into LoadBindingOverridesFromJson() with zero try/catch. PlayerPrefs is just a registry key on Windows or a plist on Mac,nothing stops it from being malformed after a game update or some curious tinkering. One bad value and the game crashed hard on launch from Awake().
Priority: Medium
Volume values left unclamped
MusicManager and SoundManager both read volume with PlayerPrefs.GetFloat and used the number as-is. Unity clamps AudioSource.volume internally, so it was not breaking audio today but SoundManager passed that value into PlaySound() as a multiplier with no clamp of its own. A tiny defensive fix would make the code less dependent on Unity catching it downstream.
Priority: Low
InteractAlternate button rebinding the wrong key
OptionsUI had a button wired to rebind GameInput.Binding.InteractAlternate. Inside GameInput.cs the InteractAlternate cases in GetBindingText() and RebindBinding() were commented out, so the enum fell through to the default case — Move_Up. Click the button, and you silently rebind the move up key instead of the alternate interact, Also the UI looked correct but the behaviour was not.
Priority: Low
Singleton access with no null checks
OptionsUI and GamePauseUI reached for GameManager.Instance, GameInput.Instance, and MusicManager.Instance in Start() without null guards. Unity does not guarantee Awake order across scripts unless you set Script Execution Order yourself.
Priority: Low
Earlier batch findings that shaped the later work
In the first deep pass on GameManager and related scripts I also flagged a shared timer variable that meant the gameplay timer never actually ran out, empty reset methods on BaseCounter / CuttingCounter / TrashCounter (commented out logic left in place), singleton Instance setters with no duplicate check and the use of the new keyword instead of override on counter reset methods. Those sat at Low priority but were important foundation issues before win/lose logic got stacked on top.
Later, Batch 5 on GameManager, Analytics, CharGrillStoveCounter, CuttingRecipeSO and Player brought a different flavour of findings: Debug.Log spam every frame (information disclosure), GameAnalyticsSDK initialising with no consent gate (Medium), missing null guards on parent and prefab references, a Player singleton that logged a duplicate but still overwrote itself, and an unvalidated cuttingProgressMax that could be set to zero or negative by a designer.
Threat Models
The project already had a full STRIDE threat model on file (NaijaKitchenChaos_ThreatModel.docx) plus a set of security requirements labelled SR-01 through SR-15. For each batch i single handedly scoped the relevant threat categories rather than just rerunning the entire model from scratch.
For the input, options, and audio scripts the main concerns mapped to:
• Tampering / Data Integrity: unvalidated PlayerPrefs data (bindings and volume) trusted on load.
• Denial of Service: a malformed binding JSON could crash the client on launch.
• Elevation of Privilege (light): not classic privilege escalation, but a rebind UI that silently changed a different action than advertised.
For Batch 5 the scoped categories were:
• Information Disclosure: debug logging and analytics telemetry that could leak gameplay state or device data (RISK-20, RISK-21).
• Denial of Service: missing null/reference checks that let a bad asset reference crash the client (RISK-22).
• Tampering / Data Integrity: unvalidated ScriptableObject fields and a singleton that could be silently overwritten (RISK-23, RISK-24).
These mapped cleanly onto the project’s own requirements: SR-06 (input and data validation), SR-09 (logging hygiene) and SR-12 (third-party SDK data handling), keeping the threat model tight to the batch meant that my findings stayed actionable instead of turning into a random made up generic security essay.
Methodology and Tools Used
While carrying out the security review, i did not make use of a single scanner and then call it a job well done. The most of the interesting issues were control flow and few design problems that static tools only hint at, the whole process I took looked like this.
• Manual line by line code review: this is basically the primary method where logic and control flow bugs rarely light up a scanner on their own.
• SonarLint (C# static analysis): I used the sonarlint which flagged few dead code, unreachable branches and incomplete switch coverage, That was how the InteractAlternate fall through stood out.
• Semgrep: The semgrep was one of main tools that was used by me, semgrep is basically a tool used go through code in case of any set of security vulnerabilities. This was how I used the semgrep tool, firstly i had to update my working environment, after I was done I went ahead to install the semgrep tool into my kali, once that was done, I went onto creating directories for each codes so I can use semgrep to scan through those directories created on my kali linux terminal.
• Gitleaks: scanned repo history for hardcoded secrets or keys, which eventually came out clean.
• Unity Profiler / Play Mode testing: used to trigger rebind and volume flows by hand and confirm that the InteractAlternate and null guard issues actually reproduced, not just looked bad on paper.
Checklist and framework
Reviews were checked against a shortlist adapted from OWASP Secure Coding Practices and the projects own SR-01–SR-15 requirements. The focus points were:
Get Nwanji kelly’s stories in your inbox
Join Medium for free to get updates from this writer.
1. Null and reference safety on cross object calls (parent/child ownership, ScriptableObject references, singleton Instance access).
2. Information disclosure through logging and third party SDKs.
3. Singleton and lifecycle integrity (Awake/Start ordering, duplicate instances).
4. Data validation on serialized and designer facing fields and on anything loaded from PlayerPrefs.
For the first GameManager focused pass the steps were concrete: clone the repo onto Kali, run Semgrep, triage the output by hand, prioritise the central scripts (GameManager and MainMenu sit at the centre of game state and session entry), and then cross check every flagged spot by reading the actual logic before writing it up. Later the batches followed the same spirit read the code, confirm the behaviour, document the fix path.
Severity Ranking of Findings
Severity was ranked by two practical questions, firstly how much control an attacker or a bad build config would have over the trigger and secondly what actually breaks or leaks if it fires.
Severity Count Basis for ranking
Medium
2 Real crash path on launch (unvalidated binding load) and real user-data / privacy exposure (analytics collection with no consent gate). Compliance-relevant even in a student project.
Low
8+ Crashes, log-based information leakage, state-integrity bugs, or logic errors reachable through normal play but that do not expose data or break core game rules on their own.
Informational
3 Code-health and robustness issues (missing integrity checks on settings, incomplete stubs, unvalidated designer fields) that are not exploitable as-is but should stay on the team's radar.
Remediation Steps Actually Taken
This is the part that well explains the steps taken to insure everything went smoothly, In the Batch 5 i applied fixes directly to the reviewed files rather than just leaving everything as open recommendations, my earlier findings were tracked with a live status table so my team could see progress instead of a static list.
Fixes applied in code (Batch 5)
• RISK-20 (Player.cs): Removed the three Debug.Log calls that fired on every interact press and every frame.
• RISK-21 (Analytics.cs): Gated GameAnalytics.Initialize() behind a PlayerPrefs consent flag, defaulting to no data collection until the player opts in via GrantAnalyticsConsent().
• RISK-22 (KitchenObject.cs): Added null checks, SetKitchenObjectParent() now rejects a null parent; SpawnKitchenObject() checks the KitchenObjectSO and its prefab before instantiating.
• RISK-23 (Player.cs): Added a return in Player.Awake() so a duplicate instance no longer overwrites the existing singleton after the error is logged.
• RISK-24 (CuttingRecipeSO.cs): Added an OnValidate() that clamps cuttingProgressMax to a minimum of 1 and warns in the editor if it was set lower.
• RISK-25 (CharGrillStoveCounter.cs): this was entirely left as an intentional empty stub and logged for a follow up review once grill interaction logic lands. That is basically a scope gap and not a vulnerability.
Recommended and tracked fixes (input / options / audio pass)
For the PlayerPrefs and UI findings the recommended path was clear:
1. Wrap the binding load in try/catch and fall back to default bindings instead of letting the exception escape Awake().
2. Clamp volume to 0–1 with Mathf.Clamp01 right after reading from PlayerPrefs in both managers.
3. Either finish wiring InteractAlternate properly or remove the button from OptionsUI until it exists.
4. Set explicit Script Execution Order for the manager singletons or add null checks with a short retry in Start() for the UI scripts that depend on them.
A remediation status table was kept current so “Not started” could turn into “In progress” or “Fixed” as the dev team picked items up. The Medium binding load issue was flagged as the one worth tackling first because it was a real crash path, not a theoretical worry.
Residual risk and follow ups
• The analytics consent flag defaults to off and still needs an options menu toggle wired to GrantAnalyticsConsent() before the flow is complete.
• CharGrillStoveCounter has no cooking or burning state machine yet, a follow up review is recommended once that logic exists.
• No automated tests were added alongside the Batch 5 fixes, a quick manual playtest on interact, cutting and grill flows remains the practical next step before merge.
SCREENSHOTS OF WORK CARRIED OUT
