Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SIP: Composable Fungible Tokens with Allowance #154

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

jio-gl
Copy link

@jio-gl jio-gl commented Sep 20, 2023

Abstract

This proposal extends the SIP-010 standard trait for fungible tokens on the Stacks blockchain to support composable fungible tokens with allowances. It addresses the limitations of the previous standard, which did not provide sufficient support for composability and security. The new trait includes functions for transferring tokens, approving allowances, checking allowances, and transferring tokens using allowances. The recommended implementation of approve used incremental allowances to avoid rance conditions and double transfering.

Copy link
Contributor

@friedger friedger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for putting this together. I am not 100% convinced that we need it. I'd like to see a better rational.

Also phishing, security threats and composability could be explained in more details.

(Some typos to fix.)


## Abstract

This proposal extends the SIP-010 standard trait for fungible tokens on the Stacks blockchain to support composable fungible tokens with allowances. It addresses the limitations of the previous standard, which did not provide sufficient support for composability and security. The new trait includes functions for transferring tokens, approving allowances, checking allowances, and transferring tokens using allowances. The recommended implementation of `approve` used incremental allowances to avoid rance conditions and double transfering.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It addresses the limitations of the previous standard, which did not provide sufficient support for composability and security.

SIP-10 is the bare minimum to have a standard for a fungible token. It allows to implement anything you want. I'd highlight the features and uses cases of the new standard, that is to let third parties control parts of your tokens.

The recommended implementation of approve used incremental allowances to avoid rance conditions and double transfering.

The recommended implementation of approve uses incremental allowances to avoid race conditions and double transferring.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ran a spell checking now

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will mention that SIP10 allows your to do basic transfers but the payments is limited or unsafe in the current form.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

improved abstract. Can I include you as a co-author, the Reference implementation if heavily based on your implementation?, although it has some syntax errors still.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I improved the Rationale with 3 current approaches to Compose DeFi that Fails if i am not wrong.

5. The User sign the `example-defi-service` and submits to blockchain.
6. The Dapp _D_ executes `example-defi-service` on-chain, this includes calling `transfer-from` to retrieve the tokens from User and, eventually, forwarding the tokens to a third-party service with `approve`, thus allowing for _Composability_.

## Backwards Compatibility
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are SIP-10 token contracts on mainnet like vibe tokens

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I see is that without allowances you are giving total control to any DeFi service (that only checks tx-sender) to grab any amount of any of your SIP10 Tokens?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, there are 40 contracts with approve function, but sometimes they use it for access control or the signature is different, they also include the owner in the signature.

https://github.com/search?q=repo%3Aboomcrypto%2Fclarity-deployed-contracts+define-public+%22%28approve+%22&type=code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For that we have post conditions @jo-tm

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only 5 tokens seems to have allowance or allowance-of I think the rest uses approve for Access Control Roles.

https://github.com/search?q=repo%3Aboomcrypto%2Fclarity-deployed-contracts+define-public+%22%28allowance-of+%22&type=code


## Backwards Compatibility

This proposal aims to maintain compatibility with the existing SIP-010 standard while introducing new functionality. Existing fungible token contracts can continue to use the SIP-010 functions without modification. Contracts that wish to utilize allowances and composable fungible tokens can implement the extended trait.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not clear whether SIP-10 tokens that use tx-sender are compatible with this SIP. Could you elaborate?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I wanted to limit the transfer(from, to, amount) to (transfer (to, amount)) and the sender is always contract-caller, removing first parameter I kept the from parameter to maintain compatibility with current approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signature is the same but the security model is not, therefore it is not compatible any more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current SIP10 Tokens are only compatibles in that they both will work on Leather Wallet using the same transfer(user,recipient,amount). Also to use a Single DeFi service, no Composability. If you use transfer to use a simple DeFi service like current approaches it will work. Not sure what the Stacks Swap Dapps are doing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

improved backwards compatiblity section


### DeFi Composability Pattern

The most common DeFi pattern, that is supported by this new standard is:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how this can't be done with SIP-10. In 5. the user could also transfer the amount of tokens required for example-defi-service. Are there blocks between 5. and 6.?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can be done with SIP10 with transfer only if tokens check tx-sender, if lets say DeFiA uses DeFiB and DeFiA transfer your tokens from your wallet to DeFiB. Then you give total control to the DeFiA and also to DeFiB called by DeFiA. Not sure your tokens can be retrieved properly because your tokens are jumping from your wallet to DeFiB.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, why do we need allowances? Is it only about the issue of tx with allow mode?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you have DeFiA is a YieldAggregator and DeFiB is a YieldService, then the following 3 current approaches does not work I think:

a) User calls defi-service from DeFiA.
b) DeFiA does transfer(User,DeFiA), good tx-sender.
c) In the same transaction DeFiA cannot do transfer(DeFiA,DefiB) because the tx-sender is still User.

a) User calls defi-service from DeFiA.
b) DeFiA does transfer(User,DeFiB), good tx-sender directly from User.
c) Incorrect, DeFiB does not supports receiving random token from third parties without calling another-defi-service from DeFiB.

a) User calls defi-service from DeFiA.
b) DeFiA does transfer(User,DeFiA), good tx-sender.
c) DeFiA calls another-defi-service from DeFiB.
d) Incorrect, tx-sender is still User, if DeFiB calls transfer(DeFiA,DeFiB) it has no permission.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 1. c) DeFiA can use as-contract to switch context, but needs to be careful. Furthermore, the token can allow transfers where sender is contract-caller OR tx-sender. Then no context switch is necessary.

In 3. c) Similarily, DeFiA can use as-contract to switch context, but needs to be careful.

In 3. DeFiA could not do b) (transfer tokens from User to DeFiA), but let DeFiB do it in d).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okey, so the solution for you is to change tx-sender to local contract by using as-contract.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


The extension of the SIP-010 trait with allowances and the ability to transfer tokens using allowances addresses the limitations of the previous standard. By introducing allowances, users can grant explicit permission for third parties to spend tokens on their behalf, improving the security and composability of DeFi contracts. The inclusion of additional functions from SIP-010 ensures compatibility with existing standards.

### Limiting Phishing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is Phishing? Could you elaborate?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will include more information in the doc.
These days is mainly a Fake Airdrop, read but you probably have alread read it https://www.coinfabrik.com/blog/tx-sender-in-clarity-smart-contracts/


The new trait should also include the functions defined in SIP-010, including `get-name`, `get-symbol`, `get-decimals`, `get-balance`, `get-total-supply`, and `get-token-uri`.

## Trait Implementation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would defined only the newly added functions in the trait. Contracts can implement both the sip-10 trait and this trait.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I though about doing that, but I felt it was better to replace the whole trait altogether to avoid the hazzle of implementing 2 traits and avoid the confusion with tx-sender that is one of the main points of the SIP. I was afraid that people implement both traits but continue checking tx-sender and avoding the new functions. From my point of view is easy to do a simpler SIP without the other functions but I was afraid that it will not replace the prev standard.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would defined only the newly added functions in the trait. Contracts can implement both the sip-10 trait and this trait.

Currently, I think I might limit this Improve Proposal to newly added functions, then the market can decide if they want to use allowances or limit to current approach. We can publish Best Practices for SIP10 that should be a combination of: tx-sender OR contract-caller check, post-conditions. If the main web3 apps do not embrace allowances no one will probably do it.


Approve an incremental allowance for a specific principal or contract to spend a certain amount of tokens on behalf of the sender. This function is similar to signing a check, granting permission for a third party to make token transfers within the specified limit. This allowance must be incremental (it adds on top of previous allowances) to avoid race condition situations where and `transfer-from` call is executed before the `approve` and then another after the `approve` call.

#### revoke
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe disallow-contract-caller like in pox contract

Copy link
Author

@jio-gl jio-gl Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems a bit long, maybe disallow ??


Transfer a specified amount of tokens from one principal to another using an allowance. The `from` principal must have previously approved the allowance for the `to` principal to transfer tokens on their behalf. This function facilitates composability by allowing third-party transfers within the approved limits.

#### approve
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe allow-contract-caller like in pox contract

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems too long, what about allow ?

@LNow
Copy link

LNow commented Sep 20, 2023

This approach decrease UX, doesn't fix any security issue and creates new ones.

When we're talking about tokens (both FT and NFT), the use of tx-sender is far more risky for a very specific subset of contracts ie. DEX or NFT exchange than for ordinary user. Normal user have to sign TX created with allow-mode to be affected by it.
In context of tx-sender the real danger for normal users lives in operations that don't involve assets transfers. Such as approve from this proposal.

@jio-gl
Copy link
Author

jio-gl commented Sep 21, 2023

@LNow thanks for you comment. I improved the Rationale with 3 cases where the current approach for doing a 2-hop DeFi Composability (User using a Yield Aggregator service using a Yield service). The current de-facto approach of checking tx-transfer makes Phishing much more dangerous (they can drain all the popular SIP10 tokens you have). Also the current approach makes platforms with arbitrary token pools, such as a Token Swap platforms, very dangerous for the same reason, they grab the tx-sender and they can start draining a whole lot of SIP10s.

@LNow
Copy link

LNow commented Sep 21, 2023

In order to steal any tokens from user first you have to convince him/her to sign a transaction with either no post-conditions at all (aka. TX with allow-mode) or with post-conditions that precisely describes what tokens will be transferred.
In both cases user must be dumb to sign such transaction.

With your proposal - all you have to do is convince user to sign TX that will not move any tokens (allowance TX have no side effects). And it is much easier than you might think.

When it comes to draining Token Swap platforms, I've covered this topic quite a few times (with some projects). There are ways to build contracts in such way, that it is almost impossible to drain them, and even if it is possible you can limit it to single pair of assets. You just need to understand the security model used in Stacks. In my opinion this proposal tries to bypass this model without bringing any real benefits (from the security standpoint).

  1. Allowance model forces user to give away the control of his/her tokens to someone else.
  2. It decrease the UX, as it forces users to sign 2 separate transactions
  3. It decrease the security, as it forces users to think to whom and for how long they are giving the power to control all or portion of their holdings.
  4. It decrease the flexibility and composability. If project decides to add new functionality to their product they will have to ask users to grant them new allowance, simply because they deployed a new contract with a new feature.
  5. It creates HUGE security risk for users, because instead of relying on security model built in into the ecosystem (post-conditions) they will have to trust contracts that 99.99999% have no idea how to read, nor analyze.

IMHO a lot of cons and risks, and no real benefits.

@friedger
Copy link
Contributor

Post conditions in clarity for contract-call? would be cool.

@friedger
Copy link
Contributor

Can we evolve this SIP to a post about best practices for defi protocols on stacks?

@MarvinJanssen
Copy link
Collaborator

MarvinJanssen commented Sep 28, 2023

With your proposal - all you have to do is convince user to sign TX that will not move any tokens (allowance TX have no side effects). And it is much easier than you might think.

This line by @LNow gets to the core of the issue. If there SIP were activated and implemented then it will become quite a bit easier to steal tokens. You can either hide a call to approve somewhere in a different contract call or convince the user to do it directly, because Hiro Wallet et al. will report "No tokens will be transferred in this call". Once the transaction is broadcast the floodgates are opened. The tx-sender guard is mainly an issue for contract principals. They are not awarded the security of post conditions and are the ones that get attacked via that route when using dynamic dispatch. That's why usually a guard like (or (is-eq tx-sender sender) (is-eq contract-caller sender)) is enough to mitigate it; for, contracts will no longer need to resort to as-contract. To solve the issue once and for all and keep composability we would need a construct in Clarity that allows contracts to specify post conditions for contract-call? as mentioned by @friedger.

@jio-gl
Copy link
Author

jio-gl commented Sep 28, 2023

@MarvinJanssen you cannot call approve allowance from another contract because only the contract caller can approve its own allowance. If they convince you on Phishing to send a approve allowance it will always be for 1 single contract. With the current approach they can literally sell the control of your tx-sender in a Phishing market once they convince you and drain any SIP10. I agree the post conditions do no add much safety on composability by being in the UI, they should be in the contract-call?. If you are doing a service aggregator, like a Yield Aggregator, is not very convenient to add UI Post-conditions to 3rd party calls, maybe you do not know the exact side-effects.

@LNow
Copy link

LNow commented Sep 28, 2023

With the current approach they can literally sell the control of your tx-sender in a Phishing market once they convince you and drain any SIP10.

That is only possible if you sign a transaction created in allow-mode. If TX is created correctly (with deny-mode) then ALL token transfers must be specified in post-conditions.

If you are doing a service aggregator, like a Yield Aggregator, is not very convenient to add UI Post-conditions to 3rd party calls, maybe you do not know the exact side-effects.

Either you use proper post conditions, or you create TX in allow-mode which by default should be considered as malicious because transactions created in allow-mode can transfer ANY tokens and user accepts it (hence the name allow-mode - you allow contract to do anything)

@friedger
Copy link
Contributor

If the approval checks for contract-caller, then contracts need to use as-contract and are in the same position as with transfer and we haven't won anything.

@jio-gl
Copy link
Author

jio-gl commented Sep 28, 2023

If the approval checks for contract-caller, then contracts need to use as-contract and are in the same position as with transfer and we haven't won anything.

Updated: yep, you are right xD got confused. If the user calls contract 1 and contract2 calls tokenA then there is not way checking only (is-eq contract-caller sender) will work.

@jio-gl
Copy link
Author

jio-gl commented Sep 28, 2023

With the current approach they can literally sell the control of your tx-sender in a Phishing market once they convince you and drain any SIP10.

That is only possible if you sign a transaction created in allow-mode. If TX is created correctly (with deny-mode) then ALL token transfers must be specified in post-conditions.

If you are doing a service aggregator, like a Yield Aggregator, is not very convenient to add UI Post-conditions to 3rd party calls, maybe you do not know the exact side-effects.

Either you use proper post conditions, or you create TX in allow-mode which by default should be considered as malicious because transactions created in allow-mode can transfer ANY tokens and user accepts it (hence the name allow-mode - you allow contract to do anything)

thanks for comment @LNow, I will research more on allow-node and deny-mode. Found this:

In addition, the Stacks blockchain supports an "allow" or "deny" mode for evaluating post-conditions: in "allow" mode, other asset transfers not covered by the post-conditions are permitted, but in "deny" mode, no other asset transfers are permitted besides those named in the post-conditions.

I thinks PCs are a great addendum to security but in case a third-party want to integrate my Web3 service I cannot enforce them to call my service with deny-all. For example, if the aggregator is using 100 pools from my service, they have to make sure to grab the token data for a specific tokens in the TX and add the deny-all flag.

@LNow
Copy link

LNow commented Sep 28, 2023

From my personal notes:

In allow mode any token operations (transfer and/or burn) not covered by post-conditions are allowed. In other words user is giving permission to perform any token operations including those that might drain his wallet.

In deny mode all token operations (transfer and/or burn) not covered by post-conditions are prohibited. In other words if contract tries to perform any token operation not listed in post-conditions, or value/size of such operation would not match what is specified in post-condition, then such TX will fail.

I thinks PCs are a great addendum to security but in case a third-party want to integrate my Web3 service I cannot enforce them to call my service with deny-all.

Correct. You have no control over how TX that interact with your contract will be created.
Transactions created in allow-mode are extremely risky for users and they should be (and AFAIK are) displayed by wallets, during signing process, as risky/dangerous/potentially malicious.
Thus it doesn't matter how difficult it is to create proper post-conditions - you should never promote signing TX without them.

TX created in allow-mode should be reserved for a very special occasions, and users who sign them should be aware what are the risks and know what and why they are doing. 99.999% of users should not use them at all.

@jcnelson
Copy link
Contributor

I thinks PCs are a great addendum to security but in case a third-party want to integrate my Web3 service I cannot enforce them to call my service with deny-all. For example, if the aggregator is using 100 pools from my service, they have to make sure to grab the token data for a specific tokens in the TX and add the deny-all flag.

@jo-tm Can you help me understand why this is a problem? Because Clarity is a decidable language, it's tractable to enumerate all of the halting states of a contract-call's execution. Thus, it's computable which token-transfers could be reached from a contract-call, so your wallet could automatically generate post-conditions for them. Am I missing something?

@jio-gl
Copy link
Author

jio-gl commented Sep 28, 2023

I thinks PCs are a great addendum to security but in case a third-party want to integrate my Web3 service I cannot enforce them to call my service with deny-all. For example, if the aggregator is using 100 pools from my service, they have to make sure to grab the token data for a specific tokens in the TX and add the deny-all flag.

@jo-tm Can you help me understand why this is a problem? Because Clarity is a decidable language, it's tractable to enumerate all of the halting states of a contract-call's execution. Thus, it's computable which token-transfers could be reached from a contract-call, so your wallet could automatically generate post-conditions for them. Am I missing something?

@jcnelson I wasn't sure that checking sender to be equal to tx-sender OR contract-caller for Web3 composability was a good enough approach for token security, but I am more convinced now after discusing. It might work this way combined with post-conditions. Like @LNow mentioned, the wallet should show a Warning if the TX is allow-all otherwise is very dangerous for phishing and Web3 service aggregators.
Not sure if Web3 Devs are using any tools to generate the post-conditions or they are manually doing what they can.
The as-contract approach to Web3 composability seems to also work as well but it seems to break the semantics of tx-sender.
I think we can discuss this SIP extension as an alternative for the community (only newly added functions allow and transfer-from) or remove it altogether.

@jcnelson
Copy link
Contributor

While checking tx-sender or contract-caller can protect the user in principal, I'm not convinced that this is the best we can do. The intent of post-conditions is that the user can stipulate what the effects of a transaction must be, regardless of what the transaction actually does. This empowers non-technical users to protect themselves from the effects of buggy or malicious code -- something that, AFAIK, no other blockchain can do. By contrast, checking tx-sender and contract-caller is something that only the contract developer can do; if the developer makes a mistake, the users suffer the consequences. Using an allowance-based system to constrain who can access your tokens is in the same category of security measures -- users must trust that someone else's code will do the Right Thing.

Now, do post-conditions today go far enough? No; I think they can be extended to do a lot more. For example, they could be extended to place constraints on who received tokens and NFTs, as well as who sent them. As another example, they could be scoped to only apply to specific contracts, or in specific functions. Both of these extensions are consistent with the intent of PCs -- they empower users to protect themselves from developers.

Does any Stacks wallet try to statically analyze the contract call graph in order to generate useful post-conditions? Not that I know of. However, this could be added due to the aforementioned reason that Clarity is decidable. Unlike Solidity, you can know the effects of a transaction before you run it. I think wallets should be in the business of doing this, but that's a topic for another day.

@LNow
Copy link

LNow commented Sep 28, 2023

Now, do post-conditions today go far enough? No; I think they can be extended to do a lot more. For example, they could be extended to place constraints on who received tokens and NFTs, as well as who sent them. As another example, they could be scoped to only apply to specific contracts, or in specific functions. Both of these extensions are consistent with the intent of PCs -- they empower users to protect themselves from developers.

Something that might be worth to consider is a middle ground between allow and deny modes. A mode where user is protected like it is in allow mode, but we don't have to specify all post conditions for tokens held by contracts.

So if I want to swap token A for Z and to do that contract I'm interacting with has to swap A -> B -> C.....X -> Y -> Z I won't have to specify conditions for all transfers, only for token A and Z because as a user I'm interested only with these two. Of course if we add conditions for let's say C and G they also must be respected in that mode.

@jio-gl
Copy link
Author

jio-gl commented Sep 28, 2023

@jcnelson @LNow I think this is a productive discussion. Something that can also be secure in my opinion is to have post-conditions in Traits, so you implement SIP10 or a trait with something like stake(token, amount) that only grabs the specified token thanks to PCs. Then you don't rely on the Token Developer but if you aggregate Token or Web3 Services you call (impl-trait some-trait.some-trait.dangerous-method)to verify that the implementation is secure before using it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants