c4-bot-5 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/contracts/L1Messenger.sol#L44-L64

Vulnerability details

Proof of Concept

First take a look at https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/contracts/L1Messenger.sol#L44-L64

    uint256 internal constant KECCAK_ROUND_GAS_COST = 40;

    uint256 internal constant KECCAK_ROUND_NUMBER_OF_BYTES = 136;

    function keccakGasCost(uint256 _length) internal pure returns (uint256) {
        return KECCAK_ROUND_GAS_COST * (_length / KECCAK_ROUND_NUMBER_OF_BYTES + 1);
    }

    uint256 internal constant SHA256_ROUND_GAS_COST = 7;

    uint256 internal constant SHA256_ROUND_NUMBER_OF_BYTES = 64;

    function sha256GasCost(uint256 _length) internal pure returns (uint256) {
        return SHA256_ROUND_GAS_COST * ((_length + 8) / SHA256_ROUND_NUMBER_OF_BYTES + 1);
    }

Evidently there are constants placed and protocol have used the logic of adding one after the division so as to sort out issues where the numerator is "<" than the denominator causing the equation to return 0, case is that this logic is somewhat wrong and could be very harmful to some users.

Now consider the three instances where it's calculated, i.e in sendL2ToL1Log()

        uint256 gasToPay = keccakGasCost(L2_TO_L1_LOG_SERIALIZE_SIZE) + 2 * keccakGasCost(64);
        SystemContractHelper.burnGas(Utils.safeCastToU32(gasToPay), 0);

In sendToL1()

        uint256 gasToPay = keccakGasCost(L2_TO_L1_LOG_SERIALIZE_SIZE) +
            3 *
            keccakGasCost(64) +
            gasSpentOnMessageHashing +
            COMPUTATIONAL_PRICE_FOR_PUBDATA *
            pubdataLen;
        SystemContractHelper.burnGas(Utils.safeCastToU32(gasToPay), uint32(pubdataLen));

And lastly in requestBytecodeL1Publication()

        uint256 gasToPay = sha256GasCost(bytecodeLen) +
            keccakGasCost(64) +
            COMPUTATIONAL_PRICE_FOR_PUBDATA *
            pubdataLen;
        SystemContractHelper.burnGas(Utils.safeCastToU32(gasToPay), uint32(pubdataLen));

Evidently, in all three instances, we can see that there is a constant query on keccakGasCost(64) which would always return 40 due to the rounding down in the calculation, this is cause 64 is < KECCAK_ROUND_NUMBER_OF_BYTES = 136, to put this in perspective, if 64 was replaced with double it's value, i.e 128, it would still return 40 indicating that users are charged in this case more than double what they were supposed to be charged, with the increment being ~ 110.9% of increment, from the keccak gas calculation we can see that this is the original increment in monetary value only for requestBytecodeL1Publication(), because the remaining instances in both sendL2ToL1Log() and sendToL1() still multiply on this value, i.e in the former the returned value is being doubled where as in the latter it's being tripled (where as the percentage increment is still the same for all instances it's important to still note this cause in their monetary value this is more weight on the users in the latter two instances).

Now keeping the constant query to keccakGasCost(64) aside, the below are also done in the calculations.

In both sendL2ToL1Log()and sendToL1() , keccakGasCost() is also queried again with the L2_TO_L1_LOG_SERIALIZE_SIZE , keeping in mind that the value of this variable is 88 this leads to an additional ~ 55% on the cost of calculating the keccak256.
Now in requestBytecodeL1Publication(), is the only place where sha256GasCost() is also queried which has the same division and then + 1 logic which means that depending on the bytecodeLen value this calculation would also overcharge users.

Impact

Users are overcharged gas for hashing in different instances in as much as there is a need to publishh pubdata , sometimes even more than triple the amount really needed to execute the hashing attached with their transactions, this amount overcharged can't also be considered that it's for the compensation of the operator's storage cause the gasToPay calculation already includes COMPUTATIONAL_PRICE_FOR_PUBDATA * pubdataLen that takes care of that.

Recommended Mitigation Steps

Consider reimplementing the logic in the formular used to calculate the gas cost for hashing in keccakGasCost and keccakGasCost to not overcharge users.

Assessed type

Context

c4-sponsor commented 5 months ago

saxenism (sponsor) disputed

saxenism commented 5 months ago

Invalid issue since hash functions has paddings that’s why the cost per 40 bytes is the same as the cost per 136 bytes

c4-judge commented 4 months ago

alex-ppg marked the issue as unsatisfactory: Invalid