is this code always using the amount of the the first OwnedTxOutput?

[spirobel]

@busyboredom can you explain this line? why is it not transfer.amount() https://github.com/busyboredom/acceptxmr/blob/104a6b68bb21a856141d6b32cb40d35d8fe13acf/src/scanner.rs#L372 the code is looping over the transfers, but seems to always use the amount of the first OwnedTxOutput in the Transaction.

            for transfer in &transfers {
                let sub_index = SubIndex::from(transfer.sub_index());

                // If this invoice is being tracked, add the amount and subindex to the result
                // set.
                if self
                    .invoice_store
                    .contains_sub_index(sub_index)
                    .map_err(AcceptXmrError::InvoiceStorage)?
                {
                    let amount = OwnedAmount {
                        sub_index,
                        amount: transfers[0]
                            .amount()
                            .ok_or(AcceptXmrError::<S::Error>::Unblind(sub_index))?,
                    };
                    amounts_received
                        .entry(tx.hash())
                        .or_insert_with(Vec::new)
                        .push(amount);
                }
            }
        }

[busyboredom]

Hmm, I think you've spotted a bug. Looks like it's only working because it's so rare for two outputs to be addressed to the same recipient in a given transaction.

I'll add an integration test to cover the case where there are multiple owned outputs in one transaction so this mistake can't happen again.

Will fix this and see about getting a patch release done for this soon, thank you for making an issue!

[spirobel]

Will fix this and see about getting a patch release done for this soon, thank you for making an issue!

Great! I think it is a good idea to address this. The potential impact is that someone crafts a transaction with 1 XMR as the first output and fills the other 15 possible outputs with 1 piconero each. So it would look like the attacker paid 16 XMR when in reality he paid 1 XMR.

(But you can pay invoices with multiple transactions, right? The scan_transactions function is called from scan_blocks which is called by the scan function: https://github.com/busyboredom/acceptxmr/blob/104a6b68bb21a856141d6b32cb40d35d8fe13acf/src/scanner.rs#L266 and the new invoice amount is calculated in the main scan function: https://github.com/busyboredom/acceptxmr/blob/104a6b68bb21a856141d6b32cb40d35d8fe13acf/src/scanner.rs#L176 so the limiting factor to the discount is the transaction fee.)

[busyboredom]

Yep, definitely something to fix quickly. Thanks again for raising it.

In the future, I'd appreciate it if issues like this could be reported in private to minimize the chance of them being exploited before they are patched. You can reach me on matrix at @busyboredom:monero.social, or through github's vulnerability reporting feature.

I'm very glad you caught this!