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

User can prevent on-chain trades by withdrawing his collateral

SBSecurity

Summary

Traders can use TradingAccountBranch::withdrawMargin to have risk-free trades and bypass the MarketOrder::marketOrderMinLifetime check by cancelling them before being settled if the price is going in the other direction :

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);
}

OrderBranch.sol

function cancelMarketOrder(uint128 tradingAccountId) external {
// load existing trading account; reverts for non-existent account
// enforces `msg.sender == owner` so only account owner can cancel
// pending orders
TradingAccount.loadExistingAccountAndVerifySender(tradingAccountId);
​
// load trader's pending order; reverts if no pending order
MarketOrder.Data storage marketOrder = MarketOrder.loadExisting(tradingAccountId);
​
// reverts if a trader has a pending order and that pending order hasn't
// existed for the minimum order lifetime; pending orders can't be cancelled
// until they have existed for the minimum order lifetime
marketOrder.checkPendingOrder();
​
// reset pending order details
marketOrder.clear();
​
emit LogCancelMarketOrder(msg.sender, tradingAccountId);
}

Vulnerability Details

The easiest way this to be achieved is the traders to have account without any other positions in it. That way they will always be able to withdraw their entire collateral since TradingAccountBranch::withdrawMargin does not perform check whether it has pending order, which is the core of the issue. For example user wants to open long position in ETH/USD market and in time of creation price is $1000, there is some time that OrderKeeper doesn’t pick this order and price increases to $1200, this situation eats from the PnL of the user, or more critically the collateral type (which is any of the assets here) is volatile and it’s price falls which will decrease the margin of the trader. Seeing that the owner can call TradingAccountBranch::withdrawMargin taking his entire collateral balance and then in SettlementBranch::_fillPrice transaction will revert and the Keeper will not execute it again:

TradingAccountBranch.sol

function _fillOrder(
uint128 tradingAccountId,
uint128 marketId,
uint128 settlementConfigurationId,
SD59x18 sizeDeltaX18,
UD60x18 fillPriceX18
)
internal
virtual
{
...MORE CODE
// reverts if the trader can't satisfy the appropriate margin requirement
tradingAccount.validateMarginRequirement(
ctx.requiredMarginUsdX18,
tradingAccount.getMarginBalanceUsd(accountTotalUnrealizedPnlUsdX18),//@audit this will be 0
ctx.orderFeeUsdX18.add(ctx.settlementFeeUsdX18)
);
...MORE CODE
}

Another impact is that marketOrderMinLifetime which is parameter used in OrderBranch::cancelMarketOrder to prevent trader from cancelling order before some time passes is bypassed.

Impact

Traders can maliciously cancel their orders when unfavorable conditions happen after order is announced and bypass the marketOrderMinLifetime by simply withdrawing their collateral.

Tools Used

Manual Review

Recommendations

In TradingAccountBranch::withdrawMargin call MarketOrder::checkPendingOrder as well.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

The Keeper can be griefed by a user who withdraw's the collateral when having a pending position

Support

FAQs

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