Insufficient Validation of Market Parameter in `setSavedCallbackContract` Call
Details:
The function call
lacks proper validation to ensure that the market parameter represents a valid and authorized market. Although a comment indicates that the market should be checked, no actual validation logic is implemented. This omission means that any address, including an incorrect or malicious one, could be passed as the market.
Root Cause:
The vulnerability arises from the absence of input validation on the market parameter before it is used in the setSavedCallbackContract function call. The developer’s note (//@audit-low check if the market is a valid market) was not followed by the necessary checks to confirm the validity of the market address.
Impact:
Unexpected Behavior: An invalid or malicious market address could lead the system to register callbacks incorrectly, causing failures in subsequent callback operations.
Security Risks: If an attacker can manipulate the market input, they may redirect or interfere with the contract’s callback logic, potentially leading to mismanagement of funds or unintended contract behavior.
Operational Disruptions: The absence of validation may lead to integration issues where the callback mechanism fails, disrupting normal operations.
Recommendation:
Implement proper validation for the market parameter before invoking setSavedCallbackContract. Possible checks include:
Ensuring the
marketaddress is not the zero address.Verifying that the
marketaddress conforms to expected market contracts (e.g., via interface checks).Optionally, maintaining a whitelist of authorized market addresses and confirming that the provided
marketexists in the whitelist.
Example of a simple check:
Where isValidMarket is a function that implements further validation logic.
Proof of Concept:
An attacker or misconfigured system could supply an arbitrary address as market. For instance, if a zero address or an address controlled by an attacker is passed:
Without validation, the callback contract is set for an invalid or malicious market. This may result in:
The contract processing callbacks for a market that does not conform to expected behavior.
Potential redirection of funds or execution errors when the callback mechanism is triggered.
By implementing the recommended checks, such risks can be mitigated, ensuring that only valid and authorized market addresses are accepted.
Lead Judging Commences
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."
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."
Prize pool breakdown
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.