Account contract does not gracefully handle panics in called contracts

[c4-bot-4]

Lines of code

https://github.com/kkrt-labs/kakarot/blob/7411a5520e8a00be6f5243a50c160e66ad285563/src/kakarot/accounts/account_contract.cairo#L347

Vulnerability details

The DualVmToken is a utility EVM contract allowing Kakarot EVM accounts to operate Starknet ERC-20s via classic balanceOf(), transfer() etc. operations.

Calls to the DualVmToken contract are routed via CairoLib -> kakarot_precompiles.cairo -> account_contract.cairo, which finally makes a call to the call_contract() Cairo 0 syscall to invoke the appropriate Starknet ERC-20 contract.

If we look at how the execute_starknet_call() function in account_countract.cairo is implemented:

File: account_contract.cairo
332: @external
333: func execute_starknet_call{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}(
334:     to: felt, function_selector: felt, calldata_len: felt, calldata: felt*
335: ) -> (retdata_len: felt, retdata: felt*, success: felt) {
336:     Ownable.assert_only_owner();
337:     let (kakarot_address) = Ownable.owner();
338:     let is_get_starknet_address = Helpers.is_zero(
339:         GET_STARKNET_ADDRESS_SELECTOR - function_selector
340:     );
341:     let is_kakarot = Helpers.is_zero(kakarot_address - to);
342:     tempvar is_forbidden = is_kakarot * (1 - is_get_starknet_address);
343:     if (is_forbidden != FALSE) {
344:         let (error_len, error) = Errors.kakarotReentrancy();
345:         return (error_len, error, FALSE);
346:     }
347:     let (retdata_len, retdata) = call_contract(to, function_selector, calldata_len, calldata);
348:     return (retdata_len, retdata, TRUE);

we see that the function admits graceful failures (by returning a success: felt value that can be FALSE), which are then properly handled in the calling precompile. However, the actual call to the target contract (at L347) does not return a success boolean, and instead uses the call_contract syscall which panics on failures.

This means that any call that panics in the called contract can cause a revert at RPC level.

To illustrate how this is not a hypothetical scenario, but a very likely one that can be achieved also with the DualVmToken contract that is in scope and consequently planned to be whitelisted to make this call, we make a simple example:

an EVM contract uses DualVmToken to transfer more tokens than what it has in its balance

In this scenario, standard OpenZeppelin ERC-20 implementations panic, bubbling up the error up to the Kakarot RPC level.

This also means that most of the ERC-20 behaviours Kakarot intends to support will lead to reverts at the RPC level if encountered. The following table shows the effect of each behaviour if encountered in a call to a DualVmToken:

Feature	Status
Missing return values	Supported
Upgradeability	-
Pausability	RPC-level reverts
Revert on approval to zero address	RPC-level reverts
Revert on zero value approvals	RPC-level reverts
Revert on zero value transfers	RPC-level reverts
Revert on transfer to the zero address	RPC-level reverts
Revert on large approvals and/or transfers	RPC-level reverts
Doesn't revert on failure	Unsupported, see separate finding
Blocklists	RPC-level reverts

Impact

Any contract can use the above "over-transfer" call to cause RPC-level crashes in the Kakarot EVM, regardless of how defensively they were called. Protective measures generally considered safe in the EVM space like ExcessivelySafeCall (which is used in LayerZero cross-chain applications to prevent channel DoSes that can permanently freeze in-flight tokens), can be bypassed by causing a RPC-level cairo revert.

More generally, any transaction containing a call to a reverting DualVmToken at any depth will revert. This means contracts whose logic requires such a call to succeed will be temporarily (if the call eventually succeeds) or permanently bricked.

Proof of Concept

The RPC-level panic can be proved by simply editing the test_should_transfer in test_dual_vm_token.py end-to-end test to transfer balance_owner_before + amount instead of amount at L110.

Recommended Mitigation Steps

The new AccountContract Cairo 1 implementation uses the Cairo 1 call_contract syscall, which offers a recoverable error interface through a nopanic signature returning a Result. If it is not possible to deploy the new version, the Cairo 1 syscall can be used from the Cairo 0 account_contract via a library call as is done with other functionality.

Assessed type

Other

[ClementWalter]

Severity: Informative

Comment: This is a know limitation of starknet. It is not a problem as relayer can decide to not relay the tx.

[c4-judge]

dmvt changed the severity to 2 (Med Risk)

[dmvt]

I find it hard to look at this as anything other than an impact to expected functionality / availability of functionality, making the severity medium. As shown above, this is likely to be encountered often with very standard functionality, much of which is included in OZ EVM implementations of common token patterns. The potential for novel uses of this dynamic to cause hypothetical exploits or denial of service attacks doesn't seem like something that should be ignored or written off to a "limitation of starknet".

[c4-judge]

dmvt marked the issue as selected for report

[obatirou]

To summarize

1. there is no way to handle a failed syscall in starknet currently. The whole tx panics and stops.
2. DualVM token are starknet token under the hood
3. As such, patterns such as the excessively safe call mentioned by @0xEVom in DualVmToken can be abused to cause RPC-level reverts by revoking native token approval to Kakarot #48 are actually not working as expected
4. A possible mitigation for the DualVMToken especially is to reimplement all the validation logic in solidity and perform the cairo calls only if it will surely succeed.

let's keep it medium

[Ahmed-Aghadi]

@obatirou What are your views regarding #20 , #2 , #19 , #18 ? They also cause RPC-level cairo revert. Shouldn't they also be at least medium? They can even be mitigated.