Open c4-submissions opened 11 months ago
GalloDaSballo marked the issue as primary issue
141345 marked the issue as sufficient quality report
miladpiri marked the issue as disagree with severity
Not a bug, just a QA, since functionality is not used and this was supposed to change with escape hatch mechanism.
miladpiri (sponsor) acknowledged
GalloDaSballo changed the severity to QA (Quality Assurance)
Important to flag, however I have to agree with the Sponsor that no impact was demonstrated
Hi @GalloDaSballo, thanks for judging, I'd appreciate if you could have a second look at this, before your final decision that's coupling it with the discussion that's been had regarding D-01 under this QA Report.
The fact that the alpha period has already passed I believe proves that this to be an issue, comparing the codebase and the previous one as was linked in the Recommended mitigation steps section of this report we can see that the intended design and normal functionality is to have a non-zero expiration timestamp, and the update to 0 days
was done for the Alpha release period, as clearly indicated by this section of the codebase, this alone in my opinion shoould be considered valid grounds for a medium finding since protocol's functionality is is impacted anot functioning as supposed to, which is why no explanations were attached in the report about popular bug cases of lack of expirations/deadlines. There are multiple examples one could attach, but I'd rather not do that since I believe @nethoxa has done due justice to this under the discussions in their report of how impactful this could be.
Thank you for your time.
I disagree with the interpretations, if anything if the value was set to the future, since it would be relative to block.timestmap, it wouldn't make any difference
Overall, after juding, Axelar, Optimism, CCIP and zkSync I believe that if the txs need to be protected from MEV they will need to use a message receiver infrastructure, which would be built on top of the Mailbox
I believe QA is most appropriate for this finding as the idea that said block.timestamp is the same as a swap deadline is not correct as the Mailbox is one level below the stack
Lines of code
https://github.com/code-423n4/2023-10-zksync/blob/72f5f16ed4ba94c7689fe38fcb0b7d27d2a3f135/code/contracts/ethereum/contracts/zksync/Config.sol#L46
Vulnerability details
Impact
PRIORITY_EXPIRATION == 0
which leads to unexpected behaviours sinceuint64 expirationTimestamp = uint64(block.timestamp + PRIORITY_EXPIRATION);
would always beblock.timestamp
hinting inability to correctly provide an expiry and potentially not having enough time to process priority transactionsProof Of Concept
First note Config.sol#L46
uint256 constant PRIORITY_EXPIRATION = 0 days;
Note that this was meant for the
Alpha release period
which ended in April, but must have been missed and is still in production code.Now take a look at Mailbox.sol#L283-L314
As seen the
PRIORITY_EXPIRATION
value is still being used to determine the deadline for the validators to process this transaction, but the value being0
would mean that all transaction would need to be processed by the timestamp the transaction was requested, which would not be possible.Tool Used
Manual Review
Recommended Mitigation Steps
Since the
Alpha release period
has passed, the necessary value for this should be passed and stored as hinted in [the previous codebase from the 2022-10-zksync contest on C4.Assessed type
Timing