ArbitraryExecution - `markPrice` decimals are expected in several instances to be 18 but are 6 instead

[sherlock-admin]

ArbitraryExecution

high

markPrice decimals are expected in several instances to be 18 but are 6 instead

Summary

There are several instances where calculations expect markPrice to be in 18 decimals, but markPrice is in 6 decimals instead.

Vulnerability Detail

There are several instances in the codebase where the decimals of markPrice changes from 18 to 6:

Comments pertaining to markPrice indicate decimals of 18:

• markPrice in IDealer.sol
• markPrice in IMarkPriceSource.sol

Tests involving markPrice swap between using 18 decimals and 6:

• test using markPrice with 6 decimals
• test using markPrice with 18 decimals

Most importantly, all decimal multiplication is done with a denominator whose value is fixed at 10**18. However, on the Arbitrum Goerli testnet, JOJO returns markPrice with 6 decimals.

Impact

markPrice is typically multiplied with paperAmount to calculate netPositionValue, exposure, and the maintenanceMargin for traders. However, because decimalMul divides the product of markPrice and paperAmount with a fixed value of 10**18, any product of paperAmount and markPrice such that the magnitude of the result is less than 10**18 will cause division truncation which will return 0. Returning 0 for non-zero values of paperAmount and markPrice is incorrect, and can cause functions like getTotalExposure, _isAllSafe, etc. to return incorrect results and/or tabulate incorrect balances for traders.

Additionally, switching between 6 and 18 decimals further increases the chance of accidentally setting markPrice with the wrong scaling. This increases the risk of integrations with JOJO anticipating the wrong scaling for price, which could under-value or over-value a user's position and deposited credit. Finally, a majority of testing is done with markPrice in 18 decimals, but on-chain markPrice is most likely going to be in 6 decimals. This provides poor test coverage and could be masking issues.

Code Snippet

https://github.com/JOJOexchange/smart-contract-EVM/blob/4a95a8e9a6367ae88dc827e29467229cb5bbad4f/contracts/utils/SignedDecimalMath.sol#L10-L18

int256 constant SignedONE = 10**18;
function decimalMul(int256 a, int256 b) internal pure returns (int256) {
    return (a * b) / SignedONE;
}
function decimalDiv(int256 a, int256 b) internal pure returns (int256) {
    return (a * SignedONE) / b;
}

Tool used

Manual review.

Recommendation

Consider updating tests and comments to consistently use either 18 or 6 decimals. For multiplication and division that uses a fixed value of 10**18, consider adding a parameter to specify the magnitude to scale by so that the correct scaling can be used in different calculations. If JOJO does not wish to change the math scaling, then consider requiring a minimum trade and deposit paperAmount such that paperAmount * markPrice >= 10**18.

[JoscelynFarr]

The value returned by markPrice is base on USDC, because the decimal of USDC on arbitrum is 6, so markPrice is base on 6 dp

[arbitrary-sparrow]

Escalate for 10 USDC

The sponsor misunderstood our issue; we are in agreement that the protocol uses 6 decimal places. However, many instances of documentation within the protocol state the decimals are 18, which is wrong:

1. https://github.com/JOJOexchange/smart-contract-EVM/blob/4a95a8e9a6367ae88dc827e29467229cb5bbad4f/contracts/intf/IDealer.sol#L123
2. https://github.com/JOJOexchange/smart-contract-EVM/blob/4a95a8e9a6367ae88dc827e29467229cb5bbad4f/contracts/intf/IMarkPriceSource.sol#L10
3. https://github.com/JOJOexchange/smart-contract-EVM/blob/4a95a8e9a6367ae88dc827e29467229cb5bbad4f/contracts/impl/Perpetual.sol#L29

Having documentation that says the protocol uses 18 decimals when it in fact uses 6 decimals is causing major confusion. For example, this escalated issue confuses the decimals as being 18 because it is so difficult to figure it out with the conflicting documentation.

Additionally, calculations within the protocol assume 18 decimals, which is wrong in certain instances which we explained in our writeup.

[sherlock-admin]

Escalate for 10 USDC

The sponsor misunderstood our issue; we are in agreement that the protocol uses 6 decimal places. However, many instances of documentation within the protocol state the decimals are 18, which is wrong:

1. https://github.com/JOJOexchange/smart-contract-EVM/blob/4a95a8e9a6367ae88dc827e29467229cb5bbad4f/contracts/intf/IDealer.sol#L123
2. https://github.com/JOJOexchange/smart-contract-EVM/blob/4a95a8e9a6367ae88dc827e29467229cb5bbad4f/contracts/intf/IMarkPriceSource.sol#L10
3. https://github.com/JOJOexchange/smart-contract-EVM/blob/4a95a8e9a6367ae88dc827e29467229cb5bbad4f/contracts/impl/Perpetual.sol#L29

Having documentation that says the protocol uses 18 decimals when it in fact uses 6 decimals is causing major confusion. For example, this escalated issue confuses the decimals as being 18 because it is so difficult to figure it out with the conflicting documentation.

Additionally, calculations within the protocol assume 18 decimals, which is wrong in certain instances which we explained in our writeup.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[JoscelynFarr]

We will update the docs; The meaning of markPrice is how much USDC can be exchanged for every 1e18 token. Thank you for letting us know that. In this situation is there any problems for that?

[arbitrary-sparrow]

Besides updating the documentation (which is appreciated), there's still the issue of possible division truncation because your decimal math functions divide/multiply by a hardcoded value of 10**18:

int256 constant SignedONE = 10**18;
function decimalMul(int256 a, int256 b) internal pure returns (int256) {
    return (a * b) / SignedONE;
}
function decimalDiv(int256 a, int256 b) internal pure returns (int256) {
    return (a * SignedONE) / b;
}

Consider this example (caveat I haven't seen this code in a while so it may not be 100% accurate):

• Alice creates a maker order with a paperAmount of 0.0000001e18 which is 1e11
• The markPrice of the perpetual token Alice creates the order under is 1e6
• When markPrice is multiplied by paperAmount as part of calculating the value for maintenanceMargin, the result from the decimalMul function will be (1e11 * 1e6) / 1e18 = 0 due to division truncation.

Truncating non-zero values of paperAmount to zero can cause an accumulation of error in your protocol, so it would also be good to consider adding a minimum amount of paperAmount that must be traded with to avoid truncation since your markPrice decimals are in 6.

[JoscelynFarr]

In our system the paperAmount will always be based on 1e18 decimal and markPrice is based on 1e6. In your case, our back end will judge the minimum amount of paperAmount. Our matching machine is now controlled by JOJO team. Thank you for the advise

[Trumpero]

I believe this is a non-issue, as indicated by the comments from Jojo's team

[hrishibhat]

Result: Low Unique In addition to the above comment, the approval of trade is also controlled by the jojo team hence validating paperAmount can be considered to be validated by the protocol team

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• ArbitraryExecution: rejected