From 35419e82026e138ac0a16ffb3a55dd761fc98883 Mon Sep 17 00:00:00 2001 From: xermicus Date: Wed, 5 Feb 2025 11:42:35 +0100 Subject: [PATCH] solidity: update custom warnings and version validation (#193) --- CHANGELOG.md | 2 + crates/solidity/src/resolc/main.rs | 2 +- crates/solidity/src/solc/mod.rs | 3 - crates/solidity/src/solc/solc_compiler.rs | 40 +--------- crates/solidity/src/solc/soljson_compiler.rs | 50 +++++-------- .../solc/standard_json/output/error/mod.rs | 63 +++++++--------- crates/solidity/src/solc/version.rs | 22 ++++++ crates/solidity/src/tests/messages.rs | 73 +++++++++---------- 8 files changed, 112 insertions(+), 143 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 88690a4..e71febd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,12 +14,14 @@ Supported `polkadot-sdk` rev: `274a781e8ca1a9432c7ec87593bd93214abbff50` - The `resolc` executable prints the help by default. - Removed support for legacy EVM assembly (EVMLA) translation. - integration: identify cached code blobs on source code to fix potential confusions. +- Setting base, include or allow paths in emscripten is now a hard error. ### Fixed - Solidity: Add the solc `--libraries` files to sources. - A data race in tests. - Fix `broken pipe` errors. - llvm-builder: Allow warnings. +- solidity: Fix the custom compiler warning messages. ## v0.1.0-dev.9 diff --git a/crates/solidity/src/resolc/main.rs b/crates/solidity/src/resolc/main.rs index 11e7a31..b308771 100644 --- a/crates/solidity/src/resolc/main.rs +++ b/crates/solidity/src/resolc/main.rs @@ -103,7 +103,7 @@ fn main_inner() -> anyhow::Result<()> { let mut solc = { #[cfg(target_os = "emscripten")] { - revive_solidity::SoljsonCompiler { version: None } + revive_solidity::SoljsonCompiler } #[cfg(not(target_os = "emscripten"))] diff --git a/crates/solidity/src/solc/mod.rs b/crates/solidity/src/solc/mod.rs index b0c88d6..c2c7ad9 100644 --- a/crates/solidity/src/solc/mod.rs +++ b/crates/solidity/src/solc/mod.rs @@ -19,9 +19,6 @@ use self::version::Version; /// The first version of `solc` with the support of standard JSON interface. pub const FIRST_SUPPORTED_VERSION: semver::Version = semver::Version::new(0, 8, 0); -/// The first version of `solc`, where `--via-ir` codegen mode is supported. -pub const FIRST_VIA_IR_VERSION: semver::Version = semver::Version::new(0, 8, 13); - /// The last supported version of `solc`. pub const LAST_SUPPORTED_VERSION: semver::Version = semver::Version::new(0, 8, 28); diff --git a/crates/solidity/src/solc/solc_compiler.rs b/crates/solidity/src/solc/solc_compiler.rs index ca8052c..947e97b 100644 --- a/crates/solidity/src/solc/solc_compiler.rs +++ b/crates/solidity/src/solc/solc_compiler.rs @@ -1,4 +1,4 @@ -//! The Solidity compiler. +//! The Solidity compiler solc interface. use std::io::Write; use std::path::Path; @@ -8,7 +8,6 @@ use crate::solc::combined_json::CombinedJson; use crate::solc::standard_json::input::Input as StandardJsonInput; use crate::solc::standard_json::output::Output as StandardJsonOutput; use crate::solc::version::Version; -use crate::solc::FIRST_INCLUDE_PATH_VERSION; use super::Compiler; @@ -16,8 +15,6 @@ use super::Compiler; pub struct SolcCompiler { /// The binary executable name. pub executable: String, - /// The lazily-initialized compiler version. - pub version: Option, } impl SolcCompiler { @@ -34,10 +31,7 @@ impl SolcCompiler { error ); } - Ok(Self { - executable, - version: None, - }) + Ok(Self { executable }) } } @@ -50,11 +44,7 @@ impl Compiler for SolcCompiler { include_paths: Vec, allow_paths: Option, ) -> anyhow::Result { - let version = self.version()?.default; - - if !include_paths.is_empty() && version < FIRST_INCLUDE_PATH_VERSION { - anyhow::bail!("--include-path is not supported in solc {version}"); - } + let version = self.version()?.validate(&include_paths)?.default; let mut command = std::process::Command::new(self.executable.as_str()); command.stdin(std::process::Stdio::piped()); @@ -225,10 +215,6 @@ impl Compiler for SolcCompiler { /// The `solc --version` mini-parser. fn version(&mut self) -> anyhow::Result { - if let Some(version) = self.version.as_ref() { - return Ok(version.to_owned()); - } - let mut command = std::process::Command::new(self.executable.as_str()); command.arg("--version"); let output = command.output().map_err(|error| { @@ -274,24 +260,6 @@ impl Compiler for SolcCompiler { .and_then(|line| line.split('-').nth(1)) .and_then(|version| version.parse().ok()); - let version = Version::new(long, default, l2_revision); - if version.default < super::FIRST_SUPPORTED_VERSION { - anyhow::bail!( - "`solc` versions <{} are not supported, found {}", - super::FIRST_SUPPORTED_VERSION, - version.default - ); - } - if version.default > super::LAST_SUPPORTED_VERSION { - anyhow::bail!( - "`solc` versions >{} are not supported, found {}", - super::LAST_SUPPORTED_VERSION, - version.default - ); - } - - self.version = Some(version.clone()); - - Ok(version) + Ok(Version::new(long, default, l2_revision)) } } diff --git a/crates/solidity/src/solc/soljson_compiler.rs b/crates/solidity/src/solc/soljson_compiler.rs index c73bdce..0c94664 100644 --- a/crates/solidity/src/solc/soljson_compiler.rs +++ b/crates/solidity/src/solc/soljson_compiler.rs @@ -1,4 +1,4 @@ -//! The Solidity compiler. +//! The Solidity compiler solJson interface. use std::path::Path; use std::path::PathBuf; @@ -18,22 +18,30 @@ extern "C" { } /// The Solidity compiler. -pub struct SoljsonCompiler { - /// The lazily-initialized compiler version. - pub version: Option, -} +pub struct SoljsonCompiler; impl Compiler for SoljsonCompiler { /// Compiles the Solidity `--standard-json` input into Yul IR. fn standard_json( &mut self, mut input: StandardJsonInput, - _base_path: Option, - _include_paths: Vec, - _allow_paths: Option, + base_path: Option, + include_paths: Vec, + allow_paths: Option, ) -> anyhow::Result { - let version = self.version()?; - input.normalize(&version.default); + if !include_paths.is_empty() { + anyhow::bail!("configuring include paths is not supported with solJson") + } + if base_path.is_some() { + anyhow::bail!("configuring the base path is not supported with solJson") + } + if allow_paths.is_some() { + anyhow::bail!("configuring allow paths is not supported with solJson") + } + + let version = self.version()?.validate(&include_paths)?.default; + input.normalize(&version); + let suppressed_warnings = input.suppressed_warnings.take().unwrap_or_default(); let input_json = serde_json::to_string(&input).expect("Always valid"); @@ -74,31 +82,11 @@ impl Compiler for SoljsonCompiler { .ok_or_else(|| anyhow::anyhow!("Soljson version parsing: metadata dropping"))? .parse() .map_err(|error| anyhow::anyhow!("Soljson version parsing: {}", error))?; - let l2_revision: Option = version .split('-') .nth(1) .and_then(|version| version.parse().ok()); - - let version = Version::new(long, default, l2_revision); - if version.default < super::FIRST_SUPPORTED_VERSION { - anyhow::bail!( - "`Soljson` versions <{} are not supported, found {}", - super::FIRST_SUPPORTED_VERSION, - version.default - ); - } - if version.default > super::LAST_SUPPORTED_VERSION { - anyhow::bail!( - "`Soljson` versions >{} are not supported, found {}", - super::LAST_SUPPORTED_VERSION, - version.default - ); - } - - self.version = Some(version.clone()); - - Ok(version) + Ok(Version::new(long, default, l2_revision)) } } diff --git a/crates/solidity/src/solc/standard_json/output/error/mod.rs b/crates/solidity/src/solc/standard_json/output/error/mod.rs index 065e160..7921654 100644 --- a/crates/solidity/src/solc/standard_json/output/error/mod.rs +++ b/crates/solidity/src/solc/standard_json/output/error/mod.rs @@ -33,13 +33,12 @@ impl Error { /// Returns the `ecrecover` function usage warning. pub fn message_ecrecover(src: Option<&str>) -> Self { let message = r#" -┌──────────────────────────────────────────────────────────────────────────────────────────────────┐ -│ Warning: It looks like you are using 'ecrecover' to validate a signature of a user account. │ -│ Polkadot comes with native account abstraction support, therefore it is highly recommended NOT │ -│ to rely on the fact that the account has an ECDSA private key attached to it since accounts might│ -│ implement other signature schemes. │ -└──────────────────────────────────────────────────────────────────────────────────────────────────┘"# - .to_owned(); +Warning: It looks like you are using 'ecrecover' to validate a signature of a user account. +Polkadot comes with native account abstraction support, therefore it is highly recommended NOT +to rely on the fact that the account has an ECDSA private key attached to it since accounts might +implement other signature schemes. +"# + .to_owned(); Self { component: "general".to_owned(), @@ -55,17 +54,13 @@ impl Error { /// Returns the `
`'s `send` and `transfer` methods usage error. pub fn message_send_and_transfer(src: Option<&str>) -> Self { let message = r#" -┌──────────────────────────────────────────────────────────────────────────────────────────────────┐ -│ Warning: It looks like you are using '
.send/transfer()' without providing │ -│ the gas amount. Such calls will fail depending on the pubdata costs. │ -│ This might be a false positive if you are using an interface (like IERC20) instead of the │ -│ native Solidity `send/transfer`. │ -│ Please use 'payable(
).call{value: }("")' instead, but be careful with the reentrancy │ -│ attack. `send` and `transfer` send limited amount of gas that prevents reentrancy, whereas │ -│ `
.call{value: }` sends all gas to the callee. Learn more on │ -│ https://docs.soliditylang.org/en/latest/security-considerations.html#reentrancy │ -└──────────────────────────────────────────────────────────────────────────────────────────────────┘"# - .to_owned(); +Warning: It looks like you are using '
.send/transfer()'. Such balance +transfer calls will supply all remaining gas and disable call re-entrancy instead of +supplying the 2300 gas stipend. However, the compiler uses a heuristic to detect the expected +2300 gas stipend. You are advised to carefully test this to ensure the desired behavior. +Learn more on https://docs.soliditylang.org/en/latest/security-considerations.html#reentrancy +"# + .to_owned(); Self { component: "general".to_owned(), @@ -81,16 +76,15 @@ impl Error { /// Returns the `extcodesize` instruction usage warning. pub fn message_extcodesize(src: Option<&str>) -> Self { let message = r#" -┌──────────────────────────────────────────────────────────────────────────────────────────────────┐ -│ Warning: Your code or one of its dependencies uses the 'extcodesize' instruction, which is │ -│ usually needed in the following cases: │ -│ 1. To detect whether an address belongs to a smart contract. │ -│ 2. To detect whether the deploy code execution has finished. │ -│ Polkadot comes with native account abstraction support (so smart contracts are just accounts │ -│ coverned by code), and you should avoid differentiating between contracts and non-contract | -| addresses. │ -└──────────────────────────────────────────────────────────────────────────────────────────────────┘"# - .to_owned(); +Warning: Your code or one of its dependencies uses the 'extcodesize' instruction, which is +usually needed in the following cases: + 1. To detect whether an address belongs to a smart contract. + 2. To detect whether the deploy code execution has finished. +Polkadot comes with native account abstraction support (so smart contracts are just accounts +coverned by code), and you should avoid differentiating between contracts and non-contract +addresses. +"# + .to_owned(); Self { component: "general".to_owned(), @@ -106,13 +100,12 @@ impl Error { /// Returns the `origin` instruction usage warning. pub fn message_tx_origin(src: Option<&str>) -> Self { let message = r#" -┌──────────────────────────────────────────────────────────────────────────────────────────────────┐ -│ Warning: You are checking for 'tx.origin' in your code, which might lead to unexpected behavior. │ -│ Polkadot comes with native account abstraction support, and therefore the initiator of a │ -│ transaction might be different from the contract calling your code. It is highly recommended NOT │ -│ to rely on tx.origin, but use msg.sender instead. │ -└──────────────────────────────────────────────────────────────────────────────────────────────────┘"# - .to_owned(); +Warning: You are checking for 'tx.origin' in your code, which might lead to unexpected behavior. +Polkadot comes with native account abstraction support, and therefore the initiator of a +transaction might be different from the contract calling your code. It is highly recommended NOT +to rely on tx.origin, but use msg.sender instead. +"# + .to_owned(); Self { component: "general".to_owned(), diff --git a/crates/solidity/src/solc/version.rs b/crates/solidity/src/solc/version.rs index 88613d0..e1ab041 100644 --- a/crates/solidity/src/solc/version.rs +++ b/crates/solidity/src/solc/version.rs @@ -36,4 +36,26 @@ impl Version { l2_revision: None, } } + + pub fn validate(self, include_paths: &[String]) -> anyhow::Result { + if self.default < super::FIRST_SUPPORTED_VERSION { + anyhow::bail!( + "`solc` versions <{} are not supported, found {}", + super::FIRST_SUPPORTED_VERSION, + self.default + ); + } + if self.default > super::LAST_SUPPORTED_VERSION { + anyhow::bail!( + "`solc` versions >{} are not supported, found {}", + super::LAST_SUPPORTED_VERSION, + self.default + ); + } + if !include_paths.is_empty() && self.default < super::FIRST_INCLUDE_PATH_VERSION { + anyhow::bail!("--include-path is not supported in solc {}", self.default); + } + + Ok(self) + } } diff --git a/crates/solidity/src/tests/messages.rs b/crates/solidity/src/tests/messages.rs index 1ddd1b1..d0ce0af 100644 --- a/crates/solidity/src/tests/messages.rs +++ b/crates/solidity/src/tests/messages.rs @@ -64,32 +64,33 @@ contract SendExample { require(success, "Failed to send Ether"); } } - "#; +"#; + +pub const BALANCE_CALLS_MESSAGE: &str = + "Warning: It looks like you are using '
.send/transfer()'"; #[test] fn send() { - assert!( - super::check_solidity_warning( - SEND_TEST_SOURCE, - "Warning: It looks like you are using '
.send/transfer()' without providing", - BTreeMap::new(), - false, - None, - ).expect("Test failure") - ); + assert!(super::check_solidity_warning( + SEND_TEST_SOURCE, + BALANCE_CALLS_MESSAGE, + BTreeMap::new(), + false, + None, + ) + .expect("Test failure")); } #[test] fn send_suppressed() { - assert!( - !super::check_solidity_warning( - SEND_TEST_SOURCE, - "Warning: It looks like you are using '
.send/transfer()' without providing", - BTreeMap::new(), - false, - Some(vec![Warning::SendTransfer]), - ).expect("Test failure") - ); + assert!(!super::check_solidity_warning( + SEND_TEST_SOURCE, + BALANCE_CALLS_MESSAGE, + BTreeMap::new(), + false, + Some(vec![Warning::SendTransfer]), + ) + .expect("Test failure")); } pub const TRANSFER_TEST_SOURCE: &str = r#" @@ -111,28 +112,26 @@ contract TransferExample { #[test] fn transfer() { - assert!( - super::check_solidity_warning( - TRANSFER_TEST_SOURCE, - "Warning: It looks like you are using '
.send/transfer()' without providing", - BTreeMap::new(), - false, - None, - ).expect("Test failure") - ); + assert!(super::check_solidity_warning( + TRANSFER_TEST_SOURCE, + BALANCE_CALLS_MESSAGE, + BTreeMap::new(), + false, + None, + ) + .expect("Test failure")); } #[test] fn transfer_suppressed() { - assert!( - !super::check_solidity_warning( - TRANSFER_TEST_SOURCE, - "Warning: It looks like you are using '
.send/transfer()' without providing", - BTreeMap::new(), - false, - Some(vec![Warning::SendTransfer]), - ).expect("Test failure") - ); + assert!(!super::check_solidity_warning( + TRANSFER_TEST_SOURCE, + BALANCE_CALLS_MESSAGE, + BTreeMap::new(), + false, + Some(vec![Warning::SendTransfer]), + ) + .expect("Test failure")); } pub const EXTCODESIZE_TEST_SOURCE: &str = r#"