diff --git a/README.md b/README.md index 716bc5d..8992aa1 100644 --- a/README.md +++ b/README.md @@ -45,6 +45,7 @@ This command ensures that development [best practices](https://book.getfoundry.s - Test names follow a convention of `^test(Fork)?(Fuzz)?(_Revert(If|When|On))?_(\w+)*$`. (To see a list of example valid test names, see [here](https://github.com/ScopeLift/scopelint/blob/1857e3940bfe92ac5a136827374f4b27ff083971/src/check/validators/test_names.rs#L106-L127)). - Constants and immutables are in `ALL_CAPS`. +- Transient storage variables are prefixed with `t_` (e.g. `t_reentrancyFlag`). - Function names and visibility in forge scripts only have 1 public `run` method per script. - Internal or private functions in the source directory start with a leading underscore. diff --git a/src/check/validators/variable_names.rs b/src/check/validators/variable_names.rs index 2c0e532..7cb1a14 100644 --- a/src/check/validators/variable_names.rs +++ b/src/check/validators/variable_names.rs @@ -19,6 +19,7 @@ fn is_matching_file(parsed: &Parsed) -> bool { /// - Storage variables should NOT have an underscore prefix /// - Non-storage variables (local variables, parameters) should have an underscore prefix /// - Variables that reference storage/storages should NOT have an underscore prefix +/// - Transient storage variables should be prefixed with `t_` (e.g. `t_reentrancyFlag`) pub fn validate(parsed: &Parsed) -> Vec { if !is_matching_file(parsed) { return Vec::new(); @@ -90,7 +91,20 @@ fn validate_function(parsed: &Parsed, f: &FunctionDefinition) -> Vec Option { v.name.as_ref().and_then(|name| { let name_str = &name.name; - if is_valid_state_variable_name(name_str) { + let is_transient = is_transient_state_variable(&parsed.src, name.loc); + + if is_transient { + if name_str.starts_with("t_") { + None + } else { + Some(InvalidItem::new( + ValidatorKind::Variable, + parsed, + name.loc, + format!("Transient state variable '{name_str}' should be prefixed with 't_'"), + )) + } + } else if is_valid_state_variable_name(name_str) { None } else { Some(InvalidItem::new( @@ -183,6 +197,190 @@ fn is_valid_state_variable_name(name: &str) -> bool { !name.starts_with('_') } +/// Detects whether a state variable declaration uses the `transient` keyword by scanning the +/// original source before the variable name. +fn is_transient_state_variable(src: &str, name_loc: solang_parser::pt::Loc) -> bool { + let start = name_loc.start(); + if start > src.len() { + return false; + } + + let decl_start = declaration_start_before_name(src, start); + contains_keyword_before_name(src, decl_start, start, "transient") +} + +#[derive(Clone, Copy, Eq, PartialEq)] +enum ScanState { + Code, + LineComment, + BlockComment, + SingleQuote, + DoubleQuote, +} + +fn declaration_start_before_name(src: &str, end: usize) -> usize { + let bytes = src.as_bytes(); + let mut idx = 0; + let mut last_boundary = 0; + let mut state = ScanState::Code; + + while idx < end { + match state { + ScanState::Code => match bytes[idx] { + b'/' if idx + 1 < end && bytes[idx + 1] == b'/' => { + state = ScanState::LineComment; + idx += 2; + } + b'/' if idx + 1 < end && bytes[idx + 1] == b'*' => { + state = ScanState::BlockComment; + idx += 2; + } + b'\'' => { + state = ScanState::SingleQuote; + idx += 1; + } + b'"' => { + state = ScanState::DoubleQuote; + idx += 1; + } + b';' | b'{' | b'}' => { + last_boundary = idx + 1; + idx += 1; + } + _ => idx += 1, + }, + ScanState::LineComment => { + if bytes[idx] == b'\n' { + state = ScanState::Code; + } + idx += 1; + } + ScanState::BlockComment => { + if bytes[idx] == b'*' && idx + 1 < end && bytes[idx + 1] == b'/' { + state = ScanState::Code; + idx += 2; + } else { + idx += 1; + } + } + ScanState::SingleQuote => { + if bytes[idx] == b'\\' { + idx = (idx + 2).min(end); + } else { + if bytes[idx] == b'\'' { + state = ScanState::Code; + } + idx += 1; + } + } + ScanState::DoubleQuote => { + if bytes[idx] == b'\\' { + idx = (idx + 2).min(end); + } else { + if bytes[idx] == b'"' { + state = ScanState::Code; + } + idx += 1; + } + } + } + } + + last_boundary +} + +fn contains_keyword_before_name(src: &str, start: usize, end: usize, keyword: &str) -> bool { + let bytes = src.as_bytes(); + let keyword = keyword.as_bytes(); + let mut idx = start; + let mut state = ScanState::Code; + + while idx < end { + match state { + ScanState::Code => { + if idx + keyword.len() <= end && + &bytes[idx..idx + keyword.len()] == keyword && + is_keyword_boundary(bytes, start, idx, idx + keyword.len(), end) + { + return true; + } + + match bytes[idx] { + b'/' if idx + 1 < end && bytes[idx + 1] == b'/' => { + state = ScanState::LineComment; + idx += 2; + } + b'/' if idx + 1 < end && bytes[idx + 1] == b'*' => { + state = ScanState::BlockComment; + idx += 2; + } + b'\'' => { + state = ScanState::SingleQuote; + idx += 1; + } + b'"' => { + state = ScanState::DoubleQuote; + idx += 1; + } + _ => idx += 1, + } + } + ScanState::LineComment => { + if bytes[idx] == b'\n' { + state = ScanState::Code; + } + idx += 1; + } + ScanState::BlockComment => { + if bytes[idx] == b'*' && idx + 1 < end && bytes[idx + 1] == b'/' { + state = ScanState::Code; + idx += 2; + } else { + idx += 1; + } + } + ScanState::SingleQuote => { + if bytes[idx] == b'\\' { + idx = (idx + 2).min(end); + } else { + if bytes[idx] == b'\'' { + state = ScanState::Code; + } + idx += 1; + } + } + ScanState::DoubleQuote => { + if bytes[idx] == b'\\' { + idx = (idx + 2).min(end); + } else { + if bytes[idx] == b'"' { + state = ScanState::Code; + } + idx += 1; + } + } + } + } + + false +} + +const fn is_keyword_boundary( + bytes: &[u8], + search_start: usize, + keyword_start: usize, + keyword_end: usize, + search_end: usize, +) -> bool { + let before = keyword_start == search_start || !is_identifier_char(bytes[keyword_start - 1]); + let after = keyword_end == search_end || !is_identifier_char(bytes[keyword_end]); + before && after +} + +const fn is_identifier_char(byte: u8) -> bool { + byte.is_ascii_alphanumeric() || byte == b'_' +} + fn is_valid_local_variable_name(name: &str, is_storage: bool) -> bool { if is_storage { // Storage variables should NOT have underscore prefix @@ -320,4 +518,83 @@ mod tests { }; expected_findings.assert_eq(content, &validate); } + + #[test] + fn test_transient_state_variable_with_correct_prefix() { + let content = r" + contract MyContract { + uint128 transient t_reentrancyFlag; + } + "; + + let expected_findings = ExpectedFindings::new(0); + expected_findings.assert_eq(content, &validate); + } + + #[test] + fn test_transient_state_variable_without_prefix() { + let content = r" + contract MyContract { + uint128 transient reentrancyFlag; + } + "; + + let expected_findings = ExpectedFindings { + src: 1, + test: 1, + handler: 1, + script: 1, + ..ExpectedFindings::default() + }; + expected_findings.assert_eq(content, &validate); + } + + #[test] + fn test_same_line_state_variable_after_transient() { + let content = r" + contract MyContract { + uint128 transient t_reentrancyFlag; uint128 normalFlag; + } + "; + + let expected_findings = ExpectedFindings::new(0); + expected_findings.assert_eq(content, &validate); + } + + #[test] + fn test_transient_state_variable_ignored_with_rule_directive() { + let content = r" + contract MyContract { + // scopelint: ignore-variable-next-line + uint128 transient reentrancyFlag; + } + "; + + let expected_findings = ExpectedFindings::new(0); + expected_findings.assert_eq(content, &validate); + } + + #[test] + fn test_transient_state_variable_ignored_with_ignore_line() { + let content = r" + contract MyContract { + uint128 transient reentrancyFlag; // scopelint: ignore-line + } + "; + + let expected_findings = ExpectedFindings::new(0); + expected_findings.assert_eq(content, &validate); + } + + #[test] + fn test_comment_text_does_not_mark_state_variable_transient() { + let content = r" + contract MyContract { + uint128 /* transient */ reentrancyFlag; + } + "; + + let expected_findings = ExpectedFindings::new(0); + expected_findings.assert_eq(content, &validate); + } } diff --git a/src/parser.rs b/src/parser.rs index f541027..24e10dd 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -8,6 +8,9 @@ use std::sync::LazyLock; static TRANSIENT_KEYWORD: LazyLock = LazyLock::new(|| Regex::new(r"\btransient\b").expect("transient regex is valid")); const TRANSIENT_REPLACEMENT: &str = " "; +// Replacement must be the same length as the keyword so all byte offsets (line numbers, +// comment and inline-config locations) remain valid against the original source. +const _: () = assert!(TRANSIENT_REPLACEMENT.len() == "transient".len()); /// Parses Solidity source code, with a fallback that strips unsupported keywords (e.g. /// `transient`). diff --git a/tests/check-proj1-AllFindings/test/Counter.t.sol b/tests/check-proj1-AllFindings/test/Counter.t.sol index 10ff7a1..786a0f7 100644 --- a/tests/check-proj1-AllFindings/test/Counter.t.sol +++ b/tests/check-proj1-AllFindings/test/Counter.t.sol @@ -7,6 +7,7 @@ contract CounterTest is Test { uint256 constant testVal = 123; Counter public counter; + uint128 transient reentrancyFlag; function setUp() public { counter = new Counter(); diff --git a/tests/check-proj2-NoFindings/test/Counter.t.sol b/tests/check-proj2-NoFindings/test/Counter.t.sol index e1795bc..4520afe 100644 --- a/tests/check-proj2-NoFindings/test/Counter.t.sol +++ b/tests/check-proj2-NoFindings/test/Counter.t.sol @@ -7,7 +7,7 @@ contract CounterTest is Test { uint256 constant TEST_VAL = 123; Counter public counter; - uint128 transient transientVar; + uint128 transient t_reentrancyFlag; function setUp() public { counter = new Counter(); diff --git a/tests/check.rs b/tests/check.rs index 59ca9bd..77bbb9e 100644 --- a/tests/check.rs +++ b/tests/check.rs @@ -52,7 +52,7 @@ fn test_check_proj1_all_findings() { "Invalid src method name in ./src/CounterIgnored3.sol on line 1: Missing SPDX-License-Identifier header", "Invalid src method name in ./src/CounterIgnored4.sol on line 1: Missing SPDX-License-Identifier header", "Invalid src method name in ./src/CounterIgnored4.sol on line 29: missingLeadingUnderscoreAndNotIgnored", - "Invalid test name in ./test/Counter.t.sol on line 16: testIncrementBadName", + "Invalid test name in ./test/Counter.t.sol on line 17: testIncrementBadName", "Invalid directive in ./src/Counter.sol: Invalid inline config item: this directive is invalid", "Invalid variable name in ./script/Counter.s.sol on line 25: Local variable 'x' should have underscore prefix", "Invalid variable name in ./src/Counter.sol on line 19: Parameter 'newNumber' should have underscore prefix", @@ -68,8 +68,9 @@ fn test_check_proj1_all_findings() { "Invalid variable name in ./src/CounterIgnored3.sol on line 39: Parameter 'someImportantUser' should have underscore prefix", "Invalid variable name in ./src/CounterIgnored3.sol on line 48: Parameter 'someImportantUser' should have underscore prefix", "Invalid variable name in ./src/CounterIgnored3.sol on line 7: State variable '_GOOD__IMMUTABLE_' should NOT have underscore prefix", - "Invalid variable name in ./test/Counter.t.sol on line 31: Local variable 'x' should have underscore prefix", - "Invalid variable name in ./test/Counter.t.sol on line 21: Parameter 'x' should have underscore prefix", + "Invalid variable name in ./test/Counter.t.sol on line 32: Local variable 'x' should have underscore prefix", + "Invalid variable name in ./test/Counter.t.sol on line 22: Parameter 'x' should have underscore prefix", + "Invalid variable name in ./test/Counter.t.sol on line 10: Transient state variable 'reentrancyFlag' should be prefixed with 't_'", "Invalid error name in ./src/Counter.sol on line 40: Error 'AnotherInvalidError' should be prefixed with 'Counter_'", "Invalid error name in ./src/Counter.sol on line 39: Error 'InvalidError' should be prefixed with 'Counter_'", "Invalid EIP712 typehash in ./src/Counter.sol: EIP712 typehash 'PERMIT_TYPEHASH' parameter mismatch: typehash defines 5 parameters but abi.encode usage uses 3 parameters",