Reentrancy check in account_contract can be easily circumvented

mediumCode4rena

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:

cairo📋
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