wavesplatform / Waves

⛓️ Reference Waves Blockchain Node (client) implementation on Scala
https://wavesplatform.com/
MIT License
1.17k stars 417 forks source link

[Discussion] Free InvokeScript transactions for user #2725

Closed pivoo81 closed 3 years ago

pivoo81 commented 4 years ago

There might be cases when a transaction fee needs to be paid by a dApp, not a user. For example you, as a dApp developer, acquired fresh new users to your dApp, and they have no any assets on their account.

Currently it’s possible to use the bug found by the dApp developers community. This bug allows sending transactions with insufficient for transaction fee amount of Waves or sponsored asset on sender’s account. Blockchain will receive such transaction in case dApp send missing fee amount:

TransferSet([
    ScriptTransfer(invoke.caller, invoke.fee, unit)
])

This bug is is only works if no payment attached to the InvokeScript transaction.

There is a problem with this bug

In case dApp script sends Waves (or sponsored asset) to the sender of the InvokeScript transaction there can be unintended behavior.

For example if such back transfer is reward depending on some conditions, than user can send InvokeScript from the account without Waves. And he doesn’t risk at all:

  1. In case conditions for reward are valid transaction will be received by the blockchain and user get his reward
  2. In case conditions fro reward failed, this transaction will not go into the blockchain because of insufficient fee amount

It’s ok if dApp developer wants exactly such behavior. But if he wants some state changes in 2 case (for example previously put collateral changed the owner) he will will be surprised.

Of course such a dApp is not an example of the very good design, but such opportunity looks dangerous.

Feature request

As a user of a dApp I want to send a zero fee transaction sponsored by the dApp.

Possible solutions

There are several ways

Expand bug - make it feature

The easiest way is to expand the bug, and allow using it also for InvokeScript transactions with attached payments.

But having in mind «There is a problem with this bug» paragraph it looks quite dangerous for dApp developers.

They need to know about such side effect.

The main problem with this - when script back transfers to the sender it’s not obvious that it can be used as fee for the current transaction.

InvokeScript and RIDE extension

Another way is to add an ability to explicitly say that script wants to pay for the caller.

It could be done this way:

  1. Additional argument for the @callable functions identifying function which dApp is ready to pay for (PayCallersFee).
  2. Additional field in the InvokeScript transaction «dAppPaysFee» (true/false)

So:

  1. dApp developer can say which @callables he can pay for the caller
  2. user can send InvokeScript for such @callable with dAppPaysFee=true

In such @callable (with «PayCallersFee» flag) script can check additional conditions and decide if it really wants to pay for caller.

Conditions can be for example: user’s address is a white-list, user provided promo key as argument, etc.

Below you can see examples of both approaches.

The main question

The main concern about the first approach - it’s not obvious. There is an ability for dApp developer to shot himself in the foot.

There is an opinion that second approach is harder for dApp developers. Which one to choose? Maybe some other approach?

Examples

Expand bug - make it feature

@Callable(invoke)
func callMeBaby(uuid: String) = {

    if (isInWhiteList(invoke.caller) && invoke.fee == 500000) then {

        let id = extract(invoke.transactionId)
        let callerAddress = toBase58String(invoke.caller.bytes)

        ScriptResult(
            WriteSet([
                DataEntry(toBase58String(id), callerAddress + uuid)
            ]), 
            TransferSet([
                ScriptTransfer(addressFromStringValue(callerAddress), invoke.fee, unit)
            ])
        )
    }
    else
        throw()
}

In the example above it's better to additionally make sure that caller's account don't have enough money for paying fee :

wavesBalance(i.caller) >= i.fee

InvokeScript and RIDE extension

@Callable(invoke, PayCallersFee)
func callMeBaby(uuid: String) = {

    if (!invoke.dAppPaysFee || invoke.dAppPaysFee && isInWhiteList(invoke.caller) && invoke.fee == 500000) then {

        let id = extract(invoke.transactionId)
        let callerAddress = toBase58String(invoke.caller.bytes)

        ScriptResult(
            WriteSet([
                DataEntry(toBase58String(id), callerAddress + uuid)
            ]),
            TransferSet([])
        )
    }
    else
        throw()
}
deemru commented 4 years ago

I have a solution how to expand this bug.

If a final WriteSet have a TransferSet with exact i.fee amount back to caller then dApps pays the fee.

So if dApp pays some reward which can be implicitly assumed as the fee, but does not provide additional TransferSet with exact i.fee then caller should pay the fee as normal.

If dApp still want to pay the reward and the fee, then it should contain both TransferSets with the reward and the fee.

KardanovIR commented 4 years ago

I have a couple of assumptions: 1) Explicit is always better than implicit 2) dApp should not care about fees, because it can be different depending on a couple of parameters (is smart asset attached? is caller a smart account? is network overloaded?).

The problem is that these two assumptions contradict each other.

When I develop a dApp the last thing I'd like to think about are edge cases with fees, so in my opinion, an option to extend bug is better for now. Also, maybe we will find a better solution in the future? :)

KardanovIR commented 4 years ago

I have a solution how to expand this bug.

If a final WriteSet have a TransferSet with exact i.fee amount back to caller then dApps pays the fee.

So if dApp pays some reward which can be implicitly assumed as the fee, but does not provide additional TransferSet with exact i.fee then caller should pay the fee as normal.

If dApp still want to pay the reward and the fee, then it should contain both TransferSets with the reward and the fee.

But it is still implicit and not clear at all, even more, it can be misleading sometimes, for example, should two examples below pay fees?

TransferSet([ScriptTransfer(i.caller, i.fee, unit)])

TransferSet([ScriptTransfer(i.caller, 500000, unit)])
deemru commented 4 years ago

But it is still implicit and not clear at all, even more, it can be misleading sometimes, for example, should two examples below pay fees?

TransferSet([ScriptTransfer(i.caller, i.fee, unit)])

TransferSet([ScriptTransfer(i.caller, 500000, unit)])

if i.fee == 500000 then yes, both would be fee payments.

i do not see why dApp should transfer to caller 500000 other than fee.

ismagin commented 4 years ago
  1. This doesn't solve any problem except for poorly designed betting game, which will be hacked anyway.
  2. It's solving an "issue" on the wrong level. dApp shouldn't care about how user pays fee. What happens now is just reordering of payments, that can be used for helping users.
  3. Introducing new entities to language/library increases cognitive complexity, and it's ok-ish if the feature adds fuctionality or simplifies other aspects. This one does neither:
    • the functionality stays the same(dapp can in some cases pay for some users)
    • the complexity of development does't decrease, but rather otherwise. It would be nice to have a solid feature like this if it takes 1 paratemterless entity like annotation or action to be introduced, but unfortulately it's not the case. What if the user "pays" fee of 100 waves? surely dApp author should take care of this and reject that, so developers now need to dive deep into fees, feeAssetIds, etc. Just sticking to 0.005 is not solid enough either: if the caller has script, the fee should be higher. Furthermore, what if the invocation fee changes over time?

I suggest we improve the "bug" by allowing payments to be attached and just(haha) document the stuff properly.

christopheSeeka commented 4 years ago

I think the Expand bug option is still an acceptable solution if well explained and documented.

DeFi-Evangelist commented 4 years ago

"dAppPaysFee" - is very explicit for dApp developers to protect themselves from unexpected behaviour.

At the same time it's offering a good approach to let users working with dApp from their ownership of account without even "going to crypto". Users will use fiat payments or subscriptions then.

pivoo81 commented 4 years ago
  1. This doesn't solve any problem except for poorly designed betting game, which will be hacked anyway.

We know that this problem is present for such poorly designed games, but I'm not sure if there is no other cases where dApp developer will be in danger.

  1. It's solving an "issue" on the wrong level. dApp shouldn't care about how user pays fee. What happens now is just reordering of payments, that can be used for helping users.

Script should know what he pays for. Is he paying for transaction fee or for something else ?

  1. Introducing new entities to language/library increases cognitive complexity, and it's ok-ish if the feature adds fuctionality or simplifies other aspects. This one does neither:
    • the functionality stays the same(dapp can in some cases pay for some users)
    • the complexity of development does't decrease, but rather otherwise.

The functionality adds clearity. And control what the dApp pays for.

It would be nice to have a solid feature like this if it takes 1 paratemterless entity like annotation or action to be introduced, but unfortulately it's not the case. What if the user "pays" fee of 100 waves? surely dApp author should take care of this and reject that, so developers now need to dive deep into fees, feeAssetIds, etc. Just sticking to 0.005 is not solid enough either: if the caller has script, the fee should be higher. Furthermore, what if the invocation fee changes over time?

All this things are in control with the second approach, except taking into account if the caller's account is scripted.

I suggest we improve the "bug" by allowing payments to be attached and just(haha) document the stuff properly.

I think documentation is not enough. But it can be used postfactum when everything bad already happened to show it to the victim dApp developer :)

The maim question actually is - can this bug be exploited in some other way that described in the post? And if yes - could the same dApp be hacked without using this bug at the same time? In this case it can be stated that it's poor design irrespective to the "bug" presense.

amurdev commented 4 years ago

Hello All,

it is sad to hear that system contains undocumented behavior where experienced users are aware of it and other not. Please think about correct and quick reaction for such cases. Try to answer on the following questions:

Possible need to create special segments in documentation

About assumptions

@KardanovIR put forward the following assumptions

  1. Explicit is always better than implicit
  2. dApp should not care about fees, because it can be different depending on a couple of parameters (is smart asset attached? is caller a smart account? is network overloaded?).

I'm totally agree with him and would like to expand his list

  1. IDEALLY new features must not affected existed dapps and even must not lead to transaction's byte order changes
  2. information about fee payer should be kept in block chain

About different suggestions

@deemru

If a final WriteSet have a TransferSet with exact i.fee amount back to caller then dApps pays the fee

fully agree with @KardanovIR. Such behavior is implicit and can lead to misunderstanding and errors during dapp development. As a result ride lang will have mess in its' structure.

@ismagin args

  1. This doesn't solve any problem except for poorly designed betting game, which will be hacked anyway.

a good example about promo codes was provided. Dapp creators can add promo code hashes into accounts, print cards and do some advertising for new inexperienced audience

Second approach is preferable solution in my opinion

Let's match extended assumptions list with this approach

:white_check_mark: 1. Explicit is always better than implicit

Developer should use special annotation for function which will pay fee for caller.    

:white_check_mark: 2. dApp should not care about fees, because it can be different depending on a couple of parameters (is smart asset attached? is caller a smart account? is network overloaded?).

Developers decide should their dapp pay fees for caller or no. If they agree to take fee management risks and care about them then let's give such opportunity.    

:large_orange_diamond: 3. IDEALLY new features must not affected existed dapps and even must not lead to transaction's byte order changes

By default callable functions use existed logic and ONLY special annotation can change it behavior. But bullet 4 can affect the current one that why it has a yellow status    

:large_orange_diamond: 4. information about fee payer should be kept in block chain

I'm not sure is it possible to keep information about fee payer without byte order changes or not. Would like to ask Node and Ride teams about possible solutions (@pivoo81 @ismagin ). But if there is a choice between 3 and 4 bullets then it is better to have 4 because the 3d is an ideal to aspire

deemru commented 4 years ago

Agree with @amurdev if it is not hard to implement ideally there should be a new thing in ScriptResult called PayFeeSet, the problem solved.

christopheSeeka commented 4 years ago

Agree with the PayFeeSet addition in ScriptResult, clear and explicit.

pivoo81 commented 4 years ago

Today we have made some progress in this question.
Ilya made good proposal: 
To add new annotation @allowOverdraft for @callables. Functions with such annotation will be allowed to «use the bug» And such behavior will be expanded to invokeScripts with payments.

amurdev commented 4 years ago

How are you planning to keep information about fee payer? Looks like «Sorry guys, we prefer to do nothing because we are too lazy»

Proposal with new PayFeeSet is really smart and corresponds to all basic requirements. It is strange for me that you prefer «use the bug» option...

ismagin commented 4 years ago

Proposal with new PayFeeSet is really smart and corresponds to all basic requirements. It is strange for me that you prefer «use the bug» option...

It is the lazy option. It will either stop solving a problem in the future when fees might change(or when sender is scripted) or it will place additional burden on developer to figure if the fee is not 100 waves, if needs to be paid at all, which(to me) doesn't look like elegant solution.

Besides, the "bug" can be avoided now:

@Callable(tx)
func foo() = {
 if( !tx.feeAssetId.isDefined() && balance(tx.sender) < tx.fee )
   then throw("Not allowing overdraft")
   else { 
      ...
   }
}

The only issue with this is that that's not there by default.

ismagin commented 4 years ago

Today we have made some progress in this question.
Ilya made good proposal: 
To add new annotation @allowOverdraft for @callables. Functions with such annotation will be allowed to «use the bug» And such behavior will be expanded to invokeScripts with payments.

I thought about it a bit later and this we follow this path, we'll see that @AllowOverdraft() is runtime configuration, and configurations tend to be parameterized. So if we later need something else of that category, we'll get

@Callable(tx)
@AllowOverdraft(true)
@MaxBananas(80)
func foo() = {
 ...
}

This is already ugly, I'd like to see something like

@Callable(tx)
@Runtime(allowOverdraft = true, maxBananas = 80)
func foo() = {
 ...
}

But it's still :

It's one syntax used for completely different purposes, which is a complete mess from language design perspective.

amurdev commented 4 years ago

Point 1.

It will either stop solving a problem in the future when fees might change(or when sender is scripted) or it will place additional burden on developer to figure if the fee is not 100 waves

First of all it is an important note. Good catch! :+1:

My proposals here are:

  1. it is an explicit option which is turned OFF by default. If developer would like to use it then he must care about different risks
  2. additional configuration parameters can be provided to manage caller types. For an example configuration bellow can allows overdraft for not scripted accounts only
    @AllowOverdraft(value=true, maxFee = 0.01, allowScripts=false)

    Even more. RIDE runtime can provide special environment variables like

And as a result we will have a nice configuration:

@AllowOverdraft(value=true, maxFee = $minScriptFee, allowScripts=false)

 

Point 2.

It's one syntax used for completely different purposes, which is a complete mess from language design perspective

a lot of Java frameworks use annotations approach and more developers prefer annotations. Another question that such annotations must be well designed.

I'm not sure that

@Runtime(allowOverdraft = true, maxBananas = 80)

is better than

@AllowOverdraft(value=true, maxBananas = 80)
ismagin commented 4 years ago

the mess is within 1.@Callable(tx)

  1. @Runtime(false, 80) or @AllowOverdaraft(allowOverdraft=false, maxbananas=80)

in (1), we define variable, in (2) we construct an object

ismagin commented 4 years ago
@AllowOverdraft(value=true, maxFee = 0.01, allowScripts=false)

You're missing the point. The only thing such construction is supposed to do is allow overdraft, not decide the amount/scripting. it's not dapp's business at that high level.

amurdev commented 4 years ago

From one side you care about developers and their burden and from other side you state that RIDE couldn't provide utilities annotations to manage difficult aspects of fees in case of overdfraft.

If you would like to popularize RIDE you need to think about such cases also. Currently nobody can create utilities methods, common libraries or annotations outsides of RIDE core and Node.

pivoo81 commented 4 years ago

@amurdev we are thinking about different options how to solve that issue in the best way. Let's take some time to crystalize the decision.

ismagin commented 4 years ago

how about object initializer syntax?

@Callable(inv) {
   allowOverdraft = true
}
func foo() = ...

This syntax can(doesn't mean should) be later used for more sophisticated structures like

let owner = getString(this, "owner")
func calledBy(...) = ...

@Callalbe(inv) {
   allowOverdraft = true
   prerequisites = calledBy(owner)
}
func foo() = ...

I'm not suggesting anything like prerequisites at the moment, just thinking for about more viable syntax in scope of future developments.

The main question is whether we should or should not complicate things at all:

  1. If we go dsl-like constructor, tons of ad-hocs are coming.
  2. If we keep it simple, a lot of checks are needed to be performed manually, like the one for this particular issue:
    @Callable(tx)
    func foo() = {
    if( !tx.feeAssetId.isDefined() && balance(tx.sender) < tx.fee )
    then throw("Not allowing overdraft")
    else { 
      ...
    }
    }
christopheSeeka commented 4 years ago

Does it really worth it to complicated when current "bug as a feature" do the job with proper checks. Its not much more "risky" than a verifier, for beginers or those that dont know how it work, i still believe proper education and documentation on how to use it wouldnt make it that bad in the end.

ismagin commented 4 years ago

@christopheSeeka I would keep it simple and just improve the "bug" too. Just explaining motivation so it's clear that we are not "lazy devs who don't want to work / implement simple functionality"

christopheSeeka commented 4 years ago

@ismagin i agree with that approach personally.

ismagin commented 4 years ago

Ok, for the upcoming release we'll extend the "bug" and later make it tuneable when we're sure about the way it's designed

thiagocapuano commented 4 years ago

I not sure it is a bug after all, but a proof of the sequence execution of code. TransferScript happen before code finish or execution only happens if transfer and/or write data was concluded.

If will bad for dApp account, is for invoker too, because if code fail after transfer, in both cases (invoker pay or dApp pay) one will of them will prejudiced, like ethereum gas.

But this not happen. Why? Because if have something wrong on script not will be possible deploy it then if deployed he will be completely executed

thiagocapuano commented 4 years ago

@AllowOverDraft ??? Not have a annotation nomenclature more friendly? (If is case for implement)

Not believe a developer not be prejudiced, because if he implemented transfer for caller, naturally he first intention was transfer amount biggest of the fee, casualty sponsor invoke for dApp use did though.

deemru commented 3 years ago

Whats in conclusion? Free invocations closed forever?