Missing Non-Negative Check for Coin Type in Msg #989
Description
the Unlock Msg don't validate non-negative properties which could lead a negative Coins.Add() process in Unlock Msghanlder, this would been a risky problem which may lead some logic problem. This Issue would lead Causes an error in the Unlock function implementation, causing malicious to lock the amount that has been unlocked in an victim account.
Lines 34 to 46 in 6d83b40
As your following code don't call any function which could check the amount value, but just use Coins.Add function, this function don't check the amount. So the Non-Negative validate is necessary for your project
func (k msgServer) Unlock(goCtx context.Context, msg *types.MsgUnlock) (*types.MsgUnlockResponse, error) {
ctx := sdk.UnwrapSDKContext(goCtx)
issuerAddr, err := sdk.AccAddressFromBech32(msg.Issuer)
if err != nil {
return nil, err
}
accountAddr, err := sdk.AccAddressFromBech32(msg.Account)
if err != nil {
return nil, err
}
// preliminary checks
acc := k.ak.GetAccount(ctx, accountAddr)
if acc == nil {
return nil, errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "account %s does not exist", accountAddr)
}
mvacc, ok := acc.(*types.ManualVestingAccount)
if !ok {
return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "receiver account is not a manual vesting account")
}
unlocker, err := sdk.AccAddressFromBech32(mvacc.Unlocker)
if err != nil {
return nil, err
}
if !issuerAddr.Equals(unlocker) {
return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "sender of this transaction is not the designated unlocker")
}
if mvacc.VestedCoins.Add(msg.UnlockAmount...).IsAnyGT(mvacc.OriginalVesting) {
return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "cannot unlock more than the original vesting amount")
}
// update vested coins
mvacc.VestedCoins = mvacc.VestedCoins.Add(msg.UnlockAmount...)
if mvacc.DelegatedVesting.IsAllGT(mvacc.OriginalVesting.Sub(mvacc.VestedCoins...)) {
unlockedDelegated := mvacc.DelegatedVesting.Sub(mvacc.OriginalVesting.Sub(mvacc.VestedCoins...)...)
mvacc.DelegatedVesting = mvacc.DelegatedVesting.Sub(unlockedDelegated...)
mvacc.DelegatedFree = mvacc.DelegatedFree.Add(unlockedDelegated...)
}
k.ak.SetAccount(ctx, mvacc)
ctx.EventManager().EmitEvent(
sdk.NewEvent(
types.EventTypeUnlock,
sdk.NewAttribute("unlocker", issuerAddr.String()),
sdk.NewAttribute("account", accountAddr.String()),
sdk.NewAttribute(sdk.AttributeKeyAmount, msg.UnlockAmount.String()),
),
)
return &types.MsgUnlockResponse{}, nil
}