Slashable FP offence won't be slashed

[maurolacy]

Summary of Bug

Taking a look at the slashing logic in

babylon/x/finality/keeper/msg_server.go

Lines 132 to 140 in fe247dd

	// if this finality provider has also signed canonical block, slash it
	canonicalSig, err := ms.GetSig(ctx, req.BlockHeight, fpPK)
	if err == nil {
	//set canonial sig
	evidence.CanonicalFinalitySig = canonicalSig
	// 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)
	}

noticed that a given FP will be slashed only when signing the canonical (i.e. indexed) block, and an alternative (fork) block at the same height.

This implies that a malicious FP could sign and submit any number of non-canonical block signatures for a given height. As long as he doesn’t sign and submit the canonical block signature as well, he won’t be slashed.

This is because slashing currently depends on a FP signing both, an offending / fork block, AND the canonical (i.e. indexed) block.

Perhaps the code / logic can be changed here. If a given FP signs two different blocks at any given height (independently of if one is the canonical or not), he’s slashed.

So, slash when signing both the canonical and other block, and also when signing two other blocks.

This seems easy to fix: If Evidence already exists in the store for this FP at that height, this is a double sign.

Update: Just noticed that the detection of double signing of forks is slightly below in the code

babylon/x/finality/keeper/msg_server.go

Lines 153 to 172 in fe247dd

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

The bug is then that, since neither fork is the canonical block, this code is never reached (the if above always returns without error).

Version

Latest.

Steps to Reproduce

N/A.