Nxt source code flaw reports

[joefox]

I just wanted to type "reserved" so I could follow this, but I'll add a comment:

I'm really enjoying this thread, and looking over the source.  I still think the idea of bounties for flaws is brilliant, because of the care and attention to the code, evidenced here, as a result.  Clearly some of the ideas mentioned by people have been noticed by Come-from-Beyond, so it's win-win as far as I'm concerned.

As an aside, I love that I can now go right to the source to answer questions instead of watching everyone pick CfB's brain.  He can focus on other things now.

[getfun12345]

I somehow doubt it is the original 0.4.7e version minus the alias/Colorcoins stuff:

In original 0.4.7e:

Code:

boolean validateAttachment()
{
    if ((this.amount > 1000000000) || (this.fee > 1000000000)) {
      return false;
 }

I think the right code should be

Code:

boolean validateAttachment()
{
	if ( this.amount + this.fee > 1000000000){
		return false;
	}

[pandaisftw]

this is his motive:

Always going to be a trade off between high liquidity and high growth.

I own BTC (30%) ... high liquidity, can easily go on margin, can trade futures.  most flexible trading vehicle out there.
I own LTC (20%) high liquidity only in BTC-E, however potential BTCChina and mtgox listing.
I own NMC (40%) decent liquidity, normally follows BTC, but higher potential because of low valuation.  2nd highest hash rate among coins, however needs more aggressive dev support. I like it because not a lot of folks have big holdings here.

and for my purely speculative play ... don't day trade this coin... liquidity isn't great at the moment.
I own IXC (10%)

Anyway, do be careful with a lot of the "pump and dump" coins.   You know, the ones with a high mint rate and with hundreds of millions of coins.

busted, your agenda is clear... can't get over the NXT alias system is it?


dont get emotional when trading, you don't have to do this you know

Pin

Wow... busted for having a very conservative crypto currency proposal?

Well, no NXT, Mastercoin, Dogecoin,  SexCoin, Lottocoin, Quark etc.  

I guess I'm only interested in mature proven solutions.

Ok... let me conjure up an agenda since you seem hell bent on finding one.  I am creating a NXT fork.

By "mature proven solutions" you mean bitcoin forks. You obviously have a vested interest in seeing NXT not do better than namecoin, litecoin and bitcoin, the very same coins NXT is poised to overtake (and already has in the case of namecoin).

I still find it ironic how you bash NXT code, yet you want to fork it. If anything, this statement alone proves you're full of it. But knock yourself out, you just have to find the 3 bugs in the source before you release the fork anyways, so I can't complain Get to it!

[FrictionlessCoin]

this is his motive:

Always going to be a trade off between high liquidity and high growth.

I own BTC (30%) ... high liquidity, can easily go on margin, can trade futures.  most flexible trading vehicle out there.
I own LTC (20%) high liquidity only in BTC-E, however potential BTCChina and mtgox listing.
I own NMC (40%) decent liquidity, normally follows BTC, but higher potential because of low valuation.  2nd highest hash rate among coins, however needs more aggressive dev support. I like it because not a lot of folks have big holdings here.

and for my purely speculative play ... don't day trade this coin... liquidity isn't great at the moment.
I own IXC (10%)

Anyway, do be careful with a lot of the "pump and dump" coins.   You know, the ones with a high mint rate and with hundreds of millions of coins.

busted, your agenda is clear... can't get over the NXT alias system is it?


dont get emotional when trading, you don't have to do this you know

Pin

Wow... busted for having a very conservative crypto currency proposal?

Well, no NXT, Mastercoin, Dogecoin,  SexCoin, Lottocoin, Quark etc.  

I guess I'm only interested in mature proven solutions.

Ok... let me conjure up an agenda since you seem hell bent on finding one.  I am creating a NXT fork.

By "mature proven solutions" you mean bitcoin forks. You obviously have a vested interest in seeing NXT not do better than namecoin, litecoin and bitcoin, the very same coins NXT is poised to overtake (and already has in the case of namecoin).

I still find it ironic how you bash NXT code, yet you want to fork it. If anything, this statement alone proves you're full of it. But knock yourself out, you just have to find the 3 bugs in the source before you release the fork anyways, so I can't complain Get to it!

Well,  a fork will only be called for if it the idea actually holds water.   So far,  I see no evidence that it works.

The key questions I've been asking have not been addressed.

I've been given the run around by the original author saying he'll need to consult with this other dude about the spec.

All this looks like make it up as you go.   I have yet to see a spec. of any sort.

[haigent]

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!

You are nitpicking over insignificant details.

This is not a beauty contest.  NXT's beauty is in the algorithms and maths that have brought forward the state-of-the-art.  I seriously doubt the creator cares about keeping tidy with the code.  In fact, I know quite a few genius computer scientists who produce really ugly, stupid code.  However, that doesn't matter one iota when the algorithms are groundbreakingly better than anything that's come before.

Dude...  where are the unit tests?  If you don't care for style,  you should care that it is at least  tested.

This code is so bad, that I'm just itching to fork it!

True

 - Single Java File with 6812 lines,
 - Single class with inner classed,
 - Over or misused static variable,
 - Nxt class is just a servlet,
 - No unit test,

As I said before, the whole thing is crazy. This is not any chance this piece of code could work, excepted generate some block and let your guys buy.

Think twice and good luck!

[haigent]

It seem to be a personal crusade of FrictionlessCoin against nxt  

Have you ever been in a code review?   The criticism I am providing is actually quite tame.

My assessment,  throw this garbage out and start again correctly.

This is a typical kind of response from a good software developer who see crap code.  Fair enough, but I must point out (again) that we are not investing in source code, we are investing in the algorithms, the big ideas (BCNext) and the huge community supporting NXT.

Source code clean up is a minor issue and will come as the project develops.  Some great developers have joined the NXT team (e.g. Jean-Luc) and are improving the software engineering practices as we speak.

However, let us get back to the point.  This is a code review for security issues, so let's try to focus on those.

I think the point about cleaning up, refactoring the code has been made and taken.

So, perhaps FrictionlessCoin has found some non-superficial security issue he would like to share?

Ok... so if you are investing on the algorithm.... then were can I find a specification of the algorithm?  (if you answer look at the source, then it is clear that you have no specification)

Thing's don't always have to start with a spec.  Ya know, this isn't a big software house.

Anything more useful to contribute, if not pure rage?

You don't have a spec. for a distributed consensus algorithm?

So you think you can seriously conjure up one that is secure without actually spending quality time thinking of a specification?


As I said in the beginning,  this is amateur hour.

Come on, we all known he does not have a spec.

How cloud you except a spec from whom wrote such code?

Distributed consensus, how dare you to dream...

[Meizirkki]

FrictionlessCoin & Co: please try to stay constructive and on topic in this thread. If you wish to discuss the competence of developers behind the code, please open another thread for it.

[FrictionlessCoin]

Why doesn't someone just write a unit test for this code instead of trying to pour manually through the code to see if it actually works?

Does anyone have a set of test cases to verify if this code is correct?

Oh, I forgot,  a specification doesn't exist, so a bunch of test cases also likely hard to come by.

Sigh!

[Come-from-Beyond]

what is the reason to add 15 milliseconds in pushBlock method to the current epochtime?

To counteract a time travel attack.

[Come-from-Beyond]

I found a bug, though I don't think it's one of the "big 3"...

in pushBlock() you check timestamp and length, create a Block and increase blockCounter. However, you might discard that block (e.g. if it has an invalid signature, next check) and never call analyze() which would add the block to blocks.
So block.index is not consecutive. I'm not sure what you do with block.index, as far as I can tell, it's only used in JS...

It's used only in JS for the web interface. Later we'll remove it completely.

[Come-from-Beyond]

It looks like there is something strange going on with User.isInactive.

If a user has over 1000 pending responses, all of them are cleared and then he is marked inactive.

If/when he is reactivated, how is he updated to reflect those cleared transactions? The action for "getNewData" is empty.

I'm not sure if I'm on to anything here ...

1000 not received responses mean that a user closed the browser. Responses r cleared to free memory.

[Come-from-Beyond]

Lines 3310 through 3427...


i) if (transaction.timestamp > curTime + 15 || transaction.deadline < 1 || transaction.timestamp + transaction.deadline * 60 < curTime || transaction.fee <= 0 || !transaction.validateAttachment()) {
                  
                  continue;
                  
               }
               

Shouldn't there be a break instead of a continue in the code above?

No. This code gets rid of incorrect and double-spending transactions.

ii) Why are the doubleSpendingTransaction conditions within a static void?  Does this way ensure that double spending transactions will not be included in the block transactions?

Yes.

[nfowest]

I have noticed that this function:

Code:

		static long getId(byte[] publicKey) throws Exception {
			
			byte[] publicKeyHash = MessageDigest.getInstance("SHA-256").digest(publicKey);
			BigInteger bigInteger = new BigInteger(1, new byte[] {publicKeyHash[7], publicKeyHash[6], publicKeyHash[5], publicKeyHash[4], publicKeyHash[3], publicKeyHash[2], publicKeyHash[1], publicKeyHash[0]});
			return bigInteger.longValue();
			
		}

returns a negative account id around 50% of the times.
I don't know if it is a flaw or I am doing something wrong:
for example
secretphrase:

Code:

fasdfdsa

publickey:

Code:

[B@756a162a

id:

Code:

-2508783372671837694

<-------MY TEST
instead of id:

Code:

15937960701037713922

<-------0.4.8 release

BUT in the case of
secretphrase:

Code:

fasdfdsaf

publickey:

Code:

[B@2cd80b2a

id:

Code:

3772219410562810950

like in the 0.4.8 release:

Code:

3772219410562810950

[Come-from-Beyond]

I am posting this on behalf of this dude on IRC that does not have a bitcointalk account:

There might be a bug here, however, i humle about this since my knowledge about java is limited. And i am sure that there is some fancy thing about this, and i have missed it somewhere.
 The parseInt function produces an integer value dictated by interpretation of the contents of the string argument according to the specified radix. Leading white space in string is ignored. If radix is undefined or 0, it is assumed to be 10 except , when the numbers in the most significant bit is 0 or 0x. If it starts with 0 it will interprete the values (1-7 and 10) as octal numbers which can result in some unwanted amounts.

(Line 6107 among other): amount = Integer.parseInt(amountValue.trim());

Also, it seems that this will not throw an exception as the logic is today.. so the value will be processed.

Therefore you should specify the radix as a second argument to parseInt -function.
Regards

j0b / operator at #nxtalk on freenode
NXT: 13570469120032392161

Thank u. This is not an injected flaw.

[Come-from-Beyond]

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.

[Come-from-Beyond]

I somehow doubt it is the original 0.4.7e version minus the alias/Colorcoins stuff:

In original 0.4.7e:

Code:

boolean validateAttachment()
{
    if ((this.amount > 1000000000) || (this.fee > 1000000000)) {
      return false;
 }

I think the right code should be

Code:

boolean validateAttachment()
{
	if ( this.amount + this.fee > 1000000000){
		return false;
	}

No, try to use Integer.MAX_VALUE for amount or fee.

[Come-from-Beyond]

I have noticed that this function:

Code:

		static long getId(byte[] publicKey) throws Exception {
			
			byte[] publicKeyHash = MessageDigest.getInstance("SHA-256").digest(publicKey);
			BigInteger bigInteger = new BigInteger(1, new byte[] {publicKeyHash[7], publicKeyHash[6], publicKeyHash[5], publicKeyHash[4], publicKeyHash[3], publicKeyHash[2], publicKeyHash[1], publicKeyHash[0]});
			return bigInteger.longValue();
			
		}

returns a negative account id around 50% of the times.
I don't know if it is a flaw or I am doing something wrong:
for example
secretphrase:

Code:

fasdfdsa

publickey:

Code:

[B@756a162a

id:

Code:

-2508783372671837694

<-------MY TEST
instead of id:

Code:

15937960701037713922

<-------0.4.8 release

BUT in the case of
secretphrase:

Code:

fasdfdsaf

publickey:

Code:

[B@2cd80b2a

id:

Code:

3772219410562810950

like in the 0.4.8 release:

Code:

3772219410562810950

Look at line 144. That method converts negative values to unsigned 64-bit ones.

[FrictionlessCoin]

Huh?

You mean to tell me that every peer has exposed its wallet to the public internet?

[Come-from-Beyond]

Huh?

You mean to tell me that every peer has exposed its wallet to the public internet?

No.

Btw, have u read the code or u just guess?

[Come-from-Beyond]

Is 1048576 also a magic number?

Yes. But I'm not allowed to reveal its sacral meaning.

By the way... can you explain lines 3504 through 3520?  Much appreciated!  

This piece of code invalidates transactions with incorrect type/subtype.