Common cairo Warnings and how to fix them

[threepwave]

tl;dr Please check the UNSOLVED SOLUTIONS below and comment with a potential solution. This will help anyone committing code in the future to make sure we have no warnings.

One of our checks on each commit runs through a series of warnings to avoid pitfalls with cairo contracts. A simple example might be unused variables or dependencies and a more complex example might be integer math that could cause an overflow if left unchecked.

In absence of stackoverflow, let's use this thread to share issues and solutions.

Here are the issues I resolved working through the C&C integration pull request:

[RESOLVED] "Unused imports could be removed".

Solution: Remove the dependency (either the whole line or an individual import element) that is not referenced in the file.

[RESOLVED] "Unused arguments might indicate a misspelled variable use or unnecessary argument."

Solution: Remove the function parameter that is not used (in this case, resourceId).

Solutions needed

"Cairo arithmetic is defined over a finite field and has potential for overflows."

Any time we are adding or subtracting via integers, there's a chance that the value exceeds the allowed values of a felt (e.g. decrementing to a negative number or exceeding the max size of a 252-bit uint).

This one seems straightforward but I wasn't able to resolve the errors. My initial idea is to wrap the arithmetic in an if/else statement that does something like: If this addition would go negative, just set it to zero. or If this addition would exceed the size of a felt, just set it to the maximum size of a felt.

Still need a solution here.

"This function returns an error code which must be properly checked."

It looks like we need the equivalent of a try/catch block around some functions. For example, ERC165_supports_interface which eventually calls this line:

with_attr error_message("ERC165: invalid interface id"):
        assert_not_equal(interface_id, INVALID_ID)
    end

"The function get_caller_address returns 0 when the contract is called directly which might be unexpected"

This reads less like a 'fix this warning' and more like a 'you should be aware of this issue.' We could decide to ignore these warnings or we could implement some special casing for when the contract is called directly. For example, we could bar the contract from being called directly (though I think this would have many unintended consequences).

[threepwave]

Attempts to fix: Cairo arithmetic is defined over a finite field and has potential for overflows.

1. Wrap the arithmetic in an with_attr (the closest thing I can find to a try/catch) so that we throw an explicit error.

This still thows a warning, despite being wrapped in a proper error.

2. Add an explicit assert: assert_le

This still throws a warning.

Upon looking at the static analysis library we're using, it says the following:
https://github.com/crytic/amarna/blob/main/amarna/rules/local_rules/ArithmeticOperationsRule.py

    Check arithmetic operations:
        - reports ALL multiplications and divisions
        - reports ONLY addition and subtraction that do not involve a register like [ap - 1]
    """```

It looks like it checks ANY arithmetic, even if you wrap it etc. The only exception is: "that do not involve a register." So I can explore that but it seems like this check is more over-reaching than it should be.

[threepwave]

Based on the above, I think we won't be able to fix some of the warnings thrown. The library we're using does not check for fixes (e.g. did you precede your arithmetic with an assert to make sure it won't over flow.

I plan to go through all the warnings and put in fixes for them. E.g. if it's an arithmetic overflow warnings, i'll wrap it in a with_attr error check and add an assert to make sure it doesn't overflow.

This won't resolve the warning, though, because the library doesn't check for stuff like this. So I'll then manually mark each in Github as fixed.

@ponderingdemocritus let me know what you think about this plan.

[ponderingdemocritus]

Good snooping. Let's make a call one ones that do not need to be fixed then manually mark them so they don't appear again.

I think in future versions of this checker it will improve and probably solve these issues we are facing.