dfinity / invoice-canister

Providing an example and simplified experience for accepting payments in smart contracts

Invoice Canister

This project provides a simple interface for creating and paying invoices in various tokens on the Internet Computer. It is a custodial solution, intended to be a simple, drop-in payments solution for any canister. To read more about the design of the canister, see the Design Doc.

IMPORTANT / DISCLAIMER

This project is to be used for educational purposes only. It has known security vulnerabilities. As such, it should not be used in any real-world / production context where money is at stake. Please proceed with thoughtfulness and attention.

Integrating with the Invoice Canister

Include this code in your dfx.json and follow our self-hosted example: TODO

Getting Started - Development

Make sure you have followed the DFX installation instructions from https://smartcontracts.org.

Run the install-local.sh script to install the ICP ledger and and the invoice canister on your device. You can make calls using the dfx sdk, or you can see test cases running through the flows under the test directory.

Testing

To test, you will need to install moc from the latest motoko-<system>-<version>.tar.gz release. https://github.com/dfinity/motoko/releases.

Then, install Vessel following the guide at https://github.com/dfinity/vessel.

You will also need to install wasmtime. For macOS, you can install with brew install wasmtime. For Linux, you can install with sudo apt-get install wasmtime.

To run unit tests, use make test.

To run the end-to-end JavaScript tests, first install fresh with with ./install-local.sh. Then, run npm test.

invoice-canister's Issues

[SEC-F20] Controller of canister could take all funds by upgrading

Observation

The invoice canister can be deployed by anyone and anyone can create invoices on it. This means that the invoice creators (sellers) will receive money that will be temporarily held by the invoice canister.

The controller of the canister can control these funds, since they control the code (upgrades).

Risk Description

The controller of the canister can steal all the funds held by the canister, e.g. by upgrading the code.

The severity of this depends on how the invoice canister is used. If only the controller is supposed to create invoices, this would not be an issue. But actually anyone can create invoices.

Recommendations

• Should there be a restriction on who can create invoices (e.g. by whitelisting principals)?
• We recommend to clearly define and document how the canister is intended to be used.
• If the current design is kept, we think the canister would need to be controlled by a governance system. This should be clearly documented with instructions on how to achieve it.
• See best practices here.

unusable with dfx 0.15.1

./install-local.sh results in following errors being dumped continuously (no perceivable delay):

Nov 06 09:11:19.362 ERRO s:lftbv-pjj44-lhaob-lpzga-soo6y-smi4l-rg745-5kpnc-of53u-ousgw-cae/n:fotuh-z4hlx-r7spc-ydzkg-rp5tq-jktzt-ba23y-ab56v-tggcd-2iwh5-pqe/ic_consensus/notary Couldn't create a signature: Cannot find MultiBls12_381 secret key with ID "KeyId(0xc0134609a2f493b0981df8eeb90739aa7a84b417f88918c8be5692332b2d02b4)"
Nov 06 09:11:19.498 ERRO s:lftbv-pjj44-lhaob-lpzga-soo6y-smi4l-rg745-5kpnc-of53u-ousgw-cae/n:fotuh-z4hlx-r7spc-ydzkg-rp5tq-jktzt-ba23y-ab56v-tggcd-2iwh5-pqe/ic_consensus/dkg Couldn't create a DKG dealing at height 200: Failed to create dealing: Secret key with id KeyId(0x4976235baf8f1c4340a9bee1ef6a9c4dc27292103c30943e8b5496546f23b4f7) not found in secret key store. Internal error: Cannot find threshold key to be reshared:
key id: KeyId(0x4976235baf8f1c4340a9bee1ef6a9c4dc27292103c30943e8b5496546f23b4f7)
Epoch: 1. NiDkgAlgorithm::load_transcript must be called prior to calling this method

dfx stop results in:

Using shared network 'local' defined in /home/andre/.config/dfx/networks.json
Error: Failed to kill all processes. Remaining: 4247 4201 4290 3863

[SEC-F13] Missing Input Validation

Observation

In several places arbitrary inputs can be passed to the canister, e.g.

create_invoice

• details of invoice can contain arbitrary long strings and Blobs.
• permissions arrays (canGet, canVerify) can be big (no limit)

Risk Description

The create_invoice parameters are stored in the canister storage forever (see also F18). These arguments can be so big that they use the maximum ingress message size and with this it is easy to fill up the canister storage, resulting in denial of service. Also, the canister will no longer be able to upgrade (F19).

Unvalidated data in the invoice (e.g. the details) could also lead to injection attacks in some frontend if the data is displayed and not properly escaped there.

Recommendations:

• Perform strict input validation, see e.g. this best practice.
• restrict the principals who are allowed to create invoices? (see also F20)
• Optional: remove the details blob

[SEC-F24] Very small refunds are possible and block later refunds

Observation

In refund_invoice, an arbitrarily small refund (larger than the fee) is possible. After a refund, no further refund is possible.

The only way to refund would be to refund by doing transfers completely outside of the invoice canister. However, that would not be documented in the invoice.

Risk Description

If accidentally a very small refund would be done, this may block further refunds.

Recommendations

• specify (in the design) the intended behavior of refund flows.
• e.g. should it be possible to refund several times? How would that be documented in the invoice stored on the canister?
• Alternately - remove refund feature

[SEC-F23] ineffective expiration of invoices

Observation

An invoice has an expiration field, but that is never checked. Is that intended? Is the expiration purely informational?

The expiration is always set to 1 week.

Risk Description

Any action on an invoice can be done even if it may be expired already.

This doesn’t seem risky at the moment because you could see this as a purely informational field.

Recommendations

• Define what properties expiration should achieve (in the design).
• Then make sure the canister checks/enforces these properties.

[SEC-F12] Copied libraries

Observation

The sha256, crc32 and hex libraries are copied into the repository.

Risk Description

If the libraries are copied, it is hard to keep them up to date. Also it is not clear if and how they are tested and where they come from.

Recommendations

Use these libraries as a dependency rather than copying them.

[SEC-F29] Incomplete design documentation

Observation

The design gives a high level description and the interface specification. However, it does not cover the following (this may not be a complete list):

• Any limitations (also wrt. security) should be clearly listed in the doc.
• It is not clear from the design what data is stored on the canister.
• Even though the interface is specified, it is not clear from the design what exactly the individual methods do: what are the preconditions? Who is authorized to perform the operation? How do they modify the state? What do they return?
• It is not documented what kinds of subaccounts the canister uses and what they are for.
• The way the funds flow through invoicing is not documented. It would be useful to have a description of how the money flows through different accounts (creation, verification, refund, transfer). Maybe our notes could help.
• The "Basic Payment Flow ( hypothetical )” section should be updated. This should not be hypothetical

Risk Description

Writing a financial dApp where most functionality is only described by the code itself is risky:

• The lack of design documentation makes it hard to judge (e.g. in security reviews) if the code behaves as intended, because the intended behavior is not made explicit and can only be inferred from the code.
• It is hard to spot design issues
• One cannot understand the app (e.g. for people using it) and its guarantees without reading the code.

Recommendations

Extend the design document and in particular address the points given here.

[SEC-F25] a refund could be paid several times

Observation

This is a TOCTOU issue. Since the await ICP.transfer in refund_invoice makes the method execution non-atomic, the call up to that point could be executed several times on the same state.

If in the current state it holds that amount<=i.amountPaid and i.refundedAtTime == null , this would trigger several calls to transfer.

Risk Description

This would lead to the following situation:

• A refund would be paid several times if the principal invoice subaccount has suffucient funds.
• The last returning transfer would overwrite the invoice and store the data (refundedAtTime / refundAccount) only specific for that call. This makes it difficult to understand what went wrong at a later point and renders the invoice data incorrect.

Recommendations

• TBD

[SEC-F17] Uncertified Queries

Observation

Query calls:

• get_invoice
• get_account_identifier
• get_balance

Risk Description

Single replicas may change the response to these queries (if they are not issued as update by the caller):

• The get_invoice response contains the destination address where the transfer should go to. If this is modified, funds will be paid to e.g. the attacker.
• get_account_identifier : if the account identifier is used to transfer money to the account, that is risky.
• get_balance may want to use update call as well. This could be used to trick the user into seeing a wrong balance. IIUC this is an update call already, though.

Recommendations

• Use update instead of query calls.
• Add documentation to these calls on why they are updates.

[SEC-F18] Invoices are never deleted

Observation

Invoices are kept in the canister forever.

Risk Description

If the canister storage is full, no more invoices can be accepted, and also existing invoices may not be modifiable anymore (F19) and the canister may stop working.

F13 makes this more severe as big invoices can stored on the canister.

This is risky as funds may get stuck.

Recommendations

• Improve monitoring of used storage on the canister by providing the relevant data in canister calls
• Consider archiving solutions (similar to ledger archiving)
• If the issue is not solved, clearly document the limitations.

[SEC-F30] Funds can get stuck in invoice accounts

Observation

if invoice is paid again after the invoice was verified, the money is locked. TODO: expand on this

Recommendations

Offer a new method:

type AmountConsolidated = Nat;
consolidate_account({accountIdentifier: AccountIdentifier}) : AmountConsolidated

[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

[SEC-F26] refunded amount is not stored in the invoice

Observation

The amount that was refunded is not stored in the invoice for later auditing.

Risk Description

This is important information about an invoice refund that should be available at a later point.

Recommendations

• record the amount that was refunded in the invoice on the canister
• alternately, remove the refund feature

[SEC-F10] Buyer could make the seller lose money by requesting refunds

Observation

verify_invoice stores the balance that was paid in the amountPaid field in the invoice:

• balance is stored in the invoice
• but what was actually transferred is balance - 10000 , where 10’000 is the transfer fee.

In refund_invoice, it is possible to refund amount == amountPaid .

This results in a situation where the seller refunds 10’000 more than what was actually received from the buyer.

The buyer will have no cost here, as they receive everything back, including the fee they paid.

Risk Description

If a buyer can repeatedly do this (e.g. in the likely case that refunding is automated), they can drain money from the seller (10’000 per iteration) at no cost for the buyer.

This would require 10’000 iterations to drain 1 ICP from the seller.

Recommendations

• Make sure the buyer pays the fee for the transfer.
• Alternately, remove the refund feature

[SEC-F19] Upgrades fail if storage is too big

Observation

Upgrades work by serializing the invoices to stable memory.

Risk Description

If the invoices HashMap reaches a certain size, the pre_upgrade hook will no longer be able to serialize the invoices because it has a limit in how much computation it can do. This will render the canister impossible to upgrade.

Recommendations

• Consider writing the invoices to stable memory directly to avoid this problem.
• See best practices here.

SEC Cleanup

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

[SEC-F27] principalToSubaccount uses no domain separator

Observation

IIUC the invoice subaccounts use ‘invoice-id’ as domain separator for hashing, whereas the principal subaccounts don’t use a domain separator.

Risk description

There could be risk of hash collisions if the domains are not properly separated (not investigated in further detail).

Recommendations

For domain separation to be effective, we recommend to also add a domain separator to principalToSubaccount.

[SEC-F22] Potentially sensitive invoice details are stored in plain text on the canister

Observation

Invoice content is usually private information (what did I order, how much, etc.)

Risk description

Every node can see the data (independent of permissions) since they are stored in plain text

Recommendations

• This should be documented.
• The data could be encrypted for the buyer

Prevent arithmetic overflow when amount in TransferArgs is below 10_000

Currently when requesting a transfer with an amount in TransferArgs below 10_000 e8s the invoice canister will trap on an arithmetic overflow, due to this line :

amount = {
                // Total amount, minus the fee
                e8s = Nat64.sub(Nat64.fromNat(args.amount), 10000);
              };

Is it expected behaviour?
I would rather have a specific error indicating that the amount is too low to pay for the fee.
For the context, I have a function (called periodically) that check the balance in the invoice canister and request a transfer to another wallet.

[SEC-F21] Anonymous principal has an account

Observation

For example, an invoice could be created using the anonymous principal as caller.

Risk Description

This is unintended behaviour, especially since this does not come with any security guarantees since everyone can act as the anonymous caller

If a seller does this by accident (e.g. by forgetting to set an identity in the client), their funds can be stolen.

Recommendations

• disallow the anonymous principal
• See best practices here.