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

[krpeacock]

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.

[atengberg]

Related to this is transferring funds out of the custody of the invoice canister: should there be a similar admin-level role for the purpose of being able to move funds from any subaccount to wherever? Should it be the same set as those who can add and remove from the creators allow list?

In particular if there should be a role other than the canister deployer who can add and remove from the allowed creators list and access funds in any subaccount. Like a principal serving as a delegated administrator that could be the canister id of one controlled by a DAO.

From another perspective, a developer might want to deploy the invoice canister on behalf of some other person who needs this kind of ability, for instance a manager of storefronts sellers employ to process inventory.

Alternatively this cannot be included, and only the original deployer will be able to add and remove from the allowed creators list.

To keep in the scope of the BNT-2, an allowed creator list was added that is only alterable by the original deployer. Additionally, recovering funds "lost" in subaccounts is available but only for the creator or those who have verify permission of the invoice of that subaccount for that invoice.

Introducing fully functional refunds is another set of features that suggest introduction of invoice status (created / active or payment pending/ void or refunded / etc). I'll include sets of links to research I did on various platforms that demonstrate possibilities.