Missing Non-Negative for Coin Type in `MsgCreateGauge` Msg

[Hellobloc]

introduce

the MsgCreateGauge Msg don't validate non-negative properties which could lead a negative Coins.Add() process in fee charge process, this would been a risky problem which may lead some free-fee problem.

osmosis/x/incentives/types/msgs.go

Lines 42 to 87 in 0135789

	func (m MsgCreateGauge) ValidateBasic() error {
	lockType := m.DistributeTo.LockQueryType
	isNoLockGauge := lockType == lockuptypes.NoLock
	if m.Owner == "" {
	return errors.New("owner should be set")
	}
	if lockuptypes.LockQueryType_name[int32(m.DistributeTo.LockQueryType)] == "" {
	return errors.New("lock query type is invalid")
	}
	if m.StartTime.Equal(time.Time{}) {
	return errors.New("distribution start time should be set")
	}
	if m.NumEpochsPaidOver == 0 {
	return errors.New("distribution period should be at least 1 epoch")
	}
	if m.IsPerpetual && m.NumEpochsPaidOver != 1 {
	return errors.New("distribution period should be 1 epoch for perpetual gauge")
	}
	if lockType == lockuptypes.ByTime {
	return errors.New("start time distr conditions is an obsolete codepath slated for deletion")
	}
	if isNoLockGauge {
	if m.PoolId == 0 {
	return errors.New("pool id should be set for no lock distr condition")
	}
	if m.DistributeTo.Denom != "" {
	return errors.New(`no lock gauge denom should be unset. It will be automatically set to the NoLockExternalGaugeDenom(<pool id>)
	format internally, allowing for querying the gauges by denom with this prefix`)
	}
	} else {
	if m.PoolId != 0 {
	return errors.New("pool id should not be set for duration distr condition")
	}
	// For no lock type, the denom must be empty and we check that above.
	if err := sdk.ValidateDenom(m.DistributeTo.Denom); err != nil {
	return fmt.Errorf("denom should be valid for the condition, %s", err)
	}
	}
	return nil
	}

details

Just like i show below, the MsgCreateGauge need to been hanlder in chargeFeeIfSufficientFeeDenomBalance and CreateGauge function, but in their process, the amount of Coins don't been validated. As for this reason, people could send a Negative fee this would lead a free-fee CreateGauge process and people could also set a negative other coins to lead another logic problem.

func (server msgServer) CreateGauge(goCtx context.Context, msg *types.MsgCreateGauge) (*types.MsgCreateGaugeResponse, error) {
	ctx := sdk.UnwrapSDKContext(goCtx)
	owner, err := sdk.AccAddressFromBech32(msg.Owner)
	if err != nil {
		return nil, err
	}

	if err := server.keeper.chargeFeeIfSufficientFeeDenomBalance(ctx, owner, types.CreateGaugeFee, msg.Coins); err != nil {
		return nil, err
	}

	gaugeID, err := server.keeper.CreateGauge(ctx, msg.IsPerpetual, owner, msg.Coins, msg.DistributeTo, msg.StartTime, msg.NumEpochsPaidOver, msg.PoolId)
	if err != nil {
		return nil, errorsmod.Wrap(sdkerrors.ErrInvalidRequest, err.Error())
	}

	ctx.EventManager().EmitEvents(sdk.Events{
		sdk.NewEvent(
			types.TypeEvtCreateGauge,
			sdk.NewAttribute(types.AttributeGaugeID, osmoutils.Uint64ToString(gaugeID)),
		),
	})

	return &types.MsgCreateGaugeResponse{}, nil
}

When focus on chargeFeeIfSufficientFeeDenomBalance, we can see that the gaugeCoins just did a Add process. But Coins.Add would not validate the negative amount. so people could set a negative amount to feeDenom and in totalCost, this value will been a positive value after add a fee.

func (k Keeper) chargeFeeIfSufficientFeeDenomBalance(ctx sdk.Context, address sdk.AccAddress, fee osmomath.Int, gaugeCoins sdk.Coins) (err error) {
	feeDenom, err := k.tk.GetBaseDenom(ctx)
	if err != nil {
		return err
	}

	totalCost := gaugeCoins.AmountOf(feeDenom).Add(fee)
	accountBalance := k.bk.GetBalance(ctx, address, feeDenom).Amount

	if accountBalance.LT(totalCost) {
		return errorsmod.Wrapf(sdkerrors.ErrInsufficientFunds, "account's balance of %s (%s) is less than the total cost of the message (%s)", feeDenom, accountBalance, totalCost)
	}

	if err := k.ck.FundCommunityPool(ctx, sdk.NewCoins(sdk.NewCoin(feeDenom, fee)), address); err != nil {
		return err
	}
	return nil
}

Recommand

As there don't have any Negative Validate in MsgHandler process and Validate process. it is very necessary to add a Non-Negative validate in validatebase

Cosmos Coins Ref

func (coins Coins) Add(coinsB ...Coin) Coins {
	return coins.safeAdd(coinsB)
}

// safeAdd will perform addition of two coins sets. If both coin sets are
// empty, then an empty set is returned. If only a single set is empty, the
// other set is returned. Otherwise, the coins are compared in order of their
// denomination and addition only occurs when the denominations match, otherwise
// the coin is simply added to the sum assuming it's not zero.
// The function panics if `coins` or  `coinsB` are not sorted (ascending).
func (coins Coins) safeAdd(coinsB Coins) (coalesced Coins) {
	// probably the best way will be to make Coins and interface and hide the structure
	// definition (type alias)
	if !coins.isSorted() {
		panic("Coins (self) must be sorted")
	}
	if !coinsB.isSorted() {
		panic("Wrong argument: coins must be sorted")
	}

	uniqCoins := make(map[string]Coins, len(coins)+len(coinsB))
	// Traverse all the coins for each of the coins and coinsB.
	for _, cL := range []Coins{coins, coinsB} {
		for _, c := range cL {
			uniqCoins[c.Denom] = append(uniqCoins[c.Denom], c)
		}
	}

	for denom, cL := range uniqCoins { //#nosec
		comboCoin := Coin{Denom: denom, Amount: math.NewInt(0)}
		for _, c := range cL {
			comboCoin = comboCoin.Add(c)
		}
		if !comboCoin.IsZero() {
			coalesced = append(coalesced, comboCoin)
		}
	}
	if coalesced == nil {
		return Coins{}
	}
	return coalesced.Sort()
}

[Hellobloc]

@p0mvn @PaddyMc