Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mathematical inaccuracy can lead to wrong prices #8

Closed
c4-bot-7 opened this issue Mar 13, 2024 · 4 comments
Closed

Mathematical inaccuracy can lead to wrong prices #8

c4-bot-7 opened this issue Mar 13, 2024 · 4 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working edited-by-warden sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-bot-7
Copy link
Contributor

c4-bot-7 commented Mar 13, 2024

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/main/code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol#L167
https://github.com/code-423n4/2024-03-zksync/blob/main/code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol#L171

Vulnerability details

Impact

The mathematical inaccuracy in the calculation involving uint256 fullPubdataPriceBaseToken and l2GasPrice can result in precision loss

Proof of Concept

In the principle of PEDMAS/BODMAS, division comes before addition. But in the case of uint256 fullPubdataPriceBaseToken, the case is reversed. This can cause the return of wrong results. For instance, let pubdataPriceBaseToken be 10 wei, batchOverheadBaseToken be 20 wei and uint256(feeParams.maxPubdataPerBatch) be 5 wei.

Without the PEDMAS, fullPubdataPriceBaseToken will give 6 wei but result in 14 wei with PEDMAS application

Tools Used

Manual Review

Recommended Mitigation Steps

Division should come first before addition in calculations or the parameters involved in the division should be encapsulated properly:

uint256 fullPubdataPriceBaseToken = pubdataPriceBaseToken +
(batchOverheadBaseToken / uint256(feeParams.maxPubdataPerBatch));

Or:
uint256 fullPubdataPriceBaseToken = (pubdataPriceBaseToken +
batchOverheadBaseToken) / uint256(feeParams.maxPubdataPerBatch

whichever calculation is intended. Same should also apply to the l2gasprice calculation

Assessed type

Error

@c4-bot-7 c4-bot-7 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Mar 13, 2024
c4-bot-7 added a commit that referenced this issue Mar 13, 2024
@c4-bot-1 c4-bot-1 changed the title Mathematical inaccuracy can lead to precision loss Mathematical inaccuracy can lead to wrong prices Mar 22, 2024
@c4-sponsor
Copy link

saxenism (sponsor) disputed

@c4-sponsor c4-sponsor added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Apr 4, 2024
@saxenism
Copy link

saxenism commented Apr 4, 2024

The parenthesis is not required as it does not lead to any wrong calculations in any case.

@alex-ppg
Copy link

The Warden describes an addition and division that utilizes an implicit order of operations that could have been incorrect.

As the Sponsor outlines and the code confirms, the desired order of operations is that the division occurs before the addition which is the present behaviour rendering this exhibit invalid.

@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Apr 29, 2024
@c4-judge
Copy link
Contributor

alex-ppg marked the issue as unsatisfactory:
Invalid

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working edited-by-warden sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

6 participants