Page MenuHomeWildfire Games

Prevent a Barter exploit.
ClosedPublic

Authored by fatherbushido on Apr 24 2017, 3:16 PM.

Details

Reviewers
elexis
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP19831: Limit the bartered amount in the simulation. Advices from mimo. Reviewed by…
Summary

Reproduce:
edit L34 in session/menu.js

const g_BarterResourceSellQuantity = 100;

and set it to 49 for example.
Then the prices won't decrease when bartering.

Test Plan

See the bug catch test.

Diff Detail

Event Timeline

fatherbushido created this revision.Apr 24 2017, 3:16 PM
fatherbushido edited the summary of this revision. (Show Details)
elexis accepted this revision.Apr 24 2017, 3:53 PM
elexis added a subscriber: elexis.

Nice find. The patch is correct and complete (can't find similar exploits).

I tried the following line when pressing F9 and typing for (let i=0; i<500; ++i) Engine.PostNetworkCommand("type": "barter", "sell": "food", "buy": "metal", "amount": 1); to buy 500 metal for 500 food without dropping food prices

Also tried selling 0 amounts or negative amounts, but we have early returns in that function, so it's ok.

Things I noticed out of scope of this diff:

  • After applying the patch and selling 1 food, we get 0 metal. Seems odd to pay something without getting anything.
  • The Barter chat notification says "User bought for 1 wood", so the message should not be sent in the first place (probably same issue as above)
  • If we ever support buying resources in amounts different of 100 in the GUI, we should probably change that then (so that the player doesn't get a bad deal if its't not a multiple of 100). Perhaps a warning could be added if the amount isnt a multiple of 100.
binaries/data/mods/public/simulation/components/Barter.js
122–123

Overall if you want

binaries/data/mods/public/simulation/components/tests/test_Barter.js
111

capped

This revision is now accepted and ready to land.Apr 24 2017, 3:53 PM
Vulcan added a subscriber: Vulcan.Apr 24 2017, 4:43 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/887/ for more details.

Adress elexis remarks.

Remove the edited gui value.

elexis accepted this revision.Apr 24 2017, 5:22 PM

Thanks for fixing those typos an the cleanup too.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/888/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/889/ for more details.

fatherbushido planned changes to this revision.EditedApr 24 2017, 7:20 PM

Defering until having better ideas (after discussion with elexis)

fatherbushido changed the visibility from "Public (No Login Required)" to "Restricted Project (Project)".Apr 26 2017, 1:36 PM

Update. Early return in the sim if someone modify its gui code. Don't add warn to handle unit test. Perhaps we could do a similar check in commands.txt (and warn at that place?).

This revision is now accepted and ready to land.Jun 5 2017, 3:33 PM
fatherbushido requested review of this revision.Jun 7 2017, 4:32 PM
fatherbushido edited edge metadata.

(As asked by elexis, ai bartering is done in petra/traderManager by 100 units.)

mimo added a subscriber: mimo.Jun 7 2017, 8:16 PM
mimo added inline comments.
binaries/data/mods/public/simulation/components/Barter.js
100–101

Wouldn't it be better to only remove the Math.round here? so that the formula will always work without such arbitrary limitations of amount%100=0.
and if doing that, move that definition of numberOfDeals where it is needed (i.e. just before l124)

fatherbushido added inline comments.Jun 7 2017, 8:39 PM
binaries/data/mods/public/simulation/components/Barter.js
100–101

(In fact with the check above I should have remove but that's not what you point out).

My first patch just changed that Math.round to a Math.ceil

But we still have some side effects. Indeed it allowed to have a really finest subdivision wich doesn't really fit to the current code. For example, with an amount of 1, we will get 1 ress (even if the price will fall, we won't get a 0 until some barters). So basically we will get almost the same thing for 100 barter of 1 than for 1 barter of 100 (but there will be some difference).
My last feeling was really that the code fits only for amount multiple of 100. Indeed, resources are integers and what we will add is the amount multiplied by that prices ratio. So to have an effect of that ratio on the prices added we need at least to have multiples of 100 (10 could be ok but less sensible, at least for the first barter).

Well I have no real idea of what is the best and your input is really welcome.

(In both cases moving it is to do)

mimo added inline comments.Jun 7 2017, 9:18 PM
binaries/data/mods/public/simulation/components/Barter.js
100–101

From what you describe, i've the impression the limitation could be a minimum (i.e. amount > 50 or whatever seems reasonnable for gameplay) if we remove the rounding (without using a ceil).

And also, whatever the limitations, a comment could be added in menu.js as otherwise a modder can have the impression he can change g_BarterResourceSellQuantity freely.

fatherbushido planned changes to this revision.Jun 7 2017, 9:24 PM
mimo added a comment.Jun 8 2017, 7:07 PM

In fact, thinking a bit to it, the exploit described in the ticket is just a poor man exploit :) the price does not increase, but it also does not increase for other players.
We can have a worse exploit by using a very big quantity (and multiple of 100 such that it is still allowed by the patch), then you can buy this huge quantity at a fixed price, and let the price increases a lot for other players. I've also the impression that if you are rich enough (i.e. putting it to 2000 or more) you can speculate and enrich yourself by buying huge quantities in one shot (so at a fixed price) and selling them immediately as the price has increased.

IMO this quantity should not be a parameter in the gui, but be fixed by the simulation as it influences the simulation: could be an additional property in each json resource, with a null value indicating a resource not barterable as this was required some time ago in the forum. (The same for the factor 5 when Shift to avoid such exploit).

But these may be too big changes for A22: we may have a temporary fix for it, just allowing 100 and 500, and defer a complete fix to A23.

In D381#25296, @mimo wrote:

In fact, thinking a bit to it, the exploit described in the ticket is just a poor man exploit :) the price does not increase, but it also does not increase for other players.
We can have a worse exploit by using a very big quantity (and multiple of 100 such that it is still allowed by the patch), then you can buy this huge quantity at a fixed price, and let the price increases a lot for other players. I've also the impression that if you are rich enough (i.e. putting it to 2000 or more) you can speculate and enrich yourself by buying huge quantities in one shot (so at a fixed price) and selling them immediately as the price has increased.

Yes :)

IMO this quantity should not be a parameter in the gui, but be fixed by the simulation as it influences the simulation: could be an additional property in each json resource, with a null value indicating a resource not barterable as this was required some time ago in the forum. (The same for the factor 5 when Shift to avoid such exploit).

Yes! I think of that too but I didn't think to use json file.

But these may be too big changes for A22: we may have a temporary fix for it, just allowing 100 and 500, and defer a complete fix to A23.

ok, thanks for your help!

Just forbid values different from 100 or 500 in the sim.

elexis accepted this revision.Jun 26 2017, 1:14 PM

Patch works as expected.
A comment for the first of those three hardcoded globals (g_BarterResourceSellQuantity, g_BarterMultiplier, g_BarterActions) in menu.js stating that these are simulation constants would be helpful for modders.
This thing should be fixed for multiplayer in the upcoming release IMO.
Any further code to add a warning that is muted in the tests or support of unconventional values can be done separately IMO. (could be simple ticket material if we lay out what to implement)

This revision is now accepted and ready to land.Jun 26 2017, 1:14 PM
fatherbushido added a comment.EditedJun 26 2017, 1:20 PM

A comment for the first of those three hardcoded globals (g_BarterResourceSellQuantity, g_BarterMultiplier, g_BarterActions) in menu.js stating that these are simulation constants would be helpful for modders.

Ah yes I forgot the menu.js comment

Add a comment in menu.js

This revision was automatically updated to reflect the committed changes.
elexis changed the visibility from "Restricted Project (Project)" to "Public (No Login Required)".Jun 26 2017, 2:21 PM

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1630/
See console output for more information: http://jw:8080/job/phabricator/1630/console