oscar-broman / samp-weapon-config

A more consistent and responsive damage system with many new features
Apache License 2.0
91 stars 84 forks source link

Fix for Knife "exploit" #239

Closed NexiusTailer closed 2 years ago

NexiusTailer commented 2 years ago

This fixes issue #238, adding damage validation for distance in knife-specific check which is applied before (and instead of) main ProcessDamage function.

The reason I didn't move ProcessDamage and other validation checks above knife check (so it could validate knife cases too) is in that any knife damage was never considered to be checked in ProcessDamage function and there needs to be a lot of additional exceptions for knife in many places, like here (where the function won't process any knife damage just because it doesn't consider it at all). Another cause to have this kind of fix with another duplication of s_WeaponRange check inside knife check instead of validation damage distance in ProcessDamage is in that we need to resync "knifed" player at least for issuerid (instead of just blocking the damage like ProcessDamage does), assuming that the distance exceeding may be not only due to deliberate spoofing, but also due to sync bugs.

P.s. Merged another PR while it was forked, so that you can see some extra commits (squash on merge if it doesn't look good).

ADRFranklin commented 2 years ago

How much testing have you done for this PR? I agree to everything you said above. Just need to make sure there are no regressions or unwanted side effects.

NexiusTailer commented 2 years ago

Checked it before the pull request and again about 3-4 days ago. Both times on the test server, but under different conditions (whether any valid actions work correctly and any spoofed does not work). However, I can't test it on the production server, as it uses a modified wc where some SKY-dependent parts were cut, including the knife kill validation system. Actually that is why I didn't notice such issue in this system before.

ADRFranklin commented 2 years ago

Alright, well I'll merge it and if any serious issues I will revert it. Thanks for the commit though, I appreciate the continued support.

NexiusTailer commented 2 years ago

So, #238 can be closed, I suppose