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

Asymmetry and inconsistent use of metadata in runNextAction can mix up and use the wrong information used for dexSwap and gmx

fresh

Summary

runNextActionis a function called by the keeper, and relies on the keeper to provide the input parameters that are used throughout the function.

The issue is how the metadatathat is passed in, is handled. It is used inconsistently, especially for its data regarding dexSwap and gmx.

The inconsistent use can lead to wrong information being used for actions that involve swapping on dexand gmxand creating positions with gmx.

Vulnerability Details

metadatais swap data that is passed in to runNextAction by the keeper and it can include Only Paraswap dexdata, gmxdata, or both. But if both are included, the first element should always be Paraswap dex. This is stated and verified in _runSwap: which is called throughout runNextAction

/**
* @dev Swap data is an array of swap data and is passed from an off-chain script.
* It could contain only Paraswap data, GMX swap data, or both of them.
* If both are included, the first element of the array should always be Paraswap.
* @param metadata Bytes array that includes swap data.
* @param isCollateralToIndex Direction of swap. If true, swap `collateralToken` to `indexToken`.
* @return completed If true, the swap action is completed; if false, the swap action will continue.
*/
function _runSwap(bytes[] memory metadata, bool isCollateralToIndex, MarketPrices memory prices) internal returns (bool completed) {

Lets look at runNextActionto see how metadatais used initially. For the scenario of 1x long, it calls runSwapand passes in the metadatathat it receieved from the keeper:

function runNextAction(MarketPrices memory prices, bytes[] memory metadata) external nonReentrant gmxLock {
_onlyKeeper();
Action memory _nextAction = nextAction;
delete nextAction;
if (_nextAction.selector == NextActionSelector.INCREASE_ACTION) {
(bool _isLong) = abi.decode(_nextAction.data, (bool));
​
if (_isLongOneLeverage(_isLong)) {
-> _runSwap(metadata, true, prices);

_runSwapthen proceeds, depending on if the metadataincludes both elements dexand gmxor just 1.

In the case it includes BOTH it enforces that the first element is dex. And if only 1 element is sent in, it checks if the first element is dex. Thorughout, it expects and operates off of metadata[0]being the dexelement.

It also reverts, if the first element is not dex

function _runSwap(bytes[] memory metadata, bool isCollateralToIndex, MarketPrices memory prices) internal returns (bool completed) {
if (metadata.length == 0) {
revert Error.InvalidData();
}
if (metadata.length == 2) {
-> (PROTOCOL _protocol, bytes memory data) = abi.decode(metadata[0], (PROTOCOL, bytes));
-> if (_protocol != PROTOCOL.DEX) {
revert Error.InvalidData();
}
swapProgressData.swapped = swapProgressData.swapped + _doDexSwap(data, isCollateralToIndex);
(_protocol, data) = abi.decode(metadata[1], (PROTOCOL, bytes));
if (_protocol != PROTOCOL.GMX) {
revert Error.InvalidData();
}
​
_doGmxSwap(data, isCollateralToIndex);
return false;
} else {
if (metadata.length != 1) {
revert Error.InvalidData();
}
-> (PROTOCOL _protocol, bytes memory data) = abi.decode(metadata[0], (PROTOCOL, bytes));
-> if (_protocol == PROTOCOL.DEX) {
uint256 outputAmount = _doDexSwap(data, isCollateralToIndex);

With the handling and expectation of the first element of metadatabeing dex.

runNextActioninconsistently uses metadatain a different way in other operations, within the same function, which was just expected to have metadatathat has the first element being dexand reverting if not.

lets examine the next line of code after the runSwap just seen.

function runNextAction(MarketPrices memory prices, bytes[] memory metadata) external nonReentrant gmxLock {
​
if (_nextAction.selector == NextActionSelector.INCREASE_ACTION) {
(bool _isLong) = abi.decode(_nextAction.data, (bool));
​
if (_isLongOneLeverage(_isLong)) {
_runSwap(metadata, true, prices);
-> } else {
// swap indexToken that could be generated from the last action into collateralToken
// use only DexSwap
if (
IERC20(indexToken).balanceOf(address(this)) * prices.indexTokenPrice.min >= ONE_USD
) {
-> (, bytes memory data) = abi.decode(metadata[1], (PROTOCOL, bytes));
_doDexSwap(data, false);
}
-> (uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
_createIncreasePosition(_isLong, acceptablePrice, prices);

This now uses metadata[1]for dexSwap-> which we just saw should have the data for dexat position metadata[0]- the first element of the metadataarray.

Also, it uses acceptablePricefrom metadata[0]- but it should be using gmxdata at metadata[1]-> because that is used for createIncreasePosition-> which interacts with gmxand never with a dexSwap.

The same misuse of metadatais used in runNextActionwhen NextActionSelector.WITHDRAW:

_withdrawuses metadatato retreive acceptablePricesame as above.

else if (_nextAction.selector == NextActionSelector.WITHDRAW_ACTION) {
// swap indexToken that could be generated from settle action or liquidation/ADL into collateralToken
// use only DexSwap
if (
IERC20(indexToken).balanceOf(address(this)) * prices.indexTokenPrice.min >= ONE_USD
) {
-> (, bytes memory data) = abi.decode(metadata[1], (PROTOCOL, bytes));
-> _doDexSwap(data, false);
}
uint256 depositId = flowData;
_withdraw(depositId, metadata[0], prices);

BUT, In the very same function, the very next scenario NextActionSelector.SWAP_ACTION

It calls _runSwapwith the same metadata passed in, but this uses the metadatain the correct way / expected way. It will use metadata[0]for dexSwapand metadata[1]for gmx

_runSwapexample was used above, this is just showing that within the same function runNextAction-> the exact same metadatais used inconsistently and asymmetrically.

else if (_nextAction.selector == NextActionSelector.SWAP_ACTION) {
(, bool isCollateralToIndex) = abi.decode(
_nextAction.data,
(uint256, bool)
);
-> _runSwap(metadata, isCollateralToIndex, prices);

Impact

The actions for swapping directly in runNExtActionwill use the wrong data for swaps on dexSwap-> it will use the data that is representing gmx.

When calling createIncreasePositionand withdraw, it will pass in data that is for dexSwap, when those functions interact only with gmx and are expecting the data representing gmx-> metadata[1]

This can lead to wrong data being used for swaps and order creations on gmx, which can have unintended consequences.

Tools Used

Manual Review

Recommendations

Follow a consistent use of metadata-> the metadataelements array does not change throughout the function runNExtSwap-> it is always the same as is when passed in.

I suggest following the expectation of the order of elements as described in _runSwapas the protocol is expecting and checks for the correct order of elements, and reverts if not.

Updates

Lead Judging Commences

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

Give us feedback!