Authorization logic of staking module: panic when handling amounts greater than authorized

[ivan-gavran]

Summary of Bug

The Accept function of staking modules's authz.go file checks whether the amount to delegate is covered by the authorization. It does so by calling the Sub function (line 106). This function will panic if the resulting amount is negative.

This is in contrast to how a similar check in the bank module handles the same situation: there, the SafeSub function is called, and the ErrInsusfficientFunds is returned if the amount is negative.

Wouldn't it be better if both modules were handling this situation in the same way? I believe that returning an error would be the way to go (but perhaps I am missing some context).

I ran into this issue when working on model-based testing of authz, generating tests covering the situation in which insufficient grant was provided for an execution.

Version

master (commit: 5491be2)

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[ivan-gavran]

hi, @atheeshp ! I just noticed that the issue was closed by a recent commit: could you please give some context? From what I can read from the commit, the panicking is removed by using SafeSub, so the banks's way was the right approach?

Furthermore, the SafeSub was used in the commit, but not to calculate the final amount (instead, SafeSub was used to check for errors, and then followed by Sub if there is none). Why is that?

[atheeshp]

hi, @atheeshp ! I just noticed that the issue was closed by a recent commit: could you please give some context? From what I can read from the commit, the panicking is removed by using SafeSub, so the banks's way was the right approach?

Furthermore, the SafeSub was used in the commit, but not to calculate the final amount (instead, SafeSub was used to check for errors, and then followed by Sub if there is none). Why is that?

we have SafeSub method for sdk.Coins, but in authz sdk.Coin is being used at a.MaxTokens, there is no SafeSub method for sdk.Coin. because of that I've converted a.MaxTokens (which is sdk.Coin) to sdk.Coins to use SafeSub method here.

cosmos-sdk/x/staking/types/authz.go

Lines 106 to 109 in 9f6f9f5

	// check sufficient balance exists.
	if _, isNegative := sdk.NewCoins(*a.MaxTokens).SafeSub(sdk.NewCoins(amount)); isNegative {
	return authz.AcceptResponse{}, sdkerrors.ErrInsufficientFunds.Wrapf("amount is more than max tokens")
	}

[ivan-gavran]

cool, thanks for the explanation!

[dalmirel]

Hello @atheeshp ! When you find some time, could you please explain is there some particular reason why SafeSub func could not be implemented for sdk.Coin in order for you to skip the conversion to sdk.Coins here?
(I assume you just did not want to introduce changes to other packages...)

It looks to me, that introducting SafeSub for sdk.Coin could be used in a same way as SafeSub for sdk.Coins:

cosmos-sdk/types/coin.go

Lines 386 to 393 in 8800d2e

	func (coins Coins) Sub(coinsB Coins) Coins {
	diff, hasNeg := coins.SafeSub(coinsB)
	if hasNeg {
	panic("negative coin amount")
	}
	return diff
	}

That is: for sdk.Coin SafeSub could be called from Sub func and if it returns true for negative coin value result
the Sub function could panic or the result of subtraction would be returned.
This new, one function could be called from authz.go instead of both conversion and:

cosmos-sdk/x/staking/types/authz.go

Line 111 in 9f6f9f5

limitLeft := a.MaxTokens.Sub(amount)

and you could gain the necessary information for returning ErrInsufficientFunds err and the limitLeft with one function call.

Since I will be working on model-based testing of authz module as well, but I'm also doing some code inspection for possible improvements, every information could be useful.

Thanks in advance!