Include a check to ensure that a transfer is sent only if the slashed_amount is greater than 0 (audit issue #14)

[james-chf]

This was finding 14 from the audit of eth-bridge-integration branch (commit 57fc202)

Severity: Informational
In apps/proof_of_stake/src/lib.rs:881 the transfer method is called regardless of the amount being slashed.
Recommendation
We recommend checking if the slashed_amount is greater than 0 before calling transfer in apps/proof_of_stake/src/lib.rs:881.

[james-chf]

Relevant code in the slash fn which the Ethereum bridge will use once Ethereum bridge slashing logic is implemented:

namada/proof_of_stake/src/lib.rs

Lines 870 to 886 in 57fc202

	let slashed_change: i128 = slashed_change.into();
	let slashed_amount = u64::try_from(slashed_change)
	.map_err(|_err| SlashError::InvalidSlashChange(slashed_change))?;
	let slashed_amount = Self::TokenAmount::from(slashed_amount);
	self.write_validator_total_deltas(validator, &total_deltas);
	self.write_validator_voting_power(validator, &voting_power);
	self.write_validator_slash(validator, validator_slash);
	self.write_validator_set(&validator_set);
	self.write_total_voting_power(&total_voting_power);
	// Transfer the slashed tokens to the PoS slash pool
	self.transfer(
	&Self::staking_token_address(),
	slashed_amount,
	&Self::POS_ADDRESS,
	&Self::POS_SLASH_POOL_ADDRESS,
	);

The slashed_amount variable goes through being a u64 before being converted to Self::TokenAmount and passed to the transfer fn, so it should be at least 0, but not necessarily strictly greater than. This is still the case in the latest eth-bridge-integration code.

namada/proof_of_stake/src/lib.rs

Lines 937 to 953 in 12ca4da

	let slashed_change: i128 = slashed_change.into();
	let slashed_amount = u64::try_from(slashed_change)
	.map_err(|_err| SlashError::InvalidSlashChange(slashed_change))?;
	let slashed_amount = Self::TokenAmount::from(slashed_amount);
	self.write_validator_total_deltas(validator, &total_deltas);
	self.write_validator_voting_power(validator, &voting_power);
	self.write_validator_slash(validator, validator_slash);
	self.write_validator_set(&validator_set);
	self.write_total_voting_power(&total_voting_power);
	// Transfer the slashed tokens to the PoS slash pool
	self.transfer(
	&Self::staking_token_address(),
	slashed_amount,
	&Self::POS_ADDRESS,
	&Self::POS_SLASH_POOL_ADDRESS,
	);

We don't explicitly check that the amount is greater than 0. We could take the recommendation to check the amount being passed in at the call site, or maybe have the transfer fn explicitly reject/no-op when it is passed an amount that is not greater than 0.

[tzemanovic]

With change to ceiling mul in #1830, slash rate and slashable token amount being greater than 0, the slash amount that's being transferred should always be non-zero. However, we're currently not transferring slashes, so we should check on this again with #1508

[cwgoes]

Not relevant.