koinos/koinos-contract-standards

KCS-1 Token

mvandeberg opened this issue · 6 comments

mvandeberg commented

Based on the choice to make NFT's KCS-2, I am assuming KCS-1 should be the already used token standard.

I am creating this issue so that we do not forget to add the standard.

jredbeard commented

Thanks @mvandeberg - yes you are correct that was the intention. I haven’t had time to write it yet myself. We were talking about it in discord last week and we think it would basically be the same as Roamin’s example token contract as that seems to have all of the required endpoints that would work for both VHP and Koin itself

joticajulian commented

I will put here the comment I was mentioning in discord with regarding this standard.

I think it's worth to bring back this issue now to come up with ideas to robust the standard (principally for the transfer function) koinos/koinos-chain#793

let me put an example:

  1. I connect my wallet to an unknown website that offers me a NFT.
  2. I accept and interact with their contract.
  3. That contract could maliciously steal my NTFs. why? because my signature is in that transaction and System.checkAuthority() accepts that as valid (assuming that I have a typical address, not a smart contract wallet).

Last year my thoughts were around updating the check_authority function. But today this change requires a governance proposal so it will take time. However, we can discuss alternatives in the standard itself. Something like this:

"approve" function:
function to be able to define the different addresses that have access to the assets
and their limits (for instance, a user authorizes to KoinDX to move assets).

"set_contract" function:
This function is to tell to the contract "hey, my address is not a typical account but
a smart contract. From now on do not trust in my signature, always call my contract
for authorizations"

"check_authority" function (no need to expose it in the ABI):
Function to check if the operation has the correct authorization from the user. The
process is as follows:
- if the user has a smart contract wallet (see set_contract function) then call the
  contract and return the response. This contract needs a specific endpoint different
  to `authorize` (because "authorize" can not be called by anyone).
- else if there is a caller (a smart contract in the middle) then check if it is approved.
- else - this is a typical account that doesn't have smart contract wallet and there is
  no caller. In this case check the signature of the transaction.

"transfer" function:
Function to transfer tokens.
- Call the internal "check_authority" function to verify if the transfer operation has
  the correct authorization.

the other solution is to continue without these functions and put more efforts in the Wallets (kondor / mkv) by using broadcast: false continuously and asking to the users if the events are OK, before making a real broadcast. This option is available in mkv but not in kondor at the moment.
The broadcast solution could work but I would prefer a more robust solution at SC level. Possible issues with the broadcast option:

  • a malicious API (the api broadcast the tx even if I set broadcast false)
  • interaction with a contract that do not implement events
  • 👍1
  • 👀1
jredbeard commented

My thoughts on the above issue: very unfortunately, since there are already contracts deployed that don't have a workaround implemented (including Koin and VHP), I think the only "good" solution at the moment is to have wallets do broadcast: false and continuously ask users if events are OK. This isn't the most desired solution but I think it's the one we've got at the moment. If we could agree on what the correct long term fix is (whether that is at the SC level or changed via governance as was previously described) I'm all for it. Correct me if I'm wrong but I think even if implemented at the SC level the Koin and VHP contracts would also need to be updated and that also is going to require a governance vote. Would love more discussion on the topic @joticajulian and @mvandeberg

joticajulian commented

Yes, for the moment we can continue using broadcast: false option and check the events.

For a long term solution I propose this:

  • Include a system call to read contract metadata koinos/koinos-chain#806
  • With this we can check if the owner has a smart contract wallet (and we can remove the set_contract function I described above).
  • Implement in the token contract the check_authority function I described above.
  • Update the implementation of KOIN and VHP contracts using this standard.
  • With regarding to the check_authority system call I don't know what to do. We can keep it as it is, but without using it at SC level. The problem of updating it is that the token contract needs to do 2 different checks (call the contract of the user if present, or check the signatures in the transaction) but with 2 different outcomes (in the case of a smart contract wallet just continue, in the case of signatures it is valid only if there is no caller in the middle). So this can not be addressed in a single true/false from the check_authority system call. It is required to know at least if the SC of the user was called.
mvandeberg commented

As I understand it, the problem is primarily concerned with not trusting a node to respect the request to not broadcast a transaction. The solution of allowances is a good solution to the problem.

However, my stance on adding allowances to the token standard has not changed. The design intent behind authority overrides was so that smart contracts would not handle these sorts of issues. In fact, our original internal design for KOIN and VHP was going to include allowances but we decided against it because those features could be implemented through authority overrides.

The default account behavior of having a single key sign for everything was never intended to be complete. Adding a requirement regarding this sort of behavior to the contract standard is inappropriate because this same requirement should then be added to every contract standard created here.

Instead, I think it is a perfectly valid decision to handle key only accounts naively and instead push users in the direction of feature rich "smart contract wallets" through tooling such as Kondor and MKW.

Why should we use smart contract wallets for this?

  1. Limiting the standard to the minimal set of required behaviors should always be a design goal. Simply because it doesn't need to be implemented here should be viewed as a positive.
  2. It puts more control in the hands of the users. Should allowances be a white list or a black list? Different users may have different opinions based on their desired use cases. A single user may even want to have different wallets configured differently. Making this decision as part of the token standard limits how users can interact with Koinos.
  3. It creates opportunities for business development. Let's say that Julian and Roamin have different opinions over the features and behaviors of smart contract wallets. Julian could develop and integrate his smart contract wallet with Kondor, while Roamin integrates with MKW. That should not be discouraged. They could integrate with the same smart contract wallet if they agree and that would not stop someone else from wanting to create a new smart contract wallet and wallet to compete, and neither Kondor and MKW should be prevented from integrating with it if they so choose.

I admit that relying entirely on smart contract wallets to implement this behavior leaves out those users who wish to continue using the default behavior, but that is a small price to pay in order to preserve user choice and freedom.

In additional to the above proposal, I believe it would also be prudent to allow test execution of transactions without requiring a signature. An additional mode of operations should be added to submit_transaction that would short circuit all check_authority calls by returning true. This would allow a user to inspect transaction events prior to signing the transaction. It is not fool proof as the API node would still need to be trusted, however this could be safely checked against multiple API nodes for consistency. The results could be trusted with a reasonable degree of certainty.

joticajulian commented

Thanks for your input @mvandeberg

I admit that relying entirely on smart contract wallets to implement this behavior leaves out those users who wish to continue using the default behavior, but that is a small price to pay in order to preserve user choice and freedom.

The solution I'm proposing do not limit the freedom of the smart contract wallets. They will continue working in the same way. The allowances feature is just for normal accounts that don't have smart contract wallets.

Let's assume Alice has a smart contract wallet and Bob doesn't.
Bob's transaction requires 2 operations: allowance + contract call (e.g. koindx).
But Alice's transaction only needs 1 operation, no allowance required.

The token contract works in this way:

  • it checks if the user has a smart contract. If so call it ... like Alice
  • if not then the rest only applies to users without contracts... here we use the allowances:
  • if there is a caller (a smart contract in the middle) check if there is an allowance.
  • if there is no caller check the signature of the transaction.

The check_authority system call was designed to do all of these steps in a single call except for the allowances. Then my proposal is not to rely on check_authority but to do this checks in the contract itself to be able to add the allowances. The missing feature is the ability to know if a user has a smart contract or not to be able to know if it should be called. This could be solved with koinos/koinos-chain#806.

Instead, I think it is a perfectly valid decision to handle key only accounts naively and instead push users in the direction of feature rich "smart contract wallets" through tooling such as Kondor and MKW.

I assume that an important group of people would prefer not to use smart contract wallets to reduce the exposure to bugs or hacks as much as possible. In any case, I agree that people should be encouraged to use them.

In additional to the above proposal, I believe it would also be prudent to allow test execution of transactions without requiring a signature. An additional mode of operations should be added to submit_transaction that would short circuit all check_authority calls by returning true.

This feature can be risky. An attacker could upload a malicious contract to share his mana for free and encourage people to use it. During the first validation (no signature) the chain will skip the call to the contract of the attacker because check_authority is computed as true. However, after the user signs and broadcast the transaction, the chain will call the contract of the attacker and this contract could steal the tokens. So in summary, in the event inspection everything looks fine, but in the real transaction no and the assets can be stolen.
(unless you refer to execute the authorize function and after that return true, which again can create side effects because the authorize function will not find the signatures).

Finally just to mention that we should name the current implementation as KCS-1. And later on create a new one with the features I mentioned (KCS-3 ?). So it's easier to correctly name the standard of current and future tokens.