Native account abstraction should allow contract accounts to be the tx.origin

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L1322-L1328
https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/ContractDeployer.sol#L40-L52
https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/scripts/process.ts#L90

Vulnerability details

Impact

ZkSync has native support for account abstraction. However, it only ever sets EOAs as the tx.origin for its transactions. Since contract accounts can initiate transactions, it only makes sense that tx.origin would return the address of the contract account and not the bootloader's. Returning the bootloader as the tx.origin for transactions initiated by contract accounts breaks expectations. Valid uses for tx.origin would no longer be possible.

For example, a contract account has a sensitive callback function that can be triggered by other contracts. If we want this callback to only be triggered by transactions initiated by the contract account, this can be done with the following code:

function callback() external {
  if (tx.origin != address(this)) revert();
  // ...snip
}

If tx.origin for contract accounts will always be the bootloader, the above logic can not be implemented for contract accounts in ZkSync.

Proof of Concept

Initiating any L2 transaction using a contract account will set the tx.origin for that transaction as the bootloader.

Tools Used

Manual Review

Recommended Mitigation Steps

Instead of setting the tx.origin as the from address only for EOAs, set the tx.origin as the from address for any account, including contract accounts. Change the following code in bootloader to:

-switch isEOA(from)
+switch isAccount(from)
case true {
    setTxOrigin(from)
}  
default {
    setTxOrigin(BOOTLOADER_FORMAL_ADDR())
}

Add the following isAccount() function to bootloader:

function isAccount(addr) -> ret {
    mstore(0, {{RIGHT_PADDED_GET_ACCOUNT_VERSION_SELECTOR}})
    mstore(4, addr)

    let success := call(
        gas(),
        CONTRACT_DEPLOYER_ADDR(),
        0,
        0,
        36,
        0,
        32
    )

    let supportedVersion := mload(0)

    if iszero(success) {
        nearCallPanic()
    }
    
    ret := gt(supportedVersion, 0)
}

RIGHT_PADDED_GET_ACCOUNT_VERSION_SELECTOR is the selector for ContractDeployer.extendedAccountVersion(). That function will return a value higher than 0 if the address is an EOA or a contract account (a contract that supports account abstraction).

Assessed type

Other

[c4-pre-sort]

bytes032 marked the issue as low quality report

[miladpiri]

It is done for security considerations towards already deployed contracts.

[c4-sponsor]

miladpiri (sponsor) disputed

[GalloDaSballo]

I cannot find any mention of tx.origin changes that haven't been mostly contested:

Majority disagree with the change:
ethereum/EIPs#637

No mentions:
https://eips.ethereum.org/EIPS/eip-4337
https://github.com/ethereum/EIPs/pull/208/files

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory:
Invalid