Nxt source code flaw reports - page 58.

xibeijan

legendary

Activity: 1232

Merit: 1001

Quote from: bitcool on January 03, 2014, 08:30:25 PM

Quote from: FrictionlessCoin on January 03, 2014, 04:35:10 PM

So your are admiting that this indeed is not the 'real source code' but some variant of it that doesn't really work.

I thought the purpose of releasing source code is to have other people to review if it is correct.

Well... I may just create a much better NXT. Something that

(1) Has a very clear specification of the distributed consensus algorithm that people can review for flaws.
(2) Follow best practice Java coding standards.
(3) Gone through extensive static code analysis.
(4) Have a battery of unit tests to exhaustive test out the code.
(5) Ensure that tests perform 100% test coverage.

but unfortunately none of that exists for NXT. It is just a high school project that some folks invested 21 BTC to get a stake on it.

I agree w/ most of your points, but if the code quality is not good, any flaws can be exploited, you don't have find the one he injected.

Next few weeks will be interesting to watch, I keep my fingers crossed for those with high stakes in NXT.

I am really looking forward to the next few weeks too because hopefully

1) Any flaws and bugs will get fixed
2) The community will gain assurance about the viability of the NXT platform

bitcool

legendary

Activity: 1441

Merit: 1000

Live and enjoy experiments

Quote from: FrictionlessCoin on January 03, 2014, 04:35:10 PM

So your are admiting that this indeed is not the 'real source code' but some variant of it that doesn't really work.

I thought the purpose of releasing source code is to have other people to review if it is correct.

Well... I may just create a much better NXT. Something that

(1) Has a very clear specification of the distributed consensus algorithm that people can review for flaws.
(2) Follow best practice Java coding standards.
(3) Gone through extensive static code analysis.
(4) Have a battery of unit tests to exhaustive test out the code.
(5) Ensure that tests perform 100% test coverage.

but unfortunately none of that exists for NXT. It is just a high school project that some folks invested 21 BTC to get a stake on it.

I agree w/ most of your points, but if the code quality is not good, any flaws can be exploited, you don't have find the one he injected.

Next few weeks will be interesting to watch, I keep my fingers crossed for those with high stakes in NXT.

Jaguar0625

sr. member

Activity: 299

Merit: 250

In the "sendMoney" action in doGet, only (amount <= 0) and (fee <= 0) are being validated before they are checked against unconfirmedBalance:

Code:

(amount + fee) * 100L > account.unconfirmedBalance

Due to the order of operations, (amount + fee) should be evaluated as an int before * 100L upcasts it to a long. As a result, (amount + fee) can overflow and a transaction will be created by someone to spend phantom money.

[Note that this is in the second switch, the first switch validates the values are less than 1B. But, the first switch is bypassed when userPasscode is not null].

This transaction apparently is then trusted by the system (i.e. fee and amount are trusted and not validated again) because Transaction.processTransactions just loops over an array of transactions and deserializes them using Transaction.getTransaction. Neither of those functions does any validation aside from ensuring fee is positive (Transaction.verify is called but it just verifies the signatures, which should be correct).

And, then it does the same addition, which can overflow:

Code:

transaction.amount + transaction.fee

The result is that if the overflow is to a negative number, (account.unconfirmedBalance < amount * 100L) will be false and the unconfirmed balance will be incremented by the sender.

Code:


int amount = transaction.amount + transaction.fee;
synchronized (account) {
	
	if (account.unconfirmedBalance < amount * 100L) {
		
		doubleSpendingTransaction = true;
		
	} else {
		
		doubleSpendingTransaction = false;
		
		account.setUnconfirmedBalance(account.unconfirmedBalance - amount * 100L);
		
	}
	
}

Unless I missed something ...

safersecs

newbie

Activity: 5

Merit: 0

Quote from: FrictionlessCoin on January 03, 2014, 08:06:23 PM

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.

Lol, litecoin is nowhere near mature (incentive in the first place? nothing than profit) or proven (scrypt) and while I see some serious issues with nxt yet I am ready to bet it'll survive litecoin.

Also, your (1) and (2) are quite the same. And if you give it a few days, I guess we'll be able to anwer that.

IveBeenBit

sr. member

Activity: 449

Merit: 250

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

Quote

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

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Looks like nobody can answer:

(1) How do you ensure "chronological order in the block chain".
(2) What are these "strict cryptographic rules"
(3) What is the mechanism of this "lottery".

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Quote from: pinarello on January 03, 2014, 07:48:24 PM

this is his motive:

Quote from: FrictionlessCoin on December 09, 2013, 03:07:45 PM

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 Wink

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.

pinarello

full member

Activity: 266

Merit: 100

NXT is the future

Quote from: klee on January 03, 2014, 04:50:33 PM

Quote from: Vega on January 03, 2014, 04:45:01 PM

Quote from: xibeijan on January 03, 2014, 04:36:23 PM

And, the devs really need to get a whitepaper together.

Among other things. As someone who has a nice amount of money invested in Nxt, I'm not that happy this days about how many thing going with Nxt.
But screaming at the devs without offering anything constructive, is less than pointless, unless he have an agenda.
And if I remember correctly, it's not even the first time he seen the code, he posted last week on btt and on reddit already about it, after he decompiled it.
So this is pretty much rehearsed bashing.

Secret agenda - who are you FREAKtionless? What is your motives?
Serving the 'truth'?? lol

Go back to your worthless dinosaur soon to extinct (PoW) investment...

this is his motive:

Quote from: FrictionlessCoin on December 09, 2013, 03:07:45 PM

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 Wink

Pin
[/quote][/b][/b]

safersecs

newbie

Activity: 5

Merit: 0

Hello,
though my understanding is yet limited I think it's continue and not break because it will just ignore the currently processed transaction and go on with the next one. IIRC the part you quoted is from the part where it walks the transactions and thus it needs to go on with the next one even if the current one can be dropped.
Anyway, thanks for correcting me already CfB Cheesy

Jaguar0625

sr. member

Activity: 299

Merit: 250

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

ricot

newbie

Activity: 56

Merit: 0

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

BaiMangal

member

Activity: 111

Merit: 10

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

Come-from-Beyond

legendary

Activity: 2142

Merit: 1010

Newbie

Guys, ignore code from line 386 to line 549. It was supposed to be removed, it's for advanced features.

Anon136

legendary

Activity: 1722

Merit: 1217

Quote from: FrictionlessCoin on January 03, 2014, 04:35:10 PM

Quote from: pandaisftw on January 03, 2014, 04:29:54 PM

I normally don't call people trolls, but:

1) It is clear you have not been following anything related to NXT. Injected flaws (in an otherwise working base code) with bounty payouts is meant to encourage people to take a very hard look a the code. Also, read the 1st post again, c-f-b explained this.
2) Bashing how "clean" the code looks. Really? If you're so good at this, why didn't you come up with a better NXT first? It's probably because you can't (hence you have to fork NXT's code... how ironic).

So your are admiting that this indeed is not the 'real source code' but some variant of it that doesn't really work.

I thought the purpose of releasing source code is to have other people to review if it is correct.

Well... I may just create a much better NXT. Something that

(1) Has a very clear specification of the distributed consensus algorithm that people can review for flaws.
(2) Follow best practice Java coding standards.
(3) Gone through extensive static code analysis.
(4) Have a battery of unit tests to exhaustive test out the code.
(5) Ensure that tests perform 100% test coverage.

but unfortunately none of that exists for NXT. It is just a high school project that some folks invested 21 BTC to get a stake on it.

yes please do! that would be wonderful! the more competition the better. i would definitely get behind this and even throw some money at it.

bitcoinpaul

hero member

Activity: 910

Merit: 1000

Quote from: FrictionlessCoin on January 03, 2014, 04:05:59 PM

If he knews of the flaws, then why is he asking this forum?

How do you create a hash of something you don't know exists?

Besides, what the heck are you even hashing? Some text that describes the flaw?

The B.S. is unbelievable and you folks are just too ignorant to see it.

I'm surprised at his stupidity.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1010

Newbie

Quote from: ?? on ??

For example, line 63 - initialBaseTarget = 153722867!?!?!

This is a magic number. 1 billion coins generate blocks at 1 block/min rate when BaseTarget == 153722867.

instacalm

hero member

Activity: 798

Merit: 500

FrictionlessCoin, this thread is clearly all about finding and reporting the flaws that are hidden in the code and then you come along babbling about how amateurish NXT is in your mind, what the hell is wrong with you?

Don't be mad and please don't get on other user's nerves. Thank you, highly appreciated. Always looking forward to productive comments.

Don't blabla... DO.

xibeijan

legendary

Activity: 1232

Merit: 1001

Quote from: Vega on January 03, 2014, 04:45:01 PM

Quote from: xibeijan on January 03, 2014, 04:36:23 PM

And, the devs really need to get a whitepaper together.

Among other things. As someone who has a nice amount of money invested in Nxt, I'm not that happy this days about how many thing going with Nxt.
But screaming at the devs without offering anything constructive, is less than pointless, unless he have an agenda.
And if I remember correctly, it's not even the first time he seen the code, he posted last week on btt and on reddit already about it, after he decompiled it.
So this is pretty much rehearsed bashing.

Hopefully we can get back to security / code review related posts now.

Funny thing, all of this talk about ugly code reminded me of a phrase I once read. "A Foolish Consistency is the Hobgoblin of Little Minds"

utopianfuture

sr. member

Activity: 602

Merit: 268

Internet of Value