Function BeforeTokenizeShareRecordRemoved
does not work as expected
Description
In the RedeemTokensForShares
function, when a user redeems all tokens, the delegation is deleted through unbonding, and the tokenized record is removed. Before the record is removed, the hook function BeforeTokenizeShareRecordRemoved
is called, which triggers WithdrawSingleShareRecordReward
to claim the reward.
// x/staking/keeper/msg_server.go
func (k msgServer) RedeemTokensForShares(goCtx context.Context, msg *types.MsgRedeemTokensForShares) (*types.MsgRedeemTokensForSharesResponse, error) {
// ...
returnAmount, err := k.Unbond(ctx, record.GetModuleAddress(), valAddr, shares)
// ...
// Note: since delegation object has been changed from unbond call, it gets latest delegation
_, err = k.GetDelegation(ctx, record.GetModuleAddress(), valAddr)
if err != nil && !errors.Is(err, types.ErrNoDelegation) {
return nil, err
}
// this err will be ErrNoDelegation
if err != nil {
if k.hooks != nil {
if err := k.hooks.BeforeTokenizeShareRecordRemoved(ctx, record.Id); err != nil {
return nil, err
}
}
err = k.DeleteTokenizeShareRecord(ctx, record.Id)
if err != nil {
return nil, err
}
}
// ...
// x/distribution/keeper/hooks.go
func (h Hooks) BeforeTokenizeShareRecordRemoved(ctx context.Context, recordID uint64) error {
err := h.k.WithdrawSingleShareRecordReward(ctx, recordID)
if err != nil {
h.k.Logger(ctx).Error(err.Error())
}
return err
}
However, in the WithdrawSingleShareRecordReward
function, it checks delegationFound
, which is always false
because the hook is called when ErrNoDelegation
is encountered. As a result, the reward-claiming process is never executed through this path.
// x/distribution/keeper/keeper.go
func (k Keeper) WithdrawSingleShareRecordReward(ctx context.Context, recordID uint64) error {
// ...
delegationFound := true
_, err = k.stakingKeeper.Delegation(ctx, record.GetModuleAddress(), valAddr)
if err != nil {
if !goerrors.Is(err, stakingtypes.ErrNoDelegation) {
return err
}
delegationFound = false
}
sdkCtx := sdk.UnwrapSDKContext(ctx)
if validatorFound && delegationFound {
// withdraw rewards into reward module account and send it to reward owner
cacheCtx, write := sdkCtx.CacheContext()
_, err = k.WithdrawDelegationRewards(cacheCtx, record.GetModuleAddress(), valAddr)
// ...
Impact
In current codebase, the Unbond
function calls the BeforeDelegationSharesModified
hook, and this function will claim rewards before removing the delegation. Nevertheless, the BeforeTokenizeShareRecordRemoved
hook does not work as expected, and the WithdrawDelegationRewards
is not reachable using this path.
Recommendations
Remove duplicate and unreachable code. If there is a possibility that the unbond hook does not exist, the call position should be adjusted to claim the rewards before the delegation is removed while redeeming all tokens.