Bug. Hack with inline actions in different contracts.

[qpIlIpp]

Let's image that I've a task to make a transaction with an actual number of actions 2. For example, first is money transfer for the second transaction. (in the second transaction I'll use user's RAM, so events are not a point) In my smart-contract I need to check transaction count to prevent doubling the second transaction, I can do it only in one way:

transaction t = read_transaction(); // skipped params and cast buffer to transaction type
auto CurSize = t.actions.size() + t.context_free_actions.size(); // do I need context_free_actions ?

According to https://github.com/EOSIO/eos/issues/6080 , I'm not able to get inline actions count because I'm managing them during my smart contract.

Now let's remember, that first transaction is eosio.token::transfer, which will notify second account - payer. The payer is a hacker, he knows, that we're using 2 actions in our transaction. The second action uses payer permission and he just repeats them in his simple smart-contract :

extern "C"
{
    void apply (uint64_t receiver,
        uint64_t code, uint64_t action)
    {
        if (code == N(eosio.token) &&
            action == N(transfer)
/* && transfer.to == TARGETACC */
)
        {
            action (permission_level (HACKER,
                N (active)),
                TARGETACC, ACTTOREPEAT, params).send ();
        }
    }
}

So we cannot inspect inline actions and we can send them from another account (just need usage of require_recipient() method). It brokes composite transaction designing. We should or 1 )know something about current transaction inline_actions or 2) don't allow cross-contract actions

Only one way to avoid it now - use tables of payment, but it's complex way and needs a lot of CPU and RAM on contract with a lot of users.

If needed, I can write full example (2 contracts) of that bug.

The main part of the bug was found by @GassaFM

[qpIlIpp]

https://github.com/EOSIO/eos/issues/2386 here "We are not going to enable reading inline actions (context for or otherwise) for the time being." @taokayan @larryk85

[taokayan]

I think you need the action_trace for inspection. Please ask in eosio.stackexchange.com if you have further questions.

[qpIlIpp]

Do you want to say, that I am able to action_trace inside smart-contract? If so, I'll go to stackexchange. Otherwise, tell me, why is it a 'SUPPORT', if I have shown the bug exactly.

[tbfleming]

This is not a complete example of an exploit. The hacker’s contract doesn’t have the necessary authority to duplicate the victim’s action, which requires the victim’s authority.

[qpIlIpp]

@tbfleming , victim's account require hacker authority. In the second action, "victim" uses "hacker" RAM. So hacker is able to send that action with his authority.

[GassaFM]

@tbfleming:

The legitimate use of the victim contract is to make a transaction of two actions:

1. eosio.token -> transfer from user to victim.
2. victim -> getgoods.

Both are authorized by user.

The exploit is to put the listener contract on hacker account. Then the hacker authorizes a legitimate transaction:

1. eosio.token -> transfer from hacker to victim.
2. victim -> getgoods.

The authorization is the usual hacker@active one. The listener receives a notification about eosio.token -> transfer, and issues another victim -> getgoods action, by the hacker@eosio.code authority.

[GassaFM]

It just occurred to me that the victim contract may distinguish between the good and the bad action by looking at the authority: as I see it, the bad victim -> getgoods action requires hacker@eosio.code authority, it does not seem to be possible to issue it with hacker@active authority.

[GassaFM]

Alternatively, the victim contract may listen for eosio.token -> transfer incoming actions, and maintain a variable like amountPaid. Then the victim -> getgoods action can use that variable, reduce it by the price of the goods, and check that the result is nonnegative.

[tbfleming]

All contracts should use require_auth when there’s a potential for abuse. Also maintaining a balance per user to track incoming transfers to be spent later is recommended.

[opcheese]

@tbfleming That's the trick of the attack - for all intents and purposes second, third and all subsequent calls to getgoods are identical to the first legitimate one. As it stands now without any resource-consuming (both RAM and CPU) and special measures there is no way for the victim to distinguish illegitimate calls from legitimate ones.

1. Both require_auth and require_auth2 are not enough, because the hacker can use hacker@active authority in legitimate and illegitimate calls (@GassaFM that actually was my first idea too, however, the hacker can easily circumvent the check by giving his contract hacker@active permission).
2. We cannot know the number of actions inside the transaction, because of #2386. There is no way to know the number of inline actions of the current transaction.
3. We cannot know if we are inside inline action or regular action.

So our only option is to create "temporary" values to track our progress through actions inside the transaction. Since we have no tools to observe transaction lifecycle and there is no way to create transient values (which exist throughout the life of 1 transaction) we need to store these "temporary" values in the state. So every time any user wants to use the smartcontract either they have to pay to store these values or we have to pay for him. Both are bad and hurt usability and scalability of our DApp.

2386 or any kind of special property to analyze actions (for example distinguish original actions and inline actions) or any way to find out where we are in a transaction and how we got here (akin to stacktrace) would solve this issue.

Looking at a large picture what we want is more "reflective" functionality for smartcontracts. I do agree that using context is often an antipattern in programming in general. But we have repeatedly come across issues where additional info about the state of the blockchain, state of the transaction, "deferredness", and other info about the context of the current action would have been immensely helpful.

[tbfleming]

for all intents and purposes second, third and all subsequent calls to getgoods are identical to the first legitimate one

That's a fault in getgoods. If getgoods can't distinguish between legitimate and illegitimate actions, then it needs a redesign. Contracts which support sales need to track per-user deposits and subtract from those deposits when making sales.

[tbfleming]

For an example of this, see https://gist.github.com/tbfleming/d230f3ab2998e8858d3e51af7e4d9aeb . It's old code which needs an update to support changes from the past several months, but it should give you an idea of how sales contracts work.

[opcheese]

Thank you for your example! It is probably the cleanest code for what we call an "event pattern". I will actually borrow it for our wiki. This pattern has 2 disadvantages:

1. You can't show Ricardian contract for service you are providing (which you charge for)
2. It is error prone in complex scenarios when the contract has to handle a lot of different types of purchases (partially demonstrated by https://www.reddit.com/r/eos/comments/9fxyd4/eosbet_transfer_hack_statement/)

I partially agree with you, the fault is with getgoods. It should be able to prevent abuse. But our scenario is more complex than dealing with balances. A better name for the action would be provideservice. Our client can get the service provided any number of times. But he should pay every time. And to accept payment we would like to use, what we call "compound transaction pattern" (have a transaction with 2 actions transfer and provideservice). The issue is that we cannot, because of the attack described by @qpIlIpp . So we have 2 options:

1. Introduce some transient balance-like table which we do not need and do not want our customer to pay for.

2. Get info about inline actions which are a part of the current transaction (or other options described by @qpIlIpp )

   Currently, we are implementing option 1. However we would really prefer option 2. Hence the existence of this issue.

[tbfleming]

The eosbet hack happened because they failed to include all the appropriate assertions. They copied code from someone who was unaware of all the necessary checks (the _EX macro author). What they needed is no more complex than my example code.

No one will pay for ram when transactions have the following 2 actions:

1. transfer funds to contract
2. call an action on the contract which uses all of the funds

In this case, the row will be created then removed within the same transaction. eosio doesn't charge for ram when that happens.

[taokayan]

@tbfleming:

The legitimate use of the victim contract is to make a transaction of two actions:

1. eosio.token -> transfer from user to victim.
2. victim -> getgoods.

Both are authorized by user.

The exploit is to put the listener contract on hacker account. Then the hacker authorizes a legitimate transaction:

1. eosio.token -> transfer from hacker to victim.
2. victim -> getgoods.

The authorization is the usual hacker@active one. The listener receives a notification about eosio.token -> transfer, and issues another victim -> getgoods action, by the hacker@eosio.code authority.

From what you described it looks like there's no require_auth check in victim -> getgoods action. If the victim contract has proper assertions, it is not possible to satisfy victim@active by hacker@eosio.code.

Therefore this is classified as "support" rather than a "bug". Please ask in eosio.stackexchange.com if you have further questions.

[GassaFM]

@taokayan

From what you described it looks like there's no require_auth check in victim -> getgoods action. If the victim contract has proper assertions, it is not possible to satisfy victim@active by hacker@eosio.code.

No, that's a wrong interpretation. The legitimate authority for victim -> getgoods is user@active, not victim@active. See, it's called by a legitimate user in a legitimate transaction. One of the things it does is to charge user for the RAM, so it can't be done without user authority.