Nxt source code flaw reports

[ricot]

I just heard back from CfB. He said it was "I insist u post this publicly. This is the best attack ever,..." I am reluctant to make it public, but when CfB says to post it, who am I to disagree. I will post on the main thread.

James

Link: https://bitcointalk.org/index.php?topic=345619.msg4329478#msg4329478

Yeah, that's not really a feasible attack... And it also works on BTC, hell even on small FIATs.
Just go to Honduras (randomly chosen small country), tell everyone there that they get 10x their local money's worth in USD, take all the Iempira (that's their currency), burn it, done.

[BloodyRookie]

Does it really stuck in this case? Have u checked?

Yes, I started with 2 well known Peer, both had hallmarks. I logged the method calls in the peer class:

[2014-01-05 19:25:00.629] Nxt 0.4.7e started.
[2014-01-05 19:25:00.630] "blockchainStoragePath" = "blockchain.nrs"
[2014-01-05 19:25:00.636] "myScheme" = "http"
[2014-01-05 19:25:00.636] "myPort" = "7874"
[2014-01-05 19:25:00.636] "myAddress" = ""
[2014-01-05 19:25:00.636] "shareMyAddress" = "true"
[2014-01-05 19:25:00.637] "myHallmark" = ""
[2014-01-05 19:25:00.637] "wellKnownPeers" = "78.46.63.221; 95.85.22.142"
[2014-01-05 19:25:00.642] Peer::addPeer (peer address = 78.46.63.221)
[2014-01-05 19:25:00.643] Peer::addPeer (peer address = 95.85.22.142)
[2014-01-05 19:25:00.643] "maxNumberOfConnectedPublicPeers" = "20"
[2014-01-05 19:25:00.644] "connectTimeout" = "2000"
[2014-01-05 19:25:00.644] "readTimeout" = "5000"
[2014-01-05 19:25:00.644] "enableHallmarkProtection" = "true"
[2014-01-05 19:25:00.644] "pushThreshold" = "0"
[2014-01-05 19:25:00.644] "pullThreshold" = "0"
[2014-01-05 19:25:00.645] "allowedUserHosts" = "127.0.0.1; localhost; 0:0:0:0:0:
0:0:1;"
[2014-01-05 19:25:00.645] "allowedBotHosts" = "127.0.0.1; localhost; 0:0:0:0:0:0
:0:1;"
[2014-01-05 19:25:00.645] "blacklistingPeriod" = "300000"
[2014-01-05 19:25:00.645] "communicationLoggingMask" = "0"
[2014-01-05 19:25:00.646] Loading transactions...
[2014-01-05 19:25:00.692] Loading blocks...
[2014-01-05 19:25:00.704] Scanning blockchain...
[2014-01-05 19:25:00.712] ...Done
[2014-01-05 19:25:00.713] Genesis block: Transactions=73 payload=9344
[2014-01-05 19:25:00.720] Peer::getAnyPeer
[2014-01-05 19:25:00.721] Peer::connect (peer address = 95.85.22.142)
[2014-01-05 19:25:00.724] Peer::getAnyPeer
[2014-01-05 19:25:00.727] Peer::getAnyPeer
[2014-01-05 19:25:00.735] Peer::getAnyPeer
2014-01-05 19:25:00.758:INFO:oejsh.ContextHandler:main: Started o.e.j.w.WebAppCo
ntext@1ddb577{/,file:/F:/Android/Projekte/nxt/webapps/root/,AVAILABLE}{F:\Androi
d\Projekte\nxt\webapps\root}
2014-01-05 19:25:00.776:INFO:oejs.ServerConnector:main: Started ServerConnector@
e3dab5{HTTP/1.1}{0.0.0.0:7874}
[2014-01-05 19:25:00.834] Peer::analyzeHallmark (peer address = 95.85.22.142)
[2014-01-05 19:25:00.835] Peer::blacklist (peer address = 95.85.22.142)
2014-01-05 19:25:01.013:INFO:oejs.ServerConnector:main: Started ServerConnector@
264154{SSL-http/1.1}{0.0.0.0:7875}
[2014-01-05 19:25:01.736] Peer::getAnyPeer
[2014-01-05 19:25:02.737] Peer::getAnyPeer
[2014-01-05 19:25:03.738] Peer::getAnyPeer
[2014-01-05 19:25:04.739] Peer::getAnyPeer
[2014-01-05 19:25:05.725] Peer::getAnyPeer
[2014-01-05 19:25:05.728] Peer::getAnyPeer
[2014-01-05 19:25:05.740] Peer::getAnyPeer
[2014-01-05 19:25:05.836] Peer::getAnyPeer
[2014-01-05 19:25:05.836] Peer::connect (peer address = 78.46.63.221)
[2014-01-05 19:25:05.890] Peer::analyzeHallmark (peer address = 78.46.63.221)
[2014-01-05 19:25:05.913] Peer::blacklist (peer address = 78.46.63.221)
[2014-01-05 19:25:06.741] Peer::getAnyPeer
[2014-01-05 19:25:10.914] Peer::getAnyPeer

[Come-from-Beyond]

Does it really stuck in this case? Have u checked?

Yes, I started with 2 well known Peer, both had hallmarks. I logged the method calls in the peer class:

Good catch. This saved my time coz I was going to find out why sometimes my node can't catch the blocks.

PS: I used the account in ur signature. Thank u.

[BloodyRookie]

PS: I used the account in ur signature. Thank u.

thx

[FrictionlessCoin]

Unless I missed something ...

Validation is done by the peers. A node doesn't adjust unconfirmedBalance and processes the transaction only after it's received from one of the peers. It's received only if passes validation check.

With the current code, what will fail the validation when (transaction.amount + transaction.fee) overflows? Since amount and fee will both be > 0 and the negative total will be able to be credited to the account.

For the life of me,  I can't understand why they use the primitive long to represent money in NXT.  It is crazy!

[FrictionlessCoin]

Btw: This thing would never go thru any kind of (government mandated) review in the banking or automotive industry, just saying...

Agree. This code is just an implementation of the idea, not a finished product.

Don't know how your investors should feel about this.

[gbeirn]

Btw: This thing would never go thru any kind of (government mandated) review in the banking or automotive industry, just saying...

Agree. This code is just an implementation of the idea, not a finished product.

Don't know how your investors should feel about this.

That's exactly what we invested in, an idea. The fact, to me, that we are this far along in such a short period of time is nothing short of amazing.

[Come-from-Beyond]

Since there are conditions in the code for when (Block.getLastBlock().height - blocks.get(commonBlockId).height < 720)...

Why are there NO conditions set for when (Block.getLastBlock().height - blocks.get(commonBlockId).height >= 720)? 

Shouldn't there be a mechanism for those that have fallen off and are > 720 blocks behind to catch up?


Sorry if I am missing something trivial here... but I am trying my best!   

Soft ignores blockchain reorgs deeper than 720 blocks.

[Come-from-Beyond]

Would you be revealing the flaw if you answered the question below...

When does curBlockId equal zero (curBlockId = 0)?

When the very last block is reached.

[gsan1]

The published code contains 3 flaws to find more bugs and to prevent copycats from copying NXT to early, right?
But why does it prevent them from copying? Could the flaws being used to "destroy" a copied NXT system?

[Come-from-Beyond]

The published code contains 3 flaws to find more bugs and to prevent copycats from copying NXT to early, right?
But why does it prevent them from copying? Could the flaws being used to "destroy" a copied NXT system?

The fatal flaw will bring a copy-coin to death within a day.

[Jaguar0625]

Code:

							Transaction transaction = Nxt.transactions.remove(block.transactions[i]);
							unconfirmedTransactions.put(block.transactions[i], transaction);
							
							Account senderAccount = accounts.get(Account.getId(transaction.senderPublicKey));
							synchronized (senderAccount) {
								
								senderAccount.setBalance(senderAccount.balance + (transaction.amount + transaction.fee) * 100L);
								
							}
							
							Account recipientAccount = accounts.get(transaction.recipient);
							synchronized (recipientAccount) {
								
								recipientAccount.setBalance(recipientAccount.balance - transaction.amount * 100L);
								recipientAccount.setUnconfirmedBalance(recipientAccount.unconfirmedBalance - transaction.amount * 100L);
								
							}
							
							JSONObject addedUnconfirmedTransaction = new JSONObject();
							addedUnconfirmedTransaction.put("index", transaction.index);
							addedUnconfirmedTransaction.put("timestamp", transaction.timestamp);
							addedUnconfirmedTransaction.put("deadline", transaction.deadline);
							addedUnconfirmedTransaction.put("recipient", convert(transaction.recipient));
							addedUnconfirmedTransaction.put("amount", transaction.amount);
							addedUnconfirmedTransaction.put("fee", transaction.fee);
							addedUnconfirmedTransaction.put("sender", convert(Account.getId(transaction.senderPublicKey)));
							addedUnconfirmedTransaction.put("id", convert(transaction.getId()));
							addedUnconfirmedTransactions.add(addedUnconfirmedTransaction);

i think there might be a bug here. accounts is just a HashMap, so accounts.get can return null.

if the sender sender is invalid, you will just get a null reference exception before doing anything.

But, if the sender is valid and the recipient is not, you would have already updated the sender's balance but throw a null reference exception before updating the recipient's balance.

And if you ever get into this situation, it appears to be non-recoverable because you will never be able to pop the last block. The code will always throw before getting to:

Code:

lastBlock = block.previousBlock;

And the popLastBlock function will return false exiting the loops you're calling it from:

Code:

while (lastBlock != commonBlockId && Block.popLastBlock())

I'll check if this situation is possible but this is not one of the injected flaws.

Were you able to check if this was possible?

[ricot]

I'm still waiting on statements concerning these two problems:

- Waiting past the creation time of a block to add additinal future transactions gains malicious Bob more coins when forging and causes the creation of unneccessary orphan blocks.
- Malicious Anna can gain ~7.5% more forges by spamming the network with early-generated blocks.

Are both of those working as intended? If yes: The network will become a lot more inefficient in the near future  

[Come-from-Beyond]

Were you able to check if this was possible?

No.

[Come-from-Beyond]

I'm still waiting on statements concerning these two problems:

- Waiting past the creation time of a block to add additinal future transactions gains malicious Bob more coins when forging and causes the creation of unneccessary orphan blocks.
- Malicious Anna can gain ~7.5% more forges by spamming the network with early-generated blocks.

Are both of those working as intended? If yes: The network will become a lot more inefficient in the near future  

- It's not a problem with TF, blocks r supposed to be forged in advance.
- This can be fixed by using more than 1 thread for blockchain downloading.

[BloodyRookie]

Code:

							Transaction transaction = Nxt.transactions.remove(block.transactions[i]);
							unconfirmedTransactions.put(block.transactions[i], transaction);
							
							Account senderAccount = accounts.get(Account.getId(transaction.senderPublicKey));
							synchronized (senderAccount) {
								
								senderAccount.setBalance(senderAccount.balance + (transaction.amount + transaction.fee) * 100L);
								
							}
							
							Account recipientAccount = accounts.get(transaction.recipient);
							synchronized (recipientAccount) {
								
								recipientAccount.setBalance(recipientAccount.balance - transaction.amount * 100L);
								recipientAccount.setUnconfirmedBalance(recipientAccount.unconfirmedBalance - transaction.amount * 100L);
								
							}
							
							JSONObject addedUnconfirmedTransaction = new JSONObject();
							addedUnconfirmedTransaction.put("index", transaction.index);
							addedUnconfirmedTransaction.put("timestamp", transaction.timestamp);
							addedUnconfirmedTransaction.put("deadline", transaction.deadline);
							addedUnconfirmedTransaction.put("recipient", convert(transaction.recipient));
							addedUnconfirmedTransaction.put("amount", transaction.amount);
							addedUnconfirmedTransaction.put("fee", transaction.fee);
							addedUnconfirmedTransaction.put("sender", convert(Account.getId(transaction.senderPublicKey)));
							addedUnconfirmedTransaction.put("id", convert(transaction.getId()));
							addedUnconfirmedTransactions.add(addedUnconfirmedTransaction);

i think there might be a bug here. accounts is just a HashMap, so accounts.get can return null.

if the sender sender is invalid, you will just get a null reference exception before doing anything.

But, if the sender is valid and the recipient is not, you would have already updated the sender's balance but throw a null reference exception before updating the recipient's balance.

And if you ever get into this situation, it appears to be non-recoverable because you will never be able to pop the last block. The code will always throw before getting to:

Code:

lastBlock = block.previousBlock;

And the popLastBlock function will return false exiting the loops you're calling it from:

Code:

while (lastBlock != commonBlockId && Block.popLastBlock())

I'll check if this situation is possible but this is not one of the injected flaws.

Were you able to check if this was possible?

Before a block can get popped, it has to be pushed. pushBlock calls analyze (if the block is considered to be valid) and in analyze, the recipient of a transaction is added if needed:

Code:

				Nxt.Account recipientAccount = accounts.get(transaction.recipient);
				if (recipientAccount == null)
				{
					recipientAccount = Account.addAccount(transaction.recipient);
				}

So when popBlock is called, the recipient is known and the Hashmap will always return a valid account.

[landomata]

The published code contains 3 flaws to find more bugs and to prevent copycats from copying NXT to early, right?
But why does it prevent them from copying? Could the flaws being used to "destroy" a copied NXT system?

The fatal flaw will bring a copy-coin to death within a day.

What if someone find this fatal flaw but doesn;t say anything?  and keeps it secret.

Edit: And builds a copy-coin....can he still defend the copycat with his knowledge?

I don;t feel good asking this question

[gsan1]

The flaws must be reported before the 3rd of April, after that date they can be revealed at any moment.

[Come-from-Beyond]

The published code contains 3 flaws to find more bugs and to prevent copycats from copying NXT to early, right?
But why does it prevent them from copying? Could the flaws being used to "destroy" a copied NXT system?

The fatal flaw will bring a copy-coin to death within a day.

What if someone find this fatal flaw but doesn;t say anything?  and keeps it secret.

Edit: And builds a copy-coin....can he still defend the copycat with his knowledge?

I don;t feel good asking this question

Well, we still have a critical and a serious flaws.

[landomata]

The published code contains 3 flaws to find more bugs and to prevent copycats from copying NXT to early, right?
But why does it prevent them from copying? Could the flaws being used to "destroy" a copied NXT system?

The fatal flaw will bring a copy-coin to death within a day.

What if someone find this fatal flaw but doesn;t say anything?  and keeps it secret.

Edit: And builds a copy-coin....can he still defend the copycat with his knowledge?

I don;t feel good asking this question

Well, we still have a critical and a serious flaws.

I don;t think the flaws should be published....for at least 3 years