PRIORITY_EXPIRATION is set to 0 which is unintended behaviour as this even causes transactions to always have their expiry as block.timestamp

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/72f5f16ed4ba94c7689fe38fcb0b7d27d2a3f135/code/contracts/ethereum/contracts/zksync/Config.sol#L46

Vulnerability details

Impact

PRIORITY_EXPIRATION == 0 which leads to unexpected behaviours since uint64 expirationTimestamp = uint64(block.timestamp + PRIORITY_EXPIRATION); would always be block.timestamp hinting inability to correctly provide an expiry and potentially not having enough time to process priority transactions

Proof Of Concept

First note Config.sol#L46

uint256 constant PRIORITY_EXPIRATION = 0 days;

Note that this was meant for the Alpha release period which ended in April, but must have been missed and is still in production code.

Now take a look at Mailbox.sol#L283-L314

    function _requestL2Transaction(
        address _sender,
        address _contractAddressL2,
        uint256 _l2Value,
        bytes calldata _calldata,
        uint256 _l2GasLimit,
        uint256 _l2GasPerPubdataByteLimit,
        bytes[] calldata _factoryDeps,
        bool _isFree,
        address _refundRecipient
    ) internal returns (bytes32 canonicalTxHash) {
        require(_factoryDeps.length <= MAX_NEW_FACTORY_DEPS, "uj");
        //@audit
        uint64 expirationTimestamp = uint64(block.timestamp + PRIORITY_EXPIRATION); // Safe to cast
        uint256 txId = s.priorityQueue.getTotalPriorityTxs();

        // Here we manually assign fields for the struct to prevent "stack too deep" error
        WritePriorityOpParams memory params;

        // Checking that the user provided enough ether to pay for the transaction.
        // Using a new scope to prevent "stack too deep" error
        {
            params.l2GasPrice = _isFree ? 0 : _deriveL2GasPrice(tx.gasprice, _l2GasPerPubdataByteLimit);
            uint256 baseCost = params.l2GasPrice * _l2GasLimit;
            require(msg.value >= baseCost + _l2Value, "mv"); // The `msg.value` doesn't cover the transaction cost
        }

        // If the `_refundRecipient` is not provided, we use the `_sender` as the recipient.
        address refundRecipient = _refundRecipient == address(0) ? _sender : _refundRecipient;
        // If the `_refundRecipient` is a smart contract, we apply the L1 to L2 alias to prevent foot guns.
        if (refundRecipient.code.length > 0) {
            refundRecipient = AddressAliasHelper.applyL1ToL2Alias(refundRecipient);
        }

        params.sender = _sender;
        params.txId = txId;
        params.l2Value = _l2Value;
        params.contractAddressL2 = _contractAddressL2;
        params.expirationTimestamp = expirationTimestamp;
        params.l2GasLimit = _l2GasLimit;
        params.l2GasPricePerPubdata = _l2GasPerPubdataByteLimit;
        params.valueToMint = msg.value;
        params.refundRecipient = refundRecipient;

        canonicalTxHash = _writePriorityOp(params, _calldata, _factoryDeps);
    }

As seen the PRIORITY_EXPIRATION value is still being used to determine the deadline for the validators to process this transaction, but the value being 0 would mean that all transaction would need to be processed by the timestamp the transaction was requested, which would not be possible.

Tool Used

Manual Review

Recommended Mitigation Steps

Since the Alpha release period has passed, the necessary value for this should be passed and stored as hinted in [the previous codebase from the 2022-10-zksync contest on C4.

Assessed type

Timing

[c4-pre-sort]

GalloDaSballo marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-sponsor]

miladpiri marked the issue as disagree with severity

[miladpiri]

Not a bug, just a QA, since functionality is not used and this was supposed to change with escape hatch mechanism.

[c4-sponsor]

miladpiri (sponsor) acknowledged

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

Important to flag, however I have to agree with the Sponsor that no impact was demonstrated

[Bauchibred]

Hi @GalloDaSballo, thanks for judging, I'd appreciate if you could have a second look at this, before your final decision that's coupling it with the discussion that's been had regarding D-01 under this QA Report.

The fact that the alpha period has already passed I believe proves that this to be an issue, comparing the codebase and the previous one as was linked in the Recommended mitigation steps section of this report we can see that the intended design and normal functionality is to have a non-zero expiration timestamp, and the update to 0 days was done for the Alpha release period, as clearly indicated by this section of the codebase, this alone in my opinion shoould be considered valid grounds for a medium finding since protocol's functionality is is impacted anot functioning as supposed to, which is why no explanations were attached in the report about popular bug cases of lack of expirations/deadlines. There are multiple examples one could attach, but I'd rather not do that since I believe @nethoxa has done due justice to this under the discussions in their report of how impactful this could be.

Thank you for your time.

[GalloDaSballo]

I disagree with the interpretations, if anything if the value was set to the future, since it would be relative to block.timestmap, it wouldn't make any difference

Overall, after juding, Axelar, Optimism, CCIP and zkSync I believe that if the txs need to be protected from MEV they will need to use a message receiver infrastructure, which would be built on top of the Mailbox

I believe QA is most appropriate for this finding as the idea that said block.timestamp is the same as a swap deadline is not correct as the Mailbox is one level below the stack