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

CancelFlow does not update the totalShare in case of Deposit Flow

TheKhans

Summary

CancelFlow ia added as a glass breaking function which will help to revert the current flow in case of any accident from Gamma side or GMX side , how ever in case of deposit flow it sent the collateral back and delete the user record but does not update the totalShare.

Vulnerability Details

The vault can enter a state where it cannot proceed to the next flow and may become stuck due to an accident. To handle such scenarios, the Gamma team has implemented the cancelFlow function as a glass-breaking mechanism. This function allows the team to reset the flow and restore the vault to a normal state. If the flow is in a deposit state, cancelFlow will return the collateral tokens to the user and reset the vault's state.

/contracts/PerpetualVault.sol:1220
1220: function _cancelFlow() internal {
1221: if (flow == FLOW.DEPOSIT) {
1222: uint256 depositId = counter;
1223: collateralToken.safeTransfer(depositInfo[depositId].owner, depositInfo[depositId].amount);
1224: totalDepositAmount = totalDepositAmount - depositInfo[depositId].amount;
1225: EnumerableSet.remove(userDeposits[depositInfo[depositId].owner], depositId);
1226: try IGmxProxy(gmxProxy).refundExecutionFee(
1227: depositInfo[counter].owner,
1228: depositInfo[counter].executionFee
1229: ) {} catch {}
1230: delete depositInfo[depositId];

From the above code, it can be observed that cancelFlow sends back the collateralToken, updates totalDepositAmount, refunds the fee, and deletes the depositInfo. However, there is an edge case where virtual shares are minted to the user, but totalShare is not updated accordingly. This issue can lead to incorrect share calculations for other users, ultimately impacting withdrawals.

POC

Add following test case to PerpetualVault.t.sol and run with command forge test --mt test_CancelFlow_Deposit_Case -vvv --rpc-url arbitrum:

function test_CancelFlow_Deposit_Case() external {
IERC20 collateralToken = PerpetualVault(vault).collateralToken();
address alice = makeAddr("alice");
payable(alice).transfer(10 ether);
vm.startPrank(address(alice));
deal(address(collateralToken), address(vault), 1e10);
uint256 beforeAnyDeposit = PerpetualVault(vault).totalShares();
assertEq(beforeAnyDeposit , 0);
depositFixture(alice, 1e10);
address keeper = PerpetualVault(vault).keeper();
MarketPrices memory prices = mockData.getMarketPrices();
​
bytes[] memory data = new bytes[](1);
data[0] = abi.encode(3380000000000000);
vm.prank(keeper);
PerpetualVault(vault).runNextAction(prices, data);
GmxOrderExecuted(true);
// @audit : assume some thing by accident happen here and we need to cancel the flow
vm.prank(keeper);
PerpetualVault(vault).cancelFlow();
​
uint afterCancelFlowTotalShares = PerpetualVault(vault).totalShares();
assertGt(afterCancelFlowTotalShares , 0);
}

For this test case we need that the positionIsClosed = false; so inside PerpetualVault::initialize() function.

Impact

Failing to update totalShare affects how users withdraw their funds. Since withdrawals rely on dividing shares by totalShare, users will consistently receive less than intended.

Tools Used

Manual Review

Recommendations

In case of deposit flow also check subtract the user shares from totalShare.

diff --git a/contracts/PerpetualVault.sol b/contracts/PerpetualVault.sol
index ed4fba2..441a03d 100644
--- a/contracts/PerpetualVault.sol
+++ b/contracts/PerpetualVault.sol
@@ -1222,6 +1222,7 @@ contract PerpetualVault is IPerpetualVault, Initializable, Ownable2StepUpgradeab
uint256 depositId = counter;
collateralToken.safeTransfer(depositInfo[depositId].owner, depositInfo[depositId].amount);
totalDepositAmount = totalDepositAmount - depositInfo[depositId].amount;
+ totalShares = totalShares - depositInfo[depositId].shares;
EnumerableSet.remove(userDeposits[depositInfo[depositId].owner], depositId);
Updates

Lead Judging Commences

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

finding_cancelFlow_can_be_called_after_order_execution_leading_to_disturb_shares_and_refund_too_many_fees

Likelihood: None/Very Low, when the keeper call cancelFlow after an order execution Impact: High, Inflation/deflation of total shares, and too many fees refunded.

Support

FAQs

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