bank.SendKeeper breaks intended least authority permissioning

[cwgoes]

bank.SendKeeper is intended only to allow transfers of coins between accounts, but in fact it allows arbitrary minting or burning of coins with keeper.inputOutputCoins, because the invariant inputs.Sum() == outputs.Sum() is only checked in ValidateBasic() on bank.MsgSend. Modules which are passed a bank.SendKeeper have just as much power as modules which are passed the full bank.BaseKeeper.

Recommended mitigation: remove inputOutputCoins from the sendKeeper (it isn't used in any other modules presently).
Alternative mitigation: repeat the necessary validation, which is very inexpensive, in the keeper.

[alexanderbez]

We'd need this for IBC, correct? I think my vote would be to remove it and possibly create a new IBC bank keeper which the IBC module uses?

[cwgoes]

We'd need this for IBC, correct? I think my vote would be to remove it and possibly create a new IBC bank keeper which the IBC module uses?

There already is such as keeper - BaseKeeper.

[alexanderbez]

I mean BaseKeeper w/o I/O coins.

[cwgoes]

I see, sure; let's wait to see exactly what the IBC module needs to do first though.