Skip to content

Latest commit

 

History

History
247 lines (176 loc) · 10.7 KB

070.md

File metadata and controls

247 lines (176 loc) · 10.7 KB

Scruffy Gingerbread Cormorant

High

Incorrect BTC Delegation Reward Calculation Due to Using Current Stake Amount Instead of Historical Stake

Summary

Missing historical stake tracking in the incentive reward system will cause incorrect reward distribution for BTC delegators as changing delegation amounts after a period but before rewards are calculated will result in rewards based on current stake rather than historical stake.

Root Cause

In https://github.com/sherlock-audit/2024-12-babylon/blob/main/babylon/x/incentive/keeper/reward_tracker.go#L200 the calculateDelegationRewardsBetween function calculates rewards using the delegation's current active satoshi amount rather than the historical amount that was active during the reward period. This means:

// This incorrectly uses current stake amount (btcDelRwdTracker.TotalActiveSat)
// rather than the historical amount that was active during that period
rewardsWithDecimals := differenceWithDecimals.MulInt(btcDelRwdTracker.TotalActiveSat)

This issue exists because the BTCDelegationRewardsTracker struct stores only the current stake amount and starting period, without preserving historical stake amounts per period. When delegations are modified (through unbonding or new activations), the stake amount is immediately updated, but there's no record of what it was at different periods. However, rewards of a block may be delayed (they are always delayed by FinalitySigTimeout to allow for voting time, but can be massively delayed when governance resumes finality after halt). Finality providers are rewarded in proportion to the correct cached total bonded delegations, however individual delegators may receive incorrect reward out of that based on their current stake.

Internal Pre-conditions

NA

External Pre-conditions

NA

Attack Path

  1. Delegator A has 100,000 sats delegated to a finality provider during Period 1
  2. Rewards accrue but are not yet calculated and distributed as finality has halted.
  3. Before rewards are calculated, Delegator A reduces their delegation to 50,000 sats (period changes to Period 2)
  4. A governance action triggers calculation of rewards for Period 1 after resuming finality
  5. The system incorrectly uses the current stake amount (50,000 sats) to calculate Period 1's rewards instead of the original amount (100,000 sats)
  6. Delegator A receives only half the rewards they should have earned

Alternatively:

  1. Delegator B has 50,000 sats delegated to a finality provider during Period 1
  2. Rewards accrue for Period 1 but are not yet calculated and distributed as finality has halted.
  3. Before rewards are calculated, Delegator B increases their delegation to 100,000 sats (period changes to Period 2)
  4. A governance action triggers calculation of rewards for Period 1
  5. The system incorrectly uses the current stake amount (100,000 sats) to calculate Period 1's rewards
  6. Delegator B receives double the rewards they should have earned

Please not this issue can happen (even without finality halt and governance resumption) for FinalitySigTimeout blocks, as protocol waits for FinalitySigTimeout blocks before finalizing a block's reward and unbonding/ bonding in that period can cause this issue.

Impact

The BTC delegators suffer from incorrect reward distribution. Those who decrease their stake before rewards are calculated receive fewer rewards than they deserve, while those who increase their stake receive more rewards than they deserve. This creates unfairness in the protocol and could potentially be exploited by sophisticated users who understand the timing of reward calculations specially in cases of governance resumption of finality.

PoC

To demonstrate the issue with concrete examples, let's walk through how the incentive system handles reward calculations when delegations change, and why this leads to incorrect reward distribution.

Scenario: Delegator Reduces Stake After Period 1 But Before Rewards Are Calculated Due to Governance Action

Initial Setup

  1. DelegatorA has 100,000 sats staked to FinalityProviderX during Period 1
  2. A finality halt occurs during Period 1, preventing rewards from being distributed
  3. Period 2 begins with the halt still in effect if delegator unbonds

Step 1: Delegation Reduction in Period 2

// DelegatorA calls to unbond 50,000 sats creating Period 2
k.BtcDelegationUnbonded(ctx, fpAddr, delegatorAddr, 50000)

// This function calls:
k.btcDelegationModifiedWithPreInitDel(ctx, fp, del, function(ctx, fp, del) {
    return k.subDelegationSat(ctx, fp, del, 50000)
})

// Which increments the period and calculates rewards for current period:
endedPeriod, err := k.IncrementFinalityProviderPeriod(ctx, fp)

// The delegation's TotalActiveSat is reduced:
if err := k.subDelegationSat(ctx, fp, del, amtSat); err != nil {
    return err
}

Here, the system:

  1. Increments the period from 1 to 2
  2. Attempts to calculate rewards for Period 1, but since there was a finality halt, no rewards are distributed yet
  3. Reduces the delegation's TotalActiveSat from 100,000 to 50,000
  4. Initializes a new BTCDelegationRewardsTracker with 50,000 sats and a starting period of 2

Step 2: Governance Action to Resume Finality

Later, a governance proposal is submitted to resume finality:

// HandleResumeFinalityProposal is called with the finality providers to jail
k.HandleResumeFinalityProposal(ctx, fpPksHex, haltingHeight)

// This jails the specified FPs and recalculates voting power for previous heights
for h := uint64(haltingHeight); h <= uint64(currentHeight); h++ {
    distCache := k.GetVotingPowerDistCache(ctx, h)
    // ... updating distCache ...
    k.SetVotingPowerDistCache(ctx, h, distCache)
}

// Then it tallies blocks
k.TallyBlocks(ctx)

Step 3: Reward Distribution

The TallyBlocks finalizes and eventually leads to reward distribution for previously finalized blocks in HandleRewarding (called via EndBlocker)

// HandleRewarding calls the reward to stakers if the block is finalized
func (k Keeper) HandleRewarding(ctx context.Context, targetHeight int64) {
	...
	for height := nextHeightToReward; height <= uint64(targetHeight); height++ {

		if !block.Finalized {
			continue
		}
		k.rewardBTCStaking(ctx, height)
	
	}

}

Step 4: Reward Calculation in RewardBTCStaking

Now, RewardBTCStaking but after DelegatorA's stake has been reduced:

// RewardBTCStaking distributes rewards based on voting power
func (k Keeper) RewardBTCStaking(ctx context.Context, height uint64, dc *ftypes.VotingPowerDistCache, voters map[string]struct{}) {
    // Get the gauge for this height
    gauge := k.GetBTCStakingGauge(ctx, height)
    
    // Calculate total voting power of voters
    var totalVotingPowerOfVoters uint64
    for i, fp := range dc.FinalityProviders {
        if _, ok := voters[fp.BtcPk.MarshalHex()]; ok {
            totalVotingPowerOfVoters += fp.TotalBondedSat
        }
    }
    
    // Distribute rewards to FPs and delegations
    for _, fp := range dc.FinalityProviders {
        if _, ok := voters[fp.BtcPk.MarshalHex()]; !ok {
            continue
        }
        
        // Calculate portion of rewards for this FP
        fpPortion := sdkmath.LegacyNewDec(int64(fp.TotalBondedSat)). => this is cached total bonded delegations to the F
            QuoTruncate(sdkmath.LegacyNewDec(int64(totalVotingPowerOfVoters)))
        coinsForFpsAndDels := gauge.GetCoinsPortion(fpPortion)
        
        // Give commission to FP
        coinsForCommission := types.GetCoinsPortion(coinsForFpsAndDels, *fp.Commission)
        k.accumulateRewardGauge(ctx, types.FinalityProviderType, fp.GetAddress(), coinsForCommission)
        
        // Distribute remaining rewards to delegations
        coinsForBTCDels := coinsForFpsAndDels.Sub(coinsForCommission...)
        k.AddFinalityProviderRewardsForBtcDelegations(ctx, fp.GetAddress(), coinsForBTCDels)
    }
}

Important Note: The finality provider's share of rewards is correctly calculated based on the cached voting power distribution (dc) from. This is properly preserved in the system and is not affected by the issue. The problem only occurs at the individual delegator level when rewards are calculated for each delegator.

This leads to:

// AddFinalityProviderRewardsForBtcDelegations adds rewards to the FP's current rewards
func (k Keeper) AddFinalityProviderRewardsForBtcDelegations(ctx context.Context, fp sdk.AccAddress, rwd sdk.Coins) error {
    fpCurrentRwd, err := k.GetFinalityProviderCurrentRewards(ctx, fp)
    if err != nil {
        return err
    }

    // Add rewards to the FP's current rewards pool
    fpCurrentRwd.AddRewards(rwd)
    return k.setFinalityProviderCurrentRewards(ctx, fp, fpCurrentRwd)
}

Step 5: Later, When Delegator Withdraws Rewards

When DelegatorA later calls to withdraw rewards:

// MsgWithdrawReward handler calls:
k.sendAllBtcDelegationTypeToRewardsGauge(ctx, sType, addr)

// Which calls:
k.sendAllBtcRewardsToGauge(ctx, del)

// Which iterates through all FPs associated with this delegator:
k.iterBtcDelegationsByDelegator(ctx, del, func(del, fp sdk.AccAddress) error {
    return k.btcDelegationModified(ctx, fp, del)
})

// This calls:
k.btcDelegationModifiedWithPreInitDel(ctx, fp, del, func(ctx context.Context, fp, del sdk.AccAddress) error { return nil })

// Which leads to:
endedPeriod, err := k.IncrementFinalityProviderPeriod(ctx, fp)

// And then:
k.CalculateBTCDelegationRewardsAndSendToGauge(ctx, fp, del, endedPeriod)

// Which calls:
rewards, err := k.CalculateBTCDelegationRewards(ctx, fp, del, endPeriod)

// Finally leading to the problematic calculation:
func (k Keeper) calculateDelegationRewardsBetween(
    ctx context.Context,
    fp sdk.AccAddress,
    btcDelRwdTracker types.BTCDelegationRewardsTracker,
    endingPeriod uint64,
) (sdk.Coins, error) {
    // ... get historical rewards ...
    
    // Calculate difference in rewards per satoshi
    differenceWithDecimals := ending.CumulativeRewardsPerSat.Sub(starting.CumulativeRewardsPerSat...)
    
    // HERE IS THE ISSUE:
    // It uses current stake (50,000) instead of correct historical stake (100,000)
    rewardsWithDecimals := differenceWithDecimals.MulInt(btcDelRwdTracker.TotalActiveSat)
    
    rewards := rewardsWithDecimals.QuoInt(types.DecimalAccumulatedRewards)
    return rewards, nil
}

Result: DelegatorA receives only 5 BBN in rewards instead of the 10 BBN they should have received based on their stake during Period 1, because the rewards were calculated using their current stake (50,000 sats) rather than their historical stake (100,000 sats).

Even worse, in a similar way if DelegatorA increases their stake, it will unfairly steal rewards of other delegators.

Mitigation

No response