Skip to content

Commit

Permalink
fix: audit (#89)
Browse files Browse the repository at this point in the history 
* execute hook on multi-send

* do contract validation before setting before send hook

* remove MsgForceTransfer and burn_from_address from the MsgBurn

* fix lint
  • Loading branch information
@beer-1
beer-1 authored Jan 16, 2025
1 parent b41cc8d commit 01a6543
Show file tree
Hide file tree
Showing 14 changed files with 407 additions and 2,451 deletions.
1,756 changes: 273 additions & 1,483 deletions api/miniwasm/tokenfactory/v1/tx.pulsar.go
View file

Large diffs are not rendered by default.

41 changes: 0 additions & 41 deletions api/miniwasm/tokenfactory/v1/tx_grpc.pb.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 0 additions & 27 deletions proto/miniwasm/tokenfactory/v1/tx.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,6 @@ service Msg {
// hook of a denom.
rpc SetBeforeSendHook(MsgSetBeforeSendHook) returns (MsgSetBeforeSendHookResponse);

// ForceTransfer defines a gRPC service method for transferring a token from
// one account to another.
rpc ForceTransfer(MsgForceTransfer) returns (MsgForceTransferResponse);

// UpdateParams defines an operation for updating the x/tokenfactory module
// parameters.
rpc UpdateParams(MsgUpdateParams) returns (MsgUpdateParamsResponse);
Expand Down Expand Up @@ -106,10 +102,6 @@ message MsgBurn {
(gogoproto.moretags) = "yaml:\"amount\"",
(gogoproto.nullable) = false
];
string burn_from_address = 3 [
(gogoproto.moretags) = "yaml:\"burn_from_address\"",
(amino.dont_omitempty) = true
];
}

// MsgBurnResponse defines the response structure for an executed
Expand Down Expand Up @@ -166,25 +158,6 @@ message MsgSetDenomMetadata {
// MsgSetDenomMetadata message.
message MsgSetDenomMetadataResponse {}

// MsgForceTransfer is the sdk.Msg type for allowing an admin account to
// transfer a token from one account to another
message MsgForceTransfer {
option (cosmos.msg.v1.signer) = "sender";
option (amino.name) = "tokenfactory/MsgForceTransfer";

string sender = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];
cosmos.base.v1beta1.Coin amount = 2 [
(gogoproto.moretags) = "yaml:\"amount\"",
(gogoproto.nullable) = false
];
string transfer_from_address = 3 [(gogoproto.moretags) = "yaml:\"transfer_from_address\""];
string transfer_to_address = 4 [(gogoproto.moretags) = "yaml:\"transfer_to_address\""];
}

// MsgForceTransferResponse defines the response structure for an executed
// MsgForceTransfer message.
message MsgForceTransferResponse {}

// MsgUpdateParams is the Msg/UpdateParams request type.
message MsgUpdateParams {
option (cosmos.msg.v1.signer) = "authority";
Expand Down
20 changes: 16 additions & 4 deletions x/bank/keeper/msg_server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,8 @@ func (k msgServer) MultiSend(goCtx context.Context, msg *types.MsgMultiSend) (*t
return nil, types.ErrNoOutputs
}

if err := types.ValidateInputOutputs(msg.Inputs[0], msg.Outputs); err != nil {
input := msg.Inputs[0]
if err := types.ValidateInputOutputs(input, msg.Outputs); err != nil {
return nil, err
}

Expand All @@ -104,18 +105,29 @@ func (k msgServer) MultiSend(goCtx context.Context, msg *types.MsgMultiSend) (*t
}
}

inAddr, err := k.ak.AddressCodec().StringToBytes(input.Address)
if err != nil {
return nil, err
}

for _, out := range msg.Outputs {
accAddr, err := k.ak.AddressCodec().StringToBytes(out.Address)
outAddr, err := k.ak.AddressCodec().StringToBytes(out.Address)
if err != nil {
return nil, err
}

if k.BlockedAddr(accAddr) {
if k.BlockedAddr(outAddr) {
return nil, errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive funds", out.Address)
}

if err := k.BlockBeforeSend(ctx, inAddr, outAddr, out.Coins); err != nil {
return nil, err
}

k.TrackBeforeSend(ctx, inAddr, outAddr, out.Coins)
}

err := k.InputOutputCoins(ctx, msg.Inputs[0], msg.Outputs)
err = k.InputOutputCoins(ctx, input, msg.Outputs)
if err != nil {
return nil, err
}
Expand Down
12 changes: 3 additions & 9 deletions x/tokenfactory/client/cli/tx.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,9 @@ func NewMintCmd(ac address.Codec) *cobra.Command {

func NewBurnCmd(ac address.Codec) *cobra.Command {
cmd := &cobra.Command{
Use: "burn [amount] [burn-from-address] [flags]",
Use: "burn [amount] [flags]",
Short: "Burn tokens from an address. Must have admin authority to do so.",
Args: cobra.RangeArgs(1, 2),
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
clientCtx, err := client.GetClientTxContext(cmd)
if err != nil {
Expand All @@ -127,15 +127,9 @@ func NewBurnCmd(ac address.Codec) *cobra.Command {
return err
}

burnFromAddress := ""
if len(args) == 2 {
burnFromAddress = args[1]
}

msg := types.NewMsgBurnFrom(
msg := types.NewMsgBurn(
fromAddr,
amount,
burnFromAddress,
)

if err = msg.Validate(ac); err != nil {
Expand Down
140 changes: 3 additions & 137 deletions x/tokenfactory/keeper/admins_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,6 @@ func TestAdminMsgs(t *testing.T) {
require.NoError(t, err)
require.True(t, bankKeeper.GetBalance(ctx, addrs[1], defaultDenom).Amount.Int64() == addr1bal, bankKeeper.GetBalance(ctx, addrs[1], defaultDenom))

// Test force transferring
_, err = msgServer.ForceTransfer(ctx, types.NewMsgForceTransfer(addrs[0].String(), sdk.NewInt64Coin(defaultDenom, 5), addrs[1].String(), addrs[0].String()))
addr1bal -= 5
addr0bal += 5
require.NoError(t, err)
require.True(t, bankKeeper.GetBalance(ctx, addrs[0], defaultDenom).Amount.Int64() == addr0bal, bankKeeper.GetBalance(ctx, addrs[0], defaultDenom))
require.True(t, bankKeeper.GetBalance(ctx, addrs[1], defaultDenom).Amount.Int64() == addr1bal, bankKeeper.GetBalance(ctx, addrs[1], defaultDenom))

// Test burning from own account
_, err = msgServer.Burn(ctx, types.NewMsgBurn(addrs[0].String(), sdk.NewInt64Coin(defaultDenom, 5)))
require.NoError(t, err)
Expand Down Expand Up @@ -176,7 +168,6 @@ func TestBurnDenom(t *testing.T) {

tokenFactoryKeeper := input.TokenFactoryKeeper
bankKeeper := input.BankKeeper
accountKeeper := input.AccountKeeper
msgServer := tokenFactorykeeper.NewMsgServerImpl(tokenFactoryKeeper)
res, _ := msgServer.CreateDenom(ctx, types.NewMsgCreateDenom(addrs[0].String(), "bitcoin"))
defaultDenom := res.GetNewTokenDenom()
Expand All @@ -189,8 +180,6 @@ func TestBurnDenom(t *testing.T) {
balances[acc.String()] = 1000
}

moduleAdress := accountKeeper.GetModuleAddress(types.ModuleName)

for _, tc := range []struct {
desc string
burnMsg types.MsgBurn
Expand All @@ -204,15 +193,6 @@ func TestBurnDenom(t *testing.T) {
),
expectPass: false,
},
{
desc: "burn is not by the admin",
burnMsg: *types.NewMsgBurnFrom(
addrs[1].String(),
sdk.NewInt64Coin(defaultDenom, 10),
addrs[0].String(),
),
expectPass: false,
},
{
desc: "burn more than balance",
burnMsg: *types.NewMsgBurn(
Expand All @@ -229,133 +209,19 @@ func TestBurnDenom(t *testing.T) {
),
expectPass: true,
},
{
desc: "success case - burn from another address",
burnMsg: *types.NewMsgBurnFrom(
addrs[0].String(),
sdk.NewInt64Coin(defaultDenom, 10),
addrs[1].String(),
),
expectPass: true,
},
{
desc: "fail case - burn from module account",
burnMsg: *types.NewMsgBurnFrom(
addrs[0].String(),
sdk.NewInt64Coin(defaultDenom, 10),
moduleAdress.String(),
),
expectPass: false,
},
{
desc: "fail case - burn non-tokenfactory denom",
burnMsg: *types.NewMsgBurnFrom(
addrs[0].String(),
sdk.NewInt64Coin("uinit", 10),
moduleAdress.String(),
),
expectPass: false,
},
} {
t.Run(fmt.Sprintf("Case %s", tc.desc), func(t *testing.T) {
_, err := msgServer.Burn(ctx, &tc.burnMsg)
if tc.expectPass {
require.NoError(t, err)
balances[tc.burnMsg.BurnFromAddress] -= tc.burnMsg.Amount.Amount.Int64()
balances[tc.burnMsg.Sender] -= tc.burnMsg.Amount.Amount.Int64()
} else {
require.Error(t, err)
}

burnFromAddr, _ := sdk.AccAddressFromBech32(tc.burnMsg.BurnFromAddress)
burnFromAddr, _ := sdk.AccAddressFromBech32(tc.burnMsg.Sender)
bal := bankKeeper.GetBalance(ctx, burnFromAddr, defaultDenom).Amount
require.Equal(t, bal.Int64(), balances[tc.burnMsg.BurnFromAddress])
})
}
}

func TestForceTransferDenom(t *testing.T) {
ctx, input := createDefaultTestInput(t)

tokenFactoryKeeper := input.TokenFactoryKeeper
bankKeeper := input.BankKeeper
msgServer := tokenFactorykeeper.NewMsgServerImpl(tokenFactoryKeeper)
res, _ := msgServer.CreateDenom(ctx, types.NewMsgCreateDenom(addrs[0].String(), "bitcoin"))
defaultDenom := res.GetNewTokenDenom()

// mint 1000 default token for all addrs
balances := make(map[string]int64)
for _, acc := range addrs {
_, err := msgServer.Mint(ctx, types.NewMsgMintTo(addrs[0].String(), sdk.NewInt64Coin(defaultDenom, 1000), acc.String()))
require.NoError(t, err)
balances[acc.String()] = 1000
}

for _, tc := range []struct {
desc string
forceTransferMsg types.MsgForceTransfer
expectPass bool
}{
{
desc: "valid force transfer",
forceTransferMsg: *types.NewMsgForceTransfer(
addrs[0].String(),
sdk.NewInt64Coin(defaultDenom, 10),
addrs[1].String(),
addrs[2].String(),
),
expectPass: true,
},
{
desc: "denom does not exist",
forceTransferMsg: *types.NewMsgForceTransfer(
addrs[0].String(),
sdk.NewInt64Coin("factory/osmo1t7egva48prqmzl59x5ngv4zx0dtrwewc9m7z44/evmos", 10),
addrs[1].String(),
addrs[2].String(),
),
expectPass: false,
},
{
desc: "forceTransfer is not by the admin",
forceTransferMsg: *types.NewMsgForceTransfer(
addrs[1].String(),
sdk.NewInt64Coin(defaultDenom, 10),
addrs[1].String(),
addrs[2].String(),
),
expectPass: false,
},
{
desc: "forceTransfer is greater than the balance of",
forceTransferMsg: *types.NewMsgForceTransfer(
addrs[0].String(),
sdk.NewInt64Coin(defaultDenom, 10000),
addrs[1].String(),
addrs[2].String(),
),
expectPass: false,
},
} {
t.Run(fmt.Sprintf("Case %s", tc.desc), func(t *testing.T) {
_, err := msgServer.ForceTransfer(ctx, &tc.forceTransferMsg)
if tc.expectPass {
require.NoError(t, err)

balances[tc.forceTransferMsg.TransferFromAddress] -= tc.forceTransferMsg.Amount.Amount.Int64()
balances[tc.forceTransferMsg.TransferToAddress] += tc.forceTransferMsg.Amount.Amount.Int64()
} else {
require.Error(t, err)
}

fromAddr, err := sdk.AccAddressFromBech32(tc.forceTransferMsg.TransferFromAddress)
require.NoError(t, err)
fromBal := bankKeeper.GetBalance(ctx, fromAddr, defaultDenom).Amount
require.True(t, fromBal.Int64() == balances[tc.forceTransferMsg.TransferFromAddress])

toAddr, err := sdk.AccAddressFromBech32(tc.forceTransferMsg.TransferToAddress)
require.NoError(t, err)
toBal := bankKeeper.GetBalance(ctx, toAddr, defaultDenom).Amount
require.True(t, toBal.Int64() == balances[tc.forceTransferMsg.TransferToAddress])
require.Equal(t, bal.Int64(), balances[tc.burnMsg.Sender])
})
}
}
Expand Down
Loading

0 comments on commit 01a6543

Please sign in to comment.