(as reported by Sergio Damien Lerner)
Bitcoin amounts are stored as uint64 in the protobuf messages. Amounts over 0x7FFFFFFFFFFFFFFF will be considered negative when this line is executed:
QList<std::pair<CScript, CAmount> > sendingTos = request.getPayTo();
(in PaymentServer::processPaymentRequest() in qt\paymentserver.cpp)
This does not pose a problem, since negative values should be discardedby the dust check : if (txOut.IsDust(::minRelayTxFee))
Nevertheless recipient.amount will still accumulate all positive values and may overflow, since the payment protocol allows the merchant to specify many output addresses and each one has a separate amount. So it is still possible to create a negative total amount by overflowing the int64 range with several positive amounts. For example, (uint64) -1 + (uint64) -1 + 3 == 1. The user will be presented with a dialog asking to pay a total of 1 BTC, but internally the operation will fail, because the independent constituent payment amounts will not be able to be fulfilled.
This has the nasty effect that the user may think he may have been robbed, because even if the user has 100 BTC, the software will tell the user "The amount exceeds your balance.".
Correction:
bool PaymentServer::processPaymentRequest(PaymentRequestPlus& request,
SendCoinsRecipient& recipient) {
....
CTxOut txOut(sendingTo.second, sendingTo.first);
// BEGIN ADD
if (!MoneyRange(sendingTo.second)) {
emit message(tr("Payment request rejected"),
tr("Invalid money range."),
CClientUIInterface::MSG_ERROR);
return false;
}
// END ADD
....
recipient.amount += sendingTo.second;
...
// BEGIN ADD
if (!MoneyRange(recipient.amount)) {
emit message(tr("Payment request rejected"),
tr("Invalid accumulated money range."),
CClientUIInterface::MSG_ERROR);
return false;
}
// END ADD
The payment request unit test should be extended to test negative and very large amounts in requests....
