Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/common/Config.sol#L40-L46

Vulnerability details

Proof of Concept

First, would be key to note that, an operator is not fully trusted but rather they are "partially trusted" and this finding depends on them being a tad malicious which makes the expectation/invariant of them not being able to break the system not hold true.

Take a look at https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/common/Config.sol#L40-L46

/// @dev Timestamp - seconds since unix epoch.
uint256 constant COMMIT_TIMESTAMP_NOT_OLDER = 3 days;

/// @dev Maximum available error between real commit batch timestamp and analog used in the verifier (in seconds)
uint256 constant COMMIT_TIMESTAMP_APPROXIMATION_DELTA = 1 hours;

One can see that the value for COMMIT_TIMESTAMP_APPROXIMATION_DELTA is set to 1 hour, but as these comments suggest, this value should instead be as low as even 12 seconds, however it's been set to a whooping 3600 seconds. (300 times the real value it should be set as).

Now consider https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Executor.sol#L103-L124

    function _verifyBatchTimestamp(
        uint256 _packedBatchAndL2BlockTimestamp,
        uint256 _expectedBatchTimestamp,
        uint256 _previousBatchTimestamp
    ) internal view {
        // Check that the timestamp that came from the system context is expected
        uint256 batchTimestamp = _packedBatchAndL2BlockTimestamp >> 128;
        require(batchTimestamp == _expectedBatchTimestamp, "tb");

        // While the fact that _previousBatchTimestamp < batchTimestamp is already checked on L2,
        // we double check it here for clarity
        require(_previousBatchTimestamp < batchTimestamp, "h3");

        uint256 lastL2BlockTimestamp = _packedBatchAndL2BlockTimestamp & PACKED_L2_BLOCK_TIMESTAMP_MASK;

        // All L2 blocks have timestamps within the range of [batchTimestamp, lastL2BlockTimestamp].
        // So here we need to only double check that:
        //@audit
        // - The timestamp of the batch is not too small.
        // - The timestamp of the last L2 block is not too big.
        require(block.timestamp - COMMIT_TIMESTAMP_NOT_OLDER <= batchTimestamp, "h1"); // New batch timestamp is too small
        require(lastL2BlockTimestamp <= block.timestamp + COMMIT_TIMESTAMP_APPROXIMATION_DELTA, "h2"); // The last L2 block timestamp is too big
    }

We can see that this function is used to ensure that the timestamps of the new L2 blocks are correct, now if a batch's last block's timestamp was bigger than Ethereum's timestamp by COMMIT_TIMESTAMP_APPROXIMATION_DELTA margin, then the verification would revert, but if the timestamp was lower than Ethereum's timstamp + COMMIT_TIMESTAMP_APPROXIMATION_DELTA then code would accept the batch.

This is clearly to show that the Operator is allowed to set blocks timestamp to the future that's higher than the Ethereum's timestamp by a margin of COMMIT_TIMESTAMP_APPROXIMATION_DELTA. Case here is that the value of COMMIT_TIMESTAMP_APPROXIMATION_DELTA is 1 hour, which was dropped from it's previous value of 365 days, in order to stop the operator from setting the timestamp as different as one year further, which means that protocol wrongly assumes "1 hour" to be a safe enough difference, or meant to change it to "1 minute" and mistakenly had it set as "1 hour".

Considering these comments we can agree that the Operator shouldn't be allowed to do this, i.e setting the timestamp as high as an hour in the future, in fact the difference is even supposed to be as low as "12 seconds", but this isn't enforced.

Impact

The Operator, if malicious could "wrongly" fast forward the zkSync 1 hour to the future, where as this might seem little, this still leads to a huge impact considering the Operator can decide to do this whenever, this breaks different logic for users and protocols and could even lead to losses, depending on how much dependence a protocol places on the timestamp, since there are multiple DEFI logics standing on this: auctions, staking, vaults, airdrop, lending... etc.

This is atleast a medium severity finding since it depends on the logic of which the applications/protocols/users have on timestamps (expecting it to always be accurate/reliable). Let's just consider a simple case/idea of dutch auctions, say a very popular protocol with massive FDV/TVL decides to have a dutch auction for hours , the operator could make the price at which bids are being settled be way lower, leading to a loss and so on with other cases.

Recommended Mitigation Steps

Assuming this was a mistake and the real value should be "1 minute" then it should be updated to it, if otherwise, i.e protocol currently assumes 1 hour is a small enough value then we beg to differ and this report explains why, so protocol should reconsider and set COMMIT_TIMESTAMP_APPROXIMATION_DELTA to a value not higher than 3-5 minutes.

Additional Notes

The second paragraph of the Impact section is just one of numerous ideas that could pop from having the timestamp not being accurate.

Also zkSync is only getting bigger by the day, in fact as of last year it was reported that over 2.5 million users have used the zkSync bridge, source.

Now, whereas this doesn't directly mean that all users that ever interact with zkSync are going to be affected by this, this shows that a lot of more onboarding has been done to zkSync (and is still being done) with new users coupled with the fact that the crypto bull run already is in play we're only going to have more users/developers churning in different ideas that the bug discussed in this report would affect in as much as a there is a dependence on the reliability of the timestamp.

Assessed type

Timing

saxenism (sponsor) acknowledged

saxenism marked the issue as disagree with severity

We consider this a QA report, since this is documented assumption

The Warden specifies that an operator may manipulate the timestamp of a block up to the permitted deviation by the code itself, which is a deliberate design choice, and thus the submission can only be considered as criticism as part of a QA / Analysis report.

alex-ppg marked the issue as unsatisfactory: Overinflated severity

Hi @alex-ppg, thanks for judging the contest, also appreciate the extensive comments attached to other submitted H/M findings of mine, helped to see how my reasoning is wrong in those cases, however I believe your reason for concluding this report to instead be part of a part of a QA / Analysis report is wrong. Following PJQA guidelines, below attached are factual based comments as to why this report should instead be tagged satisfactory and finalized as a medium severity report.

Firstly, as already hinted in the report it’s been clearly stated that the operator should be considered somewhat malicious for the scope of the audit, we can see more on that from the Attack ideas (Where to look for bugs) section of the contest’s README, quoting the second paragraph under this section:

Special scrutiny should be paid to the powers of the operator. While currently the operator is controlled by Matter Labs and is also partially trusted (for instance, it is responsible for supplying the correct L1 gas price), it should never be able to directly steal users' funds or conduct malicious upgrades...
Secondly, going to the documentation of the COMMIT_TIMESTAMP_APPROXIMATION_DELTA variable we can see that the right and intended behaviour of this concept is to have this value set to 12 seconds this is cause this variable is to be considered the maximum error between real commit batch timestamp and analog used in the verifier and in this case we are to use 12 seconds since a miner's block.timestamp value can differ on some small value, i.e 12 seconds.
Thirdly, and standing on the second stated fact, if we go through some of the current findings that you've finalized their H/M status as satisfactory for this contest we can see that some issues stem from the fact documented implementation is not been followed.
- For e.g issue 97 and it's duplicates are about how problems would arise with unfreezing the chain, cause querying unfreezeChain() actually attempts to freeze the chain because of a typo, and this was finalized as a medium, rightfully so due to the function's execution attempt to do the exact opposite of what it's meant to do... _(which showcases that the intended behaviour is not being followed just like in the case present in this report where the COMMIT_TIMESTAMP_APPROXIMATION_DELTA has been set way higher than 12 seconds contrary to documentation)._
Other subtle connections can be made from other reports to this on the grounds of misrepresented intentions in code like Issue 53 that showcases discrepancies between the docs and how the StateTransitionManager.sol has been coded, but let's stop with this connection examples so as not to rail off topic.
Fourthly, this report does not only indicate that the current intended functionality is flawed, i.e the value of COMMIT_TIMESTAMP_APPROXIMATION_DELTA has been set to 300 times the expected value, rather it also goes deeper and hints how this could be of more impact and weaponized against protocol (and extensively users) since as previously stated in this comment, special scrutiny should be applied to the operator and this report proves that the operator can indeed break the system which is not expected, also depending on the window in which this is being exploited, the operator can actually steal funds from integrators, albeit indirectly (see the minimalistic Dutch auction hint in the report), which has also been hinted as an action that shouldn't be allowed by the operator.

Finally, I appreciate your time reading through the comments and I'd like for this finding to be re-reviewed and considered for it's medium severity state, cause using the guidelines outlined for this contest, this finding is indeed in scope and of a higher severity than QA, also the bug case could be weaponized in different manners as stated in the report (anything in protocol that depends on a timing logic would be affected by this) and the operator could position themselves to have the upperhand over protocol and it's users while executing this.

Hey @Bauchibred, thanks a lot for the thorough reply and your contribution to the PJQA process! I will address all the raised concerns point-by-point:

The excerpt you have referenced explicitly states that the operator is trusted to set a correct L1 gas price. It is acceptable to consider this level of privilege to be equivalent to setting the block.timestamp to a value within the bounds already honored by the system.
The documentation can at times be out of sync with the actual code. This is where the QA grade stems from, as the value is simply configured to a different one than the one its documentation implies. Per the Changelog of the contest's page, the adjustment of the variable has been noted as deliberate and the same documentation listed 12 seconds in the original C4 contest back in 2023-10 with no finding being accepted as valid pertaining to its incorrect inference.
For #97, I will reference the SC Verdicts and specifically Alex's PoV in relation to grading submissions; a medium severity is fair for an egregious mistake that does not lead to loss of funds which #97 falls under. In this submission, there is no egregious mistake but rather an argument that the configuration is insecure.
I believe the concerns raised are in line with findings that have been awarded in the previous contest; namely issue 316. Per the contest's page, past submissions are also out-of-scope for this contest.

Culminating the above points, I believe that a QA level assessment is fair and we could argue that the exhibit is also out-of-scope. I appreciate the due diligence and hope to see more of your submissions in future contests, but the judgment on this exhibit is considered final.

Hey @Bauchibred, thanks a lot for the thorough reply and your contribution to the PJQA process! I will address all the raised concerns point-by-point:

Hi once again @alex-ppg, thanks for the re-review, I'd be leaving this as my final comment on this issue and accept the final decision made by you after taking these arguments into account.

The excerpt you have referenced explicitly states that the operator is trusted to set a correct L1 gas price. It is acceptable to consider this level of privilege to be equivalent to setting the block.timestamp to a value within the bounds already honored by the system.

I believe there is a misunderstanding around the foot of this report, you've mentioned "The excerpt you have referenced explicitly states that the operator is trusted to set a correct L1 gas price.", however as shown this is on the dependence that currently the operator is trusted, note that before talking about the trust on the operator to set the current price this has been mentioned, to quote the same excerpt this has been stated: "While currently the operator is controlled by Matter Labs and is also partially trusted (for instance, it is responsible for supplying the correct L1 gas price)", however the footnote of this report is in fact based on the grounds that when the operator gets untrusted/decentralized then this bug case comes into play, which is also explicitly stated in the same excerpt, i.e the fact that scrutiny should be applied to the powers of the operator, so I believe we can conclude that this level of privilege can only be considered when the operator is trusted, however when they are not trusted then this opens windows for valid cases, like has been mentioned in this report.

The documentation can at times be out of sync with the actual code. This is where the QA grade stems from, as the value is simply configured to a different one than the one its documentation implies. Per the Changelog of the contest's page, the adjustment of the variable has been noted as deliberate and the same documentation listed 12 seconds in the original C4 contest back in 2023-10 with no finding being accepted as valid pertaining to its incorrect inference.

As already stated in this report, the changelog was taken into account, this report argues that the reduction made to the COMMIT_TIMESTAMP_APPROXIMATION_DELTA value from it's previous value is not minute enough when coupled with the fact that an operator could indeed become malicious, in fact quote on quote this was mentioned in the report "Case here is that the value of COMMIT_TIMESTAMP_APPROXIMATIONDELTA is 1 hour, which was dropped from it's previous value of 365 days, in order to stop the operator from setting the timestamp as different as one year further, which means that protocol wrongly assumes "1 hour" to be a safe enough difference", (from this we can apply a somewhat not so similar logic where a protocol are trying to say, get the price of an asset but they use a TWAP duration value that is manipulatable)_ security researchers are indeed welcomed and required to report the flaw in this logic and it should be counted as a valid submission in as much as one can proof how the logic is indeed unsafe unlike what protocol thinks (which I believe was rightly done in this report).

Now about your comment that "The documentation can at times be out of sync with the actual code." I fully agree, however it does not apply in this case, from the report, it's been hinted that this value should be dropped to minutes, with mentions of 1, 3 & 5 minutes all around report this is because, as understood, the comment in the snippet is not stating that the COMMIT_TIMESTAMP_APPROXIMATION_DELTA should exactly be 12 seconds (it can as well be), but rather that the miner's block.timestamp value can differ on some small value, i.e 12 seconds, the time it takes to mine a block, so to translate that comment, it means that the value of COMMIT_TIMESTAMP_APPROXIMATION_DELTA should be set with the consideration of the fact that the miner's block.timestamp can differ with 12 seconds, which is why the COMMIT_TIMESTAMP_APPROXIMATION_DELTA can indeed be set to any value but not less than 12 seconds (however there is a need of an assessment as to what value is safe enough both to the protocol and it's users and then this value should be set).

About your comment on using the previous C4 contest as a reference and their being no finding being accepted due to the incorrect reference, I don't think that should be a reason why this finding should be invalidated, this is a different contest and I believe raising a valid concern shouldn't be then checked on with a previous contest to hint it's validity is a strong enough ground to stand on, since we are not hinting instances of the protocol disputing the findings, but rather that they do not exist, additionally in the current contest this bug idea was also not disputed by the sponsors.

For Freezed Chain will never be unfreeze since StateTransitionManager::unfreezeChain is calling freezeDiamond instead of unfreezeDiamond. #97, I will reference the SC Verdicts and specifically Alex's PoV in relation to grading submissions; a medium severity is fair for an egregious mistake that does not lead to loss of funds which Freezed Chain will never be unfreeze since StateTransitionManager::unfreezeChain is calling freezeDiamond instead of unfreezeDiamond. #97 falls under. In this submission, there is no egregious mistake but rather an argument that the configuration is insecure.

Fairs, about #97, as stated earlier in my previous comment, to quote the instance " For e.g issue 97 ... was finalized as a medium, rightfully so due to the function's execution attempt to do the exact opposite" I believe we are standing on the same page and both think this issue should indeed be of medium severity after considering your comment here & about this issue, however I agree with your argument that in this case it is not an egregious mistake in configuration's code, but I'd argue that it is indeed a mistake to consider a 1 hour change on the timestamp to be acceptable/safe to protocol and it's users.

I believe the concerns raised are in line with findings that have been awarded in the previous contest; namely issue 316. Per the contest's page, past submissions are also out-of-scope for this contest.

On this, to start I believe the last argument you made under "2." is already a counter statement to this, however thanks for linking this report, I had a quick glance, and I can't seem to get my head around how exactly the linked issue makes this one OOS, albeit they are all about protocol's timing logic, this report heavily stands on the fact that protocol currently assumes the 1 hour durational change to be a safe margin, conclusively we can't claim that a finding from the previous contest can finalise this as OOS, cause as at the time of that contest the configuration for this had a whole different duration.

Culminating the above points, I believe that a QA level assessment is fair and we could argue that the exhibit is also out-of-scope. I appreciate the due diligence and hope to see more of your submissions in future contests, but the judgment on this exhibit is considered final.

Appreciate the kind words, and to finalize, I believe the comments written above entail enough valid reasoning as to why this report should not be of a QA level assessment and should also be considered in-scope, as stated earlier on, this is a final comment on this report agreeing with the decision you make after taking these into account, also apologising in advance in case you weren't expecting another comment, considering you've stated that "the judgment on this exhibit is considered final" however I believe it's in a warden's right on Code4rena to pass in comments during PJQA for a little back and forth with the judge as the whole point of the PJQA section of a contest is to ensure a finding's severity is correctly assessed, which I believe has not being done in this case.

Hey @Bauchibred, thanks for providing follow-up feedback on this submission. PJQA has concluded, but I understand the desire to further clarify the exhibit in case of a strong objection to the ruling above.

I believe there is a misunderstanding around the foot of this report...

The excerpt is, at least from a syntactical perspective, using the keyword also. Matterlabs being the sole operator is inherently trusted, so we can safely assume that the and also infers to operators that are not Matterlabs (i.e. decentralized) as the project matures. All operators, regardless of whether they are Matterlabs or not, are and will remain to be partially trusted. This is confirmed by the Sponsor (i.e. the author of those statements) directly and matches the canonical meaning of the statements.

As already stated in this report, the changelog was taken into account...

I am not implying that the submission should not have been made; you were correct to submit your objection to the safety of the timestamp restriction. I also never implied that the exhibit is invalid, its mark has been Overinflated Severity. It is valid criticism for the zkSync Era to consume, however, it is not a mandatory change that they need to incorporate.

Based on the trust model the Operators behave on, it is not correct to accept the manipulation of the timestamp value as a valid M level risk. Operators are partially trusted entities and have significantly more power than setting the timestamp, and could set the L1 gas value to a significantly abnormal value with identical severity ramifications.

On this, to start I believe the last argument you made under "2." is already a counter statement to this...

Point 2 is not a counter statement to Point 4, as Point 2 argues the severity level of the submission while Point 4 argues whether it is out-of-scope or not. The contest's page explicitly lists that past findings are out-of-scope for this contest. The Sponsor themselves have strongly disputed the timestamp issue and have directly acknowledged the DeFi implications that it entails. The exhibit in the past contest concerned itself with the same root cause (an abnormally high timestamp being permitted), and as the Sponsor has acknowledged the impact you describe (DeFi implications) in their original disagreement with the exhibit, this submission is out-of-scope.

As a closing note, I would like to note that the submission not being rewarded does not imply it is an invalid concern or something that the zkSync Era team should not pay attention to. I am simply indicating that, from a C4 contest perspective, the submission is ineligible for a cut of the reward pool.

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

A malicious operator could set the timestamp to a wrong value and directly have an impact on users #88