Adversary can siphon funds from JuniorVault by sandwiching their own deposits and withdraws

Summary

Each time a user enters the JuniorVault a hedge is automatically opened against their newly added collateral. This causes WBTC/WETH to be borrowed on aave and sold on UNI V3. This trade allows a certain level of slippage to occur, which the depositor can MEV by sandwiching the deposit. The slippage loss from this trade is socialized across the entire vault, meaning that the attacker can profit from this MEV. On withdraw the a portion of the hedge is automatically closed, which can again be sandwiched to extract value. The adversary will have to pay a withdrawal fee but the current withdrawal fee doesn't make the attack unprofitable.

Vulnerability Detail

function beforeWithdraw(
    uint256 assets,
    uint256,
    address
) internal override {
    (uint256 currentBtc, uint256 currentEth) = state.getCurrentBorrows();

    //rebalance of hedge based on assets after withdraw (before withdraw assets - withdrawn assets)
    state.rebalanceHedge(currentBtc, currentEth, totalAssets() - assets, false);
}

function afterDeposit(
    uint256,
    uint256,
    address
) internal override {
    if (totalAssets() > state.depositCap) revert DepositCapExceeded();
    (uint256 currentBtc, uint256 currentEth) = state.getCurrentBorrows();

    //rebalance of hedge based on assets after deposit (after deposit assets)
    state.rebalanceHedge(currentBtc, currentEth, totalAssets(), false);
}

During withdraws and deposits, the hedge is rebalanced to hedge the new amount of assets in the vault. Currently the vault tries to hedge 20% WBTC and 30% WETH, which means that the vault borrows a total of 50% of the value of the deposit/withdraw. The 50% borrow is traded on UNI V3. Currently slippage allows for 1%, allowing the depositor to sandwich this value which is equivalent to 0.5%. The number of shares granted to the depositor is determined before the rebalance happens so the losses are socialized across the entire vault. On withdraw the number of assets received by the user is determined before the rebalance happens. Like before the 1% slippage value can be extracted. Since the assets are determined before the rebalance, the user will receive their entire deposit less the withdraw fee, regardless of the value lost during the withdraw. Currently the slippage has been set to 1% (100 BPS) and withdraw fee has been set to 0.5% (50 bps). With those values, it is profitable to attack the vault.

Example:

Assume a vault with $90,000 worth of assets and 90,000 shares. An adversary deposits $10,000 causing the vault to open a hedge of $5,000. Currently slippage is limited to 1%, which the attacker can siphon off by sandwiching. This means the attacker can steal $50, increasing the value of the assets to $99,950 ($90,000 + $10,000 - $50). Since their shares were calculated before the deposit they now have 10,000 shares, which translates to $9,995. When the attacker withdraws they will receive $9,945 ($9,995 - $50) after the withdraw fee and the vault will close $5,000 of hedge, of which the attacker can again steal $50 (1%) with a sandwich. After the withdraw the adversary now has $10,045 ($9,945 + $50 + $50). The adversary can repeat this tactic as many times as they wish to drain a large amount of value from the JuniorVault.

Impact

Attacker can repeatedly deposit and withdraw, sandwiching the rebalances to profit at the expense of other users.

Code Snippet

https://github.com/sherlock-audit/2022-10-rage-trade/blob/main/dn-gmx-vaults/contracts/libraries/DnGmxJuniorVaultManager.sol#L353-L458

Tool used

Manual Review

Recommendation

The simplest solution would be to raise the withdraw fee to 1%. Alternatively, instead of charging a withdraw fee the contract could adjust the number of shares received from the withdraw/deposit to reflect the change in totalAssets before and after.

sherlock-audit / 2022-10-rage-trade-judging

0x52 - Adversary can siphon funds from JuniorVault by sandwiching their own deposits and withdraws #65