#decimal-issue #price-validation #interface-mismatch
Noting: The prior issue #145 (about latestAnswer
via LightChaser) does not overshadow or preclude this finding, because they address two distinct problems in the contract’s approach to handling Chainlink token data.
The _check
function depends on the token's decimals() implementation to correctly scale price values. Tokens that do not conform to the ERC20 standard or return manipulated decimals can disrupt the price validation process, causing either perpetual reverts or incorrect price comparisons, and ultimately leading to faulty collateral evaluations
The contract expects every token to implement the decimals()
method consistently. This design fails in two scenarios:
Reversion Risk: A non-standard or intentionally malicious token that lacks the decimals()
method causes _check
to revert, halting the entire validation routine and impacting critical vault operations.
Manipulated Decimal Values: A token returning manipulated decimals (e.g., returning 0
or an excessive integer) distorts the calculation of price = price / 10 ** (decimals - 8)
. This distortion breaks the security guarantee of reliable price comparisons against Chainlink data. A malicious token or feed can trivially pass the priceDiffThreshold
check by exploiting incorrect scaling factors.
Impact: Medium. Non-standard tokens or deliberately adversarial implementations disrupt the contract's price validation entirely, either through perpetual reverts or skewed calculations.
Likelihood: Medium. While most reputable tokens follow ERC20
conventions, engaging with any unsupported or malicious token leads to immediate issues in _check
.
Manual Review
This change ensures any token returning a decimals value outside the acceptable range is rejected.
Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point. Keepers are added by the admin, there is no "malicious keeper" and if there is a problem in those keepers, that's out of scope. ReadMe and known issues states: " * System relies heavily on keeper for executing trades * Single keeper point of failure if not properly distributed * Malicious keeper could potentially front-run or delay transactions * Assume that Keeper will always have enough gas to execute transactions. There is a pay execution fee function, but the assumption should be that there's more than enough gas to cover transaction failures, retries, etc * There are two spot swap functionalies: (1) using GMX swap and (2) using Paraswap. We can assume that any swap failure will be retried until success. " " * Heavy dependency on GMX protocol functioning correctly * Owner can update GMX-related addresses * Changes in GMX protocol could impact system operations * We can assume that the GMX keeper won't misbehave, delay, or go offline. " "Issues related to GMX Keepers being DOS'd or losing functionality would be considered invalid."
There is no real proof, concrete root cause, specific impact, or enough details in those submissions. Examples include: "It could happen" without specifying when, "If this impossible case happens," "Unexpected behavior," etc. Make a Proof of Concept (PoC) using external functions and realistic parameters. Do not test only the internal function where you think you found something.
The contest is live. Earn rewards by submitting a finding.
This is your time to appeal against judgements on your submissions.
Appeals are being carefully reviewed by our judges.