Slow Misty Iguana
High
Due to the reward delay FinalitySigTimeout(even more) being accumulated in FinalityProviderCurrentRewards, the rewards obtained by a delegation might not correspond to the blocks in which it actually participated.
All delegation rewards are calculated through FinalityProviderCurrentRewards, but there is no snapshot of the current block number.
https://github.com/sherlock-audit/2024-12-babylon/blob/main/babylon/x/finality/abci.go#L43
func EndBlocker(ctx context.Context, k keeper.Keeper) ([]abci.ValidatorUpdate, error) {
defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyEndBlocker)
// if the BTC staking protocol is activated, i.e., there exists a height where a finality provider
// has voting power, start indexing and tallying blocks
if k.IsFinalityActive(ctx) {
// index the current block
k.IndexBlock(ctx)
// tally all non-finalised blocks
k.TallyBlocks(ctx)
@> heightToExamine := sdk.UnwrapSDKContext(ctx).HeaderInfo().Height - k.GetParams(ctx).FinalitySigTimeout
if heightToExamine >= 1 {
k.HandleLiveness(ctx, heightToExamine)
@> k.HandleRewarding(ctx, heightToExamine)
}
}
return []abci.ValidatorUpdate{}, nil
}We observe that when the height block concludes, the reward for the height - FinalitySigTimeout block is distributed through the HandleRewarding() function.
The issue lies in the fact that this reward is allocated to the delegations that were active during the height, height-1, height-2, and other blocks after height - FinalitySigTimeout.
func (k Keeper) HandleRewarding(ctx context.Context, targetHeight int64) {
// rewarding is executed in a range of [nextHeightToReward, heightToExamine]
// this is we don't know when a block will be finalized and we need ensure
// every finalized block will be processed to reward
nextHeightToReward := k.GetNextHeightToReward(ctx)
if nextHeightToReward == 0 {
// first time to call reward, set it to activated height
activatedHeight, err := k.GetBTCStakingActivatedHeight(ctx)
if err != nil {
panic(err)
}
nextHeightToReward = activatedHeight
}
copiedNextHeightToReward := nextHeightToReward
@> for height := nextHeightToReward; height <= uint64(targetHeight); height++ {
block, err := k.GetBlock(ctx, height)
if err != nil {
panic(err)
}
if !block.Finalized {
continue
}
@> k.rewardBTCStaking(ctx, height)
nextHeightToReward = height + 1
}
if nextHeightToReward != copiedNextHeightToReward {
k.SetNextHeightToReward(ctx, nextHeightToReward)
}
}Through HandleRewarding(), we observe that the rewards being distributed may correspond to blocks earlier than the targetHeight.
func (k Keeper) rewardBTCStaking(ctx context.Context, height uint64) {
// distribute rewards to BTC staking stakeholders w.r.t. the voting power distribution cache
dc := k.GetVotingPowerDistCache(ctx, height)
if dc == nil {
// failing to get a voting power distribution cache before distributing reward is a programming error
panic(fmt.Errorf("voting power distribution cache not found at height %d", height))
}
// get all the voters for the height
voterBTCPKs := k.GetVoters(ctx, height)
// reward active finality providers
@> k.IncentiveKeeper.RewardBTCStaking(ctx, height, dc, voterBTCPKs)
// remove reward distribution cache afterwards
k.RemoveVotingPowerDistCache(ctx, height)
}In the rewardBTCStaking() function, the RewardBTCStaking() function from the Incentive module is invoked.
https://github.com/sherlock-audit/2024-12-babylon/blob/main/babylon/x/incentive/keeper/btc_staking_gauge.go#L19
func (k Keeper) RewardBTCStaking(ctx context.Context, height uint64, dc *ftypes.VotingPowerDistCache, voters map[string]struct{}) {
gauge := k.GetBTCStakingGauge(ctx, height)
if gauge == nil {
// failing to get a reward gauge at previous height is a programming error
panic("failed to get a reward gauge at previous height")
}
// calculate total voting power of voters
var totalVotingPowerOfVoters uint64
for i, fp := range dc.FinalityProviders {
if i >= int(dc.NumActiveFps) {
break
}
if _, ok := voters[fp.BtcPk.MarshalHex()]; ok {
totalVotingPowerOfVoters += fp.TotalBondedSat
}
}
// distribute rewards according to voting power portions for voters
for i, fp := range dc.FinalityProviders {
if i >= int(dc.NumActiveFps) {
break
}
if _, ok := voters[fp.BtcPk.MarshalHex()]; !ok {
continue
}
// calculate the portion of a finality provider's voting power out of the total voting power of the voters
@>> fpPortion := sdkmath.LegacyNewDec(int64(fp.TotalBondedSat)).
QuoTruncate(sdkmath.LegacyNewDec(int64(totalVotingPowerOfVoters)))
coinsForFpsAndDels := gauge.GetCoinsPortion(fpPortion)
// reward the finality provider with commission
@>> coinsForCommission := types.GetCoinsPortion(coinsForFpsAndDels, *fp.Commission)
k.accumulateRewardGauge(ctx, types.FinalityProviderType, fp.GetAddress(), coinsForCommission)
// reward the rest of coins to each BTC delegation proportional to its voting power portion
@>> coinsForBTCDels := coinsForFpsAndDels.Sub(coinsForCommission...)
if err := k.AddFinalityProviderRewardsForBtcDelegations(ctx, fp.GetAddress(), coinsForBTCDels); err != nil {
panic(fmt.Errorf("failed to add fp rewards for btc delegation %s at height %d: %w", fp.GetAddress().String(), height, err))
}
}
// TODO: prune unnecessary state (delete BTCStakingGauge after the amount is used)
}It can be observed that the RewardBTCStaking() function in the Incentive module allocates the reward for the historical height block to each participating Finality Provider (FP) that voted. It then calculates the FP's share and the rewards for BTCDel. The rewards for BTCDel are recorded in the FinalityProviderCurrentRewards structure through the AddFinalityProviderRewardsForBtcDelegations() function.
type FinalityProviderCurrentRewards struct {
CurrentRewards github_com_cosmos_cosmos_sdk_types.Coins `protobuf:"bytes,1,rep,name=current_rewards,json=currentRewards,proto3,castrepeated=github.com/cosmos/cosmos-sdk/types.Coins" json:"current_rewards"`
Period uint64 `protobuf:"varint,2,opt,name=period,proto3" json:"period,omitempty"`
TotalActiveSat cosmossdk_io_math.Int `protobuf:"bytes,3,opt,name=total_active_sat,json=totalActiveSat,proto3,customtype=cosmossdk.io/math.Int" json:"total_active_sat"`
}The issue lies in the fact that Period is not tied to the block sequence number but is incremented by 1 every time operations such as BtcDelegation, BtcDelegationUnbonded, and Withdraw Reward are performed. This can be observed from the BtcDelegationActivated(), BtcDelegationUnbonded(), and btcDelegationModified() functions, all of which call the btcDelegationModifiedWithPreInitDel() function.
func (k Keeper) btcDelegationModifiedWithPreInitDel(
ctx context.Context,
fp, del sdk.AccAddress,
preInitializeDelegation func(ctx context.Context, fp, del sdk.AccAddress) error,
) error {
@> endedPeriod, err := k.IncrementFinalityProviderPeriod(ctx, fp)
if err != nil {
return err
}
@> if err := k.CalculateBTCDelegationRewardsAndSendToGauge(ctx, fp, del, endedPeriod); err != nil {
return err
}
@> if err := preInitializeDelegation(ctx, fp, del); err != nil {
return err
}
return k.initializeBTCDelegation(ctx, fp, del)
}It can be observed that the btcDelegationModifiedWithPreInitDel() function calls IncrementFinalityProviderPeriod(), which increments the Period.
func (k Keeper) IncrementFinalityProviderPeriod(ctx context.Context, fp sdk.AccAddress) (endedPeriod uint64, err error) {
// IncrementValidatorPeriod
// gets the current rewards and send to historical the current period (the rewards are stored as "shares" which means the amount of rewards per satoshi)
// sets new empty current rewards with new period
fpCurrentRwd, err := k.GetFinalityProviderCurrentRewards(ctx, fp)
// skip
// initiates a new period with empty rewards and the same amount of active sat
@> newCurrentRwd := types.NewFinalityProviderCurrentRewards(sdk.NewCoins(), fpCurrentRwd.Period+1, fpCurrentRwd.TotalActiveSat)
if err := k.setFinalityProviderCurrentRewards(ctx, fp, newCurrentRwd); err != nil {
return 0, err
}
return fpCurrentRwd.Period, nil
}The btcDelegationModifiedWithPreInitDel() function calls CalculateBTCDelegationRewardsAndSendToGauge to allocate previous rewards and invokes initializeBTCDelegation() to calculate the new total staked amount and the current reward rate.
Taking the BtcDelegation operation and BtcDelegationActivated() as an example, let's illustrate the issue. Assume the current block is the kth block:
- When a
BtcDelegationis activated, theBtcDelegation TotalSatis added to the Finality Provider's (FP) total staked amount, making it eligible for reward distribution.
func (k Keeper) BeginBlocker(ctx context.Context) error {
// update voting power distribution
@> k.UpdatePowerDist(ctx)
return nil
}-
In the
EndBlockof thekth block, the rewards for the block at heightheight - FinalitySigTimeout(or even earlier blocks) begin to be distributed. -
This
BtcDelegationwill participate in the reward distribution based on its share of the FP's total staked amount.
It is evident that this is incorrect because the TotalSat of this BtcDelegation did not contribute to the FP's historical block participation.
- When it is discovered that a specific Finality Provider (FP) has voted on a historical block and is likely to receive a significant reward,
- Immediately staking a large amount of Delegation with this FP could potentially divert a substantial portion of the reward that originally did not belong to this
BtcDelegation.
The reward distribution for BtcDelegation is unfair, as an attacker could exploit the system to unfairly benefit from rewards they did not contribute to earning.
Modify the system so that a BtcDelegation can only receive rewards allocated for blocks after it has been activated.