018.md

Petite Fleece Cheetah

Medium

Message is indexed as refundable even if the signature was over a fork

Summary

When adding a finality sig, a message is still marked as refundable even if the vote was over the fork.

Root Cause

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.

Internal Pre-conditions

External Pre-conditions

A FP adds a finality sig.

Attack Path

A FP adds a finality sig and his message is incorrectly marked as refundable due to the missing return.

Impact

The message can still be refunded even if it's a signature addition with invalid data.

PoC

The current code makes no return when slashing a provider that adds a finality sig:

https://github.com/sherlock-audit/2024-12-babylon/blob/main/babylon/x/finality/keeper/msg_server.go#L192-216

// 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:

https://github.com/sherlock-audit/2024-12-babylon/blob/main/babylon/x/incentive/keeper/refundable_msg_index.go#L23-38

// 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 refundable

The 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:

https://github.com/sherlock-audit/2024-12-babylon/blob/main/babylon/x/finality/keeper/msg_server.go#L172-178

	// 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
	}

Mitigation

After slashing a finality provider, add a response with nil as an error so the message is not marked as refundable later.