IllIllI - A single precision value may not work for both the min and max prices

[sherlock-admin]

IllIllI

medium

A single precision value may not work for both the min and max prices

Summary

The same precision may not work for the min and max prices

Vulnerability Detail

If the min price reaches the maximum value possible for the specified level of precision, the max price won't be able to use the same precision.

Impact

Depending on how the order keepers and oracle archive work, either the fetching the price from the oracle will fail, or the user will get less than they deserve. This may happen when a user is at the border of being liquidated, and it would be unfair to liquidate the user.

Code Snippet

The same precision is required to be used for both the min and max prices:

// File: gmx-synthetics/contracts/oracle/OracleUtils.sol : OracleUtils.validateSigner()   #1

254        ) internal pure {
255            bytes32 digest = ECDSA.toEthSignedMessageHash(
256                keccak256(abi.encode(
257                    SALT,
258                    info.minOracleBlockNumber,
259                    info.maxOracleBlockNumber,
260                    info.oracleTimestamp,
261                    info.blockHash,
262                    info.token,
263                    info.tokenOracleType,
264 @>                 info.precision,
265 @>                 info.minPrice,
266 @>                 info.maxPrice
267                ))
268            );
269    
270:           address recoveredSigner = ECDSA.recover(digest, signature);

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/oracle/OracleUtils.sol#L254-L274

The example from the source comments show that legitimate values feasibly can occur on precision boundaries.

Tool used

Manual Review

Recommendation

Provide separate precision values for min and max prices

[xvi10]

Will reference the ETH example, as the numbers are a bit simpler to reason with

The price of ETH is 5000, and ETH has 18 decimals.
Price would be stored as 5000 / (10 ^ 18) * (10 ^ 30) => 5000 * (10 ^ 12)
If the decimal multiplier value is set to 8, the uint32 value would be 5000 * (10 ^ 12) / (10 ^ 8) => 5000 * (10 ^ 4)

With this config, ETH prices can have a maximum value of (2 ^ 32) / (10 ^ 4) => 4,294,967,296 / (10 ^ 4) => 429,496.7296 with 4 decimals of precision.

If the price of ETH changes to be above 429,496.7296, e.g. 450,000

The decimal multiplier value can be set to 9, the uint32 value would be 450,000 * (10 ^ 12) / (10 ^ 9) => 450,000 * (10 ^ 3)

With this config, ETH prices can have a maximum value of (2 ^ 32) / (10 ^ 3) => 4,294,967,296 / (10 ^ 3) => 4,294,967.296 with 3 decimals of precision.

The mentioned case of requiring different decimals could occur if the min price of ETH is 5000 and the max price of ETH is 450,000, in case that occurs the decimal multiplier value can be set to 9 which may lead to a small amount of precision loss but we feel it is an acceptable outcome.

In case the precision difference required is too large, e.g. 2 decimals or more, we think the oracles should stop signing prices for this case, there could be a need for manual intervention to settle the market as there may be an issue with the price sources in case that occurs.

[IllIllI000]

@xvi10 can you elaborate on why this is disputed rather than no-fix? The last sentence seems to indicate a problem that needs to be worked around

[xvi10]

agree that confirmed and no-fix would be a more accurate label