It's possible to replay some upgrade transactions

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/libraries/TransactionValidator.sol#L55 https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/contracts/libraries/TransactionHelper.sol#L118-L136

Vulnerability details

Impact

Field reserved is not being taken into consideration when encoding hash of the zkSync native transaction type (TransactionHelper._encodeHashEIP712Transaction()).

Moreover, according to TransactionValidator.validateUpgradeTransaction(), upgrade transaction is valid when reserved[1] <= type(uint160).max. This means that it's possible to replay upgrade transaction by providing different values to reserved[1] field.

Proof of Concept

File: TransactionValidator.sol

        require(_transaction.reserved[0] == 0, "ue");
        require(_transaction.reserved[1] <= type(uint160).max, "uf");
        require(_transaction.reserved[2] == 0, "ug");
        require(_transaction.reserved[3] == 0, "uo");

While reserved[0], reserved[2], reserved[3] must equal to 0 - the reserved[1] just needs to be lower or equal type(uint160).max.

File: TransactionHelper.sol

function _encodeHashEIP712Transaction(Transaction calldata _transaction) private view returns (bytes32) {
        bytes32 structHash = keccak256(
            abi.encode(
                EIP712_TRANSACTION_TYPE_HASH,
                _transaction.txType,
                _transaction.from,
                _transaction.to,
                _transaction.gasLimit,
                _transaction.gasPerPubdataByteLimit,
                _transaction.maxFeePerGas,
                _transaction.maxPriorityFeePerGas,
                _transaction.paymaster,
                _transaction.nonce,
                _transaction.value,
                EfficientCall.keccak(_transaction.data),
                keccak256(abi.encodePacked(_transaction.factoryDeps)),
                EfficientCall.keccak(_transaction.paymasterInput)
            )
        );

Since reserved field is not being taken into consideration in _encodeHashEIP712Transaction(), this basically means, that it would be possible to replay some transactions with different _transaction.reserved[1].

1. Let's create a valid upgrade transaction and send it. Let's assume that its reserved is set to "0100"
2. Let's take that transaction and modify reserved[1]: now reserve is "0200".
3. Since require(_transaction.reserved[1] <= type(uint160).max, "uf"); is valid (line 55 in TransactionValidator.sol) - the whole upgrade transaction is still valid and it's possible to replay it.

Tools Used

Manual code review

Recommended Mitigation Steps

reserved fields should have constant values, until they are not taken into consideration in TransactionHelper._encodeHashEIP712Transaction(). This means, that line 55 in TransactionValidator.sol should be changed from: require(_transaction.reserved[1] <= type(uint160).max, "uf"); to require(_transaction.reserved[1] == 0, "uf");.

Assessed type

Other

[c4-sponsor]

saxenism (sponsor) disputed

[saxenism]

Considered Invalid because all fields are set on L1 contract and then the hash is double checked in the bootloader

[alex-ppg]

The Warden specifies that an upgrade transaction can be replayed by adjusting one of the reserved fields of the transaction given that it accepts multiple valid values and is not part of the TransactionValidator signature validation process.

While an interesting observation, an upgrade transaction will not be replayable due to how protocol version upgrades cannot be performed to the same version per the relevant functions in the BaseZkSyncUpgrade contract (1, 2).

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid