Revert and require reasons

README.md

@@ -18,9 +18,9 @@ Note: These goals/priorities will change over time.
   * [x] [Low: Use of ecrecover](https://github.com/Picodes/4naly3er/blob/main/src/issues/L/useOfEcrecover.ts)
   * [x] [NC: Address(0) check](https://github.com/Picodes/4naly3er/blob/main/src/issues/NC/address0Check.ts)
   * [ ] [NC: Non-reentrant before modifiers](https://github.com/Picodes/4naly3er/blob/main/src/issues/NC/nonReentrantBeforeModifiers.ts)
-  * [ ] [NC: Require with string](https://github.com/Picodes/4naly3er/blob/main/src/issues/NC/requireWithString.ts)
+  * [x] [NC: Require with string](https://github.com/Picodes/4naly3er/blob/main/src/issues/NC/requireWithString.ts)
   * [ ] [NC: Return value from approve](https://github.com/Picodes/4naly3er/blob/main/src/issues/NC/returnValueOfApprove.ts)
-  * [ ] [NC: Todo in code](https://github.com/Picodes/4naly3er/blob/main/src/issues/NC/todoLeftInTheCode.ts)
+  * [ ] ~[NC: Todo in code](https://github.com/Picodes/4naly3er/blob/main/src/issues/NC/todoLeftInTheCode.ts)~ -> Need Regex detector support unless the TODO statements are in StructuredDocumentation blocks
   * [x] [NC: Unindexed events](https://github.com/Picodes/4naly3er/blob/main/src/issues/NC/unindexedEvent.ts)
   * [x] [NC: Use constants](https://github.com/Picodes/4naly3er/blob/main/src/issues/NC/useConstants.ts)
   * [x] [NC: Useless public function](https://github.com/Picodes/4naly3er/blob/main/src/issues/NC/uselessPublic.ts)

report.md

@@ -14,51 +14,58 @@ There is a subtle difference between the implementation of solmate's SafeTransfe
 https://github.com/transmissions11/solmate/blob/main/src/utils/SafeTransferLib.sol#L9 
 `@dev Note that none of the functions in this library check that a token has code at all! That responsibility is delegated to the caller`
 
-- Found in src/DeprecatedOZFunctions.sol: 579:22:37
-- Found in src/DeprecatedOZFunctions.sol: 898:17:37
 - Found in src/T11sTranferer.sol: 294:18:43
+- Found in src/DeprecatedOZFunctions.sol: 898:17:12
+- Found in src/DeprecatedOZFunctions.sol: 579:22:12
 # Low Issues
 ## `abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()`
 Use `abi.encode()` instead which will pad items to 32 bytes, which will [prevent hash collisions](https://docs.soliditylang.org/en/v0.8.13/abi-spec.html#non-standard-packed-mode) (e.g. `abi.encodePacked(0x123,0x456)` => `0x123456` => `abi.encodePacked(0x1,0x23456)`, but `abi.encode(0x123,0x456)` => `0x0...1230...456`). Unless there is a compelling reason, `abi.encode` should be preferred. If there is only one argument to `abi.encodePacked()` it can often be cast to `bytes()` or `bytes32()` [instead](https://ethereum.stackexchange.com/questions/30912/how-to-compare-strings-in-solidity#answer-82739).
 If all arguments are strings and or bytes, `bytes.concat()` should be used instead.
 - Found in src/KeccakContract.sol: 584:16:41
-- Found in src/KeccakContract.sol: 878:16:41
 - Found in src/KeccakContract.sol: 731:16:41
+- Found in src/KeccakContract.sol: 878:16:41
 ## `ecrecover` is susceptible to signature malleability
 The `ecrecover` function is susceptible to signature malleability. This means that the same message can be signed in multiple ways, allowing an attacker to change the message signature without invalidating it. This can lead to unexpected behavior in smart contracts, such as the loss of funds or the ability to bypass access control. Consider using OpenZeppelin's ECDSA library instead of the built-in function.
 - Found in src/ExtendedInheritance.sol: 705:9:0
 ## Deprecated OpenZeppelin functions should not be used
 Openzeppelin has deprecated several functions and replaced with newer versions. Please consult https://docs.openzeppelin.com/
-- Found in src/DeprecatedOZFunctions.sol: 737:10:37
-- Found in src/DeprecatedOZFunctions.sol: 898:17:37
+- Found in src/DeprecatedOZFunctions.sol: 737:10:12
+- Found in src/DeprecatedOZFunctions.sol: 898:17:12
 ## Unsafe ERC20 Operations should not be used
 ERC20 functions may not behave as expected. For example: return values are not always meaningful. It is recommended to use OpenZeppelin's SafeERC20 library.
-- Found in src/DeprecatedOZFunctions.sol: 1062:13:37
-- Found in src/DeprecatedOZFunctions.sol: 1236:18:37
+- Found in src/DeprecatedOZFunctions.sol: 1322:13:12
+- Found in src/DeprecatedOZFunctions.sol: 1272:13:12
+- Found in src/DeprecatedOZFunctions.sol: 1062:13:12
+- Found in src/DeprecatedOZFunctions.sol: 1598:18:12
+- Found in src/DeprecatedOZFunctions.sol: 1424:13:12
 ## Solidity pragma should be specific, not wide
 Consider using a specific version of Solidity in your contracts instead of a wide version. For example, instead of `pragma solidity ^0.8.0;`, use `pragma solidity 0.8.0;`
 - Found in src/Counter.sol: 39:24:18
-- Found in src/InheritanceBase.sol: 32:23:2
 - Found in src/IContractInheritance.sol: 32:24:39
+- Found in src/InheritanceBase.sol: 32:23:2
 # NC Issues
 ## Missing checks for `address(0)` when assigning values to address state variables
 Assigning values to address state variables without checking for `address(0)`.
 - Found in src/StateVariables.sol: 2121:14:19
 ## Functions not used internally could be marked external
 
-- Found in src/StateVariables.sol: 1755:145:19
-- Found in src/StateVariables.sol: 1426:292:19
-- Found in src/Counter.sol: 120:80:18
-- Found in src/StateVariables.sol: 1906:151:19
 - Found in src/StateVariables.sol: 2148:346:19
+- Found in src/StateVariables.sol: 2500:376:19
 - Found in src/AdminContract.sol: 183:26:35
+- Found in src/Counter.sol: 120:80:18
+- Found in src/StateVariables.sol: 1426:292:19
+- Found in src/StateVariables.sol: 1755:145:19
 - Found in src/StateVariables.sol: 2063:79:19
-- Found in src/StateVariables.sol: 2500:376:19
+- Found in src/StateVariables.sol: 1906:151:19
 ## Constants should be defined and used instead of literals
 
 - Found in src/ExtendedInheritance.sol: 466:1:0
-- Found in src/Counter.sol: 393:1:18
+- Found in src/Counter.sol: 434:1:18
 ## Event is missing `indexed` fields
 Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
-- Found in src/ExtendedInheritance.sol: 144:45:0
 - Found in src/InheritanceBase.sol: 150:28:2
+- Found in src/ExtendedInheritance.sol: 144:45:0
+## `require()` / `revert()` statements should have descriptive reason strings or custom errors
+
+- Found in src/DeprecatedOZFunctions.sol: 1264:7:12
+- Found in src/DeprecatedOZFunctions.sol: 1389:6:12

src/detector/detector.rs

@@ -14,6 +14,7 @@ use crate::{
         },
         nc::{
             constants_instead_of_literals::ConstantsInsteadOfLiteralsDetector,
+            require_with_string::RequireWithStringDetector,
             unindexed_events::UnindexedEventsDetector,
             useless_public_function::UselessPublicFunctionDetector,
             zero_address_check::ZeroAddressCheckDetector,
@@ -36,6 +37,7 @@ pub fn get_all_detectors() -> Vec<Box<dyn Detector>> {
         Box::new(UselessPublicFunctionDetector::default()),
         Box::new(ConstantsInsteadOfLiteralsDetector::default()),
         Box::new(UnindexedEventsDetector::default()),
+        Box::new(RequireWithStringDetector::default()),
     ]
 }
 

src/detector/low/unsafe_erc20_functions.rs

@@ -68,7 +68,7 @@ mod unsafe_erc20_functions_tests {
         assert!(found);
         // assert that the detector found the correct abi encode packed
         // failure0, failure1 and failure3
-        assert_eq!(detector.instances().len(), 2);
+        assert_eq!(detector.instances().len(), 5);
         // assert that the severity is low
         assert_eq!(
             detector.severity(),

src/detector/nc/mod.rs

@@ -1,4 +1,5 @@
 pub mod constants_instead_of_literals;
+pub mod require_with_string;
 pub mod unindexed_events;
 pub mod useless_public_function;
 pub mod zero_address_check;

src/detector/nc/require_with_string.rs

@@ -0,0 +1,85 @@
+use std::error::Error;
+
+use crate::{
+    ast::Identifier,
+    context::loader::{ASTNode, ContextLoader},
+    detector::detector::{Detector, IssueSeverity},
+};
+use eyre::Result;
+
+#[derive(Default)]
+pub struct RequireWithStringDetector {
+    found_require_without_string: Vec<Option<ASTNode>>,
+}
+
+impl Detector for RequireWithStringDetector {
+    fn detect(&mut self, loader: &ContextLoader) -> Result<bool, Box<dyn Error>> {
+        // Collect all require statements without a string literal.
+        let requires_and_reverts: Vec<&Identifier> = loader
+            .get_identifiers()
+            .iter()
+            .filter(|id| id.name == "revert" || id.name == "require")
+            .cloned()
+            .collect();
+
+        for id in requires_and_reverts {
+            if id.name == "revert" && id.argument_types.as_ref().unwrap().len() == 0 {
+                self.found_require_without_string
+                    .push(Some(ASTNode::Identifier(id.clone())));
+            } else if id.name == "require" && id.argument_types.as_ref().unwrap().len() == 1 {
+                self.found_require_without_string
+                    .push(Some(ASTNode::Identifier(id.clone())));
+            }
+        }
+
+        Ok(!self.found_require_without_string.is_empty())
+    }
+
+    fn title(&self) -> String {
+        String::from("`require()` / `revert()` statements should have descriptive reason strings or custom errors")
+    }
+
+    fn description(&self) -> String {
+        String::from("")
+    }
+
+    fn severity(&self) -> IssueSeverity {
+        IssueSeverity::NC
+    }
+
+    fn instances(&self) -> Vec<Option<ASTNode>> {
+        self.found_require_without_string.clone()
+    }
+}
+
+#[cfg(test)]
+mod require_with_string_tests {
+    use crate::detector::detector::{detector_test_helpers::load_contract, Detector};
+
+    use super::RequireWithStringDetector;
+
+    #[test]
+    fn test_require_with_string() {
+        let context_loader = load_contract(
+            "./tests/contract-playground/out/DeprecatedOZFunctions.sol/DeprecatedOZFunctions.json",
+        );
+        let mut detector = RequireWithStringDetector::default();
+        // assert that the detector finds something
+        let found = detector.detect(&context_loader).unwrap();
+        assert!(found);
+        // assert that the detector returns the correct number of instances
+        assert_eq!(detector.instances().len(), 2);
+        // assert that the detector returns the correct severity
+        assert_eq!(
+            detector.severity(),
+            crate::detector::detector::IssueSeverity::NC
+        );
+        // assert that the detector returns the correct title
+        assert_eq!(
+            detector.title(),
+            String::from("`require()` / `revert()` statements should have descriptive reason strings or custom errors")
+        );
+        // assert that the detector returns the correct description
+        assert_eq!(detector.description(), String::from(""));
+    }
+}