Reentrancy check in account_contract can be easily circumvented

[c4-bot-10]

Lines of code

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

Vulnerability details

The account_contract has a reentrancy check in execute_starknet_call to prevent calls made from the Kakarot EVM to Starknet to re-enter the Kakarot EVM.

The check is implemented by checking that execute_starknet_call calls Kakarot's address only with the get_starknet_address getter, which is indeed harmless:

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);
349: }
350: 

However, this check leaves another possibility open, that is that the account_contract could call itself or another account_contract with a signed transaction to re-enter the Kakarot EVM, which is extremely vulnerable to reentrancy because of its extensive use of cached data.

While an exploit via this path can have critical impact on the integrity of the EVM, it's likelihood is extremely low because execute_starknet_call is accessible only via whitelisted contracts.

Proof of Concept

Because Kakarot stores the effects of the execution at the very end of it via the Starknet.commit, it openly does not follow the check-effect-interaction pattern, so the integrity of the EVM is at risk of reentrancy via submissions of signed transactions within the call cairo precompile.

Recommended Mitigation Steps

Consider removing the reentrancy check in account_contract, and adding a reentrancy guard on the Kakarot.eth_call function.

Assessed type

Other

[ClementWalter]

Severity: Medium

Comment: Re-entrancy is actually possible should the attacker be in possession of the signed tx before this tx is actually executed, and can front run it to store it is starknet before it’s processed. It’s possible, though seems very unlikely. We’ll fix the re-entrancy check to put in in the Kakarot contract level.

[obatirou]

On second thoughts because of the whitelisting it is impossible to exploit.
The only way to access the execute_starknet_call is through the cairo_precompile but this precompile is whitelisted.
At the specified commit for C4, only the DualVMToken contract is whitelisted which cannot lead to the exploit.

We will still do the mitigation with a re-entrancy check at Kakarot contract level.

[dmvt]

I'm not willing to jump to "whitelisting makes this impossible to exploit" and still think medium applies. Reentrancy is no joke and a hand wavy hypothetical is enough in this case.

[c4-judge]

dmvt marked the issue as selected for report

[Ahmed-Aghadi]

I'm not willing to jump to "whitelisting makes this impossible to exploit" and still think medium applies. Reentrancy is no joke and a hand wavy hypothetical is enough in this case.

As attack is only possible through "whitelisting", doesn't it comes under https://docs.code4rena.com/awarding/judging-criteria/severity-categorization#centralization-risks ?

[dmvt]

Not in this case. The potential for damage is enough for me to keep it in place as a medium.