015.md

Curved Glossy Dragonfly

High

Any error from GetBTCDelegationRewardsTracker and GetFinalityProviderCurrentRewards is interepreted as not found(i.e., key doesn't exist) , this could lead to incorrect state changes in case error throw is not due to the missing key

Summary

Due to incorrect error handling in GetBTCDelegationRewardsTracker and GetFinalityProviderCurrentRewards function, stakeholders may lose their earned rewards for a period of time.

Vulnerability Details

During x/incentive/keeper/msg_server::WithdrawReward() call, the CalculateBTCDelegationRewardsAndSendToGauge() calculates the rewards of delegator based on the starting and end period, which later get updated to the delegator rewardGauge store via k.accumulateRewardGauge().

// CalculateBTCDelegationRewardsAndSendToGauge calculates the rewards of the delegator based on the
// StartPeriodCumulativeReward and the received endPeriod and sends to the delegator gauge.
func (k Keeper) CalculateBTCDelegationRewardsAndSendToGauge(ctx context.Context, fp, del sdk.AccAddress, endPeriod uint64) error {
	rewards, err := k.CalculateBTCDelegationRewards(ctx, fp, del, endPeriod)
	if err != nil {
		if !errors.Is(err, types.ErrBTCDelegationRewardsTrackerNotFound) {
			return err
		}
		rewards = sdk.NewCoins()
	}

	if rewards.IsZero() {
		return nil
	}

	k.accumulateRewardGauge(ctx, types.BTCDelegationType, del, rewards)
	return nil
}

The CalculateBTCDelegationRewards calculate the rewards accrued b/w the endPeriod and the StartPeriodCumulativeReward and returned it. It utilize GetBTCDelegationRewardsTracker to retrieve the info such as active delegated staked to fp, and the timestamp of last withdraw((i.e. StartPeriodCumulativeReward).

// GetBTCDelegationRewardsTracker returns the BTCDelegationRewardsTracker based on the delegation key (fp, del)
// It returns an error in case the key is not found.
func (k Keeper) GetBTCDelegationRewardsTracker(ctx context.Context, fp, del sdk.AccAddress) (types.BTCDelegationRewardsTracker, error) {
	value, err := k.BTCDelegationRewardsTracker.Get(ctx, collections.Join(fp.Bytes(), del.Bytes()))
	if err != nil {
		return types.BTCDelegationRewardsTracker{}, types.ErrBTCDelegationRewardsTrackerNotFound   // @audit-issue
	}
	return value, nil
}

Note that in case the k.BTCDelegationRewardsTracker.Get(ctx, collections.Join(fp.Bytes(), del.Bytes())) get call, it may throw error, possibly due to Decoding/I/O issue, an error other than the types.ErrBTCDelegationRewardsTrackerNotFound. However, since default error types is considered to be types.ErrBTCDelegationRewardsTrackerNotFound, it proceed wrapping up with accumulateRewardGauge() call with empty coins, along with k.setBTCDelegationRewardsTracker in upper function, which updates the last StartPeriodCumulativeReward value to a new value. The accumulated rewards for the ending-starting period is lost forever.

Same issue in GetFinalityProviderCurrentRewards, which also has critical role in updating the fp rewards.

Impact

Loss of delegator rewads

Mitigation

Return err in case its other than the "not found" one