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

_runSwap() is missing Global State Update in Deposit Flow

leogold

Summary

During the processing of a deposit swap in the _runSwap function, when flow == FLOW.DEPOSIT, the contract calls _mint to update state and mint tokens. However, the subsequent global state reset—which is expected to occur via a call to the _finalize function—is missing. This omission results in stale global state variables (swapProgressData, flowData, and flow) remaining unchanged, potentially leading to unintended behavior in later operations.

Vulnerability Details

  • Location:
    The issue is located in the _runSwap function, specifically in the branch handling deposit flows.

  • Code Context:
    In _runSwap, when the contract detects a single swap metadata element and the protocol is PROTOCOL.DEX, it processes the swap as follows:

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

The expected behavior is that after a deposit swap, the contract should reset global state variables. The _finalize function, defined as follows, is responsible for this reset:

function _finalize(bytes memory data) internal {
if (flow == FLOW.WITHDRAW) {
(uint256 prevCollateralBalance, bool positionClosed, bool refundFee) = abi.decode(data, (uint256, bool, bool));
uint256 withdrawn = collateralToken.balanceOf(address(this)) - prevCollateralBalance;
_handleReturn(withdrawn, positionClosed, refundFee);
} else {
delete swapProgressData;
delete flowData;
delete flow;
}
}

  • Issue:
    In the deposit flow (flow == FLOW.DEPOSIT), the contract does not invoke _finalize after calling _mint, which results in the global state (i.e., swapProgressData, flowData, and flow) remaining unchanged. This oversight may allow outdated state data to persist, adversely affecting subsequent operations.

Impact

The failure to reset the global state can lead to inconsistencies, where residual data from previous deposit swaps may interfere with new transactions like deposit and withdraw function which are not functional until flow is reset. This might unintended fund management behaviors as the transaction returns true as it is expected to be complete.

POC

function test_flow_not_reset_When_Using_Paraswap_Only() external {
address keeper = PerpetualVault(vault).keeper();
address alice = makeAddr("alice");
depositFixture(alice, 1e10);
​
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);
​
IERC20 collateralToken = PerpetualVault(vault).collateralToken();
uint256 amount = 1e10;
deal(address(collateralToken), alice, amount);
deal(alice, 1e18);
uint256 executionFee = PerpetualVault(vault).getExecutionGasLimit(true);
vm.startPrank(alice);
collateralToken.approve(vault, amount);
PerpetualVault(vault).deposit{value: executionFee * tx.gasprice}(amount);
vm.stopPrank();
​
prices = mockData.getMarketPrices();
uint256 ethBalBefore = alice.balance;
vm.prank(keeper);
PerpetualVault(vault).runNextAction(prices, swapData);
assertTrue(ethBalBefore < alice.balance);
// assert flow is not reset
assertNotEq(uint8(PerpetualVault(vault).flow()), 0);
//assert it is still in deposit
assertEq(uint8(PerpetualVault(vault).flow()), 1);
​
}
​

add the above test to perpetualvault.t.sol and run with forge test --mt test_flow_not_reset_When_Using_Paraswap_Only --mp test/PerpetualVault.t.sol --rpc-url arbitrum -vvvvv
the traces below is the traces of the 3 asserts done at the end of the test case

├─ [0] VM::assertTrue(true) [staticcall]
│ └─ ← [Return]
├─ [1065] TransparentUpgradeableProxy::fallback() [staticcall]
│ ├─ [440] PerpetualVault::flow() [delegatecall]
│ │ └─ ← [Return] 1
│ └─ ← [Return] 1
├─ [0] VM::assertNotEq(1, 0) [staticcall]
│ └─ ← [Return]
├─ [1065] TransparentUpgradeableProxy::fallback() [staticcall]
│ ├─ [440] PerpetualVault::flow() [delegatecall]
│ │ └─ ← [Return] 1
│ └─ ← [Return] 1
├─ [0] VM::assertEq(1, 1) [staticcall]
│ └─ ← [Return]
├─ storage changes:
│ @ 151: 2 → 1
│ @ 0xc72d342fadcf766933e0a8af1975f06057ddbde275343c23eafc8b1eef88a140: 0 → 0x00000000000000000000000000000000000000000000000000000002540be400
│ @ 0xa26c2c1578d512bc12d7d9d1e5de86f2cd82e9396c1762eeaaed50c8da249024: 0 → 1
│ @ 0xa2670d1138393cceca9d84066a3fd71bb3564b80db298bd8e6d1f3427163055f: 0 → 0x00000000000000000000000000000000000000000000000000000002540be400
│ @ 0xa26c2c1578d512bc12d7d9d1e5de86f2cd82e9396c1762eeaaed50c8da249021: 0 → 0xc72d342fadcf766933e0a8af1975f06057ddbde275343c23eafc8b1eef88a140
└─ ← [Stop]

Tools Used

manual review / foundry

Recommendations

Modify the _runSwap() function to ensure that, after handling a deposit flow, the contract calls _finalize(). For example, immediately after the _mint function call, invoke _finalize() with the appropriate parameters to clear the global state as stated in the natspec:

if (flow == FLOW.DEPOSIT) {
_mint(counter, outputAmount + swapProgressData.swapped, true, prices);
+ _finalize(hex'');
}
Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding_deposit_1x_long_dex_positionIsOpened_DoS_Flow

Likelihood: Medium/High, - Leverage = 1x - beenLong = True - positionIsClosed = False - Metadata → 1 length and Dex Swap Impact: Medium/High, DoS on any new action before the admin uses setVaultState Since this seems to be the most probable path for a 1x PerpVault, this one deserves a High.

Support

FAQs

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

Give us feedback!