Zaros Part 1
Zaros
DeFiFoundry
60,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Lack of slippage protection in `withdrawMargin` function

kwakudr

Summary

Lack of slippage protection in withdrawMargin function

Vulnerability Details

/// @notice Withdraws available margin collateral from the given trading account.
/// @param tradingAccountId The trading account id.
/// @param collateralType The margin collateral address.
/// @param amount The UD60x18 amount of margin collateral to withdraw.
function withdrawMargin(uint128 tradingAccountId, address collateralType, uint256 amount) external {
// fetch storage slot for this collateral's config config
MarginCollateralConfiguration.Data storage marginCollateralConfiguration =
MarginCollateralConfiguration.load(collateralType);
​
​
// load existing trading account; reverts for non-existent account
// enforces `msg.sender == owner` so only account owner can withdraw
TradingAccount.Data storage tradingAccount =
TradingAccount.loadExistingAccountAndVerifySender(tradingAccountId);
​
​
// convert uint256 -> UD60x18; scales input amount to 18 decimals
UD60x18 amountX18 = marginCollateralConfiguration.convertTokenAmountToUd60x18(amount);
​
​
// enforce converted amount > 0
_requireAmountNotZero(amountX18);
​
​
// enforces that user has deposited enough collateral of this type to withdraw
_requireEnoughMarginCollateral(tradingAccount, collateralType, amountX18);
​
​
// deduct amount from trader's collateral balance
tradingAccount.withdraw(collateralType, amountX18);
​
​
// load account required initial margin requirement & unrealized USD profit/loss
// ignores "required maintenance margin" output parameter
(UD60x18 requiredInitialMarginUsdX18,, SD59x18 accountTotalUnrealizedPnlUsdX18) =
tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);
​
​
// get trader's margin balance
SD59x18 marginBalanceUsdX18 = tradingAccount.getMarginBalanceUsd(accountTotalUnrealizedPnlUsdX18);
​
​
// check against initial margin requirement as initial margin > maintenance margin
// hence prevent the user from withdrawing all the way to the maintenance margin
// so that they couldn't be liquidated very soon afterwards if their position
// goes against them even a little bit
tradingAccount.validateMarginRequirement(requiredInitialMarginUsdX18, marginBalanceUsdX18, UD60x18_ZERO);
​
​
// finally send the tokens
IERC20(collateralType).safeTransfer(msg.sender, amount);
​
​
emit LogWithdrawMargin(msg.sender, tradingAccountId, collateralType, amount);
}

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/TradingAccountBranch.sol#L354C5-L398C6

Suppose a user has deposited 100 USDC as collateral, and the current price of ETH is $2,000.

  1. The user initiates a withdrawal of 50 USDC worth of ETH.

  2. At the time of initiation, this would be equivalent to 0.025 ETH (50 / 2000).

  3. The user submits the transaction to withdraw 0.025 ETH.

  4. However, due to network congestion, the transaction doesn't get processed immediately.

  5. In the meantime, the price of ETH suddenly drops to $1,800.

  6. When the transaction is finally processed, 0.025 ETH is now worth only $45 USDC.

In this scenario, the user has effectively lost $5 worth of value due to slippage.

The current withdrawMargin function doesn't protect against this scenario. It simply withdraws the amount specified without any checks on the current value of the collateral.

Impact

Users will lose their money due to lack of slippage protection

Tools Used

Manual Review

Recommendations

Add minAmountOut and deadline parameters. Also ensure transaction has not expired.

Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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