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

`totalShares` accounted improperly when canceling flow

0xjoyboy03

Summary

when the current position is long and the position is open, canceling any deposit leads to unhandling the totalShareswhich leads to breaking the invariant.

Vulnerability Details

the invariant Total Shares Consistency: The sum of all individual depositor shares must always equal the totalShares variable in the contract.breaks when the beenLong is true (with 1x leverage) and the positionIsClosedfalse.

In this state, when the user deposits to the PerpetualVault.sol, the next action is to register the requested position parameters. Then when the keeper calls runNextAction(MarketPrices memory prices, bytes[] memory metadata)

with the metadata.length == 1, the _mintmethod in the _runSwap() will be called and associated shares will be minted for the corresponding user. if the keeper decides to cancel the flow, the associated depositInfo[counter]will be deleted and the depositInfo[counter].sharesgets 0 but the totalShares won't be accounted which leads to breaking the main invariant.

the prover from certora catched this bug actually through this invariant (Total Shares Consistency):

// report of this job for cancelFlow() method: https://prover.certora.com/output/604718/f532816636ba42ea81f4ab3a597fa04f?anonymousKey=85834c6e9ebdec7254ae6fc2aa1e78c875e1fc70
invariant sumSupplySharesCorrect(env e) to_mathint(totalShares()) == sumSupplyShares;

here is the POC: (we used setVaultStatesmethod for declaring our scenario simply)

// run: forge test --mt test_totalShares_UnConsistency_when_cancelFlow --via-ir --rpc-url arbitrum
function test_totalShares_UnConsistency_when_cancelFlow() external {
address keeper = PerpetualVault(vault).keeper();
address alice = makeAddr("alice");
payable(alice).transfer(1 ether);
address owner = PerpetualVault(vault).owner();
IERC20 collateralToken = PerpetualVault(vault).collateralToken();
deal(address(collateralToken), address(PerpetualVault(vault)), 1e18);
​
(PerpetualVault.NextActionSelector selector, bytes memory data) = PerpetualVault(vault).nextAction();
​
vm.prank(owner);
PerpetualVault(vault).setVaultState(PerpetualVault.FLOW.NONE, 0, true, bytes32(0), false, false, PerpetualVault.Action(selector, data));
​
depositFixture(alice, 1e10);
​
MarketPrices memory prices = mockData.getMarketPrices();
bytes[] memory metadata = new bytes[](1);
bytes memory paraSwapData = mockData.getParaSwapData(vault);
metadata[0] = abi.encode(PROTOCOL.DEX, paraSwapData);
vm.prank(keeper);
PerpetualVault(vault).runNextAction(prices, metadata);
​
vm.prank(keeper);
PerpetualVault(vault).cancelFlow();
​
(,uint shares,,,,) = PerpetualVault(vault).depositInfo(1);
assertTrue(PerpetualVault(vault).totalShares() != shares);
}

impact

unhandling the totalShares leads to breaking the main invariant of the protocol

Tools Used

foundry, certora prover

Recommendations

you must handle the totalShares in the _cancelFlowmethod

​
function _cancelFlow() internal {
if (flow == FLOW.DEPOSIT) {
uint256 depositId = counter;
collateralToken.safeTransfer(depositInfo[depositId].owner, depositInfo[depositId].amount);
totalDepositAmount = totalDepositAmount - depositInfo[depositId].amount;
EnumerableSet.remove(userDeposits[depositInfo[depositId].owner], depositId);
try IGmxProxy(gmxProxy).refundExecutionFee(
depositInfo[counter].owner,
depositInfo[counter].executionFee
) {} catch {}
+ totalShares = totalShares - depositInfo[depositId].shares;
delete depositInfo[depositId];
} else if (flow == FLOW.WITHDRAW) {
try IGmxProxy(gmxProxy).refundExecutionFee(
depositInfo[counter].owner,
depositInfo[counter].executionFee
) {} catch {}
}
Updates

Lead Judging Commences

n0kto Lead Judge 8 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.