Nxt source code flaw reports

[xibeijan]

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?

[bitcoinpaul]

The prewritten statements for the 3 flaws.

[ImmortAlex]

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

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

[rlh]

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...

[Come-from-Beyond]

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.

[Come-from-Beyond]

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.

[lr127]

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

[Come-from-Beyond]

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.

[BloodyRookie]

Jean-Luc stated:

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]

Jean-Luc stated:

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.

[rlh]

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]

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.

[gbeirn]

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.

[Skerberus]

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<Long, Block>)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<Long, Block>) 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)...)

[FrictionlessCoin]

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]

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.");

	}

[Come-from-Beyond]

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.

[Come-from-Beyond]

@Skerberus:

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

[FrictionlessCoin]

Nxt source code has been released - https://bitcointalk.org/index.php?topic=345619.msg4287127#msg4287127

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.