bug: lookup and return validators from liquid stake accounting functions

[sampocs]

Overview of Bug

When re-delegating to an existing validator bond delegation, upstream changes to the validator struct are accidentally overwritten, preventing a validator set update. More specifically, in msg_server.Delegate, IncreaseValidatorBondShares is called with a validator struct; however, at this point, the validator struct is stale relative to the underlying validator in the store, which was modified above by k.Delegate. Thus, the validator in the store is overwritten.

[Relevant code]

validator, found := k.GetValidator(ctx, valAddress)
...

k.Delegate(..., validator,...) // updates the validator in the store but does not change the validator variable
...

if delegation.ValidatorBond {
   k.IncreaseValidatorBondShares(..., validator, ...) // stale validator passed, changes from k.Delegate overwritten
}

Solution

The solution to the specific bug was to re-read the validator from the store in IncreaseValidatorBondShares so it doesn't accidentally overwrite earlier changes.

To be extra cautious, the other validator liquid stake accounting functions were also modified with this same behavior. Additionally, a validator was returned from the function rather than relying on the calling function to refresh the variable with a lookup after. This was not entirely necessary and, there are no bugs in these other functions to our knowledge, but this should give some added protection should these functions be re-ordered in the future.

Old:

func SomeAccountingFunction(validator types.Validator) error { // validator passed
   validator.Attribute = something
   k.SetValidator(validator)
   return nil
}

func SomeMsgServerFunction() {
   validator, found := k.GetValidator(valAddress) 
   if !found {
      return err
   }

   if err := SomeAccountingFunction(validator) {
      return err
   }

   validator, found := k.GetValidator(valAddress) // calling function must remember to look up validator again
   if !found {
      return err
   }

  k.SomeDownstreamFunction(validator) 
}

New:

func SomeAccountingFunction(valAddress sdk.ValAddress) (types.Validator, error) { // val address passed instead
   validator, found := k.GetValidator(valAddress) // fresh lookup
   if !found {
      return err
   }
   validator.Attribute = something
   k.SetValidator(validator)
   return validator, nil // returns validator
}

func SomeMsgServerFunction() {
   validator, found := k.GetValidator(valAddress) 
   if !found {
      return err
   }

   validator, err = SomeAccountingFunction(validatorAddress) // no need to re-read from store after
   if err != nil {
      return err
   }

  k.SomeDownstreamFunction(validator) 
}

Brief Changelog

• Modified IncreaseValidatorBond, SafelyDecreaseValidatorBond, SafelyIncreaseValidatorLiquidShares and DecreaseValidatorLiquidShares to take the val address as a parameter and do a validator lookup at the start
• Modified SafelyIncreaseValidatorLiquidShares and DecreaseValidatorLiquidShares to return the new validator from the function

[mpoke]

LGTM.