search

code-423n4 / 2023-10-zksync-findings

3 stars 0 forks source link

L2 transactions with type 0, 1, or 2 are not compliant with the standards because v-value is fixed to 27 or 28 #155

Open c4-submissions opened 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/ebec640786cf3cb791fa77a856ad9d2a554dad3f/code/system-contracts/contracts/BootloaderUtilities.sol#L92 https://github.com/code-423n4/2023-10-zksync/blob/ebec640786cf3cb791fa77a856ad9d2a554dad3f/code/system-contracts/contracts/BootloaderUtilities.sol#L191 https://github.com/code-423n4/2023-10-zksync/blob/ebec640786cf3cb791fa77a856ad9d2a554dad3f/code/system-contracts/contracts/BootloaderUtilities.sol#L286

Vulnerability details

Impact

Current implementation of L2 transactions with types 0 (legacy), 1 (access list), 2 (EIP1559) is not compliant with the EIP standards. To use these transaction types within the zkSync network, customization of all dApps or libraries, such as ethers.js, viem, and web3.js, will be necessary.

Proof of Concept

As per the established standards and popular library implementations:

  1. For legacy transactions, the v value should be either 27, 28 (without replay protection), or calculated as CHAIN_ID * 2 + 35 + {0,1} (with replay protection). For a regular transaction on zkSync with a chain ID of 324, the v value should be 683 or 684.

  2. In the case of access list transactions, the v value should be either 0 or 1.

  3. For EIP-1559 transactions, the v value should also be either 0 or 1.

In all three cases of the implementation, there's a requirement for v to be either 27 or 28, which is compliant only with legacy non-protected transactions: https://github.com/code-423n4/2023-10-zksync/blob/ebec640786cf3cb791fa77a856ad9d2a554dad3f/code/system-contracts/contracts/BootloaderUtilities.sol#L92 https://github.com/code-423n4/2023-10-zksync/blob/ebec640786cf3cb791fa77a856ad9d2a554dad3f/code/system-contracts/contracts/BootloaderUtilities.sol#L191 https://github.com/code-423n4/2023-10-zksync/blob/ebec640786cf3cb791fa77a856ad9d2a554dad3f/code/system-contracts/contracts/BootloaderUtilities.sol#L286

The only way to process transactions of these types is to manually set v to 27 or 28, which does not comply with the established standards.

Additionally, it's important to highlight that both DefaultAccount._isValidSignature() and functions within BootloaderUtilities parse only one byte for v. However, in the case of a legacy transaction with replay protection on zkSync, the v value (683 or 684) is encoded as three bytes: one for length and two for the value. This non-standard encoding should be taken into account.

Bootloader.saveTxHashes() calls BootloaderUtilities.getTransactionHashes() and then checks the success. If the call failed, it'll revert the batch. https://github.com/code-423n4/2023-10-zksync/blob/ebec640786cf3cb791fa77a856ad9d2a554dad3f/code/system-contracts/bootloader/bootloader.yul#L2044-L2076

A similar problem was previously identified in OpenZeppelin's audit and was categorized as a High-severity issue. It was marked as resolved: https://blog.openzeppelin.com/zksync-bootloader-audit-report#non-standard-transaction-hash

However, in this case, the issue is classified as Medium severity. This is because the current implementation permits these transactions to be sent if users manually modify the v value, even though it deviates from the standard.

Tools Used

Manual review, MyCrypto to decode transactions

Recommended Mitigation Steps

Fix the parsing of v-value accordingly to the standards.

Assessed type

Invalid Validation

c4-pre-sort commented 11 months ago

bytes032 marked the issue as primary issue

c4-pre-sort commented 11 months ago

141345 marked the issue as sufficient quality report

miladpiri commented 11 months ago

By design, they are compliant for hash calculation though. So, invalid issue.

c4-sponsor commented 11 months ago

miladpiri (sponsor) disputed

GalloDaSballo commented 10 months ago

@miladpiri can you please give me a bit more info about why the issue was initially resolved from OZ but is now being disputed?

miladpiri commented 10 months ago

thanks @GalloDaSballo

Basically in the case of the OZ, there was an issue that we use the number 27/28 in transaction hashes. However, now in the transaction hashes we include the correct values. For instance, here: https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/BootloaderUtilities.sol#L193 Even though the original value of v is either 27 or 28, the correct value is used during hash calculation.

Although the RLP encoded v is 3 bytes indeed, the correct RLP encoding is used nonetheless: https://github.com/code-423n4/2023-10-zksync/blob/ebec640786cf3cb791fa77a856ad9d2a[…]54dad3f/code/system-contracts/contracts/BootloaderUtilities.sol

c4-judge commented 10 months ago

GalloDaSballo changed the severity to QA (Quality Assurance)

GalloDaSballo commented 10 months ago

Downgrading to Informational in lack of a spefiic vulnerability since the underlying logic is compliant but the input is shifted and should possibly require a different SDK which I'm not convinced can be classified as a Smart Contract risk

ustas-eth commented 10 months ago

I want to bring attention of the judge to the fact that EIP-155 explicitly states that v-value MUST be equal to {0,1} + CHAIN_ID * 2 + 35, and zkSync does not mention the issue in any way in the description of the types of transactions nor in the known differences from Ethereum, which I believe will lead to many integration issues, because the project does mention EIPs' names without actual compliance. It may not result in direct smart contract losses, but this contest contains the entire blockchain of zkSync in scope and is not limited to smart contracts only.

Moreover, I want to point out the inconsistency in combination with my other issue: https://github.com/code-423n4/2023-10-zksync-findings/issues/154. I understand that the sponsors intend to maintain backward compatibility with all types of transactions to support older dApps. However, the current issue disrupts this idea because older dApps will have to change their code anyway. As a result:

I suggest the sponsors implement a more elegant solution to the problem. Instead of using the non-compliant fixed values {27, 28} or implementing the compliant fixed values {0, 1, 27, 28, 683, 684} (which would require many changes to the code), I propose to accept any v-value by user choice, and % 2 it. In any case, this will produce {0, 1}, so you can add 27 and get a standardized v-value that erecovery will accept. With this approach, you'll:

c4-judge commented 10 months ago

This previously downgraded issue has been upgraded by GalloDaSballo

c4-judge commented 10 months ago

GalloDaSballo marked the issue as selected for report

GalloDaSballo commented 10 months ago

In contrast to other reports this one shows how the v value for EIPs is implemented in a way that is not consistent with the EIPs

I personally do not recommend mitigating this nor changing the code, and another Judge suggested that the %2 logic will cause further issues

However, per our rules, breaking an EIP that is being implemented is a valid Medium Severity finding and in this case it's clear that the EIP is being implemented

vladbochok commented 10 months ago

Already answered in another issue, in fact transaction hashes are calculated with expected v value, but the canonical transaction structure contains v as 27 or 28

c4-judge commented 9 months ago

GalloDaSballo changed the severity to QA (Quality Assurance)

GalloDaSballo commented 9 months ago

Per extensive discussion with the sponsor, the txs are handled as intended

Quoting a colleague judge and Zk SR

Does this have a material impact? 
No, it would be trivial for the operator to receive signatures with the correct signature and convert the v when submitting a batch, which is probably what they're doing otherwise their mainnet would not be able to execute any transactions.
Additionally, since the transactions are hashed correctly there is no issues with block explorers or anything like that.

Given the discussion above am downgrading to QA

c4-judge commented 9 months ago

GalloDaSballo marked the issue as not selected for report