Nxt source code flaw reports - page 63.

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Quote from: Come-from-Beyond on January 03, 2014, 07:26:48 AM

Nxt source code has been released - https://bitcointalksearch.org/topic/m.4287127

The code contains 3 flaws - serious, critical and fatal. The 1st person who reports these flaws will get 1'000, 10'000 or 100'000 NXT reward accordingly.

Each flaw has a small description. Here r SHA256 hashes of these descriptions:

bd34c891e9e3df9ea8b8eafc4dc3edc129f81365d42bf204ea58271e320f3ce5 - 1K reward
888f278c773d39b8334a651d84ee78871bd0e5d45e09be8fdb190ba1b2969530 - 10K reward
f5236644f4306699bb0fa90a905afe2454683c0aad6995e4433d712e2fdb257c - 100K reward

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

If u think that u found a flaw, post here its description. Mathematical proof is not necessary, common sense should be enough. If ur guess is correct u may* get the reward, if u find a non-injected flaw then u'll be asked for more formal proof (u may get a reward too).

NB: Some guys mentioned that they would just decompile 0.4.7e binaries and compare the source codes to find the flaws. As a countermeasure against such the trick u still must explain why there is a flaw.

-------------
* - BCNext reserves the right to refuse to pay a reward without any explanation. This is an anti-troll countermeasure.

Man, the B.S. is unbelievable.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1010

Newbie

@Skerberus:

Thank u. If Jean-Luc has not fixed this yet, he will do it I believe.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1010

Newbie

Quote from: FrictionlessCoin on January 03, 2014, 01:36:51 PM

Talk about amateur hour!!

So let's get this right, the source code is released and we find it all in a single .java file.

One class with a lot of inner classes.

Does the developer ever know what a java package is for?

Then he declares all the member variables of the Nxt class as static. Does he know the difference between static and instance variable?

To place matters worse, the Nxt class happens to be a servlet. Does he not know that a new servlet is instantiated per thread??!

Well, to be honest, 21 BTC original invested may have been too generous!

All ur statements r irrelevant. They r just personal insults ("Does he know the difference between static and instance variable?") or incorrect guesses ("Does he not know that a new servlet is instantiated per thread?")
Trolls r not allowed here, so create another thread plz.

Skerberus

hero member

Activity: 913

Merit: 664

Another code-change I would suggest:

Line 6795:

Old Code:

Code:

@Override
	public void destroy() {
		
		scheduledThreadPool.shutdown();
		cachedThreadPool.shutdown();
		
		try {
			
			Block.saveBlocks("blocks.nxt");
			
		} catch (Exception e) { }
		try {
			
			Transaction.saveTransactions("transactions.nxt");
			
		} catch (Exception e) { }
		
		try {
			
			blockchainChannel.close();
			
		} catch (Exception e) { }
		
		logMessage("Nxt stopped.");
		
	}

I would at least change it this way, because the Shutdown-Methods of scheduledThreadPool and cahedThreadPool can throw the following e.g. Exceptions ( java.lang.SecurityException,java.lang.RuntimePermission)
If one of these exceptions is thrown the blocks.nxt, transactions.nxt will not be saved.
Moreover it's not good style to leave catch-Blocks empty, so I would suggest to log those errors at least

!

Code:

		@Override
	public void destroy() {

		
		try {
			scheduledThreadPool.shutdown();
			
			
		} catch (Exception e) {
			logMessage(e.getMessage());
		}
		
		try {
			cachedThreadPool.shutdown();
		} catch (Exception e) {
			logMessage(e.getMessage());

		}
		

		
		
		try {

			Block.saveBlocks("blocks.nxt");

		} catch (Exception e) {
			logMessage(e.getMessage());
		}
		try {

			Transaction.saveTransactions("transactions.nxt");

		} catch (Exception e) {
			logMessage(e.getMessage());
		}

		try {

			blockchainChannel.close();

		} catch (Exception e) {
			logMessage(e.getMessage());
		}

		logMessage("Nxt stopped.");

	}

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Talk about amateur hour!!

So let's get this right, the source code is released and we find it all in a single .java file.

One class with a lot of inner classes.

Does the developer ever know what a java package is for?

Then he declares all the member variables of the Nxt class as static. Does he know the difference between static and instance variable?

To place matters worse, the Nxt class happens to be a servlet. Does he not know that a new servlet is instantiated per thread??!

Well, to be honest, 21 BTC original invested may have been too generous!

Skerberus

hero member

Activity: 913

Merit: 664

Here are some code-changes I would suggest:

The autoclosable-feature of Java 7 should be used, to prevent not closed streams in case of an exception:

Example:

Old Code:(805-815)

Code:

static void loadBlocks(String fileName) throws Exception {
			
			FileInputStream fileInputStream = new FileInputStream(fileName);
			ObjectInputStream objectInputStream = new ObjectInputStream(fileInputStream);
			blockCounter = objectInputStream.readInt();
			blocks = (HashMap)objectInputStream.readObject();
			lastBlock = objectInputStream.readLong();
			objectInputStream.close();
			fileInputStream.close();
			
		}

better version (even closed in case of an exception during read):

Code:

try (FileInputStream fileInputStream = new FileInputStream(fileName);
					ObjectInputStream objectInputStream = new ObjectInputStream(
							fileInputStream);) {
				blockCounter = objectInputStream.readInt();
				blocks = (HashMap) objectInputStream.readObject();
				lastBlock = objectInputStream.readLong();
			}

Please keep in mind that there are more place where the code should be changed this way (e.g. methods loadTransactions (3299), savetransactions (3437)...)

gbeirn

full member

Activity: 126

Merit: 100

Quote from: rlh on January 03, 2014, 01:01:11 PM

Ok, to be sure. If I compile this code and run it, I should be able to download and process the blockchain up to block 30k? It would be nice is there was a testnet for just for the source-- that way we could test-process transactions.

I have nodes up and running for a testnet but haven't been able to get anyone who was in the genesis block to give me a passphrase so we would have coins to use on it.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1010

Newbie

Quote from: rlh on January 03, 2014, 01:01:11 PM

Ok, to be sure. If I compile this code and run it, I should be able to download and process the blockchain up to block 30k? It would be nice is there was a testnet for just for the source-- that way we could test-process transactions.

U'll be able to catch blocks till #22000. U would get all 30000 blocks but BCNext removed Alias System so ur version will reject block 22001.

Edit: U can remove all well-known peers and leave only ur own IP. Then ur node will talk to itself only and u will be able to send transactions that won't go outside.

rlh

hero member

Activity: 804

Merit: 1004

Ok, to be sure. If I compile this code and run it, I should be able to download and process the blockchain up to block 30k? It would be nice is there was a testnet for just for the source-- that way we could test-process transactions.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1010

Newbie

Quote from: BloodyRookie on January 03, 2014, 12:39:28 PM

Jean-Luc stated:

Quote

This is the source of the Nxt Reference Software (NRS) corresponding to version 0.4.7e, as provided by its original developer, BCNext.

Does that mean, the flaws were incorporated into 0.4.7e which was distributed to the nxt users?

It means that original 0.4.7e source code was modified before publishing.

Edit: U can try to find differences between 0.4.7e binaries and 0.4.7e source code, but u still must explain found flaws to get bounties.

BloodyRookie

hero member

Activity: 687

Merit: 500

Jean-Luc stated:

Quote

This is the source of the Nxt Reference Software (NRS) corresponding to version 0.4.7e, as provided by its original developer, BCNext.

Does that mean, the flaws were incorporated into 0.4.7e which was distributed to the nxt users?

Come-from-Beyond

legendary

Activity: 2142

Merit: 1010

Newbie

Quote from: lr127 on January 03, 2014, 11:51:24 AM

Come-from-Beyond
Where is aliases? In getTransaction as example.
In version NRS 0.4.7 they exists.

Alias System is an advanced feature and was removed from the source code.

lr127

newbie

Activity: 35

Merit: 0

Come-from-Beyond
Where is aliases? In getTransaction as example.
In version NRS 0.4.7 they exists.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1010

Newbie

Quote from: Jaguar0625 on January 03, 2014, 09:47:45 AM

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.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1010

Newbie

Quote from: ImmortAlex on January 03, 2014, 09:18:56 AM

Quote from: Come-from-Beyond on January 03, 2014, 09:05:52 AM

Edit: I can't explain why, coz I can leak info about a flaw accidentally, sorry.

Let me guess, we need to add amount -= transaction.amount when transaction.senderPublicKey is equals to publicKey?
I mean loop in Account.getEffectiveBalance() method.

It can be wrong assumption because I still doesn't understand well what is effective balance

It's an incorrect guess. The flaws r more sophisticated.

rlh

hero member

Activity: 804

Merit: 1004

I'm not a BTC hater, but a small part of me hopes that the 100k flaw is an injection of a technique that is part of the bitcoin, and all other altcoin, protocols.

Let's just say it would be a passive way of proving you've done your crypto-currency homework.

Kudos to this event. I hope this drums up a lot more support and attention for this innovative coin.

Now if we can just get someone to write this in C...

Jaguar0625

sr. member

Activity: 299

Merit: 250

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())

ImmortAlex

hero member

Activity: 784

Merit: 501

Quote from: Come-from-Beyond on January 03, 2014, 09:05:52 AM

Edit: I can't explain why, coz I can leak info about a flaw accidentally, sorry.

Let me guess, we need to add amount -= transaction.amount when transaction.senderPublicKey is equals to publicKey?
I mean loop in Account.getEffectiveBalance() method.

It can be wrong assumption because I still doesn't understand well what is effective balance

bitcoinpaul

hero member

Activity: 910

Merit: 1000

The prewritten statements for the 3 flaws.

xibeijan

legendary

Activity: 1232

Merit: 1001

I don't understand this part

Each flaw has a small description. Here r SHA256 hashes of these descriptions:

bd34c891e9e3df9ea8b8eafc4dc3edc129f81365d42bf204ea58271e320f3ce5 - 1K reward
888f278c773d39b8334a651d84ee78871bd0e5d45e09be8fdb190ba1b2969530 - 10K reward
f5236644f4306699bb0fa90a905afe2454683c0aad6995e4433d712e2fdb257c - 100K reward

What do you mean by "small description"?

What is the input that gives these hashes? The source code?

Topic: Nxt source code flaw reports - page 63. (Read 113369 times)