Help to fix bug on Store in Game (Injection of points using wpe)

[_M4G0_]

A few days i have alerting, asking for help to fix this bug, but no one has helped until now. I hope someone can help

change quality

 

[andu]

It's not a bug.

Minute 1:20, you started to execute +25 points every 100ms for infinite (continously), instead of only once.
So after 1h your one char will have 900k points and second one -89975 (tibia will show 0).

Your bad.

 

[_M4G0_]

It's not a bug.

Minute 1:20, you started to execute +25 points every 100ms for infinite (continously), instead of only once.
So after 1h your one char will have 900k points and second one -89975 (tibia will show 0).

Your bad.

if you say

 

[Znote]

What sources are you using?
If this goes through the server, it does not seem to be verifying a players balance before transferring the points.

What happens when you relog?

 

[_M4G0_]

What sources are you using?
If this goes through the server, it does not seem to be verifying a players balance before transferring the points.

What happens when you relog?

otx

If relog char you are transferring, will be with 0 points, and will continue sending points even with 0 after the action recorded in wpe.

i have tested on tfs in this package the bug works
https://otland.net/threads/10-95-cast-tfs-1-2-custom-modifications-v2.244282/

as the @MatheusMkalo said need a function, but i dont know how to insert
https://github.com/mattyx14/otxserv.../modules/scripts/gamestore/init.lua#L116-L141

i think i solved by inserting the line

if not player:canRemoveCoins(amount) then
    return addPlayerEvent(sendStoreError, 250, player, GameStore.StoreErrors.STORE_ERROR_NETWORK, "We couldn't remove coins from your account, try again later.")
    end

function parseTransferCoins(player, msg)
    local reciver = msg:getString()
    local amount = msg:getU32()
   
    if reciver:lower() == player:getName():lower() then
        return addPlayerEvent(sendStoreError, 350, player, GameStore.StoreErrors.STORE_ERROR_TRANSFER, "You can't transfer coins to yourself.")
    end
   
    local resultId = db.storeQuery("SELECT `account_id` FROM `players` WHERE `name` = " .. db.escapeString(reciver:lower()) .. "")
    if not resultId then
        return addPlayerEvent(sendStoreError, 350, player, GameStore.StoreErrors.STORE_ERROR_TRANSFER, "We couldn't find that player.")
    end
   
    local accountId = result.getDataInt(resultId, "account_id")
    if accountId == player:getAccountId() then
        return addPlayerEvent(sendStoreError, 350, player, GameStore.StoreErrors.STORE_ERROR_TRANSFER, "You cannot transfer coin to a character in the same account.")
    end
        if not player:canRemoveCoins(amount) then
    return addPlayerEvent(sendStoreError, 250, player, GameStore.StoreErrors.STORE_ERROR_NETWORK, "We couldn't remove coins from your account, try again later.")
    end
    db.asyncQuery("UPDATE `accounts` SET `coins` = `coins` + " .. amount .. " WHERE `id` = " .. accountId)
    player:removeCoinsBalance(amount)
    addPlayerEvent(sendStorePurchaseSuccessful, 550, player, "You have transfered " .. amount .. " coins to " .. reciver .. " successfully")
   
    -- Adding history for both reciver/sender
    GameStore.insertHistory(accountId, GameStore.HistoryTypes.HISTORY_TYPE_NONE, player:getName() .. " transfered you this amount.", amount)
    GameStore.insertHistory(player:getAccountId(), GameStore.HistoryTypes.HISTORY_TYPE_NONE, "You transfered this amount to " .. reciver, -1 * amount) -- negative

end

 

[Znote]

@_M4G0_
In function parseTransferCoins(player, msg)

Before:

db.asyncQuery("UPDATE `accounts` SET `coins` = `coins` + " .. amount .. " WHERE `id` = " .. accountId)

Add:

if player:canRemoveCoins(amount) == false then
    return addPlayerEvent(sendStoreError, 350, player, GameStore.StoreErrors.STORE_ERROR_TRANSFER, "You don't have enough funds to transfer these coins.")
end

Edit: Haha, yeah I saw this is basically what you did. Clicked the github link and began coding without reading the rest of your post. xD

Also change the line of:

player:removeCoinsBalance(amount)

From the line AFTER the asyncQuery to the line BEFORE the asyncQuery. Ensuring that the coins is removed from the player before added to the other player, to avoid potential simultanious calls.

It's not a bug.

Minute 1:20, you started to execute +25 points every 100ms for infinite (continously), instead of only once.
So after 1h your one char will have 900k points and second one -89975 (tibia will show 0).

Your bad.

It seems to be a bug, the Lua function that adds the coins does not verify that the user can send the coins before doing so. He is not bad. You on the other hand seems to be some sort of script for profit guy, who failed to realize what was happening. Ignorance may be bliss, but try not to be rude about it.

 

[andu]

It seems to be a bug, the Lua function that adds the coins does not verify that the user can send the coins before doing so. He is not bad. You on the other hand seems to be some sort of script for profit guy, who failed to realize what was happening. Ignorance may be bliss, but try not to be rude about it.

Checked it again. Hmm. It looks like player can exploit client to transfer points for infinity and there's no server side code what will verify it.
Does this hole can be used to send more malicious data to server?

 

[_M4G0_]

Thanks @Znote for suporte

is possible fix one post or this post?

 

[Znote]

Checked it again. Hmm. It looks like player can exploit client to transfer points for infinity and there's no server side code what will verify it.
Does this hole can be used to send more malicious data to server?

Yes (especially another variation), any functions that is directly linked up to a network packet must be properly evaluated, even if it was evaluated in an earlier packet.

@_M4G0_ Your changes seems fine, and the otx server has been updated now, so you can also grab latest source code there and recompile it. Would actually be great if you could do that and test it out with wpe.