alexbosworth / ln-service

Node.js interface to LND
MIT License
319 stars 60 forks source link

Creating invoice with big amount leads to invoice without an amount #163

Open adambor opened 2 years ago

adambor commented 2 years ago

When I create a hodl invoice by:

    const secret = crypto.randomBytes(32);
    console.log("Generated secret preimage: "+secret.toString("hex"));
    const invoiceId = crypto.createHash('sha256').update(secret).digest('hex');

    const invoice = await lncli.createHodlInvoice({
        lnd,
        tokens: "104500000000000000000000000000000000000000000000000000000000000000000000000000000000000",
        id: invoiceId
    });

    console.log(invoice);

Following response is returned (payment request without an amount, but tokens returned are 1.045e+86):

{
  chain_address: undefined,
  created_at: '2022-08-12T13:56:24.000Z',
  description: undefined,
  id: 'f222dbd0c96195e2b6513a6cccd7a835b1d828e1d72300e73fe14cda5b8edc21',
  mtokens: '104500000000000000000000000000000000000000000000000000000000000000000000000000000000000000',
  request: 'lnbc1p30vhqgpp57g3dh5xfvx279dj38fkve4agxkcas28p6u3speelu9xd5kuwmsssdqqcqzpgxqyz5vqsp5f6rxyrgzp6w6rcpq3dh59vk368cnjvnyugn4y3m6hm2ymzq267xs9qyyssq3h3d9f8wxhk5kf0mwds3t5udjqzywvq5sw2hqjq4ztmgfvlsh0kpfa504
esknpmdt9rfn7kkhhvkthvwzgczumqqh2qehn5yaft0gqqqf3478x',
  secret: undefined,
  tokens: 1.045e+86
}

If I use createInvoice the response then is (at least the tokens returned are 0):

{
  chain_address: undefined,
  created_at: '2022-08-12T13:59:43.000Z',
  description: undefined,
  id: '336400e8539dacf95677d8aabfba1d8751478dbaef61ea465f702a2a28f4168c',
  mtokens: '0',
  payment: 'c5e5f07d3bb4f5517f25da66502f74eb97ba2db9545f959c3b33dc6e3c8ca39c',
  request: 'lnbc1p30vhx0pp5xdjqp6znnkk0j4nhmz4tlwsasag50rd6aas753jlwq4z5285z6xqdqqcqzpgxqr23ssp5chjlqlfmkn64zle9mfn9qtm5awtm5tde230et8pmx0wxu0yv5wwq9qyyssqntchagjhewgk7ef4z4ncuk26v273kyaflvsz2n086rgcursnxc4juxwfz9
9q5hv4q45mk2yxjkqqgzk3tnjx72slkvjzplhksaqdvlsqa6e034',
  secret: '73fdcef61a5b3a1086567c9f622ae35f1f25840483b4ab076c25c893b70772fa',
  tokens: 0
}

However this might lead to some bugs in implementation (like it happened for me) expecting that when the invoice is paid I should increment the user's balance by 1.045e+86, even though the user might've just paid 1 sat.

I would expect the lib to throw an error in such a case, and not an invoice without an amount.

alexbosworth commented 2 years ago

invoices without amounts are used for overpayment purposes, that's part of the LN protocol

if you want to see how much was paid, please use getInvoice or subscribeToInvoice

technically speaking in LN protocol any invoice can be overpaid

adambor commented 2 years ago

Yep, I get that, and that's how I fixed the issue. I just think that when you specify a non-zero amount for an invoice, that function call should either return the invoice of that non-zero amount or throw an error, not return an invoice without any amount... In this case it can result in underpayment, not overpayment.

alexbosworth commented 2 years ago

is the problem that you used an incorrect number for the amount? it should be a JS number not a string

adambor commented 2 years ago

Yep, using string instead of the number, so that looks like it is a source of the issue. Very much unpredictable with strings, for smaller amounts it works, for larger it overflows and tries to create negative value invoice, and for super large ones it creates an invoice without an amount. Maybe there should be a type check? I am just trying to make sure no one else falls for this, because this resulted in some amount of BTC getting stolen from me.

alexbosworth commented 2 years ago

Yeah I think it could type check but this is a general issue with JS in general that large numbers are undefined in their behavior and types aren't built in to the language

There is also typescript support that you could look at, I don't use that myself but potentially it can help with type enforcement

adambor commented 2 years ago

Yep, those pesky JS numbers, that's also why I am not using them at all and rather resort to js-big-decimal library and that's also a reason I am passing that number as string instead of number. Will be looking to move the whole codebase to typescript soon. But I think that simple type check might save someone some money :)

alexbosworth commented 2 years ago

I think another approach is to specify the amount as mtokens, this takes a string and that's how you can specify large numbers

alexbosworth commented 2 years ago

I added validation of tokens to be numeric here https://github.com/alexbosworth/lightning/commit/7bd1794cf0c5bdc61e0c1541651907ccc7795bf4

But the real way to stop problems is to totally lock down user input and that has to live at the application level