amarna icon indicating copy to clipboard operation
amarna copied to clipboard

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

Open pscott opened this issue 2 years ago • 5 comments

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

pscott avatar Jul 26 '22 17:07 pscott

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.

fcasal avatar Jul 27 '22 10:07 fcasal

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

0xLucqs avatar Aug 15 '22 12:08 0xLucqs

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 :)

pscott avatar Aug 15 '22 12:08 pscott

Yes right ! Not for long hopefully :)

0xLucqs avatar Aug 15 '22 12:08 0xLucqs

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;
}

james-miller-93 avatar Nov 29 '22 16:11 james-miller-93