[Informational] serai-primitives - Missing validation of External Coin and Network association

[vanhauser-thc]

Issue description

The crate defines ExternalNetworkId and ExternalCoin as logically associated types, but it does not enforce their consistency when constructing or processing external transfer instructions. For example, ExternalNetworkId::Ethereum is implicitly expected to be used with ExternalCoin::Ether, but no runtime check ensures that a coin actually belongs to the declared network. As a result, a transfer instruction may claim to send Ether on the Bitcoin network or XMR on Ethereum, violating domain constraints.

src/coin.rs

pub enum ExternalCoin {
    Btc,
    Ether,
    Dai,
    Xmr,
}

impl ExternalCoin {
    pub fn network(&self) -> ExternalNetworkId {
        match self {
            ExternalCoin::Bitcoin => ExternalNetworkId::Bitcoin,
            ExternalCoin::Ether | ExternalCoin::Dai => ExternalNetworkId::Ethereum,
            ExternalCoin::Monero => ExternalNetworkId::Monero,
        }
    }
}

src/network_id.rs:

pub enum ExternalNetworkId {
    Bitcoin,
    Ethereum,
    Monero,
}

impl ExternalNetworkId {
    pub fn coins(&self) -> &'static [ExternalCoin] {
        match self {
            ExternalNetworkId::Bitcoin => &[ExternalCoin::Btc],
            ExternalNetworkId::Ethereum => &[ExternalCoin::Ether, ExternalCoin::Dai],
            ExternalNetworkId::Monero => &[ExternalCoin::Xmr],
        }
    }
}

However, beyond these mappings, the code does not enforce validity checks at runtime. In particular, there is no dedicated function like validate_coin_for_network(coin, network) that would reject mismatched pairs. The two mapping functions above simply provide the canonical pairing but do not guard other code paths against misuse.

We found no validity checks in the functionality that processes ExternalNetworkId and/or ExternalCoin.

This structural gap could allow semantically incorrect or misleading messages to propagate if not being caught at higher level APIs outside of serai-primitives.

However, we expect the higher level APIs to enforce this logic. As we are currently not auditing these we still felt we should raise the issue.

Risk

An attacker could potentially craft a transfer instruction that combines mismatched network and coin identifiers. For example, they could declare a Bitcoin network withdrawal of Ether, potentially tricking off-chain relayers or bridge infrastructure into executing incorrect asset movements. This mismatch could lead to loss of funds, misrouting of value, or security violations in external systems that rely on this data. If no higher-level enforcement exists, these inconsistencies can silently propagate, violating assumptions across modules or between on-chain and off-chain logic.

Mitigation

Introduce runtime checks wherever ExternalCoin and ExternalNetworkId appear together to ensure the coin belongs to the declared network. Refactor the type system to encapsulate coin-network relationships more tightly, possibly by introducing a struct that pairs a network with its valid coins and disallows invalid constructions at the type level. Validate incoming instructions during decoding or execution to reject mismatched combinations.

[kayabaNerve]

In general, the policy of Serai (and my professional career as a whole) is never to duplicate data because it can fall out of sync as discussed above. I don't believe we have any location which presents both the (External)NetworkId and (External)Coin and allows this discrepancy.

For the potential risk regarding burns, to cite the current literal code,

serai/substrate/primitives/src/instructions/out.rs

Lines 7 to 21 in 9748168

	/// An instruction on how to transfer coins out.
	#[derive(Clone, PartialEq, Eq, Debug, Zeroize, BorshSerialize, BorshDeserialize)]
	pub enum OutInstruction {
	/// Transfer to the specified address.
	Transfer(ExternalAddress),
	}
	/// An instruction on how to transfer coins out with the balance to use for the transfer out.
	#[derive(Clone, PartialEq, Eq, Debug, Zeroize, BorshSerialize, BorshDeserialize)]
	pub struct OutInstructionWithBalance {
	/// The instruction on how to transfer coins out.
	pub instruction: OutInstruction,
	/// The balance to use for the transfer out.
	pub balance: ExternalBalance,
	}

We solely specify the coin being burnt. The off-chain code then invokes coin.network() to determine how exactly to handle it. This is while ExternalAddress is an opaque byte vector not subject to on-chain consensus rules. This is while the burn_with_instruction call solely accepts a OutInstructionWithBalance as an argument:

serai/substrate/abi/src/modules/coins.rs

Lines 22 to 26 in 9748168

	/// Burn these coins with an `OutInstruction` specified.
	burn_with_instruction {
	/// The `OutInstruction`, with the coins to burn.
	instruction: OutInstructionWithBalance,
	},

The only exception to this policy I can think of is within Batch, which is parameterized by an ExternalNetworkId and contains InInstructions (each with their own ExternalCoin):

serai/substrate/primitives/src/instructions/in/batch.rs

Lines 42 to 52 in 9748168

	/// The network this batch of instructions is coming from.
	network: ExternalNetworkId,
	/// The ID of this `Batch`.
	id: u32,
	/// The hash of the external network's block which produced this `Batch`.
	external_network_block_hash: BlockHash,
	/// The instructions to execute.
	// We could bound this, as we can calculate a bound from the size limit, but we want the
	// length prefix to be four bytes now to be mindful of future scaling changes which may
	// require such a large prefix: https://github.com/serai-dex/serai/issues/715
	instructions: Vec<InInstructionWithBalance>,

At this time, we check the consistency of each coin to the declared network at a higher level (as noted here as required and not yet in scope to the ongoing review):

serai/substrate/in-instructions/src/lib.rs

Lines 268 to 273 in 9748168

	// Verify every coin with the `Batch` corresponds to this network
	for instruction in batch.batch.instructions() {
	if instruction.balance.coin.network() != network {
	Err(InvalidTransaction::Custom(2))?;
	}
	}

This is not enforced within the Batch struct however, which may be an improvement, and it also may be better to re-define the Batch struct to avoid this issue. Specifically, for any case where !instructions.is_empty(), we can identify the network from the first instruction's coin (requiring all others to match). This does not cover the case where instructions.is_empty(), which is valid and is used when an external network needs to publish acknowledgement of an external block onto Serai but has no InInstructions associated. This could be resolved by re-defining Batch to be encoded as an enum, similar to how UnsignedCall and SignedCalls are handled, where an empty instructions vector appends the NetworkId explicitly (omitted for non-empty vectors).

This is extensive commentary on Batch, a specific case of this potential problem, but again, that's as the pattern which would risk this problem generally should not exist in the codebase. Because there is no literal finding here at this time, mitigation is solely with how we can harden the codebase to ensure this doesn't become a problem in the future. To me, that would mean reinforcing this practice as our policy, and ensuring it's followed, hence why I went into such detail on the only violation I can immediately think of.

---

With regards to the suggested mitigations:

• Runtime checks, yes, whenever these are both presented (such as in Batch), we are absolutely liable to handle this.
• With regards to a new type/struct for encapsulation, coin.network() and network.coins() are already supposed to serve this purpose. From one piece of data, not two pieces which may become disjoint, it's possible to obtain the complimentary data.
• Checking instructions during decoding, not execution, is possible and inlining these checks into Batch (as relevant to Batch specifically) is likely an improvement. We could define InInstructionForNetwork<Network>, yet that would require statically-typing the network (which would be a major change to the current presentation, and potentially make how all of these networks are intended to be completely abstracted over less considered). Removing BorshDeserialize for InInstruction however in order to solely offer fn deserialize_reader_with_network_context(mut reader: impl io::Read, network: NetworkId) -> io::Result<Self> could be considered (so deserializing an InInstruction requires contextualizing it with a network, where this is checked on deserialization).

---

I don't have an immediate opinion on how to move forward, but thank you for the note and I do think this can be improved :)