Bitcoin Forum
March 19, 2024, 05:05:01 AM *
News: Latest Bitcoin Core release: 26.0 [Torrent]
 
   Home   Help Search Login Register More  
Pages: [1]
  Print  
Author Topic: Dangerous bug in current client  (Read 3435 times)
Forp (OP)
Full Member
***
Offline Offline

Activity: 195
Merit: 100


View Profile
June 18, 2011, 11:08:03 PM
 #1

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

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 !



1710824701
Hero Member
*
Offline Offline

Posts: 1710824701

View Profile Personal Message (Offline)

Ignore
1710824701
Reply with quote  #2

1710824701
Report to moderator
According to NIST and ECRYPT II, the cryptographic algorithms used in Bitcoin are expected to be strong until at least 2030. (After that, it will not be too difficult to transition to different algorithms.)
Advertised sites are not endorsed by the Bitcoin Forum. They may be unsafe, untrustworthy, or illegal in your jurisdiction.
fpgaminer
Hero Member
*****
Offline Offline

Activity: 560
Merit: 517



View Profile WWW
June 19, 2011, 12:01:24 AM
 #2

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
Hero Member
*****
Offline Offline

Activity: 527
Merit: 500


View Profile
June 19, 2011, 12:07:16 AM
 #3

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
Hero Member
*****
Offline Offline

Activity: 560
Merit: 517



View Profile WWW
June 19, 2011, 12:12:35 AM
 #4

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 Wink 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.
Member
**
Offline Offline

Activity: 84
Merit: 10


View Profile WWW
June 19, 2011, 12:22:50 AM
 #5

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

I have linked this forum post for the developers.

--

If you like what I've written here, consider tipping the messenger:
1GZu4CtHa6ai8iWoWiVFxV5VVoNte4SkoG

If you don't like what I've written, send me a Tip and I'll stop talking.
gim
Member
**
Offline Offline

Activity: 90
Merit: 10


View Profile
June 19, 2011, 12:42:16 AM
 #6

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
Hero Member
*****
Offline Offline

Activity: 938
Merit: 500


https://youengine.io/


View Profile WWW
June 19, 2011, 03:02:19 PM
 #7

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

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

Luke-Jr
Legendary
*
expert
Offline Offline

Activity: 2576
Merit: 1186



View Profile
June 19, 2011, 05:56:55 PM
 #8

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
Newbie
*
Offline Offline

Activity: 42
Merit: 0


View Profile
June 19, 2011, 06:28:00 PM
 #9

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
Hero Member
*****
qt
Offline Offline

Activity: 812
Merit: 1022

No Maps for These Territories


View Profile
June 19, 2011, 07:09:38 PM
 #10

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.

Bitcoin Core developer [PGP] Warning: For most, coin loss is a larger risk than coin theft. A disk can die any time. Regularly back up your wallet through FileBackup Wallet to an external storage or the (encrypted!) cloud. Use a separate offline wallet for storing larger amounts.
gim
Member
**
Offline Offline

Activity: 90
Merit: 10


View Profile
June 19, 2011, 09:19:51 PM
 #11

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
Legendary
*
qt
Offline Offline

Activity: 1652
Merit: 2164


Chief Scientist


View Profile WWW
June 20, 2011, 01:39:32 AM
 #12

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.

How often do you get the chance to work on a potentially world-changing project?
prof7bit
Hero Member
*****
Offline Offline

Activity: 938
Merit: 500


https://youengine.io/


View Profile WWW
June 20, 2011, 03:13:29 PM
 #13

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
Full Member
***
Offline Offline

Activity: 129
Merit: 118


View Profile
June 20, 2011, 03:46:53 PM
 #14

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
Hero Member
*****
qt
Offline Offline

Activity: 812
Merit: 1022

No Maps for These Territories


View Profile
June 20, 2011, 03:47:28 PM
Last edit: June 20, 2011, 07:36:43 PM by John Smith
 #15

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.

Quote
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

Bitcoin Core developer [PGP] Warning: For most, coin loss is a larger risk than coin theft. A disk can die any time. Regularly back up your wallet through FileBackup Wallet to an external storage or the (encrypted!) cloud. Use a separate offline wallet for storing larger amounts.
Dansker
Hero Member
*****
Offline Offline

Activity: 740
Merit: 500


Hello world!


View Profile
June 20, 2011, 06:20:23 PM
 #16

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

Gavin Andresen
Legendary
*
qt
Offline Offline

Activity: 1652
Merit: 2164


Chief Scientist


View Profile WWW
July 04, 2011, 01:51:43 PM
 #17

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

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

How often do you get the chance to work on a potentially world-changing project?
hello_good_sir
Hero Member
*****
Offline Offline

Activity: 1008
Merit: 531



View Profile
July 04, 2011, 03:08:10 PM
 #18

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
Legendary
*
Offline Offline

Activity: 1232
Merit: 1077


View Profile
July 04, 2011, 03:39:29 PM
 #19

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

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.

1LxbG5cKXzTwZg9mjL3gaRE835uNQEteWF
Rob P.
Member
**
Offline Offline

Activity: 84
Merit: 10


View Profile WWW
July 05, 2011, 12:43:09 PM
 #20

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.

--

If you like what I've written here, consider tipping the messenger:
1GZu4CtHa6ai8iWoWiVFxV5VVoNte4SkoG

If you don't like what I've written, send me a Tip and I'll stop talking.
Pages: [1]
  Print  
 
Jump to:  

Powered by MySQL Powered by PHP Powered by SMF 1.1.19 | SMF © 2006-2009, Simple Machines Valid XHTML 1.0! Valid CSS!