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

chore: x/tokenfactory audit #1962

Merged
merged 8 commits into from
Jul 5, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion proto/osmosis/tokenfactory/v1beta1/genesis.proto
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ message GenesisState {
];
}

// GenesisDenom defines a tokenfactory denom that is defined within genesis
// state. The structure contains DenomAuthorityMetadata which defines the
// denom's admin.
message GenesisDenom {
option (gogoproto.equal) = true;

Expand All @@ -26,4 +29,4 @@ message GenesisDenom {
(gogoproto.moretags) = "yaml:\"authority_metadata\"",
(gogoproto.nullable) = false
];
}
}
2 changes: 1 addition & 1 deletion proto/osmosis/tokenfactory/v1beta1/params.proto
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import "cosmos/base/v1beta1/coin.proto";

option go_package = "github.com/osmosis-labs/osmosis/v7/x/tokenfactory/types";

// Params holds parameters for the tokenfactory module
// Params defines the parameters for the tokenfactory module.
message Params {
repeated cosmos.base.v1beta1.Coin denom_creation_fee = 1 [
(gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins",
Expand Down
17 changes: 16 additions & 1 deletion proto/osmosis/tokenfactory/v1beta1/query.proto
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,22 @@ option go_package = "github.com/osmosis-labs/osmosis/v7/x/tokenfactory/types";

// Query defines the gRPC querier service.
service Query {
// Params returns the total set of minting parameters.
// Params defines a gRPC query method that returns the tokenfactory module's
// parameters.
rpc Params(QueryParamsRequest) returns (QueryParamsResponse) {
option (google.api.http).get = "/osmosis/tokenfactory/v1beta1/params";
}

// DenomAuthorityMetadata defines a gRPC query method for fetching
// DenomAuthorityMetadata for a particular denom.
rpc DenomAuthorityMetadata(QueryDenomAuthorityMetadataRequest)
returns (QueryDenomAuthorityMetadataResponse) {
option (google.api.http).get =
"/osmosis/tokenfactory/v1beta1/denoms/{denom}/authority_metadata";
}

// DenomsFromCreator defines a gRPC query method for fetching all
// denominations created by a specific admin/creator.
rpc DenomsFromCreator(QueryDenomsFromCreatorRequest)
returns (QueryDenomsFromCreatorResponse) {
option (google.api.http).get =
Expand All @@ -38,19 +43,29 @@ message QueryParamsResponse {
Params params = 1 [ (gogoproto.nullable) = false ];
}

// QueryDenomAuthorityMetadataRequest defines the request structure for the
// DenomAuthorityMetadata gRPC query.
message QueryDenomAuthorityMetadataRequest {
string denom = 1 [ (gogoproto.moretags) = "yaml:\"denom\"" ];
}

// QueryDenomAuthorityMetadataResponse defines the response structure for the
// DenomAuthorityMetadata gRPC query.
message QueryDenomAuthorityMetadataResponse {
DenomAuthorityMetadata authority_metadata = 1 [
(gogoproto.moretags) = "yaml:\"authority_metadata\"",
(gogoproto.nullable) = false
];
}

// QueryDenomsFromCreatorRequest defines the request structure for the
// DenomsFromCreator gRPC query.
message QueryDenomsFromCreatorRequest {
string creator = 1 [ (gogoproto.moretags) = "yaml:\"creator\"" ];
}

// QueryDenomsFromCreatorRequest defines the response structure for the
// DenomsFromCreator gRPC query.
message QueryDenomsFromCreatorResponse {
repeated string denoms = 1 [ (gogoproto.moretags) = "yaml:\"denoms\"" ];
}
42 changes: 22 additions & 20 deletions proto/osmosis/tokenfactory/v1beta1/tx.proto
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,27 @@ import "cosmos/base/v1beta1/coin.proto";

option go_package = "github.com/osmosis-labs/osmosis/v7/x/tokenfactory/types";

// Msg defines the Msg service.
// Msg defines the tokefactory module's gRPC message service.
service Msg {
rpc CreateDenom(MsgCreateDenom) returns (MsgCreateDenomResponse);
rpc Mint(MsgMint) returns (MsgMintResponse);
rpc Burn(MsgBurn) returns (MsgBurnResponse);
rpc ChangeAdmin(MsgChangeAdmin) returns (MsgChangeAdminResponse);

// ForceTransfer is deactivated for now because we need to think through edge
// cases rpc ForceTransfer(MsgForceTransfer) returns
// (MsgForceTransferResponse);
rpc ChangeAdmin(MsgChangeAdmin) returns (MsgChangeAdminResponse);
}

// MsgCreateDenom is the sdk.Msg type for allowing an account to create
// a new denom. It requires a sender address and a subdenomination.
// The (sender_address, sub_denomination) pair must be unique and cannot be
// re-used. The resulting denom created is `factory/{creator
// address}/{subdenom}`. The resultant denom's admin is originally set to be the
// creator, but this can be changed later. The token denom does not indicate the
// current admin.
// MsgCreateDenom defines the message structure for the CreateDenom gRPC service
// method. It allows an account to create a new denom. It requires a sender
// address and a sub denomination. The (sender_address, sub_denomination) tuple
// must be unique and cannot be re-used.
//
// The resulting denom created is defined as
// <factory/{creatorAddress}/{subdenom}>. The resulting denom's admin is
// originally set to be the creator, but this can be changed later. The token
// denom does not indicate the current admin.
message MsgCreateDenom {
string sender = 1 [ (gogoproto.moretags) = "yaml:\"sender\"" ];
// subdenom can be up to 44 "alphanumeric" characters long.
Expand Down Expand Up @@ -61,7 +64,16 @@ message MsgBurn {

message MsgBurnResponse {}

// // ===================== MsgForceTransfer
// MsgChangeAdmin is the sdk.Msg type for allowing an admin account to reassign
// adminship of a denom to a new account
message MsgChangeAdmin {
string sender = 1 [ (gogoproto.moretags) = "yaml:\"sender\"" ];
string denom = 2 [ (gogoproto.moretags) = "yaml:\"denom\"" ];
string new_admin = 3 [ (gogoproto.moretags) = "yaml:\"new_admin\"" ];
}

message MsgChangeAdminResponse {}
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved

// message MsgForceTransfer {
// string sender = 1 [ (gogoproto.moretags) = "yaml:\"sender\"" ];
// cosmos.base.v1beta1.Coin amount = 2 [
Expand All @@ -75,13 +87,3 @@ message MsgBurnResponse {}
// }

// message MsgForceTransferResponse {}

// MsgChangeAdmin is the sdk.Msg type for allowing an admin account to reassign
// adminship of a denom to a new account
message MsgChangeAdmin {
string sender = 1 [ (gogoproto.moretags) = "yaml:\"sender\"" ];
string denom = 2 [ (gogoproto.moretags) = "yaml:\"denom\"" ];
string new_admin = 3 [ (gogoproto.moretags) = "yaml:\"new_admin\"" ];
}

message MsgChangeAdminResponse {}
99 changes: 63 additions & 36 deletions x/tokenfactory/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,33 +11,43 @@ created denom. Once a denom is created, the original creator is given
- Mint their denom to any account
- Burn their denom from any account
- Create a transfer of their denom between any two accounts
- Change the admin In the future, more admin capabilities may be
added. Admins can choose to share admin privileges with other
accounts using the authz module. The `ChangeAdmin` functionality,
allows changing the master admin account, or even setting it to
`""`, meaning no account has admin privileges of the asset.

- Change the admin In the future, more admin capabilities may be added. Admins
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Asking to learn - what are the current admin capabilities? They are not implemented yet, is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK, current admin caps are just who created the token and that gives them the ability to perform certain actions. It does not extend beyond that AFAIK.

cc @sunnya97

Copy link
Collaborator

Choose a reason for hiding this comment

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

The current admin capabilities are only minting and burning

can choose to share admin privileges with other accounts using the authz
module. The `ChangeAdmin` functionality, allows changing the master admin
account, or even setting it to `""`, meaning no account has admin privileges
of the asset.

## Messages

### CreateDenom
- Creates a denom of `factory/{creator address}/{subdenom}` given the denom creator address and the subdenom. Subdenoms can contain `[a-zA-Z0-9./]`.
``` {.go}

Creates a denom of `factory/{creator address}/{subdenom}` given the denom creator
address and the subdenom. Subdenoms can contain `[a-zA-Z0-9./]`.

```go
message MsgCreateDenom {
string sender = 1 [ (gogoproto.moretags) = "yaml:\"sender\"" ];
string subdenom = 2 [ (gogoproto.moretags) = "yaml:\"subdenom\"" ];
}
```

**State Modifications:**
- Fund community pool with the denom creation fee from the creator address, set in `Params`
- Set `DenomMetaData` via bank keeper
- Set `AuthorityMetadata` for the given denom to store the admin for the created denom `factory/{creator address}/{subdenom}`. Admin is automatically set as the Msg sender
- Add denom to the `CreatorPrefixStore`, where a state of denoms created per creator is kept

- Fund community pool with the denom creation fee from the creator address, set
in `Params`.
- Set `DenomMetaData` via bank keeper.
- Set `AuthorityMetadata` for the given denom to store the admin for the created
denom `factory/{creator address}/{subdenom}`. Admin is automatically set as the
Msg sender.
- Add denom to the `CreatorPrefixStore`, where a state of denoms created per
creator is kept.

### Mint
- Minting of a specific denom is only allowed for the creator of the denom registered during `CreateDenom`
``` {.go}

Minting of a specific denom is only allowed for the creator of the denom
registered during `CreateDenom`.

```go
message MsgMint {
string sender = 1 [ (gogoproto.moretags) = "yaml:\"sender\"" ];
cosmos.base.v1beta1.Coin amount = 2 [
Expand All @@ -48,16 +58,18 @@ message MsgMint {
```

**State Modifications:**

- Safety check the following
- Check that the denom minting is created via `tokenfactory` module
- Check that the sender of the message is the admin of the denom
- Mint designated amount of tokens for the denom via `bank` module

### Burn

Burning of a specific denom is only allowed for the creator of the denom
registered during `CreateDenom`.

### Burn
- Burning of a specific denom is only allowed for the creator of the denom registered during `CreateDenom`
``` {.go}
```go
message MsgBurn {
string sender = 1 [ (gogoproto.moretags) = "yaml:\"sender\"" ];
cosmos.base.v1beta1.Coin amount = 2 [
Expand All @@ -68,15 +80,18 @@ message MsgBurn {
```

**State Modifications:**

- Saftey check the following
- Check that the denom minting is created via `tokenfactory` module
- Check that the sender of the message is the admin of the denom
- Burn designated amount of tokens for the denom via `bank` module


### ChangeAdmin
- Burning of a specific denom is only allowed for the creator of the denom registered during `CreateDenom`
``` {.go}

Burning of a specific denom is only allowed for the creator of the denom
Copy link
Member

Choose a reason for hiding this comment

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

I think this text might be about Burn, not ChangeAdmin

registered during `CreateDenom`.

```go
message MsgChangeAdmin {
string sender = 1 [ (gogoproto.moretags) = "yaml:\"sender\"" ];
string denom = 2 [ (gogoproto.moretags) = "yaml:\"denom\"" ];
Expand All @@ -85,33 +100,45 @@ message MsgChangeAdmin {
```

**State Modifications:**

- Check that sender of the message is the admin of denom
- Modify `AuthorityMetadata` state entry to change the admin of the denom

## Expectations from the chain

The chain's bech32 prefix for addresses can be at most 16 characters long.

This comes from denoms having a 128 byte maximum length, enforced from the SDK, and us setting longest_subdenom to be 44 bytes.
A token factory token's denom is:
`factory/{creator address}/{subdenom}`
This comes from denoms having a 128 byte maximum length, enforced from the SDK,
and us setting longest_subdenom to be 44 bytes.

A token factory token's denom is: `factory/{creator address}/{subdenom}`

Splitting up into sub-components, this has:
* `len(factory) = 7`
* `2 * len("/") = 2`
* `len(longest_subdenom)`
* `len(creator_address) = len(bech32(longest_addr_length, chain_addr_prefix))`.
Longest addr length at the moment is `32 bytes`.
Due to SDK error correction settings, this means `len(bech32(32, chain_addr_prefix)) = len(chain_addr_prefix) + 1 + 58`.

- `len(factory) = 7`
- `2 * len("/") = 2`
- `len(longest_subdenom)`
- `len(creator_address) = len(bech32(longest_addr_length, chain_addr_prefix))`.

Longest addr length at the moment is `32 bytes`. Due to SDK error correction
settings, this means `len(bech32(32, chain_addr_prefix)) = len(chain_addr_prefix) + 1 + 58`.
Adding this all, we have a total length constraint of `128 = 7 + 2 + len(longest_subdenom) + len(longest_chain_addr_prefix) + 1 + 58`.
Therefore `len(longest_subdenom) + len(longest_chain_addr_prefix) = 128 - (7 + 2 + 1 + 58) = 60`.

The choice between how we standardized the split these 60 bytes between maxes from longest_subdenom and longest_chain_addr_prefix is somewhat arbitrary. Considerations going into this:
* Per [BIP-0173](https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki#bech32) the technically longest HRP for a 32 byte address ('data field') is 31 bytes. (Comes from encode(data) = 59 bytes, and max length = 90 bytes)
* subdenom should be at least 32 bytes so hashes can go into it
* longer subdenoms are very helpful for creating human readable denoms
* chain addresses should prefer being smaller. The longest HRP in cosmos to date is 11 bytes. (`persistence`)
The choice between how we standardized the split these 60 bytes between maxes
from longest_subdenom and longest_chain_addr_prefix is somewhat arbitrary.
Considerations going into this:

- Per [BIP-0173](https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki#bech32)
the technically longest HRP for a 32 byte address ('data field') is 31 bytes.
(Comes from encode(data) = 59 bytes, and max length = 90 bytes)
- subdenom should be at least 32 bytes so hashes can go into it
- longer subdenoms are very helpful for creating human readable denoms
- chain addresses should prefer being smaller. The longest HRP in cosmos to date is 11 bytes. (`persistence`)

For explicitness, its currently set to `len(longest_subdenom) = 44` and `len(longest_chain_addr_prefix) = 16`.

Please note, if the SDK increases the maximum length of a denom from 128 bytes, these caps should increase.
So please don't make code rely on these max lengths for parsing.
Please note, if the SDK increases the maximum length of a denom from 128 bytes,
these caps should increase.

So please don't make code rely on these max lengths for parsing.
Loading