crytic / amarna

Amarna is a static-analyzer and linter for the Cairo programming language.
https://blog.trailofbits.com/2022/04/20/amarna-static-analysis-for-cairo-programs/
GNU Affero General Public License v3.0
149 stars 7 forks source link

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

Open pscott opened 1 year ago

pscott commented 1 year ago

[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.

fcasal commented 1 year ago

Hi @pscott, indeed this is something we would like to improve the current rule on. Currently, there is no direct way of doing control-flow analysis with Amarna, and validating if the returned value is checked in a successor node like an if statement or an assert before being used would not be easy. Thanks for raising the issue, as this will serve to track progress on this end.

LucasLvy commented 1 year ago

since txs have to go through an account contract wouldn't it make sense to disable this rule by default ?

pscott commented 1 year ago

since txs have to go through an account contract wouldn't it make sense to disable this rule by default ?

I don't think transactions have to go through an account contract :) See for example, in a deploy_transaction :)

LucasLvy commented 1 year ago

Yes right ! Not for long hopefully :)

james-miller-93 commented 1 year ago

the same point applies to overflow checking on things like uint256_add, for example:

let (total: Uint256, carry: felt) = uint256_add(a, b);
with_attr error_message("OVERFLOW") {
    assert carry = 0;
}