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

Incorrect State Management Order in Withdrawal Flow Creates Fund Lock Risk

chainsentryaudits

Summary and Impact

The PerpetualVault contract's withdrawal function has a critical state management flaw in its ordering of operations. The contract processes token transfers before updating crucial vault state variables, violating best practices and potentially leading to stuck transactions or locked funds. This directly contradicts a key invariant stated in the protocol documentation: "Withdrawal Locks: No depositor should be able to withdraw their funds before the lockTime period has passed since their deposit."

The issue lies in _handleReturn and _transferToken, where state updates occur after external calls. This ordering means the contract's state doesn't accurately reflect the withdrawal process until after potentially risky external interactions complete.

Vulnerability Details

The core issue manifests in the withdrawal flow's state management:

function _transferToken(uint256 depositId, uint256 amount) internal {
uint256 fee;
if (amount > depositInfo[depositId].amount) {
fee = (amount - depositInfo[depositId].amount) * governanceFee / BASIS_POINTS_DIVISOR;
if (fee > 0) {
collateralToken.safeTransfer(treasury, fee);
}
}
try collateralToken.transfer(depositInfo[depositId].recipient, amount - fee) {}
catch {
collateralToken.transfer(treasury, amount - fee);
emit TokenTranferFailed(depositInfo[depositId].recipient, amount - fee);
}
totalDepositAmount -= depositInfo[depositId].amount; // State update AFTER external call
emit GovernanceFeeCollected(address(collateralToken), fee);
}
​
function _handleReturn(uint256 withdrawn, bool positionClosed, bool refundFee) internal {
(uint256 depositId) = flowData;
uint256 shares = depositInfo[depositId].shares;
uint256 amount;
if (positionClosed) {
amount = collateralToken.balanceOf(address(this)) * shares / totalShares;
} else {
uint256 balanceBeforeWithdrawal = collateralToken.balanceOf(address(this)) - withdrawn;
amount = withdrawn + balanceBeforeWithdrawal * shares / totalShares;
}
if (amount > 0) {
_transferToken(depositId, amount);
}
emit Burned(depositId, depositInfo[depositId].recipient, depositInfo[depositId].shares, amount);
_burn(depositId); // Critical state update happens too late
​
if (refundFee) {
uint256 usedFee = callbackGasLimit * tx.gasprice;
if (depositInfo[depositId].executionFee > usedFee) {
try IGmxProxy(gmxProxy).refundExecutionFee(depositInfo[counter].owner, depositInfo[counter].executionFee - usedFee) {} catch {}
}
}

The flow violates best practices in several ways:

  1. Token transfers occur before state updates

  2. Share burning happens after token transfers

  3. Total deposit amount updates after external interactions

Here's a test demonstrating the issue:

function testStateManagementOrder() public {
// Setup vault with initial state
uint256 depositAmount = 10000;
token.mint(user, depositAmount);
vm.startPrank(user);
token.approve(address(vault), depositAmount);
vault.deposit(depositAmount);
// Record initial states
uint256 initialTotalDeposits = vault.totalDepositAmount();
uint256 initialShares = vault.depositInfo(1).shares;
// Initiate withdrawal
vault.withdraw(user, 1);
// Check state ordering
assertEq(vault.totalDepositAmount(), initialTotalDeposits,
"Total deposits updated before transfer completed");
assertEq(vault.depositInfo(1).shares, initialShares,
"Shares burned before transfer completed");
}

This vulnerability is particularly concerning because:

  1. It violates the protocol's documented invariant about deposit/withdrawal consistency

  2. The codebase shows consistent concerns about proper state management elsewhere (e.g., using enumerableSet for deposits)

  3. The protocol documentation emphasizes the importance of "Total Shares Consistency"

Tools Used

  • Manual Review

  • Foundry

Recommendations

Implement proper state management order following the Checks-Effects-Interactions pattern.

Updates

Lead Judging Commences

n0kto Lead Judge 3 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational or Gas

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.

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.

n0kto Lead Judge 3 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational or Gas

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.

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.