Projected logic issue: The clone war

[jpcsupplies]

Although we record the UID; i have mostly focused on the player nickname (which can be changed in steam) potentially this means if someone deliberately changes their nickname; they can transfer all of another players balance to themself; or the wrong person can be accidentally paid.

Our database checks can however compare both nick and uid.. so hopefully this prevents the scenario of being able to spend someone elses money from occuring. The receiving player however may be a potential security weakness. If another player has the same nickname higher up in our database than the desired person we want to pay - then they may end up getting the payment.

Not sure how best to deal with this - we can add an extra check to search for duplicate nicknames, but then the only way to tell them apart is their UID, which most players generally wont know.

Just need to be mindful of the possibility, and not focus too much on nicks in our code (which i did at one section in the pay command) ie https://gist.github.com/jpcsupplies/1f998a573b8f743d17d6

Although i realize the weakness, i cant see any better way to go about it, except perhaps restricting the pay command to only work with online players - which adds the difficulty of testing our code offline :)

[jpcsupplies]

Another potential issue with a player changing their nickname - two bank entries for the same player with different nicks and the same UID..

[midspace]

this is why the UpdateLastSeen(...) will also update the nickname of the player. https://github.com/jpcsupplies/Economy_mod/blob/master/Economy/Data/Scripts/Economy.scripts/Config/BankConfig.cs

It's called by the MessageConnectionRequest as soon as a player connects to a server, making sure their current NickName is up to date in the records. https://github.com/jpcsupplies/Economy_mod/blob/master/Economy/Data/Scripts/Economy.scripts/Messages/MessageConnectionRequest.cs

There is always the potential that MessageConnectionRequest will not invoke when a player connects. This is part of the trouble we had with MOTD sometimes not showing in the Admin mod. We fixed that by adding a Timer to resend the MessageConnectionRequest 10 seconds after the mod initialized to make sure the server received the message.

[midspace]

I'll have to look at putting the same Timer logic in to resend the MessageConnectionRequest.

What I'm also concerned about, is the potential of having two accounts created for the same user. There is a risk, though small, that two simultaneous calls can come through to access the account for a particular user before the account has been created. It may actually create two accounts. The risk is mitigated slightly, in the fact the Accounts is a List<BankAccountStruct>. List will guarantee the order of the items unless you purposely rearrange them by removing and adding them back in. As such, calls to Accounts.FirstOrDefault(...) will return the first account, and never the second account. So even if there are two, we'll continue to only see the first account.

The fix for that would be merging the two accounts, and subtracting extra the StartBalance that the second account would have gotten.

Accounts.Remove(account2); // Remove it first, to prevent any access from occurring whilst this operation runs.
account1.BankBalance += account2.BankBalance;
account1.BankBalance -= EconomyConsts.DefaultStartingBalance;

About the only question is, where to run this. I'm guessing UpdateLastSeen(...) could actually run this check and fix.

[jpcsupplies]

ahh good idea.. so seen is becoming more and more handy when it started as just a footnote lol

I noticed when i was testing my /reset command at one point i spawned two entries for myself (as i had commented out a section) to fix it i just commented out anther section so that reset only ran the remove part skipping the add part until my balance was fixed lol

[jpcsupplies]

potential fix for OP issue - when we implement range/distance checks we could use that on the pay command too.. purely as a discriminator.. If we check for duplicate user names to pay to, if we find more than one, only allow the player to pay the one that is physically nearest to them.. (say within 50, or in sight) if there is no duplicates then the current "wire transfer" unlimited range pay behavior continues. As such the security check would be totally invisible to players, unless the situation arises that we have two players with the same name, at which point it will tell the player to get closer to the desired recipient?

eg.. PAY: which XXclone do you want to pay? Please get closer to them.

Sound good?

The only downside to this is trying to pay offline players - then a range check wont work - a less friendly second level of security here then is to use the presumption they player they want to play is on their steam friend list - and request than they use (paste from their friend list profile page) that players steam ID to complete the transaction.

eg. PAY: too many XXclone named players, please use steam ID /pay pastesteamIDhere 123 reason

steamid should be a trivial change in the search part of messagepayuser

of course then we have the potential exploit that a player using the nickname matching the steam ID of the desired player... sigh ok maybe /pay XXclone 123 pastesteamIDhere_instead_of_reason??

thoughts?

[jpcsupplies]

So far no sign of people doing this on purpose to scam money, and the steam ID stuff seems pretty solid. I am going to close this one for now. If we get players reporting issues with copied player names.. maybe think about dealing with it then, but in the real world people steal identities anyway.. going to toss this into the realism basket and close it.