search

code-423n4 / 2024-03-zksync-findings

1 stars 1 forks source link

Users are charged too much as the memory overhead gas #92

Closed c4-bot-3 closed 2 months ago

c4-bot-3 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/libraries/TransactionValidator.sol#L128-L137 https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/bootloader/bootloader.yul#L37-L53 https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/bootloader/bootloader.yul#L2010-L2017

Vulnerability details

Proof of Concept

Consider https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/libraries/TransactionValidator.sol#L128-L137

    function getOverheadForTransaction(
        uint256 _encodingLength
    ) internal pure returns (uint256 batchOverheadForTransaction) {
        // The overhead from taking up the transaction's slot
        batchOverheadForTransaction = TX_SLOT_OVERHEAD_L2_GAS;

        // The overhead for occupying the bootloader memory can be derived from encoded_len
        uint256 overheadForLength = MEMORY_OVERHEAD_GAS * _encodingLength;
        batchOverheadForTransaction = Math.max(batchOverheadForTransaction, overheadForLength);
    }

This function is used to get the overhead that a user is to pay upfront for a tx, now both TX_SLOT_OVERHEAD_L2_GAS and MEMORY_OVERHEAD_GAS are 10000 and 10 respectively.

Navigating to the bootloader we can see that the logic/values are as expected the same, i.e https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/bootloader/bootloader.yul#L37-L53


            function TX_SLOT_OVERHEAD_GAS() -> ret {
                ret := 10000
            }

            function MEMORY_OVERHEAD_GAS() -> ret {
                ret := 10
            }

And https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/bootloader/bootloader.yul#L2010-L2017

            function getTransactionUpfrontOverhead(
                txEncodeLen
            ) -> ret {
                ret := max(
                    safeMul(txEncodeLen, MEMORY_OVERHEAD_GAS(), "iot"),
                    TX_SLOT_OVERHEAD_GAS()
                )
            }

Evidently, the MEMORY_OVERHEAD_GAS value has been set to 10, issue with this is that, according to the docs we can see that to get this value this is the formula used TX_MEMORY_OVERHEAD_GAS = MAX_GAS_PER_BATCH / MAX_MEMORY_FOR_BATCH.

Now would be key to note that, for L1->L2 transactions the MAX_GAS_PER_BATCH is considered to be 80kk and MAX_MEMORY_FOR_BATCH is 32 * 600_000, i.e 19.2kk.

Causing the TX_MEMORY_OVERHEAD_GAS = 80kk/19.2kk ~= 4,17, now protocol decided to use 10 gas value so as to better take into account the load on the operator from storing the information about the transaction, but this is too much of an increase, i.e ~ a 140% increase that is implaced on the users to pay.

To put this in perspective, due to the same reason of taking into account the load on the operator for storing the information the TX_OVERHEAD_GAS was only increased by a mere 25%, which is less than 1/5 of the increase applied the memory overhead gas.

Impact

Medium since this leads to extremely unfair gas prices for users, way over the original gas price that could be paid.

Keep in mind that the current implementation of getting the overhead price mixes both incrementing instances, i.e it first places a ~140% from safeMul(txEncodeLen, MEMORY_OVERHEAD_GAS() and then if this value is less than TX_SLOT_OVERHEAD_GAS it applies up to 25% more increment Remember https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/bootloader/bootloader.yul#L2010-L2018

            function getTransactionUpfrontOverhead(
                txEncodeLen
            ) -> ret {
                ret := max(
                    safeMul(txEncodeLen, MEMORY_OVERHEAD_GAS(), "iot"),
                    TX_SLOT_OVERHEAD_GAS()
                )
            }

Lets show case two scenarios within two cases: Case A - doesn't take into account the load on the operator from storing the information about the transaction for both variables whereas Case B does(i.e Under A ( MEMORY_OVERHEAD_GAS = 4,17 and TX_SLOT_OVERHEAD_GAS = 8kk ) and Under B ( MEMORY_OVERHEAD_GAS = 10 and TX_SLOT_OVERHEAD_GAS = 10kk )

TLDR of Case A: In comparison to the real _safeMul(txEncodeLen, MEMORY_OVERHEAD_GAS()_ users could get charged as high as 190K% due to the minimum enforcement... let's just pick one from the aforementioned scenarios, i.e txEncodeLen = 100 this still leads to an increase of > 1800% (keep in mind that we are not considering the operator's storage load but users are still being charged this much increment).

TLDR of Case B: In comparison to the real _safeMul(txEncodeLen, MEMORY_OVERHEAD_GAS()_ users could get charged as high as 99K% due to the minimum enforcement, keep in mind that here we have considered the operator's storage load since MEMORY_OVERHEAD_GAS == 10 and users are being charged this much.

TLDR of Both Cases: Comparing Case A to B, we can see that whereas the encodedLength is the same, Case A would have a massive increase up until txEncodeLen = 1918 where the real _safeMul(txEncodeLen, MEMORY_OVERHEAD_GAS(),_ would be ~8000 as shown in the first scenarios of both cases, but from there onwards Case B charges even higher since automatically using scenario 2000, overhead gas for case A is 8340 where as that of case B is 20000 a ~140% increase

NB: Would be key to note that it's understandable to enforce a minimum value and as such the outcome of scenarios 1..=999 should be accepted by users (since one can say they are only paying for the TX_SLOT_OVERHEAD_GAS ), however from there onwards all other scenarios with longer length of encoded data is just unfair, also putting this in percentage might undermine the burden on users, considering monetary value users now have to pay 20000 gas instead of 8340, and it could even be higher dependent on the txEncodeLen.

So in short, transactions that would get succeeded would need to provide insane amount as their gas limit otherwise this check would cause their transaction to revert

Recommended Mitigation Steps

Consider reimplementing the logic for the amount of increase being applied to MEMORY_OVERHEAD_GAS , since there is already an increase for the slot overhead to consider the operator's attempt of storing the data, i.e the increment of the overhead slot as at current already sorts out when the length of the encoded data is little, since it defaults to the max from bootloader.yul#L2013-L2016, i.e safeMul(txEncodeLen, MEMORY_OVERHEAD_GAS(), "iot") & TX_SLOT_OVERHEAD_GAS(), which is understandable.

Assessed type

Context

c4-sponsor commented 3 months ago

saxenism (sponsor) acknowledged

c4-sponsor commented 3 months ago

saxenism marked the issue as disagree with severity

saxenism commented 3 months ago

Design Decision, We consider this a QA report.

alex-ppg commented 2 months ago

The Warden describes that gas adjustments overcompensate operator overheads, which we consider an acceptable business practice as it is in the interest of the protocol to protect the operators instead of the users when it comes to transaction gas costs. As such, this finding is better suited as part of an Analysis or QA report as criticism.

c4-judge commented 2 months ago

alex-ppg marked the issue as unsatisfactory: Overinflated severity