Source: #20
0xeix
There is currently a refund logic that transfers the funds to a fee payer.
The root cause lies in the fact that the transaction fees can be paid by other enitities and the funds are returned to a fee payer and not a granter.
Fee granter has to pay for the fees.
Fee grantor paid for the fees but the funds are returned to a fee payer.
Loss of funds for a fee granter.
Consider the current refund mechanism:
// RefundTx refunds the given tx by sending the fee back to the fee payer.
func (k Keeper) RefundTx(ctx context.Context, tx sdk.FeeTx) error {
txFee := tx.GetFee()
if txFee.IsZero() {
// not possible with the global min gas price mechanism
// but having this check for compatibility in the future
return nil
}
txFeePayer := tx.FeePayer()
return k.bankKeeper.SendCoinsFromModuleToAccount(ctx, k.feeCollectorName, txFeePayer, txFee)
}The problem is that the fees are refunded to the feePayer (the sender of the message) without taking into account the fact that the actual payer can be feeGranter resulting in a loss of funds for the granter:
type FeeTx interface {
[Tx](https://pkg.go.dev/github.com/cosmos/cosmos-sdk/types#Tx)
GetGas() [uint64](https://pkg.go.dev/builtin#uint64)
GetFee() [Coins](https://pkg.go.dev/github.com/cosmos/cosmos-sdk/types#Coins)
FeePayer() [][byte](https://pkg.go.dev/builtin#byte)
FeeGranter() [][byte](https://pkg.go.dev/builtin#byte)
}Check out the Celestia solution for this issue:
// getRecipient returns the address that should receive the refund.
func getRecipient(feeTx sdk.FeeTx) sdk.AccAddress {
if feeGranter := feeTx.FeeGranter(); feeGranter != nil {
return feeGranter
}
return feeTx.FeePayer()
}Here the funds are actually sent to the right entity.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: babylonlabs-io/babylon#594
Source: #33
LZ_security
The EXPIRED judgment does not include the current block, fp can still be unbonded after Delegation EXPIRED.
btcHeight+d.UnbondingTime > d.EndHeight should be btcHeight+d.UnbondingTime >= d.EndHeight
- The Delegation of fp is about to expire.
- fp executes BTCUndelegate in the same block that expires.
When the unbonded event is executed after the EXPIRED event, a painc occurs and the chain is halted.
Or it will result in a double reduction in theTotalBondedSat of fp.
The BTCUndelegate function gets the status through GetStatus, and Delegation EXPIRED returns an error:
func (ms msgServer) BTCUndelegate(goCtx context.Context, req *types.MsgBTCUndelegate) (*types.MsgBTCUndelegateResponse, error) {
defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), types.MetricsKeyBTCUndelegate)
ctx := sdk.UnwrapSDKContext(goCtx)
// basic stateless checks
if err := req.ValidateBasic(); err != nil {
return nil, status.Errorf(codes.InvalidArgument, "%v", err)
}
btcDel, bsParams, err := ms.getBTCDelWithParams(ctx, req.StakingTxHash)
if err != nil {
return nil, err
}
// ensure the BTC delegation with the given staking tx hash is active
btcTip := ms.btclcKeeper.GetTipInfo(ctx)
-> btcDelStatus := btcDel.GetStatus(
btcTip.Height,
bsParams.CovenantQuorum,
)
-> if btcDelStatus == types.BTCDelegationStatus_UNBONDED || btcDelStatus == types.BTCDelegationStatus_EXPIRED {
return nil, types.ErrInvalidBTCUndelegateReq.Wrap("cannot unbond an unbonded BTC delegation")
}
......
}The problem is that GetStatus does not include the current block in its judgment. If the current block is in d.EndHeight - d.UnbondingTime,
BTCDelegationStatus_EXPIRED will not be returned:
func (d *BTCDelegation) GetStatus(
btcHeight uint32,
covenantQuorum uint32,
) BTCDelegationStatus {
......
-> // btcHeight = currentBtcTipHeight
-> // if btcHeight > d.EndHeight - d.UnbondingTime
-> if btcHeight+d.UnbondingTime > d.EndHeight {
return BTCDelegationStatus_EXPIRED
}
return BTCDelegationStatus_ACTIVE
}The EXPIRED event executes at: d.EndHeight - d.UnbondingTime
func (k Keeper) AddBTCDelegation(
ctx sdk.Context,
btcDel *types.BTCDelegation,
) error {
......
// record event that the BTC delegation will become expired (unbonded) at EndHeight-w
// This event will be generated to subscribers as block event, when the
// btc light client block height will reach btcDel.EndHeight-wValue
expiredEvent := types.NewEventPowerDistUpdateWithBTCDel(&types.EventBTCDelegationStateUpdate{
StakingTxHash: stakingTxHash.String(),
NewState: types.BTCDelegationStatus_EXPIRED,
})
// NOTE: we should have verified that EndHeight > btcTip.Height + unbonding_time
-> k.addPowerDistUpdateEvent(ctx, btcDel.EndHeight-btcDel.UnbondingTime, expiredEvent)
......
}So BTCUndelegate and EXPIRED can be executed in the same block.
When handling EXPIRED events, delegater's unbond state is determined first(IsUnbondedEarly), but since the event is handled in BeginBlocker and sent before BTCUndelegate, so DelegatorUnbondingInfo is not set:
func (k Keeper) ProcessAllPowerDistUpdateEvents(
ctx context.Context,
dc *ftypes.VotingPowerDistCache,
events []*types.EventPowerDistUpdate,
) *ftypes.VotingPowerDistCache {
........
case types.BTCDelegationStatus_UNBONDED:
// add the unbonded BTC delegation to the map
//@audit-info unbonded 之fp 应该从 fpByBtcPkHex 缓存列表中删除 UNBONDED 和 EXPIRED 同时发送?
k.processPowerDistUpdateEventUnbond(ctx, fpByBtcPkHex, btcDel, unbondedSatsByFpBtcPk)
case types.BTCDelegationStatus_EXPIRED:
types.EmitExpiredDelegationEvent(sdkCtx, delStkTxHash)
-> // IsUnbondedEarly -> return d.BtcUndelegation.DelegatorUnbondingInfo != nil
-> if !btcDel.IsUnbondedEarly() {
// only adds to the new unbonded list if it hasn't
// previously unbonded with types.BTCDelegationStatus_UNBONDED
k.processPowerDistUpdateEventUnbond(ctx, fpByBtcPkHex, btcDel, unbondedSatsByFpBtcPk)
}
}This leads to a painc:
UpdatePowerDist -> ProcessAllPowerDistUpdateEvents -> MustProcessBtcDelegationUnbonded :
func (k Keeper) MustProcessBtcDelegationUnbonded(ctx context.Context, fp, del sdk.AccAddress, sats uint64) {
err := k.IncentiveKeeper.BtcDelegationUnbonded(ctx, fp, del, sats)
-> if err != nil {
-> panic(err)
}
}
func (k Keeper) BtcDelegationUnbonded(ctx context.Context, fp, del sdk.AccAddress, sat uint64) error {
amtSat := sdkmath.NewIntFromUint64(sat)
return k.btcDelegationModifiedWithPreInitDel(ctx, fp, del, func(ctx context.Context, fp, del sdk.AccAddress) error {
-> return k.subDelegationSat(ctx, fp, del, amtSat)
})
}
func (k Keeper) subDelegationSat(ctx context.Context, fp, del sdk.AccAddress, amt sdkmath.Int) error {
btcDelRwdTracker, err := k.GetBTCDelegationRewardsTracker(ctx, fp, del)
if err != nil {
return err
}
-> btcDelRwdTracker.SubTotalActiveSat(amt)
-> if btcDelRwdTracker.TotalActiveSat.IsNegative() {
-> return types.ErrBTCDelegationRewardsTrackerNegativeAmount
}
if err := k.setBTCDelegationRewardsTracker(ctx, fp, del, btcDelRwdTracker); err != nil {
return err
}
return k.subFinalityProviderStaked(ctx, fp, amt)
}The event is handled in BeginBlocker, and painc causes the chain to halt.
finality/abci.go/BeginBlocker -> UpdatePowerDist -> ProcessAllPowerDistUpdateEvents
Theamt passed by thesubDelegationSat function, which is actually the amount of delegation, is set whenAddBTCDelegation is added.
UpdatePowerDist function, through k.BTCStakingKeeper.GetBTCDelegation(ctx, delStkTxHash) get this value and use this value to update fp.TotalBondedSat. If UNBOND is repeated, fp.TotalBondedSat will also be reduced repeatedly.
func (d *BTCDelegation) GetStatus(
btcHeight uint32,
covenantQuorum uint32,
) BTCDelegationStatus {
......
- if btcHeight+d.UnbondingTime > d.EndHeight {
+ if btcHeight+d.UnbondingTime >= d.EndHeight {
return BTCDelegationStatus_EXPIRED
}
return BTCDelegationStatus_ACTIVE
}sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: babylonlabs-io/babylon#597
Source: #35
LZ_security
If the Covenant signature does not pass , EXPIRED events it will still be executed, causes fp.TotalBondedSat to be incorrectly reduced.
If the Covenant signature does not pass , EXPIRED events it will still be executed.
- Delegater uses the
CreateBTCDelegationcommand to create a Delegation. TheEXPIREDmessage is sent. - The Covenants began to sign.
- Delegater unstake before the number of signatures reaches the threshold.
- The signature fails. The Active message cannot be sent, but the EXPIRED message is sent.
- The EXPIRED file is executed after a period of time.
fp.TotalBondedSat to be incorrectly reduced.
Create a Delegation, if by AddBTCDelegationInclusionProof function, activeEvent and expiredEvent will be added at the same time:
func (ms msgServer) AddBTCDelegationInclusionProof(
goCtx context.Context,
req *types.MsgAddBTCDelegationInclusionProof,
) (*types.MsgAddBTCDelegationInclusionProofResponse, error) {
......
activeEvent := types.NewEventPowerDistUpdateWithBTCDel(
&types.EventBTCDelegationStateUpdate{
StakingTxHash: stakingTxHash.String(),
NewState: types.BTCDelegationStatus_ACTIVE,
},
)
ms.addPowerDistUpdateEvent(ctx, timeInfo.TipHeight, activeEvent)
// record event that the BTC delegation will become unbonded at EndHeight-w
expiredEvent := types.NewEventPowerDistUpdateWithBTCDel(&types.EventBTCDelegationStateUpdate{
StakingTxHash: req.StakingTxHash,
NewState: types.BTCDelegationStatus_EXPIRED,
})
// NOTE: we should have verified that EndHeight > btcTip.Height + min_unbonding_time
ms.addPowerDistUpdateEvent(ctx, btcDel.EndHeight-params.UnbondingTimeBlocks, expiredEvent)
......
}The problem is that if the CreateBTCDelegation function is used, the expiredEvent is sent first, and the activeEvent is sent after the Covenants signature is passed.
If the signature does not pass the activeEvent, it will never be sent, but expiredEvent has been sent and will eventually execute:
If staker/Delegater is unstake in the btc chain after the CreateBTCDelegation enters the Covenants signature phase,Covenants should not sign:
In addition,BTCUndelegate causes the signature to fail:
func (ms msgServer) AddCovenantSigs(goCtx context.Context, req *types.MsgAddCovenantSigs) (*types.MsgAddCovenantSigsResponse, error) {
......
// ensure BTC delegation is still pending, i.e., not unbonded
btcTipHeight := ms.btclcKeeper.GetTipInfo(ctx).Height
status := btcDel.GetStatus(btcTipHeight, params.CovenantQuorum)
-> if status == types.BTCDelegationStatus_UNBONDED || status == types.BTCDelegationStatus_EXPIRED {
ms.Logger(ctx).Debug("Received covenant signature after the BTC delegation is already unbonded", "covenant pk", req.Pk.MarshalHex())
return nil, types.ErrInvalidCovenantSig.Wrap("the BTC delegation is already unbonded")
}
......
}Covenants If the signature is not passed, the BTCUndelegate can be executed only when the UNBONDED or EXPIRED status is unavailable
Undelegate, So staker can block Covenants' signatures by using BTCUndelegate.
func (ms msgServer) BTCUndelegate(goCtx context.Context, req *types.MsgBTCUndelegate) (*types.MsgBTCUndelegateResponse, error) {
defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), types.MetricsKeyBTCUndelegate)
ctx := sdk.UnwrapSDKContext(goCtx)
// basic stateless checks
if err := req.ValidateBasic(); err != nil {
return nil, status.Errorf(codes.InvalidArgument, "%v", err)
}
btcDel, bsParams, err := ms.getBTCDelWithParams(ctx, req.StakingTxHash)
if err != nil {
return nil, err
}
// ensure the BTC delegation with the given staking tx hash is active
btcTip := ms.btclcKeeper.GetTipInfo(ctx)
-> btcDelStatus := btcDel.GetStatus(
btcTip.Height,
bsParams.CovenantQuorum,
)
-> if btcDelStatus == types.BTCDelegationStatus_UNBONDED || btcDelStatus == types.BTCDelegationStatus_EXPIRED {
return nil, types.ErrInvalidBTCUndelegateReq.Wrap("cannot unbond an unbonded BTC delegation")
}
......
}expiredEvent causes fp's TotalBondedSat to decrease, or a negative number in the system causes painc.
The balance of Delegater is set upon creation and stored in the Store:
// CreateBTCDelegation creates a BTC delegation
func (ms msgServer) CreateBTCDelegation(goCtx context.Context, req *types.MsgCreateBTCDelegation) (*types.MsgCreateBTCDelegationResponse, error) {
......
// 7.all good, construct BTCDelegation and insert BTC delegation
// NOTE: the BTC delegation does not have voting power yet. It will
// have voting power only when it receives a covenant signatures
newBTCDel := &types.BTCDelegation{
StakerAddr: parsedMsg.StakerAddress.String(),
BtcPk: parsedMsg.StakerPK.BIP340PubKey,
Pop: parsedMsg.ParsedPop,
FpBtcPkList: parsedMsg.FinalityProviderKeys.PublicKeysBbnFormat,
StakingTime: uint32(parsedMsg.StakingTime),
StartHeight: timeInfo.StartHeight,
EndHeight: timeInfo.EndHeight,
-> TotalSat: uint64(parsedMsg.StakingValue),
StakingTx: parsedMsg.StakingTx.TransactionBytes,
StakingOutputIdx: paramsValidationResult.StakingOutputIdx,
SlashingTx: types.NewBtcSlashingTxFromBytes(parsedMsg.StakingSlashingTx.TransactionBytes),
DelegatorSig: parsedMsg.StakerStakingSlashingTxSig.BIP340Signature,
UnbondingTime: uint32(parsedMsg.UnbondingTime),
CovenantSigs: nil, // NOTE: covenant signature will be submitted in a separate msg by covenant
BtcUndelegation: &types.BTCUndelegation{
UnbondingTx: parsedMsg.UnbondingTx.TransactionBytes,
SlashingTx: types.NewBtcSlashingTxFromBytes(parsedMsg.UnbondingSlashingTx.TransactionBytes),
DelegatorSlashingSig: parsedMsg.StakerUnbondingSlashingSig.BIP340Signature,
CovenantSlashingSigs: nil, // NOTE: covenant signature will be submitted in a separate msg by covenant
CovenantUnbondingSigList: nil, // NOTE: covenant signature will be submitted in a separate msg by covenant
DelegatorUnbondingInfo: nil,
},
ParamsVersion: paramsVersion, // version of the params against which delegation was validated
BtcTipHeight: timeInfo.TipHeight, // height of the BTC light client tip at the time of the delegation creation
}
// add this BTC delegation, and emit corresponding events
-> if err := ms.AddBTCDelegation(ctx, newBTCDel); err != nil {
panic(fmt.Errorf("failed to add BTC delegation that has passed verification: %w", err))
}When handling expiredEvent events, the balance of Delegater is read and fp TotalBondedSat is reduced:
func (k Keeper) ProcessAllPowerDistUpdateEvents(
ctx context.Context,
dc *ftypes.VotingPowerDistCache,
events []*types.EventPowerDistUpdate,
) *ftypes.VotingPowerDistCache {
// a map where key is finality provider's BTC PK hex and value is a list
// of BTC delegations satoshis amount that newly become active under this provider
activedSatsByFpBtcPk := map[string][]uint64{}
// a map where key is finality provider's BTC PK hex and value is a list
// of BTC delegations satoshis that were unbonded or expired without previously
// being unbonded
unbondedSatsByFpBtcPk := map[string][]uint64{}
// a map where key is slashed finality providers' BTC PK
slashedFPs := map[string]struct{}{}
// a map where key is jailed finality providers' BTC PK
jailedFPs := map[string]struct{}{}
// a map where key is unjailed finality providers' BTC PK
unjailedFPs := map[string]struct{}{}
// simple cache to load fp by his btc pk hex
fpByBtcPkHex := map[string]*types.FinalityProvider{}
/*
filter and classify all events into new/expired BTC delegations and jailed/slashed FPs
*/
sdkCtx := sdk.UnwrapSDKContext(ctx)
for _, event := range events {
switch typedEvent := event.Ev.(type) {
case *types.EventPowerDistUpdate_BtcDelStateUpdate:
delEvent := typedEvent.BtcDelStateUpdate
delStkTxHash := delEvent.StakingTxHash
-> btcDel, err := k.BTCStakingKeeper.GetBTCDelegation(ctx, delStkTxHash)
if err != nil {
panic(err) // only programming error
}
switch delEvent.NewState {
case types.BTCDelegationStatus_ACTIVE:
// newly active BTC delegation
// add the BTC delegation to each restaked finality provider
for _, fpBTCPK := range btcDel.FpBtcPkList {
fpBTCPKHex := fpBTCPK.MarshalHex()
activedSatsByFpBtcPk[fpBTCPKHex] = append(activedSatsByFpBtcPk[fpBTCPKHex], btcDel.TotalSat)
}
k.processRewardTracker(ctx, fpByBtcPkHex, btcDel, func(fp, del sdk.AccAddress, sats uint64) {
k.MustProcessBtcDelegationActivated(ctx, fp, del, sats)//
})
case types.BTCDelegationStatus_UNBONDED:
// add the unbonded BTC delegation to the map
-> k.processPowerDistUpdateEventUnbond(ctx, fpByBtcPkHex, btcDel, unbondedSatsByFpBtcPk)
case types.BTCDelegationStatus_EXPIRED:
types.EmitExpiredDelegationEvent(sdkCtx, delStkTxHash)
// IsUnbondedEarly -> return d.BtcUndelegation.DelegatorUnbondingInfo != nil
if !btcDel.IsUnbondedEarly() {
// only adds to the new unbonded list if it hasn't
// previously unbonded with types.BTCDelegationStatus_UNBONDED
-> k.processPowerDistUpdateEventUnbond(ctx, fpByBtcPkHex, btcDel, unbondedSatsByFpBtcPk)
}
}
.....
// process all new unbonding BTC delegations under this finality provider
if fpUnbondedSats, ok := unbondedSatsByFpBtcPk[fpBTCPKHex]; ok {
// handle unbonded delegations for this finality provider
for _, unbodedSats := range fpUnbondedSats {
-> // RemoveBondedSats -> v.TotalBondedSat -= sats
-> fp.RemoveBondedSats(unbodedSats)
}
// remove the finality provider entry in fpUnbondedSats map, so that
// after the for loop the rest entries in fpUnbondedSats belongs to new
// finality providers that might have btc delegations entries
// that activated and unbonded in the same slice of events
delete(unbondedSatsByFpBtcPk, fpBTCPKHex)
}
......
}
}
func (k Keeper) processPowerDistUpdateEventUnbond(
ctx context.Context,
cacheFpByBtcPkHex map[string]*types.FinalityProvider,
btcDel *types.BTCDelegation,
unbondedSatsByFpBtcPk map[string][]uint64,
) {
for _, fpBTCPK := range btcDel.FpBtcPkList {
fpBTCPKHex := fpBTCPK.MarshalHex()
-> unbondedSatsByFpBtcPk[fpBTCPKHex] = append(unbondedSatsByFpBtcPk[fpBTCPKHex], btcDel.TotalSat)
}
k.processRewardTracker(ctx, cacheFpByBtcPkHex, btcDel, func(fp, del sdk.AccAddress, sats uint64) {
k.MustProcessBtcDelegationUnbonded(ctx, fp, del, sats)
})
}Send EXPIRED and Active messages at the same time instead of separately.
Issue M-1: Btcstaking module allows stakingTx to be coinbase transaction which is unslashable for 100 blocks
Source: #6
n4nika
Coinbase transactions have a special property of not being spendable for 100 bocks after creation. If now a staker uses such a transaction as a staking transaction (by adding the required outputs), that transaction will be recognized as a valid staking TX but if the owner of it double-signs, he cannot be slashed for 100 blocks due to the unspendability of the coinbase TX.
Looking at CreateBTCDelegation and ValidateParsedMessageAgainstTheParams which does verification on the provided TX, there are no specific checks for whether a transaction is a coinbase transaction.
None
Attacker needs to be the creator of a block (a miner) in order to build the coinbase TX like they want
- Create a coinbase TX which is a valid staking TX
- Call
CreateBTCDelegationwith that TX - It gets accepted and its value added as voting power
The README states under High impact: Inability to slash BTC stake for which the voting power was involved in double-signing.
That is exactly what happens here. Even if the staker's delegator misbehaves, the staker's stakingTx cannot be spent until 100 blocks after. Adding to the impact, if now the minimum staking time is less than 100 bitcoin blocks, this allows the malicious staker to unstake before getting slashed (if the slashing even gets retried after 100 blocks)
No response
Coinbase transactions can be identified since the ID of their input must be all zeros. Therefore consider checking whether a staking transaction has one input with an ID of all-zeros and reject it if so.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: babylonlabs-io/babylon#563
Source: #18
0xeix
When adding a finality sig, a message is still marked as refundable even if the vote was over the fork.
The root cause lies in the fact that there is no return after slashing the finality provider resulting in a message being indexed as refundable in the IncentiveKeeper.
A FP adds a finality sig.
A FP adds a finality sig and his message is incorrectly marked as refundable due to the missing return.
The message can still be refunded even if it's a signature addition with invalid data.
The current code makes no return when slashing a provider that adds a finality sig:
// if this finality provider has signed the canonical block before,
// slash it via extracting its secret key, and emit an event
if ms.HasEvidence(ctx, req.FpBtcPk, req.BlockHeight) {
// the finality provider has voted for a fork before!
// If this evidence is at the same height as this signature, slash this finality provider
// get evidence
evidence, err := ms.GetEvidence(ctx, req.FpBtcPk, req.BlockHeight)
if err != nil {
panic(fmt.Errorf("failed to get evidence despite HasEvidence returns true"))
}
// set canonical sig to this evidence
evidence.CanonicalFinalitySig = req.FinalitySig
ms.SetEvidence(ctx, evidence)
// slash this finality provider, including setting its voting power to
// zero, extracting its BTC SK, and emit an event
ms.slashFinalityProvider(ctx, req.FpBtcPk, evidence)
}
// at this point, the finality signature is 1) valid, 2) over a canonical block,
// and 3) not duplicated.
// Thus, we can safely consider this message as refundable
ms.IncentiveKeeper.IndexRefundableMsg(ctx, req)As you can see here, after slashing a provider, the message is marked as refundable:
// IndexRefundableMsg indexes the given refundable message by its hash.
func (k Keeper) IndexRefundableMsg(ctx context.Context, msg sdk.Msg) {
msgHash := types.HashMsg(msg)
err := k.RefundableMsgKeySet.Set(ctx, msgHash)
if err != nil {
panic(err) // encoding issue; this can only be a programming error
}
}The problem is that there has to be some conditions to be satisfied outlined in the comments for the message to be refundable:
// at this point, the finality signature is 1) valid, 2) over a canonical block,
// and 3) not duplicated.
// Thus, we can safely consider this message as refundableThe problem is that there is no return after slashing the provider previously meaning the next line executed will be the indexing of the message. In contrary, there is another slashing logic in the same function that correctly returns the response:
// save evidence
ms.SetEvidence(ctx, evidence)
// NOTE: we should NOT return error here, otherwise the state change triggered in this tx
// (including the evidence) will be rolled back
return &types.MsgAddFinalitySigResponse{}, nil
}
After slashing a finality provider, add a response with nil as an error so the message is not marked as refundable later.
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: babylonlabs-io/babylon#592
Source: #23
valuevalk
maybeResendFromStore in relayer.go in vigilante module, currently resends the same checkpoint tx again, no matter what the error returned from the json-rpc function GetRawTransactionFunc is, which in some situations could obstruct the correct flow, as we could end up submitting the same transaction twice.
Bitcoin rpc errors: https://github.com/btcsuite/btcd/blob/bb52d7d78d9cf335e0611b9ae06ad8c77e75de0b/btcjson/jsonrpcerr.go#L177
When submitting a checkpoint tx to bitcoin, we use maybeResendFromStore which has a fail-safe mechanism which only proceeds with sending the transaction if there is an error when calling GetRawTransactionFunc, this is to prevent sending already send transactions, if for example the submitter service is restarted.
However there is a flaw, currently we do that no matter what the returned error is, this poses a problem as there is possibility that error could be returned due to problems with the rpc( rate limits exceeded, network error & etc ), or other json-rpc error codes that are not transaction not found.
func (rl *Relayer) SendCheckpointToBTC(ckpt *ckpttypes.RawCheckpointWithMetaResponse) error {
.........
if rl.shouldSendCompleteCkpt(ckptEpoch) || rl.shouldSendTx2(ckptEpoch) {
@>> hasBeenProcessed, err := maybeResendFromStore(
ckptEpoch,
rl.store.LatestCheckpoint,
rl.GetRawTransaction,
rl.sendTxToBTC,
)
if err != nil {
return err
}
if hasBeenProcessed {
return nil
}
}
....
}
@>> // maybeResendFromStore - checks if we need to resubmit txns from a store
@>> // in case "submitter" service was restarted, we want to ensure that we don't send txns again for a checkpoint
@>> // that has already been processed.
@>> // Returns true if the first transactions are in the mempool (no resubmission needed),
@>> // and false if any transaction was re-sent from the store.
func maybeResendFromStore(
epoch uint64,
getLatestStoreCheckpoint GetLatestCheckpointFunc,
getRawTransaction GetRawTransactionFunc,
sendTransaction SendTransactionFunc,
) (bool, error) {
storedCkpt, exists, err := getLatestStoreCheckpoint()
if err != nil {
return false, err
} else if !exists {
return false, nil
}
if storedCkpt.Epoch != epoch {
return false, nil
}
maybeResendFunc := func(tx *wire.MsgTx) error {
txID := tx.TxHash()
@>> _, err = getRawTransaction(&txID) // todo(lazar): check for specific not found err
@>> if err != nil {
@>> _, err := sendTransaction(tx)
if err != nil {
@>> return err //we could end up here
}
// we know about this tx, but we needed to resend it from already constructed tx from db
return nil
}
// tx exists in mempool and is known to us
return nil
}
@>> if err := maybeResendFunc(storedCkpt.Tx1); err != nil {
return false, err
}
@>> if err := maybeResendFunc(storedCkpt.Tx2); err != nil {
return false, err
}
return true, nil
}A few bad scenarios could happen here:
- If in reality
storedCkpt.Tx2needs to be resend, but we failed onmaybeResendFunc(storedCkpt.Tx1)because we falsely send the same tx1 again, we would not be able to re-submitstoredCkpt.Tx2. - The
MaybeResubmitSecondCheckpointTxflow will not be executed because we callcontinue.
func (s *Submitter) processCheckpoints() {
defer s.wg.Done()
quit := s.quitChan()
for {
select {
case ckpt := <-s.poller.GetSealedCheckpointChan():
s.logger.Infof("A sealed raw checkpoint for epoch %v is found", ckpt.Ckpt.EpochNum)
if err := s.relayer.SendCheckpointToBTC(ckpt); err != nil {
s.logger.Errorf("Failed to submit the raw checkpoint for %v: %v", ckpt.Ckpt.EpochNum, err)
s.metrics.FailedCheckpointsCounter.Inc()
@>> continue
}
if err := s.relayer.MaybeResubmitSecondCheckpointTx(ckpt); err != nil {
s.logger.Errorf("Failed to resubmit the raw checkpoint for %v: %v", ckpt.Ckpt.EpochNum, err)
s.metrics.FailedCheckpointsCounter.Inc()
}
s.metrics.SecondsSinceLastCheckpointGauge.Set(0)
case <-quit:
// We have been asked to stop
return
}
}
}- getRawTransaction returns an error, due to network connection ( for example )
- sendTransaction(tx) is called, however it fails with error as its already submitted to the bitcoin chain
- Error is propagated from
relayer.go maybeResendFromStoretosubmitter.go - As result even though checkpoint 2 needed to be re-submitted its not. ( Either with the
maybeResendFunc(storedCkpt.Tx2)flow, or with the bump fee flow in MaybeResubmitSecondCheckpointTx )
If sendTx is called for the 1st checkpoint tx, when the tx is already on bitcoin chain, the operation will fail and will obstruct the flow, possibly leading to not correctly submitting the 2nd tx if it needed to be resubmitted.
The correct behaviours would be to handle separately RPC/curl request error or any json-rpc error code different from "transaction not found".
sherlock-admin2
The protocol team fixed this issue in the following PRs/commits: babylonlabs-io/vigilante#250
Issue M-4: Incorrect BTC Delegation Reward Calculation Due to Using Current Stake Amount Instead of Historical Stake
Source: #70
0xNirix
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.
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.
NA
NA
- Delegator A has 100,000 sats delegated to a finality provider during Period 1
- Rewards accrue but are not yet calculated and distributed as finality has halted.
- Before rewards are calculated, Delegator A reduces their delegation to 50,000 sats (period changes to Period 2)
- A governance action triggers calculation of rewards for Period 1 after resuming finality
- 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)
- Delegator A receives only half the rewards they should have earned
Alternatively:
- Delegator B has 50,000 sats delegated to a finality provider during Period 1
- Rewards accrue for Period 1 but are not yet calculated and distributed as finality has halted.
- Before rewards are calculated, Delegator B increases their delegation to 100,000 sats (period changes to Period 2)
- A governance action triggers calculation of rewards for Period 1
- The system incorrectly uses the current stake amount (100,000 sats) to calculate Period 1's rewards
- 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.
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.
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
DelegatorAhas 100,000 sats staked toFinalityProviderXduring Period 1- A finality halt occurs during Period 1, preventing rewards from being distributed
- Period 2 begins with the halt still in effect if delegator unbonds
// 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:
- Increments the period from 1 to 2
- Attempts to calculate rewards for Period 1, but since there was a finality halt, no rewards are distributed yet
- Reduces the delegation's
TotalActiveSatfrom 100,000 to 50,000 - Initializes a new
BTCDelegationRewardsTrackerwith 50,000 sats and a starting period of 2
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)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)
}
}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)
}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.
No response