diff --git a/rules/src/b011_address_format.rs b/rules/src/b011_address_format.rs new file mode 100644 index 00000000..70eacf60 --- /dev/null +++ b/rules/src/b011_address_format.rs @@ -0,0 +1,342 @@ +//! Rule B011: Target Chain Address Format Validation +//! +//! Static rule that flags cross-chain outbound transfer functions which +//! restrict their recipient parameter to a 20-byte EVM `address` (or +//! `address payable`) type when the function is plausibly routing to a +//! non-EVM destination chain. Such functions should instead accept a +//! `bytes` (dynamic) or `bytes32` recipient, since non-EVM chains (Solana, +//! Bitcoin, Cosmos, ...) commonly use address encodings that don't fit in +//! 20 bytes, or aren't fixed-length at all. +//! +//! This codebase's own `contracts/router/BatchBridgeRouter.sol` and +//! `contracts/router/NativeBridgeRouter.sol` already follow the desired +//! convention: they accept `bytes32 recipient` rather than `address +//! recipient` for exactly this reason. +//! +//! ## Heuristic +//! +//! There is no syntactic marker in Solidity source that says "this +//! function is a cross-chain outbound transfer" the way, say, a `payable` +//! keyword marks value-accepting functions. So this rule uses a two-part +//! name-based heuristic to decide whether a function is in scope at all: +//! +//! 1. **Function name** contains one of: `bridge`, `transfer`, `send`, +//! `deposit`, `dispatch` (case-insensitive substring match). +//! 2. The function has **both**: +//! - a parameter whose name contains `recipient`, `destination`, or +//! `target` (case-insensitive) — the "recipient-like" parameter, and +//! - a *separate* parameter whose name contains `chain` (case-insensitive) +//! and whose declared type is an unsigned integer narrower than 256 +//! bits (`uint8`..`uint128`) — a "chain-ID-like" parameter. Narrow +//! unsigned integers are the idiomatic Solidity encoding for a chain +//! ID selector (see `uint32 destinationChainId` in +//! `BatchBridgeRouter.sol`); requiring this second parameter is what +//! lets the rule tell a genuine cross-chain router apart from a +//! purely-domestic function that merely happens to have a +//! `recipient` parameter (e.g. a plain ERC-20 `transfer`/`withdraw` +//! wrapper). +//! +//! Only functions matching *both* (1) and (2) are considered in scope. For +//! every recipient-like parameter of an in-scope function, the rule flags +//! it if its declared type is `address` or `address payable`, and passes it +//! if the type is `bytes` (dynamic) or `bytes32`. Any other declared type +//! (e.g. `string`) is left alone — this rule is specifically about the +//! address-vs-fixed-20-byte-type distinction, not general address encoding +//! validation. +//! +//! ### Documented tradeoffs +//! +//! False negatives: +//! - A cross-chain function named something the heuristic doesn't +//! recognize (e.g. `relayOut`, `moveAssets`) will be silently skipped. +//! - A cross-chain function whose chain-ID parameter is named without +//! "chain" (e.g. `dstEid`, `targetDomain`) won't be recognized as +//! cross-chain, even though it is. +//! - A cross-chain function whose chain selector is encoded as `uint256` +//! rather than a narrower uint won't match the "chain-ID-like" type +//! check as currently written. +//! +//! False positives: +//! - A purely-domestic function that happens to be named e.g. +//! `depositToChainVault` with an unrelated `uint16 chainCount` parameter +//! and an `address recipient` parameter could be incorrectly flagged, +//! even though no address is actually crossing chains. +//! - Interface/abstract declarations (function bodies of `None`) are still +//! checked; this is intentional since interfaces define the ABI contract +//! that implementers must honor, but it means a stray interface stub +//! with these names would also be flagged. +//! +//! This is an approximate, syntactic, first-version heuristic. It has no +//! access to cross-file/interface call-graph information or semantic +//! knowledge of "is this parameter's value ever routed off-chain," so it +//! is expected to need refinement (e.g. configurable name lists, opt-out +//! annotations) as it's used on real codebases. + +use solang_parser::pt::{ + ContractPart, Expression, FunctionDefinition, Loc, Parameter, SourceUnit, SourceUnitPart, Type, +}; + +/// Name fragments (case-insensitive) that mark a function as a plausible +/// cross-chain outbound transfer/dispatch entrypoint. +const TRANSFER_NAME_HINTS: &[&str] = &["bridge", "transfer", "send", "deposit", "dispatch"]; + +/// Name fragments (case-insensitive) that mark a parameter as recipient-like. +const RECIPIENT_NAME_HINTS: &[&str] = &["recipient", "destination", "target"]; + +/// Name fragment (case-insensitive) that marks a parameter as a chain-ID-like +/// selector, when paired with a narrow unsigned integer type. +const CHAIN_NAME_HINT: &str = "chain"; + +/// A single rule violation produced by [`check_source`] / [`check_file`]. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Violation { + /// Name of the offending function. + pub function_name: String, + /// Name of the offending recipient-like parameter. + pub parameter_name: String, + /// 1-based source line number of the function definition, if it could + /// be computed from the AST's byte-offset location. + pub line: Option, + /// Human-readable description of the violation. + pub message: String, +} + +/// Parses `source` as Solidity and runs rule B011 against it. +/// +/// Returns `Err` with human-readable parse diagnostics if the source could +/// not be parsed at all. A successfully parsed source that has zero +/// violations returns `Ok(vec![])`. +pub fn check_source(source: &str) -> Result, String> { + let (source_unit, _comments) = + solang_parser::parse(source, 0).map_err(|diags| format!("{diags:?}"))?; + + Ok(check_source_unit(&source_unit, source)) +} + +/// Reads `path` from disk and runs rule B011 against its contents. +pub fn check_file(path: &std::path::Path) -> Result, String> { + let source = std::fs::read_to_string(path) + .map_err(|e| format!("failed to read {}: {e}", path.display()))?; + check_source(&source) +} + +/// Walks every contract's functions (plus any free/top-level functions) in +/// `source_unit`, applying the B011 heuristic and type check. +fn check_source_unit(source_unit: &SourceUnit, source: &str) -> Vec { + let mut violations = Vec::new(); + + for part in &source_unit.0 { + match part { + SourceUnitPart::ContractDefinition(contract) => { + for cpart in &contract.parts { + if let ContractPart::FunctionDefinition(func) = cpart { + check_function(func, source, &mut violations); + } + } + } + SourceUnitPart::FunctionDefinition(func) => { + check_function(func, source, &mut violations); + } + _ => {} + } + } + + violations +} + +/// Applies the cross-chain-transfer heuristic and, if the function is in +/// scope, checks each recipient-like parameter's type. +fn check_function(func: &FunctionDefinition, source: &str, out: &mut Vec) { + let Some(name_ident) = &func.name else { + return; + }; + let function_name = name_ident.name.as_str(); + + if !name_contains_any(function_name, TRANSFER_NAME_HINTS) { + return; + } + + let params: Vec<&Parameter> = func + .params + .iter() + .filter_map(|(_, p)| p.as_ref()) + .collect(); + + let has_chain_param = params.iter().any(|p| is_chain_id_param(p)); + if !has_chain_param { + return; + } + + for param in ¶ms { + let Some(param_name_ident) = ¶m.name else { + continue; + }; + let param_name = param_name_ident.name.as_str(); + + if !name_contains_any(param_name, RECIPIENT_NAME_HINTS) { + continue; + } + + if let Some(restricted_type) = restricted_address_type(¶m.ty) { + let line = line_number(func.loc, source); + out.push(Violation { + function_name: function_name.to_string(), + parameter_name: param_name.to_string(), + line, + message: format!( + "function `{function_name}` accepts a recipient parameter `{param_name}` of \ + restricted type `{restricted_type}` (exactly 20 bytes), but this function \ + appears to route to a non-EVM-aware destination (has a chain-ID-like \ + parameter). Use `bytes` or `bytes32` instead so non-EVM recipient addresses \ + (e.g. Solana, Bitcoin, Cosmos) are not truncated." + ), + }); + } + } +} + +/// Returns true if `param` looks like a chain-ID selector: its name +/// contains "chain" and its declared type is a narrow (< 256-bit) unsigned +/// integer. +fn is_chain_id_param(param: &Parameter) -> bool { + let Some(name_ident) = ¶m.name else { + return false; + }; + if !name_ident.name.to_lowercase().contains(CHAIN_NAME_HINT) { + return false; + } + + matches!( + type_of(¶m.ty), + Some(Type::Uint(bits)) if *bits < 256 + ) +} + +/// If `ty` is a restricted 20-byte address type (`address` / `address +/// payable`), returns a display label for it; otherwise `None` (i.e. the +/// type is fine, e.g. `bytes` / `bytes32` / anything else). +fn restricted_address_type(ty: &Expression) -> Option<&'static str> { + match type_of(ty) { + Some(Type::Address) => Some("address"), + Some(Type::AddressPayable) => Some("address payable"), + _ => None, + } +} + +/// Extracts the `pt::Type` out of a parameter's `Expression` slot, if the +/// expression is a plain elementary type reference (as it is for all +/// built-in Solidity types like `address`, `bytes32`, `uint32`, etc.). +fn type_of(ty: &Expression) -> Option<&Type> { + match ty { + Expression::Type(_, t) => Some(t), + _ => None, + } +} + +/// Case-insensitive "does `haystack` contain any of `needles`" check. +fn name_contains_any(haystack: &str, needles: &[&str]) -> bool { + let lower = haystack.to_lowercase(); + needles.iter().any(|n| lower.contains(n)) +} + +/// Converts a byte offset captured in `loc` into a 1-based source line +/// number by counting newlines in `source` up to that offset. +/// `solang_parser::pt::Loc` only carries byte offsets, not line/column +/// info, so this is computed manually. +fn line_number(loc: Loc, source: &str) -> Option { + match loc { + Loc::File(_, start, _) => { + let prefix = source.get(..start)?; + Some(prefix.matches('\n').count() + 1) + } + _ => None, + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::path::Path; + + /// Locates `test/fixtures/b011_samples.sol` relative to this crate's + /// root (`rules/`), i.e. `/test/fixtures/b011_samples.sol`. + fn fixture_path() -> std::path::PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")).join("../test/fixtures/b011_samples.sol") + } + + fn run_fixture() -> Vec { + let path = fixture_path(); + check_file(&path).unwrap_or_else(|e| { + panic!("failed to check fixture at {}: {e}", path.display()); + }) + } + + #[test] + fn fixture_file_exists_and_parses() { + let path = fixture_path(); + assert!( + path.exists(), + "expected fixture file to exist at {}", + path.display() + ); + let source = std::fs::read_to_string(&path).expect("read fixture"); + check_source(&source).expect("fixture should parse as valid Solidity"); + } + + #[test] + fn flags_address_recipient_on_cross_chain_transfer() { + let violations = run_fixture(); + let hit = violations + .iter() + .find(|v| v.function_name == "bridgeTransfer"); + assert!( + hit.is_some(), + "expected `bridgeTransfer` (address recipient + chain-id param) to be flagged; got {violations:?}" + ); + let hit = hit.unwrap(); + assert_eq!(hit.parameter_name, "recipient"); + assert!(hit.line.is_some(), "expected a resolved line number"); + } + + #[test] + fn passes_bytes32_recipient_on_cross_chain_transfer() { + let violations = run_fixture(); + assert!( + violations + .iter() + .all(|v| v.function_name != "bridgeTransferBytes32"), + "did not expect `bridgeTransferBytes32` (bytes32 recipient) to be flagged; got {violations:?}" + ); + } + + #[test] + fn passes_bytes_recipient_on_cross_chain_transfer() { + let violations = run_fixture(); + assert!( + violations + .iter() + .all(|v| v.function_name != "bridgeTransferBytes"), + "did not expect `bridgeTransferBytes` (bytes calldata recipient) to be flagged; got {violations:?}" + ); + } + + #[test] + fn does_not_flag_non_cross_chain_function_with_recipient_param() { + let violations = run_fixture(); + assert!( + violations.iter().all(|v| v.function_name != "withdraw"), + "did not expect `withdraw` (no chain-id param, non-cross-chain name) to be flagged \ + even though it has an `address recipient` param; got {violations:?}" + ); + } + + #[test] + fn total_violation_count_matches_expected() { + let violations = run_fixture(); + assert_eq!( + violations.len(), + 1, + "expected exactly 1 violation (bridgeTransfer only); got {violations:?}" + ); + } +} diff --git a/test/fixtures/b011_samples.sol b/test/fixtures/b011_samples.sol new file mode 100644 index 00000000..6f21ede5 --- /dev/null +++ b/test/fixtures/b011_samples.sol @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +/// @title B011Samples +/// @notice Fixture contract for rule B011 (Target Chain Address Format +/// Validation). Not intended to compile as a deployable contract — +/// function bodies are stubs. Used purely as an AST-parsing sample +/// for the `rules` static-analysis tool's test suite. +/// +/// Cases covered: +/// 1. bridgeTransfer -> cross-chain transfer using `address recipient` +/// alongside a chain-ID param => VIOLATION +/// 2. bridgeTransferBytes32 -> equivalent function using `bytes32 recipient` +/// => PASS +/// 3. bridgeTransferBytes -> equivalent function using `bytes calldata +/// recipient` => PASS +/// 4. withdraw -> merely has a parameter named "recipient" but +/// is not a cross-chain transfer (no chain-ID +/// param, ordinary-looking name) and uses +/// `address recipient` => should NOT be flagged +contract B011Samples { + // --- Case 1: VIOLATION --- + // Cross-chain outbound transfer (name contains "bridge" + "transfer", + // has a chain-ID-ish param) but restricts the recipient to a 20-byte + // EVM `address`, which cannot represent non-EVM chain recipients + // (e.g. Solana pubkeys, Bitcoin/Cosmos addresses). + function bridgeTransfer( + uint32 destinationChainId, + address recipient, + uint256 amount + ) external { + // stub + } + + // --- Case 2: PASS --- + // Same shape as Case 1, but the recipient is normalized to a `bytes32`, + // matching this codebase's real convention (see + // contracts/router/BatchBridgeRouter.sol / NativeBridgeRouter.sol). + function bridgeTransferBytes32( + uint32 destinationChainId, + bytes32 recipient, + uint256 amount + ) external { + // stub + } + + // --- Case 3: PASS --- + // Same shape as Case 1, but the recipient is a dynamic `bytes` value, + // which can hold variable-length non-EVM address encodings. + function bridgeTransferBytes( + uint32 destinationChainId, + bytes calldata recipient, + uint256 amount + ) external { + // stub + } + + // --- Case 4: should NOT be flagged --- + // Has a parameter literally named "recipient" and uses a restricted + // `address` type, but this is an ordinary domestic withdrawal with no + // chain-ID parameter and a non-cross-chain-sounding name. The rule's + // heuristic must not flag every "recipient"-named `address` parameter + // everywhere — only ones plausibly bound for a non-EVM destination. + function withdraw(address recipient, uint256 amount) external { + // stub + } +}