Limit Refuel to 1 GAS #2561

erikzhang · 2021-08-04T08:00:25Z

No description provided.

erikzhang · 2021-08-04T08:00:39Z

@shargon @roman-khimov What do you think?

shargon · 2021-08-04T08:09:31Z

I think that if the account it's related to any transaction in the block, and it remove the total gas can poduce a Denial of service during onPersist, I agree with @roman-khimov in ABORT

ZhangTao1596 · 2021-08-04T08:10:48Z

Attackers can call refuel multi times to occupy system resource.

ZhangTao1596 · 2021-08-04T08:19:19Z

Limit sum of refueled gas to 1 gas?

shargon · 2021-08-04T08:28:16Z

Limit Refuel to only one call per invocation

ZhangTao1596 · 2021-08-04T08:32:48Z

How about this #2563

src/neo/SmartContract/ApplicationEngine.cs

src/neo/SmartContract/Native/GasToken.cs

Co-authored-by: ZhangTao <[email protected]>

erikzhang · 2021-08-04T08:48:12Z

@roman-khimov What do you think?

erikzhang · 2021-08-04T08:50:38Z

Can anyone fix the UT?

shargon · 2021-08-04T08:52:10Z

Can anyone fix the UT?

I will

shargon · 2021-08-04T08:59:18Z

@superboyiii could you test it if it could produce a DoS if during onPersist we remove the gas from other transaction fee?

roman-khimov · 2021-08-04T09:08:39Z

@roman-khimov What do you think?

It's very limiting.
It doesn't solve the problem.

ABORT is still there to make node's computational efforts almost free, so amplification is still possible.

erikzhang · 2021-08-04T09:13:19Z

Another protection mechanism can also be introduced: we can only allow Refuel calls from contracts. In this way, if the function is abused, we can sanction the contracts in the future.

superboyiii · 2021-08-04T09:21:34Z

@roman-khimov What do you think?

It's very limiting.

It doesn't solve the problem.

ABORT is still there to make node's computational efforts almost free, so amplification is still possible.

I agree with Roman. ABORT now and think for better solution later. It's too hurry find another way which might cause other issue.

shargon · 2021-08-04T09:25:21Z

Or can we even require the contract to pay for Refuel in advance?

Too much better

chenzhitong · 2021-08-04T09:37:48Z

Pay GAS in advance and refund the remaining system fee to the user. (No refund if an exception occurs)

erikzhang · 2021-08-04T09:55:50Z

I have another idea. We can limit it to tx.SystemFee + tx.NetworkFee. What do you think?

roman-khimov · 2021-08-04T09:57:56Z

Even more limiting, making Refuel less and less practical.
Limits amplification factor to 2. Not a lot, true, but still amplification.

erikzhang · 2021-08-04T10:01:45Z

Even more limiting, making Refuel less and less practical.

I think it's better than removing it.

Limits amplification factor to 2. Not a lot, true, but still amplification.

If we limit it to tx.SystemFee + tx.NetworkFee, I'm sure no one will attack this way again.

shargon · 2021-08-04T10:02:23Z

Pay gas in advance, make a bag, and decrement this bag during refuel, like this never can affect to OnPersist

shargon · 2021-08-04T10:03:35Z

src/neo/SmartContract/Native/GasToken.cs

@@ -40,10 +40,12 @@ internal override async ContractTask OnPersist(ApplicationEngine engine)
 [ContractMethod(CpuFee = 1 << 15, RequiredCallFlags = CallFlags.States | CallFlags.AllowNotify)]
 private async ContractTask Refuel(ApplicationEngine engine, UInt160 account, long amount)
 {
- if (amount < 0) throw new ArgumentOutOfRangeException(nameof(amount));
+ if (amount <= 0) throw new ArgumentOutOfRangeException(nameof(amount));
+ if (ContractManagement.GetContract(engine.Snapshot, engine.CallingScriptHash) is null)


What do you mean?

Who pay te gas it's the account argument

erikzhang · 2021-08-04T10:17:45Z

I think it's perfect now.

chenzhitong · 2021-08-04T10:22:02Z

UT

roman-khimov · 2021-08-04T10:22:23Z

Even more limiting, making Refuel less and less practical.

I think it's better than removing it.

The intention behind #2444 can be traced via #2443 to neo-project/proposals#137 and that intention to me is to have contract-sponsored actions. Limiting Refuel to fee amount makes the user inevitably pay half of the real price, so it's a discount, but the intention was rather to have contract paying 90%+ of fees. I've questioned usefulness of this method already in #2443 (from a bit different perspective though), but with this limit it's hard for me to imagine who is going to use it. Methods should solve some problems and I'm not sure it solves anything now.

erikzhang · 2021-08-04T10:27:43Z

It is difficult for a contract to determine whether the sponsorship has been abused. Now it does not need to worry too much, because users have to pay at least half of the cost.

erikzhang · 2021-08-04T10:43:09Z

but the intention was rather to have contract paying 90%+ of fees

Maybe we can limit it to 10 * (tx.SystemFee + tx.NetworkFee)?

ZhangTao1596 · 2021-08-04T11:07:51Z

Maybe we can limit it to 10 * (tx.SystemFee + tx.NetworkFee)

~~(tx.SystemFee + tx.NetworkFee) * execFactor / 3?~~

superboyiii · 2021-08-04T11:24:23Z

but the intention was rather to have contract paying 90%+ of fees

Maybe we can limit it to 10 * (tx.SystemFee + tx.NetworkFee)?

I think system_fee + = Refuel /exe_Factor*3 is more resonable. Don't make a static limit to a dynamic result.

roman-khimov · 2021-08-04T11:40:06Z

Maybe we can limit it to 10 * (tx.SystemFee + tx.NetworkFee)?

Increases usefulness, raises amplification factor.

Don't make a static limit to a dynamic result.

ExecFeeFactor is already priced-in with system fee.

roman-khimov · 2021-08-04T11:48:21Z

Pay gas in advance, make a bag, and decrement this bag during refuel, like this never can affect to OnPersist

And BTW, while this probably is the only valid solution (if not counting Refuel deletion) it would also mean that execution will produce some side-effect for failed transaction which was never the case before (fees written off is a bit different story).

Which technically allows to attack contract itself (draining balance from it), so maybe it's not a valid solution either.

roman-khimov · 2021-08-04T12:12:58Z

As a side note, think of MaxBlockSystemFee also that Refuel technically breaks. Not the biggest problem with it, but still.

ZhangTao1596 · 2021-08-04T13:19:27Z

We should revert first. Seems we can't get agreement soon.

igormcoelho · 2021-08-05T00:58:32Z

For this Refuel to work we need to make sure that the assets are innevitably taken from user or contract account. It should have similar logic to network or system fees, so relating a factor into it is a clever solution, that only introduces a price reduction, at worst.

For the moment, I agree with @erikzhang in putting a limiting factor of 2, or even zero, if necessary. Best thing if we could put it in some global constant, that can be updated on time. Then, we take the time to discuss it further.

I also agree to take funds only from a deployed contract.
In my opinion, nodes can calculate how much refunds are being done from a contract (including FAULT ones), and if it reaches some threshold, Refuel gets disabled for that contract. It means that contracts willing to sponsor anything should be careful to only authorize only effective transactions.

ZhangTao1596 · 2021-08-05T01:56:55Z

Another problem. Once we set the limit inApplicationEngine, we have to keep it there and add height check when we apply new refuel mechanism in the future. Because old tx can't be executed in new mechanism.

It can be used to attack the network (amplifying DOS), so it's broken beyond repair. See also neo-project/neo#2560 and neo-project/neo#2561.

It can be used to attack the network (amplifying DOS), so it's broken beyond repair. This reverts ac60160. See also neo-project/neo#2560 and neo-project/neo#2561.

Limit Refuel to 1 GAS

73cb78a

erikzhang closed this Aug 4, 2021

erikzhang deleted the limit_refuel branch August 4, 2021 08:14

erikzhang restored the limit_refuel branch August 4, 2021 08:29

More limit

7a5b775

erikzhang reopened this Aug 4, 2021

Update ApplicationEngine.cs

dfbabf9

shargon reviewed Aug 4, 2021

View reviewed changes

src/neo/SmartContract/ApplicationEngine.cs Outdated Show resolved Hide resolved

erikzhang mentioned this pull request Aug 4, 2021

Add gas refuel sum limit #2563

Closed

ZhangTao1596 reviewed Aug 4, 2021

View reviewed changes

src/neo/SmartContract/Native/GasToken.cs Outdated Show resolved Hide resolved

erikzhang and others added 2 commits August 4, 2021 16:42

Add message

d286441

Update src/neo/SmartContract/Native/GasToken.cs

1556f12

Co-authored-by: ZhangTao <[email protected]>

Merge branch 'master' into limit_refuel

1fb77d2

shargon previously approved these changes Aug 4, 2021

View reviewed changes

shargon self-requested a review August 4, 2021 08:51

fix UT

3f4a7e7

shargon dismissed their stale review via 3f4a7e7 August 4, 2021 08:54

Fix numbers

6d71701

shargon previously approved these changes Aug 4, 2021

View reviewed changes

Allow calls from deployed contracts only

2fca9c3

erikzhang dismissed shargon’s stale review via 2fca9c3 August 4, 2021 09:58

shargon reviewed Aug 4, 2021

View reviewed changes

Limit it to SystemFee+NetworkFee

7c1bd04

erikzhang closed this Aug 5, 2021

erikzhang deleted the limit_refuel branch August 5, 2021 05:05

roman-khimov added a commit to nspcc-dev/neo-go that referenced this pull request Aug 5, 2021

native: drop Refuel method from GAS

603822e

It can be used to attack the network (amplifying DOS), so it's broken beyond repair. See also neo-project/neo#2560 and neo-project/neo#2561.

roman-khimov mentioned this pull request Aug 5, 2021

native: drop Refuel method from GAS nspcc-dev/neo-go#2111

Merged

Limit Refuel to 1 GAS #2561

Limit Refuel to 1 GAS #2561

Conversation

erikzhang commented Aug 4, 2021

erikzhang commented Aug 4, 2021

shargon commented Aug 4, 2021

ZhangTao1596 commented Aug 4, 2021

ZhangTao1596 commented Aug 4, 2021

shargon commented Aug 4, 2021

ZhangTao1596 commented Aug 4, 2021

erikzhang commented Aug 4, 2021

erikzhang commented Aug 4, 2021

shargon commented Aug 4, 2021

shargon commented Aug 4, 2021 • edited Loading

roman-khimov commented Aug 4, 2021

erikzhang commented Aug 4, 2021

superboyiii commented Aug 4, 2021

shargon commented Aug 4, 2021

chenzhitong commented Aug 4, 2021

erikzhang commented Aug 4, 2021

roman-khimov commented Aug 4, 2021

erikzhang commented Aug 4, 2021

shargon commented Aug 4, 2021

shargon Aug 4, 2021

Choose a reason for hiding this comment

erikzhang Aug 4, 2021

Choose a reason for hiding this comment

shargon Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

erikzhang commented Aug 4, 2021

chenzhitong commented Aug 4, 2021

roman-khimov commented Aug 4, 2021

erikzhang commented Aug 4, 2021

erikzhang commented Aug 4, 2021

ZhangTao1596 commented Aug 4, 2021 • edited Loading

superboyiii commented Aug 4, 2021 • edited Loading

roman-khimov commented Aug 4, 2021

roman-khimov commented Aug 4, 2021 • edited Loading

roman-khimov commented Aug 4, 2021

ZhangTao1596 commented Aug 4, 2021

igormcoelho commented Aug 5, 2021 • edited Loading

ZhangTao1596 commented Aug 5, 2021 • edited Loading

shargon commented Aug 4, 2021 •

edited

Loading

shargon Aug 4, 2021 •

edited

Loading

ZhangTao1596 commented Aug 4, 2021 •

edited

Loading

superboyiii commented Aug 4, 2021 •

edited

Loading

roman-khimov commented Aug 4, 2021 •

edited

Loading

igormcoelho commented Aug 5, 2021 •

edited

Loading

ZhangTao1596 commented Aug 5, 2021 •

edited

Loading