Liquidity Management
Gamma
DeFiFoundry
50,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Unbounded Price Deviation Threshold Enables Price Validation Bypass

tiannah

Description

The setThreshold function in the KeeperProxy contract allows setting price deviation thresholds without proper bounds or validation. This could enable price manipulation by allowing transactions to pass with highly deviated prices, potentially leading to significant financial losses.

Vulnerability Details

The setThreshold function allows the owner to set the acceptable price deviation threshold for each token:

function setThreshold(address token, uint256 _threshold) external onlyOwner {
require(_threshold > 0, "zero value");
priceDiffThreshold[token] = _threshold;
}

This threshold is used in the _check function to validate prices:

require(
_absDiff(price, chainLinkPrice.toUint256()) * BPS / chainLinkPrice.toUint256() < priceDiffThreshold[token],
"price offset too big"
);

Key vulnerabilities:

  1. No upper bound on threshold values

    • Only checks that threshold > 0

    • Can be set to extremely high values (e.g., uint256.max)

    • No reasonable maximum deviation limit

  2. Missing token validation

    • No verification if token exists in system

    • No check for associated price feed

    • Allows setting thresholds for invalid tokens

  3. No event emission for tracking changes

    • Critical parameter changes aren't logged

    • Reduces transparency and auditability

Impact

  1. Price Manipulation

    • Setting an extremely high threshold would allow almost any price to pass validation

    • Example: Setting the threshold to 1,000,000 (10,000%) would accept prices that deviate by 100x from Chainlink

    • Malicious keepers could execute trades at manipulated prices

  2. Financial Losses

    • Users could have positions liquidated at unfair prices

    • Orders could be executed at extremely deviated prices

    • System's price validation becomes effectively useless

  3. Governance Risk

    • No transparency in threshold changes

    • Difficult to monitor or alert on dangerous threshold values

    • Accidental misconfigurations harder to detect

Tools Used

  • Manual code review

  • Foundry testing framework

Recommendations

Here's the direct and necessary mitigation for the threshold vulnerability:

// Add event
event ThresholdUpdated(address indexed token, uint256 oldThreshold, uint256 newThreshold);
​
// Add constant for max allowed deviation
uint256 public constant MAX_THRESHOLD = 1000; // 10% max deviation (since BPS is 10_000)
​
function setThreshold(address token, uint256 _threshold) external onlyOwner {
require(token != address(0), "Invalid token");
require(dataFeed[token] != address(0), "Token has no price feed");
require(_threshold > 0 && _threshold <= MAX_THRESHOLD, "Invalid threshold");
uint256 oldThreshold = priceDiffThreshold[token];
priceDiffThreshold[token] = _threshold;
emit ThresholdUpdated(token, oldThreshold, _threshold);
}

This fixes exactly what's needed:

  • Upper bound on threshold (MAX_THRESHOLD) to prevent excessive price deviations

  • Token validation to ensure the threshold is only set for tokens with price feeds

  • Event emission for transparency and monitoring

  • Clear error messages for each validation

Updates

Lead Judging Commences

n0kto Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Admin is trusted / Malicious keepers

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."

n0kto Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Admin is trusted / Malicious keepers

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."

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.