TorqueGameEngines / Torque3D

Other
1.48k stars 161 forks source link

"Speed hack" potential in MoveManager #368

Open Valcle opened 3 years ago

Valcle commented 3 years ago

First, notice this line: https://github.com/TorqueGameEngines/Torque3D/blob/ac395775439985cc7332e777485d252055bdb042/Engine/source/T3D/gameBase/moveManager.cpp#L273

It writes 6 bits for the "py" field, so the value ends up being anywhere [0 to 64).

Now look at this line: https://github.com/TorqueGameEngines/Torque3D/blob/ac395775439985cc7332e777485d252055bdb042/Engine/source/T3D/gameBase/moveManager.cpp#L199

So, if the value is 63 here, the y field will be much higher than 1, and it will end up being passed to the respective ::processTick(...) eventually and cause 1. players to move much faster and 2. vehicles to accelerate much faster, maybe other things. This was the case in Blockland.

This doesn't happen i.e. when you do $mvForwardAction = 3 or something, because the range for these members on the move struct are clamped on the client side!

https://github.com/TorqueGameEngines/Torque3D/blob/ac395775439985cc7332e777485d252055bdb042/Engine/source/T3D/gameBase/moveManager.cpp#L230 and https://github.com/TorqueGameEngines/Torque3D/blob/ac395775439985cc7332e777485d252055bdb042/Engine/source/T3D/gameBase/moveManager.cpp#L176

For a fix, changing the bits read to 5 probably isn't the right solution, since ideally the range should be [0 to 32], not [0 to 32). So, throw in a call to ::clamp(...) right after the move is unclamped on the server end, or simply write in some if-thens to limit the range of x/y/z after unclamping (or px/py/pz before).

(note I haven't tested if this bug applies to this version of the engine, I'm simply reporting from my experience with Blockland which is a TGE 1.5.1 game)

chaigler commented 3 years ago

Interesting!

Clamping the values on the server seems like an easy solution but I'm wondering if this entire bit of MoveManger code is still sensible. It's jumping through hoops (and introducing slight inaccuracies to the move state) just to shave a few bits off each packet. That definitely made sense in 2001 but how about now?

On the other end, this old post makes it sound like we could go even further with the optimization and cut another 3 bits/packet with some more clever float packing.

WageCrusader commented 2 years ago

This does not seem to affect Torque3D MIT, please list some steps that could be taken to reproduce this behavior.