First of all, my bad, I should have created an issue first. Nonetheless, feel free to close this PR.

TODO:

• fix readme links to work with 0xKitsune repo (only a few)

Introduces --match-file-name command which allows users to generate a report for a specific contract within path.

Usage

solstat --match-file-name "Contract.sol"

reproduce error:

1. forge init a blank repo

2. cd repo ; solstat -p src ... should work

3. create random folder in src ; solstat -p src ... should not work

Command:

solstat --path ./src

Error:

thread 'main' panicked at 'Unable to read overview.md: Os { code: 2, kind: NotFound, message: "No such file or directory" }', /home/dev/.cargo/registry/src/github.com-1ecc6299db9ec823/solstat-0.1.0/src/report/optimization_report.rs:16:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Closes #62

Hi! This is my first contribution to Open Source ever 😊

Here's the basic gist of the vulnerability implementation:

Iterate over all function definitions:

• Skip constructors and internal or private functions (as they are not vulnerable)
• Analyze the body of the remaining functions:
  • check if they contain any selfdestruct calls
  • if they do, check if the function is protected (with modifiers or conditions)

For the last step, I am checking the following criteria to decide if a function is protected or not:

• If it contains a modifier that contains "only" in its name, the function is considered protected
• If its body contains any function calls that take msg.sender as argument, the function is considered protected
• If its body contains any function calls that evaluate an equality or inequality expression on msg.sender, the function is considered protected.

This check is not exhaustive. For instance, it doesn't check if the modifier applied is implemented correctly. I made sure to specify this in a comment at line 54 of the vulnerability file.

I had to implement a few helper functions inside the vulnerability file, because indentation was getting too crazy and I believe it is more readable like this. Please do let me know if you wish to remove them and keep everything in the top-level vulnerability function instead.

At lines 173-181 and 187-194 in the vulnerability file, there is some repeated code. I am aware of this fact, and were considering adding another helper function to alleviate it, but I decided not to in order to keep the helpers more specialized and clear in their functionality.

Please let me know if I missed anything!

Currently, the unsafe ERC20 operation report section is generated with an extra indent from the rest of the report so that it looks like this:

# Gas Optimizations - (Total Vulnerabilities 2)

The following sections detail the high, medium and low severity vulnerabilities found throughout the codebase.

<br>

## Low Risk

        ERC20 operations can be unsafe due to different implementations and vulnerabilities in the standard. To account for this, either use OpenZeppelin's SafeERC20 library or wrap each operation in a require statement.
        Additionally, ERC20's approve functions have a known race-condition vulnerability. To account for this, use OpenZeppelin's SafeERC20 library's `safeIncrease` or `safeDecrease` Allowance functions.
        
        #### Unsafe Transfer
        ```js
        IERC20(token).transfer(msg.sender, amount);
        ```
        #### OpenZeppelin SafeTransfer
        ```js        
        import {SafeERC20} from "openzeppelin/token/utils/SafeERC20.sol";
        //--snip--
        
        IERC20(token).safeTransfer(msg.sender, address(this), amount);
        ```

        #### Safe Transfer with require statement.
        ```js
        bool success = IERC20(token).transfer(msg.sender, amount);
        require(success, "ERC20 transfer failed");
        ```

        #### Unsafe TransferFrom
        ```js
        IERC20(token).transferFrom(msg.sender, address(this), amount);
        ```
        #### OpenZeppelin SafeTransferFrom
        ```js        
        import {SafeERC20} from "openzeppelin/token/utils/SafeERC20.sol";
        //--snip--
        
        IERC20(token).safeTransferFrom(msg.sender, address(this), amount);
        ```

        #### Safe TransferFrom with require statement.
        ```js
        bool success = IERC20(token).transferFrom(msg.sender, address(this), amount);
        require(success, "ERC20 transfer failed");
        ```
    
    
### Lines
- Contract.sol:438
- Contract.sol:207

When it seems it should look more like this:

# Gas Optimizations - (Total Vulnerabilities 2)

The following sections detail the high, medium and low severity vulnerabilities found throughout the codebase.

<br>

## Low Risk

ERC20 operations can be unsafe due to different implementations and vulnerabilities in the standard. To account for this, either use OpenZeppelin's SafeERC20 library or wrap each operation in a require statement.
Additionally, ERC20's approve functions have a known race-condition vulnerability. To account for this, use OpenZeppelin's SafeERC20 library's `safeIncrease` or `safeDecrease` Allowance functions.

#### Unsafe Transfer
```js
IERC20(token).transfer(msg.sender, amount);
```
#### OpenZeppelin SafeTransfer
```js        
import {SafeERC20} from "openzeppelin/token/utils/SafeERC20.sol";
//--snip--

IERC20(token).safeTransfer(msg.sender, address(this), amount);
```

#### Safe Transfer with require statement.
```js
bool success = IERC20(token).transfer(msg.sender, amount);
require(success, "ERC20 transfer failed");
```

#### Unsafe TransferFrom
```js
IERC20(token).transferFrom(msg.sender, address(this), amount);
```
#### OpenZeppelin SafeTransferFrom
```js        
import {SafeERC20} from "openzeppelin/token/utils/SafeERC20.sol";
//--snip--

IERC20(token).safeTransferFrom(msg.sender, address(this), amount);
```

#### Safe TransferFrom with require statement.
```js
bool success = IERC20(token).transferFrom(msg.sender, address(this), amount);
require(success, "ERC20 transfer failed");
```
    
    
### Lines
- Contract.sol:438
- Contract.sol:207

Was this intentional?

this line panics https://github.com/0xKitsune/solstat/blob/main/src/analyzer/optimizations/immutable_variables.rs#L218

git clone https://github.com/Rari-Capital/vaults
cd vaults
solstate --path ./src

Solstat has been bumped to version 0.5.0 in the cargo.toml. Shouldn't the cargo.lock get updated as well?

Not 100% sure on this one, so please correct me if I'm wrong.

The get_type_size seems to return invalid sizes, the comment states that it returns the size in bytes, but it returns the size in bits.

• it returns 256 bits for address and payable which both are 160 bits only
• the bytesx returns x*4 but should be x*8 to have the proper size, currently it believes that two bytes24 could be packed in the same storage slot.
• a bool is stored with a whole byte, so should return 8 bits instead. To go down to only a single bit needs some bitpacking.

It would be helpful to see what the AST representation of a contract would look like when creating a new optimization/vuln/qa file. A contributor could use this to print out the AST and know what nodes to target when designing their pattern.

This is very straightforward to do this. Just create an examples directory and then create a parse_contract.rs file in the examples.

Then you can use something like the code below to convert a contract string to an AST and print the result.

 let file_contents = r#"
    
    contract Contract0 {

    }
    "#;

    let source_unit = solang_parser::parse(file_contents, 0).unwrap().0;


println!("{:?}", source_unit);

Currently, a non-value type variable is marked as an immutable candidate, but immutability is not available for such variables and they must be defined as constants instead.

https://docs.soliditylang.org/en/v0.8.13/contracts.html?highlight=immutable#constant-and-immutable-state-variables

Related issue: #42

In the test case file, it mentions that public and externals names must not be prefixed with an underscore. Maybe this should be addressed in an separated analyzer?

In this PR both cases are taken account.

solstat thread 'main' panicked at 'called Result::unwrap() on an Err value: [Diagnostic { loc: File(0, 342, 343), level: Error, ty: ParserError, message: "unrecognised token ',', expected ")", ";", "="", notes: [] }]', src\analyzer\vulnerabilities\mod.rs:102:73 note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

Contracts that authorize users using the tx.origin variable are typically vulnerable to phishing attacks which can trick users into performing authenticated actions.

Right now, the unsafe ERC20 operations vulnerability pattern is matching on MemberAccess nodes in the AST where the function identifier is transfer, transferFrom and approve. This needs to be updated to ignore instances where the MemberAccess is on an address type.