Description

In the BeginBlock function of the keyshare module, validators who are no longer in the bonded status are removed from the validator set. However, their associated authorized addresses are not removed, which can result in orphaned references to these validators. This is observed in the following code snippet:

bondedVal, err := am.stakingKeeper.GetValidator(ctx, sdk.ValAddress(accAddr))
if err != nil {
    am.keeper.RemoveValidatorSet(ctx, eachValidator.Validator)
    continue
}
if !bondedVal.IsBonded() {
    am.keeper.RemoveValidatorSet(ctx, eachValidator.Validator)
}

In this case, RemoveValidatorSet removes the validator from the set but does not clean up the authorized addresses associated with it. Although this does not currently lead to security issues — since there is a check before each use of authorized addresses — it leaves invalid references in AllAuthorizedAddress, which could result in minor inefficiencies or unintended behavior in future updates.

For comparison, the DeRegisterValidator function removes both the validator and its associated authorized addresses, as shown here:

// DeRegisterValidator removes a validator from the validator set and its associated authorized addresses.
func (k msgServer) DeRegisterValidator(goCtx context.Context, msg *types.MsgDeRegisterValidator) (*types.MsgDeRegisterValidatorResponse, error) {
    ctx := sdk.UnwrapSDKContext(goCtx)

    _, found := k.GetValidatorSet(ctx, msg.Creator)
    if !found {
        return nil, types.ErrValidatorNotRegistered.Wrap(msg.Creator)
    }

    for _, v := range k.GetAllAuthorizedAddress(ctx) {
        if v.AuthorizedBy == msg.Creator {
            k.RemoveAuthorizedAddress(ctx, v.Target)
            k.DecreaseAuthorizedCount(ctx, msg.Creator)
            break
        }
    }

    k.RemoveValidatorSet(ctx, msg.Creator)

    ctx.EventManager().EmitEvent(
        sdk.NewEvent(types.DeRegisteredValidatorEventType,
            sdk.NewAttribute(types.DeRegisteredValidatorEventCreator, msg.Creator),
        ),
    )

    return &types.MsgDeRegisterValidatorResponse{
        Creator: msg.Creator,
    }, nil
}

In DeRegisterValidator, authorized addresses linked to the validator are removed alongside the validator itself. This prevents orphaned references and ensures efficient resource management.

Impact

This issue leads to orphaned authorized addresses for validators no longer bonded, resulting in unused references in AllAuthorizedAddress. While this does not pose a direct security risk, it may lead to minor inefficiencies and complicates resource management.

Recommendations

To maintain efficient resource management and consistency, ensure that authorized addresses associated with removed validators are also removed in BeginBlock. Implementing this change will align BeginBlock's behavior with DeRegisterValidator, which removes both the validator and its associated authorized addresses.

Remediation

This issue has been acknowledged by Fairblock Inc., and a fix was implemented in commit 5b830510↗.