invoice-canister
invoice-canister copied to clipboard
dfinity
[SEC-F05] TOCTOU in verify_invoice
Observation
The balance could have changed by the time of the transfer out and updating of the invoice.
Consider if Alice pays an invoice I of 1 ICP created by Bob in two installments of 1 ICP each. Alice has double paid. This could happen by accident. Suppose after the first payment, if verify_invoice is called and gets the balance, then sometime after, the second payment happens. Then verify_invoice will have transferred out the initial 1 ICP and recorded the invoice as verified. Now Bob cannot call verify_invoice again to get out the extra 1 ICP. Now consider a modification to the above. Bob now calls verify_invoice twice, once after each payment by Alice. Both calls can reach the transfer step before either one has finished. This means both transfers may go through, each for 1 ICP. However, the second call to finish will overwrite the new invoice instance created by the first call to finish. This means the invoice will lose track of what actually happened.
Risk Description
Leads to inconsistencies in the invoice. Currently low security risk as long as the implementation is unchanged: it can only cause accidental self-harm
This is dangerous: if e.g. it was decided that not the full balance should be paid in verify_invoice, this could make it possible to verify an invoice twice.
Recommendations
- unclear
My mental model for verify_invoice is that it should be "eventually true". As a seller, it is less important exactly which call was the one that succeeded, and it is more important to simply know that the payment has been satisfied.
Along with the potential sweeping solution in #13 , I recommend treating this a low-risk advisory given the current implementation
@andrew-lee-work calls out that we should ideally be able to lock the invoice so that verify_invoice cannot be called concurrently. @MarioDfinity points out that this may not be supported in Motoko from a language perspective, and could be out of scope
I recently saw the Dfinity R&D video which echoed the plan I had for resolving this issue--could introducing a "verifying caller" "lock mechanism" (a generic example pictured below) solve this problem?
https://www.youtube.com/watch?v=PneRzDmf_Xw
