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

Unregistered Tokens Cause `_validatePrice()` to Revert

cipherhawk

In the KeeperProxy contract

Summary:

In the current design, any newly added token or uninitialized dataFeed[token] will cause _check(token, price) to revert, blocking the entire keeper flow. Adding a require-check or forcibly registering data feeds for tokens is essential to avoid a system-wide Denial of Service whenever a new token is introduced or the feed is missing.

RootCause & Where It Happens

In _validatePrice(), the contract calls _check(...) for each of the market’s three tokens (index, long, and short). For each _check(token, price):

function _check(address token, uint256 price) internal view {
// This aggregator call reverts if dataFeed[token] == address(0)
(, int chainLinkPrice, , uint256 updatedAt, ) = AggregatorV2V3Interface(dataFeed[token]).latestRoundData();
...
}
​

there isn't **any kind of **safeguard that dataFeed[token] is nonzero. If dataFeed[token] was never set for that token, this aggregator call uses address(0) as the feed address and will revert or fail. This halts the entire keeper function (run, runNextAction, etc.) and blocks any actions for that vault.

Impact

  1. Denial of Service:

    • If a new token is introduced to the vault’s market (for example, a new short token) without updating dataFeed[newToken], all keeper calls revert in _validatePrice().

    • No user can open/close positions or proceed with next actions, effectively freezing the vault.

  2. Operational Breakage:

    • The system relies on this keeper for essential operations. Even a short lapse in updating the data feed can temporarily block all actions.

  3. Confusion / Missed Setup:

    • If a deployment script or admin forgets to set the aggregator for any of the market’s tokens, the vault’s keeper flow is bricked. This could happen in upgrades or expansions of the market set.

Proof of Concept

  1. Suppose the vault’s market changes its shortToken from TokenA to TokenB.

  2. The admin forgets to call setDataFeed(TokenB, aggregatorForTokenB, ...).

  3. Now dataFeed[TokenB] is still address(0).

  4. Keeper tries _check(TokenB, price), which does:

    AggregatorV2V3Interface(dataFeed[TokenB]).latestRoundData();
    // i.e., aggregator(0x000...).latestRoundData() => revert
    ​

  5. The entire call to run() or runNextAction() reverts, locking the system until the feed is correctly set.

Recommended Fix

  1. Ensure dataFeed[token] Is Nonzero

    • In _check(...), add:

      require(dataFeed[token] != address(0), "Missing aggregator for token");
      ​

    • This produces a clear revert message if a feed is not registered rather than a cryptic “call to zero address.”

  2. Require Setup During Token Changes

    • The protocol can forcibly set new aggregator addresses whenever new tokens are introduced. If the token aggregator is not set, the transaction or configuration update for the new token should revert.

  3. Document the Onboarding Process

    • The admin or deployment script must always call setDataFeed(token, aggregator, maxTimeWindow, threshold) before the keeper can handle that token.

Updates

Lead Judging Commences

n0kto Lead Judge 3 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."

Suppositions

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.

n0kto Lead Judge 3 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."

Suppositions

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.

Support

FAQs

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