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

TEP: SBT Standard #85

Merged
merged 14 commits into from
Oct 11, 2022
Merged

TEP: SBT Standard #85

merged 14 commits into from
Oct 11, 2022

Conversation

xssnick
Copy link
Contributor

@xssnick xssnick commented Sep 1, 2022

Soul bound token (SBT) is a special kind of NFT which can be transferred only between its owner's accounts. For this, it stores immutable public key of the owner, and it is needed to send transfer from new address with signature in payload to change owner's address.

There is a useful type of token which allows to give social permissions/roles or certificates to some users. For example, it can be used by marketplaces to give discounts to owners of SBT, or by universities to give attestation certificates in SBT form. Mechanics with ownership proof allows to easily prove to any contract that you are an owner of some SBT.

SBT implements NFT standard interface but transfer should always be rejected, pull_ownership is used instead.

@xssnick xssnick changed the title SBT Standard TEP: SBT Standard Sep 1, 2022

In case when `verify_ownership` was bounced back to SBT, SBT should send message to owner with schema:
```
verify_ownership_bounced#81b510c2 query_id:uint64 sbt_id:uint256 owner:MsgAddress
Copy link
Member

Choose a reason for hiding this comment

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

Does owner need sbt_id, owner, data and content in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is like proxy of bounced message, technically i think most of that it is not so useful, but just for similarity with original.

As a use case, sbt_id can be used to confirm that bounce was from real SBT, by calculating address and comparing with sender.

Sender address is not an owner's address.

**Otherwise should do:**
Set owner's address to null and set public key to 0.
Copy link
Member

Choose a reason for hiding this comment

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

Do we need response with excess money here back to owner?
Do we want to keep enough balance on SBT to ensure it will not be frozen and destroyed for a long time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, I think we can add reserve of 0.05 TON and return excess back, if balance is less than 0.05 throw an error. Do you think it is good approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

1. `get_public_key()` - returns `int`, that is owner's public key.
2. `get_nonce()` - returns `int`, which current nonce.
3. `get_nft_data()` - same as in [NFT standard](https://github.com/ton-blockchain/TIPs/issues/62).
4. `get_authority_address()` - returns `slice`, that is authority's address. Authority can revoke SBT.
Copy link
Member

Choose a reason for hiding this comment

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

If authority can not be changed may be it is better to force collection to be authority? Not sure though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then it will require custom collection to work with SBT, with additional revoke method, not sure if it will be good.

@hacker-volodya
Copy link
Collaborator

"Drawbacks" section is missing.

Added initiator address to verify_onwership
@xssnick
Copy link
Contributor Author

xssnick commented Sep 6, 2022

"Drawbacks" section is missing.

Added


`sbt_nonce` - nonce, required for protection of signature replay attacks.

`new_owner` - address of the new owner of the SBT item, should be the same as sender's address.

Choose a reason for hiding this comment

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

Why don't we use sender's address directly, without duplicating it in the message?

Copy link
Member

Choose a reason for hiding this comment

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

In your case, imagine situation when we have quite sharded chain and new owner from shard 0b00000 try to pull_ownership from sbt deployed in 0b11111. Message from new owner will migrate to new address some time during which attacker from the same shard may be able to send exactly the same message (signature will be valid since it doesn't include anything specific to new owner) and get sbt. While it is not critical (ownership is still bound to the public key, not the address), it is bad enough to avoid such attacks.

Choose a reason for hiding this comment

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

Could you clarify what does "address of the new owner [...] should be the same as sender's address" mean? If we need to deal with sharding in some specific way, shouldn't we put this into spec?

Copy link
Member

Choose a reason for hiding this comment

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

pull_ownership request should cover by signature the same address from which request came. Yes, in theory, we may not include sender address into the request and instead construct signed body from request payload and sender address. However in practice it is much simpler to explicitly provide payload which was signed.

Choose a reason for hiding this comment

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

Ok, i see, this makes signature verification simpler this way. In principle, we should weigh the cost of transmitting extra 32 bytes vs gas spent constructing a cell for signature, but i doubt those extra 32 bytes would make a noticeable difference.

@oleganza
Copy link

I looked at the spec, I have a few cosmetic suggestions:

1. Owner & Wallet

Let's use consistent naming for owner and wallet. The SBT is designed to be bound to the owner, but leaves ability to switch the wallet in case the owner upgrades from v3 to v4 or to v5, while keeping the same private key inside.

Rename pubkey method to clarify the role of the pubkey:

owner field
get_public_key() -> get_owner_key()

Rename "pull ownership" because it sounds like some authority is taking away your ownership. We use it to switch the wallet, but keep the same owner.

pull_ownership(... new_owner: ...) -> switch_wallet(... new_wallet: ...)

2. Nonce vs Seqno

Seems like doing seqno+=1 like in the wallet code is the simplest replay protection possible, so maybe we should rename nonce into seqno.

3. Unify revoke & destroy?

Maybe the spec could be made simpler if we unify revoke and destroy as one method destroy. If we want to tag things in the explorer or API, we could simply check if the msg sender is the SBT's wallet or the authority and mark the transaction accordingly.

@oleganza
Copy link

From the UX perspective the SBT always belongs to the wallet, so switch_wallet method (currently pull_ownership) is going to be used within some migration flow, not as a general purpose "transfer" UI. This also means that the wallet is not concerned with transferring the privkey of SBT to any other location and securing it along the way.

@xssnick
Copy link
Contributor Author

xssnick commented Sep 12, 2022

I looked at the spec, I have a few cosmetic suggestions:

1. Owner & Wallet

Let's use consistent naming for owner and wallet. The SBT is designed to be bound to the owner, but leaves ability to switch the wallet in case the owner upgrades from v3 to v4 or to v5, while keeping the same private key inside.

Rename pubkey method to clarify the role of the pubkey:

owner field get_public_key() -> get_owner_key()

Rename "pull ownership" because it sounds like some authority is taking away your ownership. We use it to switch the wallet, but keep the same owner.

pull_ownership(... new_owner: ...) -> switch_wallet(... new_wallet: ...)

2. Nonce vs Seqno

Seems like doing seqno+=1 like in the wallet code is the simplest replay protection possible, so maybe we should rename nonce into seqno.

3. Unify revoke & destroy?

Maybe the spec could be made simpler if we unify revoke and destroy as one method destroy. If we want to tag things in the explorer or API, we could simply check if the msg sender is the SBT's wallet or the authority and mark the transaction accordingly.

  1. Yea, agreed with switch_wallet, it looks better, will update. But not sure about changing get_public_key to get_owner_key, because currently interface is same with wallet’s method, it allows to universally get the owner’s key of both entities.
  2. Nonce was used here because with seqno we have a low chance of replay attack, if 2 sbts of the same owner of same collection are connected to one key. If seqno is same, attacker can repeat the request on second sbt and change wallet address to same as on first one. It is not so critical, since it is very rare case, but nonce resolves it and looks better here.
  3. I think revoke and destroy are different processes and in some cases they can contain additional logic, so imo it is better to keep them as separate operations.

@oleganza
Copy link

Another note: conceptually, we need to put as little logic in the contract as possible to enforce relationship between the designated parties. In our case the parties are: "the owner" (identified by a fixed pubkey) and "the authority" (maybe rename this to "issuer"?).

Do we really need a concept of a "wallet" here at all? If we don't then:

  1. We do not need to have switch_wallet logic at all. When user migrates the wallet from v5 to v7 nothing has to happen to SBP. We can move that signature verification into the prove_ownership method instead.
  2. Wallets need to have a special support for SBTs anyway (to prove ownership and display them distinctly from NFTs because SBTs can't be transferred, to support token-specific features), so they might as well pick them up by the pubkey instead of the NFT ownership API.

NFTItem interface is cool because you control that thing with an arbitrary address (which could be plain wallet, multisig, etc.), but SBT is not really controlled by the wallet, but explicitly controlled by a fixed pubkey.

TL-B schema of inbound message:
```
prove_ownership#04ded148 query_id:uint64 dest:MsgAddress
forward_payload:^Cell with_content:Bool = InternalMsgBody;

Choose a reason for hiding this comment

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

Suggested change
forward_payload:^Cell with_content:Bool = InternalMsgBody;
data:^Cell with_content:Bool = InternalMsgBody;

?


TL-B schema of inbound message:
```
pull_ownership#08496845 query_id:uint64 signature:^(bits 512)
Copy link

@oleganza oleganza Sep 13, 2022

Choose a reason for hiding this comment

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

why query_id goes before the signature? Shouldn't it be covered by the signature and it'd be easier to verify it if sig comes first?

@xssnick
Copy link
Contributor Author

xssnick commented Sep 18, 2022

After additional discussions standard was split to this one and NFT Ownership, and simplified.

Implementation and description was changed.

@oleganza
Copy link

I am pretty happy with the concept of making SBT a ≈subset of NFTs. The definition "SBT = NFT where only the minter/issuer/authority can transfer" seems like the minimal possible one. From the perspective of the ecosystem that's enough.

But for the security of the issuer they need to verify that the recipient's wallet is a regular wallet v3/v4 so they don't accidentally issue SBT to a transferrable "holding contract" that would effectively "wrap" SBT into a transferrable NFT and hamper the bonding property.

@EmelyanenkoK
Copy link
Member

LGTM

Copy link

@dvlkv dvlkv left a comment

Choose a reason for hiding this comment

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

LGTM.

@tvorogme
Copy link

Great work, LGTM

@hacker-volodya
Copy link
Collaborator

This TEP enters its Final Comment Period. It ends in 10 calendar days, 11.10.2022.

@DeFiTON
Copy link

DeFiTON commented Oct 1, 2022

LGTM

@hacker-volodya
Copy link
Collaborator

The Final Comment Period is complete with a disposition to merge. Thanks to the authors of TEP, @xssnick, @Naltox, @EmelyanenkoK, @oleganza, and to all who took part in the discussion. This pull request will be merged now.

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

Successfully merging this pull request may close these issues.

10 participants