Users might be charged unfair amount of baseToken for L2Gas due to vulnerable baseToken price conversion implementation

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol#L159-L160 https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol#L79

Vulnerability details

Impact

Users might be charged unfair amount of baseToken for L2Gas due to vulnerable baseToken price conversion implementation.

Proof of Concept

Hyperchain allows non-eth baseToken chains. When user request L1->L2 transactions to a non-eth chain, users will pay L2 gas in baseToken instead of ETH.

The problem is baseToken l2 gas conversion might be using an outdated ratio of baseToken/ETH due to a vulnerable baseToken price update implementation setTokenMultiplier.

When a user request a L1->L2 transaction, Mailbox checks if user sent enough baseToken(mintValue) to cover gas(baseCost) in _requestL2Transaction(). uint256 baseCost = _params.l2GasPrice * _params.l2GasLimit; l2GasPrice is calculated in _deriveL2GasPrice() based on baseToken multipliers.

//code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol
    function _deriveL2GasPrice(
        uint256 _l1GasPrice,
        uint256 _gasPerPubdata
    ) internal view returns (uint256) {
...
 |>       uint256 l1GasPriceConverted = (_l1GasPrice *
            s.baseTokenGasPriceMultiplierNominator) /
            s.baseTokenGasPriceMultiplierDenominator;
        uint256 pubdataPriceBaseToken;
...

(https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol#L159-L160)

However, s.baseTokenGasPriceMultiplierNominator and s.baseTokenGasPriceMultiplierDenominator can only be updated in setTokenMultiplier() by the chain admin, which is most likely a multi-sig contract. Since base token is not updated dynamically, this will result in an old or assumed baseToken/ETH ratio being used for baseCost check. User is charged a baseCost based on and outdated baseToken/ETH ratio. The user can be overcharged.

//code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol
//@audit setTokenMultiplier can only called by chain Admin after chain genesis, the chain Admin is most likely a multi-sig contract, which will not update baseToken/ETH ratio in time, resulting in stale price.
    function setTokenMultiplier(
        uint128 _nominator,
        uint128 _denominator
|>    ) external onlyAdminOrStateTransitionManager {
...
        s.baseTokenGasPriceMultiplierNominator = _nominator;
        s.baseTokenGasPriceMultiplierDenominator = _denominator;
...

(https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Admin.sol#L79)

Tools Used

Manual

Recommended Mitigation Steps

Consider allowing baseToken multipliers to be updated dynamically, not just by admin multi-sig.

Assessed type

Other

[saxenism]

This is a design decision and does not have any security issues. However, this can be considered valid criticism, so we can consider making changes in the future versions. However, no changes are required right now.

[c4-sponsor]

saxenism (sponsor) disputed

[alex-ppg]

The Warden specifies that the token multipliers for L1 to L2 hyperchain transactions may be outdated and overcharge / undercharge as a result.

All evidence points to this being a conscious design decision, and while I do consider it valid criticism as part of an Analysis report I do not believe a case for a non-QA vulnerability can be made here as long as we consider that the multipliers are responsibly maintained by the zkSync Era team.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid