Dangerous bug in current client

[Forp]

In the german part of the forum we currently are discussing a very delicate, dangerous bug in the GUI.

If you enter 0,005 as amount to be sent, the client sends 5.00

For the US-only localized guys I must add: 0,005 is, for example, in Germany the natural way to type what in the US would be typed as 0.005 - and this is consistent with all kinds of localizations in the operating system.

So a German user is likely to send a much higher amount than she intended to. 

We have the bug confirmed on the 0.3.23-beta client on Windows 7 by Dennis1234 and on the 0.3.23-beta client on Linux self compilation by mself. Since my client is running in testnet currently I did some testing:

0,0005 is parsed as "error in amount"
0,005 is reparsed as 5.00
0,05 produces an "error in amount"
0,5 produces an "error in amount"

Reparsed as 5.00 means the following:

I enter 0,005 and upon "Send" the displayed amount changes into 5.00 and I get an error on insufficient funds (I do not have 5 BTC in my current testnet account). From the normal behaviour of the client I assume that, if I had more than 5.00 i would just lose these 5.00.

I am not into wxWidgets programming otherwise I would do it myself. I think it is VERY important that we get that fixed ASAP !

[fpgaminer]

Useful dev information:

The bug is in src/util.cpp:ParseMoney. In particular, on line 375 the function explicitly checks for ',' and interprets it as the American/English (or other?) digit grouping symbol. It checks if there is a digit before the comma, and three digits after it (e.g. One Million Dollars can be written as $1,000,000).

Because of this 0,005 is considered valid, even though it makes no sense even from an a digit grouping perspective. I'd say several patches are in order:

1) 0,005 should not be considered valid when interpreted as digit grouping
2) ParseMoney should be locale sensitive
3) OnButtonSend should display the valid it interpreted (after line 1925, nValue) and ask for confirmation.

3 is necessary because bugs like this are likely to crop up for a long time, and it's just all around a good idea to give the user a second chance to verify their input.

[randomguy7]

Additionally, like mentioned by some one in the german subforum, we could split the entry field into 2 entry fields with a fixed , or . between them.

[fpgaminer]

I don't know if ThreadSafeMessageBox is safe to use inside of CSendDialog::OnButtonSend, inside the cs_sendlock critical block. A real Bitcoin dev will have to do the heavy lifting there But for what it's worth, I wrote the following off-hand:

Code:

bool ThreadSafeConfirmSendMoney(int64 nAmount, const string& strDestination, const string& strCaption, wxWindow* parent)
{
	if (fDaemon)
		return true;
	
	string strMessage = strprintf(
        _("About to Send %s to %s."
          "Is this correct?"),
        FormatMoney(nAmount).c_str(), strDestination.c_str());
	
	return (ThreadSafeMessageBox(strMessage, strCaption, wxYES_NO, parent) == wxYES);
}

and can potentially be called after the nValue checks, before parsing the bitcoin address, like so:

Code:

if (!ThreadSafeConfirmSendMoney(nValue, strAddress, _("Sending..."), this))
{
	return;
}

[Rob P.]

Reported as Issue #330:
https://github.com/bitcoin/bitcoin/issues/330

I have linked this forum post for the developers.

[gim]

3 is necessary because bugs like this are likely to crop up for a long time, and it's just all around a good idea to give the user a second chance to verify their input.

Yes!
I was shocked the first time I saw that the transaction was generated without confirmation.

Edit: even bitcoind send* should ask for confirmation somehow, and use sequence numbers to prevent disasters in case of duplicated or looping calls.
 

[prof7bit]

Why not use already existing helpers instead of reinventing the wheel?

http://docs.wxwidgets.org/trunk/classwx_number_formatter.html

[Luke-Jr]

At least one developer of the Satoshi client has in the past expressed that they intentionally standardized the formatting on the "US" form, to ideally eliminate the ambiguity issues. If this is to be kept the case, "0,005" should probably be an error (ie, forbid leading zeros)

[de_bert]

At least one developer of the Satoshi client has in the past expressed that they intentionally standardized the formatting on the "US" form, to ideally eliminate the ambiguity issues. If this is to be kept the case, "0,005" should probably be an error (ie, forbid leading zeros)

That would still allow for stuff like 5,005, which would then be parsed as 5005.

I, for my part, think that even allowing digit grouping is not wise, and the comma could be forbidden completely (to avoid ambiguities for people wanting to use digit grouping).

[wumpus]

I also stumbled on this during development of my Qt GUI. First I made it use the standard number parsing functions of Qt, which take the locale into account. Then I noticed that the official client explicitly defaults to US number format (which makes sense for uniformity) so simply I took that over.

Additionally, like mentioned by some one in the german subforum, we could split the entry field into 2 entry fields with a fixed , or . between them.

I've thought about this as well. Simply force the '.' in the middle and regard it as two fields, one right aligned and the other left-aligned. Pressing the '.' while in the first field will jump to the second field.

I'm going to implement this and see whether it is useful.

[gim]

I've thought about this as well. Simply force the '.' in the middle and regard it as two fields, one right aligned and the other left-aligned. Pressing the '.' while in the first field will jump to the second field.

I'm going to implement this and see whether it is useful.

Best thing to do IMHO. Great!

[Gavin Andresen]

I, for my part, think that even allowing digit grouping is not wise, and the comma could be forbidden completely (to avoid ambiguities for people wanting to use digit grouping).

I agree; I think allowing commas in numbers is a bitcoin GUI mis-feature.

[prof7bit]

I don't think fixing the number entry to US notation only is a good decision for a software that is intended to be used outside the US also. Numbers should be entered in the same format they are entered in all other software on the user's computer and this depends on the locale.

I don't see why this could be of any concern or reason for any discussion at all, there are standard library functions to parse strings to floats and format floats to strings according to the user's locale. They should simply be used, just like all other software uses them too. I don't see where the problem is.

[sebastian]

Also the locale on the computer can be incorrectly set, so thats an bad idea too.

I think the best idea would be:
Disallow multiple ,/. completely.
Allow only one single , OR a . in a amount, and treat it as decimal comma.

so:
1,000 = sends one BTC
1.000 = sends one BTC
1,000,000 = invalid
1.000.000 = invalid
1.000,000 = invalid
1,000.000 = invalid
0,1 = sends one tenth of a BTC
0.1 = sends one tenth of a BTC


Thats much better, since if a US person accidentially enter for example 3,000 as digit grouping to sent three tousands of BTC, that would fail on the "good" side since only 3 BTC is sent. The person can easly correct this by sending 2997 BTC more.

So thats a good fix to this bug.
I think using locale-specific functions can be dangerous in financial software at all, imagine someone who installs a US version of windows or imagine a swedish person visiting a US internet cafe with a USB stick with bitcoin on it.

[wumpus]

There's another reason we can't use the built-in parsing functions of Qt/Wx: for precision, coin amounts are not doubles, but fixed-point integers.

I agree with GA, number grouping characters should be disabled in number entry. It should either be automatic or not used at all. There is no reason to allow manually entering them.

For display purposes, on the other hand, they are useful because they aid in counting zeroes.

Allow only one single , OR a . in a amount, and treat it as decimal comma.

Or even better: use the two-fields solution, and make both , and . skip to the second field. That makes it clear to the user that decimals are being entered.

Edit: I've just committed this to bitcoin-qt

[Dansker]

This seems to reinforce the notion that the decimal point needs to be moved...

[Gavin Andresen]

https://github.com/bitcoin/bitcoin/pull/379

I think this is critical enough to include in 0.3.24rc2.

[hello_good_sir]

This seems to reinforce the notion that the decimal point needs to be moved...

I agree.  Unless the decimal point is handled property (moved) bitcoin will never take off.  The ideal solution would be to move it two places now and move it one place at a time after that (to keep the value always worth less than a dollar).  An adequate solution would be to move it all the way over, so that there is no such thing as a fractional bitcoin.

[TierNolan]

I agree.  Unless the decimal point is handled property (moved) bitcoin will never take off.  The ideal solution would be to move it two places now and move it one place at a time after that (to keep the value always worth less than a dollar).  An adequate solution would be to move it all the way over, so that there is no such thing as a fractional bitcoin.

At 50% increase in value per year, $1 will be equal to the minimum bitcoin value in 26 years.

Has there been any consideration for this?  Ofc, it is unlike that it would keep increasing by that amount per year .

21 million bit coins by 100 million units = $2100 trillion, which is a lot more than the world GDP.

The number if an 64 bit number, but the spare bits mean that it can go higher rather than lower.

[Rob P.]

https://github.com/bitcoin/bitcoin/pull/379

I think this is critical enough to include in 0.3.24rc2.

Thanks Gavin, nice work.  Appreciate your dedication to the client.