Address aliasing is wrongfully applied even to EOAs

[howlbot-integration]

Lines of code

https://github.com/kkrt-labs/kakarot/blob/b8f5f2a20bd733cc8885c010291885e1df7dc50e/solidity_contracts/src/L1L2Messaging/L1KakarotMessaging.sol#L8

Vulnerability details

Proof of Concept

Kakarot inherits the address aliasing logic from Optimism as hinted in AddressAliasHelper.sol:

// from https://github.com/ethereum-optimism/optimism/blob/a080bd23666513269ff241f1b7bc3bce74b6ad15/packages/contracts-bedrock/src/vendor/AddressAliasHelper.sol

pragma solidity ^0.8.0;

library AddressAliasHelper {
    ..snip
}

Now this is a key property of the Optimism bridge, which is that all contract addresses are aliased. And this is done inorder to avoid a contract on L1 to be able to send messages as the same address on L2, because often these contracts will have different owners.

To go into more details about why & how this is used, we can see this: https://docs.optimism.io/chain/differences#address-aliasing:

Address aliasing is an important security feature that impacts the behavior of transactions sent from L1 to L2 by smart contracts. Make sure to read this section carefully if you are working with cross-chain transactions. Note that the CrossChainMessenger contracts will handle address aliasing internally on your behalf.
When transactions are sent from L1 to L2 by an Externally Owned Account, the address of the sender of the transaction on L2 will be set to the address of the sender of the transaction on L1. However, the address of the sender of a transaction on L2 will be different if the transaction was triggered by a smart contract on L1.
Because of the behavior of the CREATE opcode, it is possible to create a contract on both L1 and on L2 that share the same address but have different bytecode. Even though these contracts share the same address, they are fundamentally two different smart contracts and cannot be treated as the same contract. As a result, the sender of a transaction sent from L1 to L2 by a smart contract cannot be the address of the smart contract on L1 or the smart contract on L1 could act as if it were the smart contract on L2 (because the two contracts share the same address).
To prevent this sort of impersonation, the sender of a transaction is slightly modified when a transaction is sent from L1 to L2 by a smart contract. Instead of appearing to be sent from the actual L1 contract address, the L2 transaction appears to be sent from an "aliased" version of the L1 contract address. This aliased address is a constant offset from the actual L1 contract address such that the aliased address will never conflict with any other address on L2 and the original L1 address can easily be recovered from the aliased address.
This change in sender address is only applied to L2 transactions sent by L1 smart contracts. In all other cases, the transaction sender address is set according to the same rules used by Ethereum.
Transaction Source Sender Address
L2 user (Externally Owned Account) The user's address (same as in Ethereum)
L1 user (Externally Owned Account) The user's address (same as in Ethereum)
L1 contract (using OptimismPortal.depositTransaction) L1_contract_address + 0x1111000000000000000000000000000000001111

That's to say we expect this aliasing logic to:

• Only be applied to contracts.

Issue however is that Kakarot applies this aliasing logic not only on contracts but even EOAs, see https://github.com/kkrt-labs/kakarot/blob/b8f5f2a20bd733cc8885c010291885e1df7dc50e/solidity_contracts/src/L1L2Messaging/L1KakarotMessaging.sol#L8:

    function sendMessageToL2(address to, uint248 value, bytes calldata data) external payable {
        uint256 totalLength = data.length + 4;
        uint256[] memory convertedData = new uint256[](totalLength);
        convertedData[0] = uint256(uint160(AddressAliasHelper.applyL1ToL2Alias(msg.sender)));
        convertedData[1] = uint256(uint160(to));
        convertedData[2] = uint256(value);
        convertedData[3] = data.length;
        for (uint256 i = 4; i < totalLength; ++i) {
            convertedData[i] = uint256(uint8(data[i - 4]));
        }

        // Send the converted data to L2
        starknetMessaging.sendMessageToL2{value: msg.value}(kakarotAddress, HANDLE_L1_MESSAGE_SELECTOR, convertedData);
    }

Evidently, the classic contract check of msg.sender != tx.origin is missing which means even when the caller is an EOA it's still get aliased.

Impact

Broken functionality for aliasing senders considering even EOAs get aliased instead of it just being restricted to smart contracts, i.e if developers implement access control checks on the sender of the transactions, say a deposit tx for e., they would have to believe that the address of the contract from the L1 is not aliased when it is an EOA however in Kakarot's case, it is. Therefore would result in unintentional bugs where the access control will be implemented incorrectly and these transactions will always fail.

Recommended Mitigation Steps

Apply an if msg.sender != tx.origin check instead and in the case where this ends up being true then alias the address otherwise let it be.

Assessed type

Context

[ClementWalter]

Severity: Low

Comment: There is no security risk of any kind. But this will be fixed.
dup 25n closed by bot

[c4-judge]

dmvt marked the issue as selected for report

[dmvt]

I think this fits as a medium given that functionality is unexpectedly impaired / blocked.

[obatirou]

Following https://docs.code4rena.com/awarding/judging-criteria/severity-categorization

Low risk (e.g. assets are not at risk: state handling, function incorrect as to spec, issues with comments)

the function of the protocol or its availability could be impacted

This is related to "incorrect as to spec" but not "impacted function" to us

[dmvt]

Noted, but I still thing medium is appropriate here.