crytic/amarna

Amarna is a static-analyzer and linter for the Cairo programming language.

amarna's Issues

Parser fails when file does not end with newline

The cairo grammar does not support files without a newline.
The compiler adds a newline, and we should do the same

https://github.com/starkware-libs/cairo-lang/blob/167b28bcd940fd25ea3816204fa882a0b0a49603/src/starkware/cairo/lang/compiler/parser.py#L241-L250

Warn on public functions in files that are being imported from

Consider a Cairo contract made out of two files, main.cairo and library.cairo (shortened for clarity):

# main.cairo

from library import mint_internal, assert_owner

@external
func mint(to : felt, amount : felt):
    assert_owner()
    mint_internal(to, amount)
    return ()
end

# library.cairo

func assert_owner():
    let (caller) = get_caller_address()
    let (owner) = owner_storage.read()
    assert caller = owner
    return ()
end

func mint_internal(to : felt, amount : felt):
    let (balance) = balance_of.read(to)
    balance_of.write(to, balance + amount)
    return ()
end

@external
func test_mint(to, amount):
    mint_internal(to, amount)
    return ()
end

Notice that I only import assert_owner and mint_internal in the main contract. However, when compiled and deployed, the unsecured test_mint function (originally only intended to expose the mint_internal for testing) will also be brough into scope and made available in the deployed contract. This is bad. That's why OpenZeppelin came up with the extensibility pattern.

Can Amarna also warn in these cases, when a module (an "import from" file) contains public functions?

Handle function import aliasing to detect function usage

If functions are imported with an alias from file import function as another_name, they are not handled correctly in the unused functions rule.

Access controls during cross-chain calls

Add a detector to check if a function marked with @l1_handler calls get_caller_address (either directly or indirectly). It will return 0x0 and can cause issues in some cases.

Noisy fail when amarna runs on non-cairo file

Running amarna on a text file will cause a python fail.

Warn on storage_var collision

Cairo lets you write a contract with @storage_var collisions. Here's an example courtesy of @andrew-fleming. Obviously, this can lead to an unintended behaviour or an exploit.

It would be great if Amarna could detect a collision like this and raise a warning.

From my experimentation, the compiler does detect this but only if the func declaration is slightly different. For example, this will raise an error when compiling:

# a.cairo
@storage_var
func balance() -> (res : felt):
end

# b.cairo
# notice the different name for the returned value
@storage_var
func balance() -> (value : felt):
end

This will compile without a problem:

# a.cairo
@storage_var
func balance() -> (res : felt):
end

# b.cairo
@storage_var
func balance() -> (res : felt):
end

The compiler also doesn't raise a warning if the --no_debug_info flag is passed.

Don't warn about overflows on const values

If possible, it would be great to avoid false positives when detecting potential overflows like this one on the screenshot

It's clear that 8000 * 5 is a constant value that fits inside a felt, there's no overflow hazard.

Here's a gif of a dachshund kissing a lion for no particular reason

Warn when emitting an event from a view function

I'd like to suggest a new rule:

There should be a warning if an event is being emitted from a @view function.

Granted, the rule is somewhat subjective, but I don't see a point in emitting from a @view. WDYT?

Solve for recursive imports

Multiple detectors built with ImportGatherer does not solve correctly for recursive imports, as pointed out #16 (comment)

• Solution can mimic compiler behaviour, ref

Add a rule to exclude functions under a namespace

Add a rule here

amarna/amarna/rules/post_process_rules/UnusedFunctionsRule.py

Line 30 in 2bb4869

def run_rule(self, gathered_data: Dict) -> List[Result]:

to exclude, function under namespace in libraries, from being flagged with “unused function” warning.

Improve Unused arguments rule

Currently, the rule finds unused function arguments. However, Cairo programs commonly use the pattern:

struct BatchConfig:
    member general_config : GeneralConfig*
    member signed_min_oracle_prices : OraclePrice*
    member signed_max_oracle_prices : OraclePrice*
    member n_oracle_prices : felt
    member min_expiration_timestamp : felt
end

func batch_config_new(
        general_config : GeneralConfig*, signed_min_oracle_prices : OraclePrice*,
        signed_max_oracle_prices : OraclePrice*, n_oracle_prices, min_expiration_timestamp) -> (
        batch_config : BatchConfig*):
    let (fp_val, pc_val) = get_fp_and_pc()
    return (batch_config=cast(fp_val - 2 - BatchConfig.SIZE, BatchConfig*))
end

https://github.com/starkware-libs/stark-perpetual/blob/9327c640e59bd15764833505560478d33aa6a8b1/src/services/perpetual/cairo/transactions/batch_config.cairo#L1-L19

We need to

• check the usage of cast(fp_val - 2 - STRUCT.SIZE, STRUCT*)
• check that all struct members and function parameters match in number and type

Cairo 0.9.1 support

Cairo 0.9.1 introduced support for and in if statements (if x == y and z == w). And running Amarna on a contract using such statement will fail:

Could not parse contract.cairo: Unexpected token Token('IDENTIFIER', 'and') at line 1158, column 31.
Expected one of: 
        * LBRACE
        * DOT
        * SLASH
        * LPAR
        * LSQB
        * MINUS
        * COLON
        * STAR
        * PLUS
        * _DBL_STAR

Support error codes and specifying error levels

We would love to use this tool in our CI. It would be helpful if the tool supported non zero exit codes and some kind of whitelisting feature. Are you considering this for this tool?

Missing pydot dependency

I cloned the repo and pip install -e . the package. When I tried to use the -png cmdline arg, I got the following error:

amarna contracts/settling_game/S06_Combat.cairo -png
Traceback (most recent call last):
  File "/Users/m/.pyenv/versions/starkware/bin/amarna", line 33, in <module>
    sys.exit(load_entry_point('amarna', 'console_scripts', 'amarna')())
  File "/Users/m/projects/amarna/amarna/command_line.py", line 61, in main
    results = analyze_file(filename, png=args.png)
  File "/Users/m/projects/amarna/amarna/amarna.py", line 140, in analyze_file
    return amarna.run_local_rules(fname, parsed_cairo_file, png)
  File "/Users/m/projects/amarna/amarna/amarna.py", line 73, in run_local_rules
    make_png(parsed_cairo_file, png_filename)
  File "/Users/m/projects/amarna/amarna/amarna.py", line 15, in make_png
    tree.pydot__tree_to_png(t, out_name)
  File "/Users/m/.pyenv/versions/3.9.6/envs/starkware/lib/python3.9/site-packages/lark/tree.py", line 184, in pydot__tree_to_png
    graph = pydot__tree_to_graph(tree, rankdir, **kwargs)
  File "/Users/m/.pyenv/versions/3.9.6/envs/starkware/lib/python3.9/site-packages/lark/tree.py", line 204, in pydot__tree_to_graph
    import pydot
ModuleNotFoundError: No module named 'pydot'

pip install pydot solved it

Fails to parse `using` expressions

using Range = (min : felt, max : felt)

Fails to parse as:

Could not parse foo.cairo: Unexpected token Token('COLON', ':') at line 93, column 20.
Expected one of:
        * _DBL_STAR
        * STAR
        * RPAR
        * COMMA

Don't flag constructor as never called

I've enabled Amarna on one of my Cairo repos. It flagged an issue that @constructor is never called (even though the repo is public I don't think I can link directly to the static analysis issue so here's a screenshot):

I would say this rule should be disabled for @constructors.

False positives in @contract_interface

Consider this interface declaration (a simplified version of this one):

@contract_interface
namespace IModuleController {
    func set_write_access(module_id_doing_writing: felt, module_id_being_written_to: felt) {
    }
}

Amarna produces 2 false positives - unused argument (for both arguments) and function never called.

Of course, neither make sense in a @contract_interface declaration.

Function flagged as implicit import when explicitly imported

Running Amarna incorrectly flags auth_read_storage as implicit when it is explicit.

~proxy.cairo

%lang starknet

from utils import auth_read_storage

~utils.cairo

%lang starknet

from starkware.starknet.common.syscalls import storage_read, storage_write, get_caller_address

# Helpers for auth users to interact with contract's storage 
@view
func auth_read_storage{
        syscall_ptr : felt*,
    }(auth_account : felt, address : felt) -> (value : felt):
    let (caller) = get_caller_address()

    assert caller = auth_account

    let (value) = storage_read(address=address)

    return (value=value)
end

@external
func auth_write_storage{
        syscall_ptr : felt*,
    }(auth_account : felt, address : felt, value : felt):
    let (caller) = get_caller_address()

    assert caller = auth_account

    storage_write(address=address, value=value)
    return()
end

$ amarna -s .
[implicit-import] [This](0) function will be imported by [here](1), even though it was not explicitly imported. in utils.cairo:19:1 and proxy.cairo:3:19
[implicit-import] [This](0) function will be imported by [here](1), even though it was not explicitly imported. in utils.cairo:6:1 and proxy.cairo:3:19
[must-check-caller-address] in utils.cairo:10:10
[must-check-caller-address] in utils.cairo:23:10
[unused-imports] in proxy.cairo:3:19

amarna crashes on starkware.cairo.common.patricia

Traceback (most recent call last):
  File "/opt/homebrew/lib/python3.9/site-packages/lark/parsers/lalr_parser.py", line 126, in feed_token
    action, arg = states[state][token.type]
KeyError: 'IDENTIFIER'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/opt/homebrew/bin/amarna", line 8, in <module>
    sys.exit(main())
  File "/opt/homebrew/lib/python3.9/site-packages/amarna/command_line.py", line 154, in main
    results = analyze_directory(filename, rule_set_names)
  File "/opt/homebrew/lib/python3.9/site-packages/amarna/amarna.py", line 149, in analyze_directory
    parsed_cairo_file = amarna.parse_cairo_file(fname)
  File "/opt/homebrew/lib/python3.9/site-packages/amarna/amarna.py", line 130, in parse_cairo_file
    return self.parser.parse(f.read() + "\n", start="cairo_file")
  File "/opt/homebrew/lib/python3.9/site-packages/lark/lark.py", line 625, in parse
    return self.parser.parse(text, start=start, on_error=on_error)
  File "/opt/homebrew/lib/python3.9/site-packages/lark/parser_frontends.py", line 96, in parse
    return self.parser.parse(stream, chosen_start, **kw)
  File "/opt/homebrew/lib/python3.9/site-packages/lark/parsers/lalr_parser.py", line 41, in parse
    return self.parser.parse(lexer, start)
  File "/opt/homebrew/lib/python3.9/site-packages/lark/parsers/lalr_parser.py", line 171, in parse
    return self.parse_from_state(parser_state)
  File "/opt/homebrew/lib/python3.9/site-packages/lark/parsers/lalr_parser.py", line 188, in parse_from_state
    raise e
  File "/opt/homebrew/lib/python3.9/site-packages/lark/parsers/lalr_parser.py", line 179, in parse_from_state
    state.feed_token(token)
  File "/opt/homebrew/lib/python3.9/site-packages/lark/parsers/lalr_parser.py", line 129, in feed_token
    raise UnexpectedToken(token, expected, state=self, interactive_parser=None)
lark.exceptions.UnexpectedToken: Unexpected token Token('IDENTIFIER', 'PatriciaUpdateConstants') at line 476, column 43.
Expected one of: 
	* LBRACE
	* MINUS
	* _DBL_STAR
	* COMMENT
	* STAR
	* SLASH
	* _NEWLINE
	* LSQB
	* RPAR
	* LPAR
	* PLUS
	* DOT
	* COMMA

I believe it crashes on this file:

./lib/python3.9/site-packages/starkware/cairo/common/patricia.cairo:# Holds the constants needed for Patricia updates.
./lib/python3.9/site-packages/starkware/cairo/common/patricia.cairo:struct PatriciaUpdateConstants:
./lib/python3.9/site-packages/starkware/cairo/common/patricia.cairo:# Performs an efficient update of multiple leaves in a Patricia Merkle tree.
./lib/python3.9/site-packages/starkware/cairo/common/patricia.cairo:# preimage - a dictionary from the hash value of a Patricia node to either
./lib/python3.9/site-packages/starkware/cairo/common/patricia.cairo:    let (patricia_update_constants : PatriciaUpdateConstants*) = patricia_update_constants_new()
./lib/python3.9/site-packages/starkware/cairo/common/patricia.cairo:func patricia_update_constants_new() -> (patricia_update_constants : PatriciaUpdateConstants*):
./lib/python3.9/site-packages/starkware/cairo/common/patricia.cairo:    return (patricia_update_constants=new PatriciaUpdateConstants(globals_pow2=globals_pow2))
./lib/python3.9/site-packages/starkware/cairo/common/patricia.cairo:    patricia_update_constants : PatriciaUpdateConstants*,

I'm using cairo-lang 0.8.2.1.

It seems the new PatriciaUpdateConstants(...) expression is not supported.

Allow dismissing alerts without a full page reload

So I'm not sure this is something you can actually do anything about, but if you can, that would be great.

Here's an example of a PR that's full of Amarna alerts. A lot of them are false positives in the context of the PR - the "potential for overflows" is not really a problem when we're multiplying small numbers that are (more or less) under our control.

So during the PR review, I want to go through each alert and dismiss the alert:

However, when I click the Dismiss alert button, a full page reload happens which takes a couple of seconds and I'm then yanked out of the context, because the page doesn't even load back to the file and line I was reviewing (where I dismissed the alert).

Ideally, I would want the alert dismissal to happen "inline," silently, without a full page refresh, so I can continue with the review. Is that something you can control or is it a GitHub thing?

zsh: command not found: amarna

I installed amarna globally on MacOS, and pip freeze shows that the package has been installed successfully, yet CLI shows zsh: command not found: amarna when using the command.

Warn when an import shadows a locally defined function

We would flag the following foo import

# library.cairo

func foo() -> ():
end

and

# main.cairo
from library import foo as bar

func bar() -> ():
end

func main() -> ():
    bar()
end

This should also work for unaliased import.

We should also check the compiler behavior so we could provide further details when this is found.

Detect potentially revoked references

Warn if there is a label or a call instruction between the definition of a reference that depends on ap and its usage. This is ambiguous as discussed in the docs, since ap might change in an unknown way. We could potentially add a detector and flag it as undefined behavior.

Add a rule to point out unused variables

Amarna already lovingly warns about unused imports, function call arguments or functions themselves. Can you add a rule to warn about unused variables to its arsenal as well?

Example:

func foo{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}() -> (val):
    let (caller) = get_caller_address()
    return (42)
end

In the above example, caller is never used. The rule would point it out.

False positive for dead-store rule

Hey,

just got a false positive on the dead-store rule.

Here's a link to the report in my PR but it'll probably get stale since I'm working on it.

In which case, here's a screenshot of the issue:

Already in there you can see that both strength and spawn_ts are used.

Here's a permalink to the code that triggered the report.

Check return value of [must-check-caller-address]

[must-check-caller-address] finds the calls to get_caller_address and warns the user about it.

Ideally, this rule should NOT emit any warning if the user checks the return value. E.g.:

let (address) = get_caller_address()
if address == 0: # returned values is compared to 0
  return () # Could be any other code
end

The above code should NOT emit a warning, as the return value of get_valler_address is checked against 0.

A second example of code that should not emit a warning would be the usage of assert_not_zero:

let (address) = get_caller_address()
assert_not_zero(address)

Indeed, address will be compared (asserted) to 0.

An example of code that SHOULD emit a warning would be:

let (address) = get_caller_address()
call_contract(contract_address=address, ...)

Indeed, the return value is NOT checked against 0.

Pin packages in requirements.txt

Hello team, thanks a lot for building amarna! Could you pin the packages on the file requirements.txt? There is a currently conflict on cairo-format with the current latest versions of cairo-lang and amarna. To reproduce it, on Python 3.7.2, you can run the following code:

python -m venv .venvtest
source .venvtest/bin/activate
pip install --upgrade pip
pip install cairo-lang

cairo-format # it works

pip install git+https://github.com/crytic/amarna.git@main

cairo-format # it fails

Refactor`result_multiple_positions` to allow more than 2 positions

Currently, result_multiple_positions is only implemented for two positions. As rules evolve, we need to consider results with more than 2 related locations.

Originally posted by @montyly in #16 (comment)

Unused import should not be triggered for `@external`, `@view`

When using a library/preset to import & re-export public methods, i.e. https://github.com/OpenZeppelin/cairo-contracts#use-a-basic-preset the import is unused in the code but it's meant to provide a user facing API.

Switch to trusted publishing

All of our PyPI-packaged projects should use trusted publishing, rather than a manually configured API token.

Example trusted publishing workflow:

https://github.com/trailofbits/blight/blob/master/.github/workflows/release.yml

Resources:

• https://docs.pypi.org/trusted-publishers/
• https://packaging.python.org/en/latest/guides/publishing-package-distribution-releases-using-github-actions-ci-cd-workflows/

Noisy fail when running amarna on directories with no cairo files

It might make more sense to have amarna "fail" silently when run on an empty directory. Currently there is an error trace:

pytest_venv) coolhill@coolhill:~/amarna$ amarna -s /tmp/
Traceback (most recent call last):
  File "/home/coolhill/pytest_venv/bin/amarna", line 33, in <module>
    sys.exit(load_entry_point('amarna', 'console_scripts', 'amarna')())
  File "/home/coolhill/amarna/amarna/command_line.py", line 154, in main
    results = analyze_directory(filename, rule_set_names)
  File "/home/coolhill/amarna/amarna/amarna.py", line 155, in analyze_directory
    all_results += amarna.run_post_process_rules()
  File "/home/coolhill/amarna/amarna/amarna.py", line 120, in run_post_process_rules
    results += Rule.run_rule(self.data)
  File "/home/coolhill/amarna/amarna/rules/post_process_rules/ImportedExternalsRule.py", line 21, in run_rule
    DeclaredFunctionsGatherer.GATHERER_NAME
KeyError: 'DeclaredFunctionsGatherer'

Maybe because post process rules are being run even if the directory is empty, as per

amarna/amarna/amarna.py

Line 155 in d170deb

all_results += amarna.run_post_process_rules()

Add a way how to disable a rule on granular basis

It would be awesome if there was a way how to disable a rule (or multiple at the same time) on a per line, per function and per file basis. Probably something like an inline comment as is common with other tools.

For example, if I'd want to disable arithmetic checks (the arithmetic-expr_add rule), I would put a # amarna-disable: arithmetic-expr_add either at the top of the file (to disable this check in the whole file), as the first comment in the function body (to disable it only in that function) or somewhere inside the function (to disable it on the next line).

Are you thinking about adding something like this?

Flag state modifications within view functions

Since the compiler does not enforce that @view functions does not contain any code that modifies state, we could implement a detector for it. Reference: not-so-smart-cairo

Wrong grammar in the package pushed to pip

The package pushed to pypi still uses the old cairo grammar

Run gatherer and post process rules even when analyzing single files

With the initial set of gatherer and post process rules we had it made no difference running these on a single file. With new rules being added, some will also make sense to run on single files.