[M-01] Use of TX.GASPRICE in Mailbox contract

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/7ed3944429f437a611c32e782a12b320f6a67c17/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L283-L327 https://github.com/code-423n4/2023-10-zksync/blob/7ed3944429f437a611c32e782a12b320f6a67c17/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L304

Vulnerability details

Impact

It is good practice for the user to set the TX.GASPRICE and not for the developer to do that. This contract is using TX.GASPRICE on line referenced.

Proof of Concept

Vulnerable Code Snippet

// code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol => Line: 304
            params.l2GasPrice = _isFree ? 0 : _deriveL2GasPrice(tx.gasprice, _l2GasPerPubdataByteLimit);

Vulnerable Function

// code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol => Lines: 283-327
    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");
        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);
    }

Tools Used

VS Code.

Recommended Mitigation Steps

When developers suggest the gas amount using TX.GASPRICE, it might open up vulnerabilities in the contract based on potential incorrect business logic. The developer should set a maximum tx.gasprice to protect the users funds.

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as low quality report

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid