SEC Cleanup

[krpeacock]

7 additional notes were provided from the security audit that have minimal risk and are trivial to clean up.

• redundant argument in verify_invoice: The caller is no longer needed in ICPVerifyInvoiceArgs. It may be safer to remove it to reduce the risk of confusing the caller with the creator or so.
• In verify_invoice the from_subaccount is re-computed. Would it be better to add this information to the invoice when creating it? IMO That would be safer / easier to review since the info would be generated in a single place.
• verify_invoice still has a TODO that should be removed.
• in refund_invoice let replaced = invoices.put(i.id, updatedInvoice) is irritating since put does not return anything according to the docs. Also, in refund_invoice this variable is never used.
• get_invoice returns invoice by default: If the switch statement does not return, the method returns the invoice by default (last statement in get_invoice).
• get_invoice permissions: IIUC one always needs to put the buyer principal in the canGet permissions. Is this intended and does one always know the buyer when creating the invoice?
• unused code: defaultSubAccount is never used and should be deleted

[atengberg]

Just a note, generating the payment address when the invoice is created and including it in the invoice's metadata seems like the cleanest way to resolve the second bullet point--I would assume it should only be visible to the invoice creator's principal (and by extension to the buyer if requested or returned to them when the invoice is originally created).

As far as the buyer (I take to mean the customer) principal is concerned, is the buyer's principal necessary for any part of the actual access control / permissions of the invoice? Since any of the buyer's requests are done through the seller any of those calls would actually be come from the seller's principal (as the buyer doesn't directly interact with the invoice canister itself).

[krpeacock]

I agree - it can be covered under the same read permissions as the rest of the metadata

[atengberg]

Any thoughts on "In verify_invoice the from_subaccount is re-computed" ?

Initially I agreed with this, but actually computing the subaccount is direct and only requires the id and creator principal which are immutable fields of a given invoice and thus always available when this subaccount would be used for from_subaccount.