Fix trivial underflow/overflow issues#940
Conversation
|
I've assigned @tnull as a reviewer! |
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
| &self, amount_sats: u64, description: &str, expiry_sec: u32, | ||
| ) -> Result<String, Error> { | ||
| let onchain_address = self.onchain_payment.new_address()?; | ||
| let amount_msats = amount_sats.checked_mul(1_000).ok_or(Error::InvalidAmount)?; |
There was a problem hiding this comment.
Rather than doing this here, shouldn't we rather do the check in BOLT11/BOLT12 receive methods, as this method is just meant to forward to them, with as little custom logic on top as possible. Apart from that we might also be okay to punt on it as we intend to switch to an LightningAmount type soon everywhere, that should enforce such checks at conversion time across the board.
There was a problem hiding this comment.
If we wanted to do this check in the bolt11/12 functions then we'd have to have those take sats rather than msats. I think this is the right location for this. LightningAmount is the correct choice eventually.
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Please also include This finding was discovered by Project Loupe in the commit messages, just so we can keep track of how many reported things were fixed.
benthecarman
force-pushed
the
over-underflows
branch
from
66a8c03 to
07a0e78
Compare
Return InvalidAmount when converting the requested satoshi amount to millisatoshis would overflow. This keeps debug and release behavior consistent and avoids producing a URI whose on-chain amount differs from its Lightning payment amount. This commit was created with assistance from OpenAI Codex. This finding was discovered by Project Loupe
Use saturating arithmetic when accounting for skimmed JIT-channel fees while validating manually claimed payments. This prevents an oversized skimmed fee from underflowing the expected claimable amount. This commit was created with assistance from OpenAI Codex. This finding was discovered by Project Loupe
Track registered transaction IDs in a set so repeated filter registrations do not grow the collection or slow block-connected checks. This keeps the wallet's registered-transaction lookup bounded by unique transaction IDs. This commit was created with assistance from OpenAI Codex. This finding was discovered by Project Loupe
benthecarman
force-pushed
the
over-underflows
branch
from
07a0e78 to
4a449fc
Compare
3 participants
A bunch of small issues found by project-loupe. Can drop any commits that we don't think are worth it (the electrum fee one might be a bit much).