`DefaultAccount.executeTransactionFromOutside` always reverts

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/DefaultAccount.sol#L132 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/DefaultAccount.sol#L28 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/bootloader/bootloader.yul#L2301

Vulnerability details

Impact

DefaultAccount.executeTransactionFromOutside always reverts; the function can never be used as the fallback to the standard Ethereum transactions handling procedure. Users won't be able to use the function to execute transactions when the operator is not responsive.

Proof of Concept

As per the documentation of the function, DefaultAccount.executeTransactionFromOutside should let users execute L2 transactions by interacting with the contract directly (not via the operator) from L1:

/// @notice Method that should be used to initiate a transaction from this account /// by an external call. This is not mandatory but should be implemented so that0 /// it is always possible to execute transactions from L1 for this account.

Also, from the documentation:

executeTransactionFromOutside, technically, is not mandatory, but it is highly encouraged, since there needs to be some way, in case of priority mode (e.g. if the operator is unresponsive), to be able to start transactions from your account from ``outside'' (basically this is the fallback to the standard Ethereum approach, where an EOA starts transaction from your smart contract).

The functions has the ignoreNonBootloader modifier that only the bootloader to call the function. However, when the function is intended to be called, the bootloader is never the msg.sender.

As per the description of the function, it should be called from the L1 network by an EOA. When the bootloader executes L1 transactions, it sets msg.sender to the from field value of the transaction:

1. it calls msgValueSimulatorMimicCall to execute the transaction and sets the from argument value to the from field value of the transaction;
2. msgValueSimulatorMimicCall then calls the mimicCallOnlyResult function and passes the "from" address as the whoToMimic argument value;
3. mimicCallOnlyResult calls the "mimic call" verbatim and passes the whoToMimic address as the msg.sender value that will be used in the call.

Thus, an L1 transaction is always executed on behalf of the EOA address that created this transaction on the L1. Calling DefaultAccount.executeTransactionFromOutside from the L1 will always revert because the msg.sender will be set to the transaction sender but the function can only be called by the bootloader.

Tools Used

Manual review

Recommended Mitigation Steps

Consider removing the ignoreNonBootloader modifier from the DefaultAccount.executeTransactionFromOutside function and replacing it with a modifier that allows calling the function only to address(this), e.g.:

modifier ignoreNonAccount() {
  if (msg.sender != address(this)) {
    assembly {
      return(0, 0)
    }
  }

  _;
}

While it's hard to see the far-going consequences of this change (they should be figured out via extensive testing), it seems to align with the logic of the other parts:

1. The bootloader should always set msg.sender to the creators of L1 transactions–this looks unquestionable. It doesn't seem correct to set msg.sender to the bootloader address only when executeTransactionFromOutside is called.
2. The modifier aligns with the signature checking logic: only the account itself can execute transactions via the Abstract Account implementation. Likewise, executeTransactionFromOutside can only be called by the EOA itself, even though a transaction may be created on the L1.
3. Calling executeTransactionFromOutside on the L2 directly or from other contracts shouldn't execute the function and should return nothing instead.
4. DefaultAccount doesn't call the executeTransactionFromOutside function so there's no possibility of triggering the function on the L2.

[c4-judge]

GalloDaSballo marked the issue as primary issue

[c4-sponsor]

vladbochok marked the issue as sponsor disputed

[vladbochok]

The executeTransactionFromOutside function is needed only for the custom accounts, while the EOA can initiate the transaction via L1 -> L2 communication directly.

[GalloDaSballo]

Because of the discussion with the Sponsor on #30, I have to agree with a lack of impact

Because the function is factually not-usable in the reference given, but no specific risk was shown, am downgrading to QA Low Severity

I believe that the reference in-scope is a demo implementation, but since it always reverts, am awarding the finding as valid but at a lowered severity

[GalloDaSballo]

L

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

GalloDaSballo marked the issue as grade-c