Skip to content

Commit

Permalink
IF-STRIDE-STAKEIBC-UPDATEDELEGATIONBALANCES fix (#447)
Browse files Browse the repository at this point in the history
Underflow possible when decreasing staked balance for host zone

# Involved artifacts

- [x/stakeibc/keeper/icacallbacks_undelegate.go](https://github.com/Stride-Labs/stride/blob/v2.0.3/x/stakeibc/keeper/icacallbacks_undelegate.go#L128)

# Description

When undelegating from each validator, host zone's staked balance is being decreased according to undelegation amount of particular validator:

```go
for _, undelegation := range undelegateCallback.SplitDelegations {
		undelegateAmt, err := cast.ToInt64E(undelegation.Amount)
		k.Logger(ctx).Info(fmt.Sprintf("UndelegateCallback, Undelegation: %d, validator: %s", undelegateAmt, undelegation.Validator))
		if err != nil {
			errMsg := fmt.Sprintf("Could not convert undelegate amount to int64 in undelegation callback | %s", err.Error())
			k.Logger(ctx).Error(errMsg)
			return sdkerrors.Wrapf(types.ErrIntCast, errMsg)
		}
		undelegateVal := undelegation.Validator
		success := k.AddDelegationToValidator(ctx, zone, undelegateVal, -undelegateAmt)
		if !success {
			return sdkerrors.Wrapf(types.ErrValidatorDelegationChg, "Failed to remove delegation to validator")
		}
		zone.StakedBal -= undelegation.Amount
	}
```

However, `zone.StakedBal` is defined as `uint64`; therefore, it might underflow if `undelegation.Amount` > `zone.StakedBal`. Undelegation amount for each undelegation is calculated in [GetTargetValAmtsForHostZone](https://github.com/Stride-Labs/stride/blob/v2.0.3/x/stakeibc/keeper/unbonding_records.go#L74) and have been additionaly modified during [overflow](https://github.com/Stride-Labs/stride/blob/v2.0.3/x/stakeibc/keeper/unbonding_records.go#L103-L139) handling.


# Problem Scenarios

If underflow happens, staked balance of particular zone will unexpectedly and quietly become tremendously large having adverse consequences during both, `UndelegateCallback` and `DelegateCallback`.


# Recommendation

Add validation check before decreasing the staked balance amount: 

```go
  if undelegation.Amount > zone.StakedBal {
  // handle incoming underflow
  } else {
    zone.StakedBal -= undelegation.Amount
  }
```
  • Loading branch information
asalzmann authored Dec 3, 2022
1 parent 928618b commit a8a1658
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 1 deletion.
8 changes: 7 additions & 1 deletion x/stakeibc/keeper/icacallbacks_undelegate.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,13 @@ func (k Keeper) UpdateDelegationBalances(ctx sdk.Context, zone types.HostZone, u
if !success {
return sdkerrors.Wrapf(types.ErrValidatorDelegationChg, "Failed to remove delegation to validator")
}
zone.StakedBal -= undelegation.Amount
if undelegation.Amount > zone.StakedBal {
// handle incoming underflow
// Once we add a killswitch, we should also stop liquid staking on the zone here
return sdkerrors.Wrapf(types.ErrUndelegationAmount, "undelegation.Amount > zone.StakedBal, undelegation.Amount: %d, zone.StakedBal %d", undelegation.Amount, zone.StakedBal)
} else {
zone.StakedBal -= undelegation.Amount
}
}
k.SetHostZone(ctx, zone)
return nil
Expand Down
1 change: 1 addition & 0 deletions x/stakeibc/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,5 @@ var (
ErrHostZoneICAAccountNotFound = sdkerrors.Register(ModuleName, 1537, "host zone's ICA account not found")
ErrNoValidatorAmts = sdkerrors.Register(ModuleName, 1538, "could not fetch validator amts")
ErrMaxNumValidators = sdkerrors.Register(ModuleName, 1539, "max number of validators reached")
ErrUndelegationAmount = sdkerrors.Register(ModuleName, 1540, "Undelegation amount is greater than stakedBal")
)

0 comments on commit a8a1658

Please sign in to comment.