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

COMPOUND action can be frontrun unintentionally/maliciously

vinica_boy

Summary

runNextAction in PerpetualVault is triggered by keepers to continue the current workflow and the next action is decided based on nextAction.selector. In case there is not nextAction.selector saved, the function will execute the COMPOUND workflow which aims to utilize collateralToken tokens and swap them to indexToken or use them as a collateral to long/short indexToken.

This design has a problem when keeper want to run the COMPOUND workflow. There is not any guarantee that the system wont go into DEPOSIT flow between the time of submitting the keeper's transaction and the time it is executed. On Arbitrum, the chances are really low that it can happen unintentionally, but in Avalanche where mempool is not completely private (stakers can see mempool and do MEV), it is possible that COMPOUND workflow is maliciously or unintentionally frontrun.

Vulnerability Details

Taking a look into how DEPOSIT and COMPOUND flows are handled, we can see that they accept the same parameters and do the same actions. The problem is that if parameters passed are intended for COMPOUND workflow, they will be used for DEPOSIT.

DEPOSIT:

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);
}
}

COMPOUND:

else if (positionIsClosed == false && _isFundIdle()) {
flow = FLOW.COMPOUND;
if (_isLongOneLeverage(beenLong)) {
_runSwap(metadata, true, prices);
} else {
(uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
_createIncreasePosition(beenLong, acceptablePrice, prices);
}
}

For cases different than 1x Leverage Long, the main difference would be acceptablePrice. Not sure how it is decided off-chain and how users agree with that price, but if price for COMPOUND operation is usually set to lower value than in DEPOSIT, user will suffer from this slippage.

For the 1x Leverage Long case, it can have bigger consequences since passed metadata will be used to execute the swap operation and result from swap will be used to determine the amount of shares user will get.

Consider the following example:

  • There is 1000 idle tokens in PerpetualVault and keeper try to run COMPOUND operation. For this, metadata is constructed to swap those 1000 tokens.

  • User tries to deposit 100 tokens and turn the protocol flow into DEPOSIT.

  • Now, DEPOSIT flow will be executed with metadata to swap 1000 tokens and result of DEX swap is used to calculate user's shares allocation.

  • This will result in shares equal to depositing 1000 tokens instead of shares equal to actual deposit.

Taking a look into _runSwap() to verify scenario above:

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);
// update global state
if (flow == FLOW.DEPOSIT) {
// last `depositId` equals with `counter` because another deposit is not allowed before previous deposit is completely processed
_mint(counter, outputAmount + swapProgressData.swapped, true, prices);
} else if (flow == FLOW.WITHDRAW) {
_handleReturn(outputAmount + swapProgressData.swapped, false, true);
} else {
// in the flow of SIGNAL_CHANGE, if `isCollateralToIndex` is true, it is opening position, or closing position
_updateState(!isCollateralToIndex, isCollateralToIndex);
}
return true;
} else {
_doGmxSwap(data, isCollateralToIndex);
return false;
}
}
}
function _doDexSwap(bytes memory data, bool isCollateralToIndex) internal returns (uint256 outputAmount) {
(address to, uint256 amount, bytes memory callData) = abi.decode(data, (address, uint256, bytes));
IERC20 inputToken;
IERC20 outputToken;
if (isCollateralToIndex) {
inputToken = collateralToken;
outputToken = IERC20(indexToken);
} else {
inputToken = IERC20(indexToken);
outputToken = collateralToken;
}
uint256 balBefore = outputToken.balanceOf(address(this));
ParaSwapUtils.swap(to, callData);
outputAmount = IERC20(outputToken).balanceOf(address(this)) - balBefore;
emit DexSwap(address(inputToken), amount, address(outputToken), outputAmount, isCollateralToIndex);
}

Impact

Unexpected behavior.
Possible incorrect calculations for user deposits.

Tools Used

Manual review.

Recommendations

Consider adding an explicit way to start COMPOUND operation. For example as a parameter of runNextAction().

Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Appeal created

vinica_boy Submitter
9 months ago
n0kto Lead Judge
9 months ago
n0kto Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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

Give us feedback!