c4-bot-3 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/contracts/SystemContext.sol#L117-L121 https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/contracts/libraries/SystemContractHelper.sol#L279-L288 https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/contracts/libraries/SystemContractHelper.sol#L227-L229 https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/bootloader/bootloader.yul#L2709

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/contracts/SystemContext.sol#L117-L121

    function getCurrentPubdataSpent() public view returns (uint256) {
        uint256 pubdataPublished = SystemContractHelper.getZkSyncMeta().pubdataPublished;
        return pubdataPublished > basePubdataSpent ? pubdataPublished - basePubdataSpent : 0;
    }

The above is one of the instances where a check for how much of the pubdata has been spent, evidently it makes a query to SystemContractHelper.getZkSyncMeta().pubdataPublished which is implemented here https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/contracts/libraries/SystemContractHelper.sol#L279-L288

    function getZkSyncMeta() internal view returns (ZkSyncMeta memory meta) {
        uint256 metaPacked = getZkSyncMetaBytes();
        meta.pubdataPublished = getPubdataPublishedFromMeta(metaPacked);
         //@audit
        //..snip
    }

And https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/contracts/libraries/SystemContractHelper.sol#L227-L229

    function getPubdataPublishedFromMeta(uint256 meta) internal pure returns (uint32 pubdataPublished) {
        pubdataPublished = uint32(extractNumberFromMeta(meta, META_GAS_PER_PUBDATA_BYTE_OFFSET, 32));
    }

Case is that this function instead returns the previous implemented gasPerPubdata and not the pubdata that's been published, keep in mind that the value returned from this is what's been used here return pubdataPublished > basePubdataSpent ? pubdataPublished - basePubdataSpent : 0; which indicates that protocol wrongly attempts to subtract from the "price" of gas per pubdata and not the amount of pubdata already "published", a hint on how the correct implementation is can be seen from the previous codebase, which indicates that this value represents the cost of one pubdata byte and not the amount of pubdata already spent as is being represented in this context.

Even if we wouldn't use the previous codebase as hint, we can see the same logic has been attached to the current natspec comment of getPubdataPublishedFromMeta(), i.e from https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/system-contracts/contracts/libraries/SystemContractHelper.sol#L222-L223, showcasing how using the value returned from this function as the amount of pubdata already spent wrong in SystemContext::getCurrentPubdataSpent().

Impact

All queries to the getCurrentPubdataSpent would return an invalid answer causing a wrong evaluation of gas already spent even via getCurrentPubdataCost() since this function calculates it as gasPerPubdataByte * getCurrentPubdataSpent(), i.e protocol wrongly calculates the amount of pubdata already spent in the SystemContext.sol contract, it currently calculates the value as the amount it takes **a single byte** sent to L1 as pubdata cost (gasPerPubdataByte) minus the basePubdataSpent instead of the amount it took for the whole published pubdata minus the basePubdataSpent

The correct implementation can be seen in the bootloader, where the value used is the real amount of pubdata that's been spent since a pointer, and not the gasPerPubdata - basePubdataSpent, which also shows how the SystemContext.sol deviates from the bootloader's implementation with it's flawed calculation

Recommended Mitigation Steps

Make SystemContext::getCurrentPubdataSpent() to be more inline with getting the real value of the current pubdata spent and not deviate from the bootloader's implementation.

Assessed type

Error

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

This is definitely a QA issue since the name is incorrect but not a security issue since the logic is sound.

alex-ppg commented 2 months ago

The Warden specifies that a calculation within the SystemContext will produce incorrect results due to utilizing different value denominations (i.e. amount instead of price).

Per the Sponsor's comments, this appears to be a misnomer at the SystemContractHelper level rather than an invalid calculation and thus cannot constitute anything higher than QA. The contest's changelog did imply this change, however, it appears the code was insufficiently updated to reflect it in terms of documentation as well as the constant variable's name. As such, I consider this to be at most a QA submission.

c4-judge commented 2 months ago

alex-ppg marked the issue as unsatisfactory: Overinflated severity

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