DualVmToken can be abused to cause RPC-level reverts by revoking native token approval to Kakarot

[c4-bot-6]

Lines of code

https://github.com/kkrt-labs/kakarot/blob/7411a5520e8a00be6f5243a50c160e66ad285563/solidity_contracts/src/CairoPrecompiles/DualVmToken.sol#L210

Vulnerability details

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

These classic operations are offered in two flavors, one that accepts Starknet addresses (uint256) and one that accepts EVM addresses (address). In case an EVM address is provided, the corresponding Starknet address is looked up through Kakarot first.

If we look at the approve(uint256, ...) function in DualVmToken:

File: DualVmToken.sol
206:     /// @dev Approve a starknet address for a specific amount
207:     /// @param spender The starknet address to approve
208:     /// @param amount The amount of tokens to approve
209:     /// @return True if the approval was successful
210:     function approve(uint256 spender, uint256 amount) external returns (bool) {
211:         _approve(spender, amount);
212:         emit Approval(msg.sender, spender, amount);
213:         return true;
214:     }
215: 
216:     function _approve(uint256 spender, uint256 amount) private {
217:         if (spender >= STARKNET_FIELD_PRIME) {
218:             revert InvalidStarknetAddress();
219:         }
220:         // Split amount in [low, high]
221:         uint128 amountLow = uint128(amount);
222:         uint128 amountHigh = uint128(amount >> 128);
223:         uint256[] memory approveCallData = new uint256[](3);
224:         approveCallData[0] = spender;
225:         approveCallData[1] = uint256(amountLow);
226:         approveCallData[2] = uint256(amountHigh);
227: 
228:         starknetToken.delegatecallCairo("approve", approveCallData);
229:     }

we can see that no check whatsoever is done on the spender input, except for felt overflow at L217.

This means that the provided address could be any Starknet account, including a contract that is not an EVM account, and most importantly including the Kakarot contract.

This is particularly relevant, because native token transfers in the Kakarot EVM work under the assumption that Starknet account contracts have infinite approval granted to the Kakarot contract, as we can see from the account_contract initialization:

File: library.cairo
083:     func initialize{
084:         syscall_ptr: felt*,
085:         pedersen_ptr: HashBuiltin*,
086:         range_check_ptr,
087:         bitwise_ptr: BitwiseBuiltin*,
088:     }(evm_address: felt) {
---
101:         IERC20.approve(native_token_address, kakarot_address, infinite);

By removing this approval and attempting to move native tokens, an EVM contract account can jeopardize EVM state finalization (that is where native token transfers are settled on the Starknet ERC20) and 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.

Proof of Concept

The following contract can trigger an RPC-level revert whenever called with any calldata, assuming that target is initialized with the DualVmToken associated to Kakarot's native ERC20.

contract DualVmTokenHack {
    DualVmToken target;
    
    constructor(DualVmToken _target) {
        target = _target;
    }

    fallback {
        uint256 kakarot = DualVmToken.kakarot();
        target.approve(kakarot, 0);
        address(0xdeadbeef).transfer(1 wei);
    }
}

Recommended Mitigation Steps

Consider adding the following check to the DualVmToken.approve function:

    require(spender != kakarot);

Assessed type

Other

[ClementWalter]

Severity: Low

Comment: This is a low issue as there is no clear external attack path. A user would need to decide to disabling its own account by making an approval to kakarot with 0.

[c4-judge]

dmvt changed the severity to 2 (Med Risk)

[dmvt]

Availability and expected functionality are impacted but there are no funds at risk as far as I can see. This can be used for hypothetical griefing attacks. Warning to the warden: this was clearly overinflated and I almost invalidated it completely as a result.

[c4-judge]

dmvt marked the issue as selected for report

[Ahmed-Aghadi]

Availability and expected functionality are impacted but there are no funds at risk as far as I can see. This can be used for hypothetical griefing attacks. Warning to the warden: this was clearly overinflated and I almost invalidated it completely as a result.

How come this comes under hypothetical griefing attack whereas my submission which can also lead to griefing attack aren't considered medium? My submissions having griefing attack: #20 #2 #19 #18

[0xEVom]

@dmvt just to explain our High severity assessment for this finding as well as #40, #49, #52 and #53, which share the root cause of RPC-level reverts that is very specific to this codebase.

The scenario we had in mind (which we could admittedly have made a better job of explaining) was that in which a contract requires a call to be made to reach a subsequent state. For example: a bridge contract that requires messages to be processed sequentially. It is ok for the call not to succeed, but it must be attempted.

In this situation, the call reverting at the RPC level leads to the transaction never succeeding, and hence all funds in the contract being locked irreversibly.

This pattern is quite widely used, such as in LayerZero's _blockingLzReceive, and is not safe on Kakarot, with the highest impact being the permanent freezing of funds.

[obatirou]

see #49

[dmvt]

Ruling stands. You're relying on handwavy hypotheticals and external factors. Medium is correct.