x/interchainstaking: Keeper.UpdateWithdrawalRecordsForSlash should validate that each Distribution.Valoper's amount is positive before additions otherwise an overflow can trigger wrong results

[odeke-em]

Summary of Bug

This code

quicksilver/x/interchainstaking/keeper/keeper.go

Lines 362 to 363 in bbf9ef9

	newAmount := sdk.NewDec(int64(d.Amount)).Quo(delta).TruncateInt()
	thisSubAmount := sdkmath.NewInt(int64(d.Amount)).Sub(newAmount)

blindly creates an sdk.NewInt(int64(d.Amount)) then performs operations on it assuming the result will always be positive! Same as in

quicksilver/x/interchainstaking/keeper/keeper.go

Line 369 in bbf9ef9

record.Amount = record.Amount.Sub(sdk.NewCoin(zone.BaseDenom, recordSubAmount))

Firstly let's note that WithdrawalRecord.Distribution[].Amount is a uint64 per

quicksilver/proto/quicksilver/interchainstaking/v1/interchainstaking.proto

Line 86 in c26b22e

uint64 amount = 2;

If someone wanted to control how much money was to be withdrawn they could intercept traffic or maliciously set a large to be sent into amount that's larger than int64 (given .Amount is a uint64) and that would cause a negative value which could then when subtracted from record.Amount cause a large value or any value of the attacker's choosing

Suggestion

dAmount := sdk.NewDec(int64(d.Amount))
if dAmount.IsNegative() {
    // TODO: Log or report or punish for this serious overflow
    continue
}

then also for

quicksilver/x/interchainstaking/keeper/keeper.go

Line 369 in bbf9ef9

record.Amount = record.Amount.Sub(sdk.NewCoin(zone.BaseDenom, recordSubAmount))

we could do

deductedTotal := record.Amount.Sub(sdk.NewCoin(zone.BaseDenom, recordSubAmount))
if deductedTotal.GTE(record.Amount) {
    // TODO: Log or report or punish for this serious overflow
    return false
}

record.Distribution = distr
record.Amount = deductedTotal
k.SetWithdrawalRecord(ctx, record)

Version

13bf586

/cc @elias-orijtech

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[ThanhNhann]

I see this code is updated WithdrawalRecords for the slashing when the validator jailed so I think the result will always be positive
cc: @joe-bowman