Zaros Part 1

Summary

Due to rounding down in withdrawMarginUsd, traders can avoid paying for settlement and order fees when adding on to an existing position.

Vulnerability Detail

When an order is filled, TradingAccount.deductAccountMargin is called to deduct the account's collateral to pay for settlementFeeUsdX18, orderFeeUsdX18 and pnlUsdX18.

deductAccountMargin loops over each collateral type in order of liquidation priority, and tries to deduct the required amount. If there is insufficient balance of one collateral type, ctx.isMissingMargin is set to true and the loop will continue to the next collateral type.

However, if pnlUsdX18 is small enough, it will round down to zero in withdrawMarginUsd and set ctx.isMissingMargin == false and allow the loop to exit before all fees are deducted.

Specific conditions are required for this to happen:

• Account has at least 2 collateral types, and insufficient collateral in the first loop iteration to pay for fees

• pnlUsdX18 > 0 but small enough to round down to 0 when dividing by marginCollateralPriceUsdX18. This implies a trade that is adding on to an existing position, with the existing position experiencing a slight loss

Run the POC here: https://gist.github.com/giraffe0x/c41221b274753a316d349e238fa1be77

Impact

High. Traders can take advantage of this loophole to execute trades without paying for any settlement and order fees. Protocol loses potential revenue from fees and maker/taker fees do not achieve its intended impact.

Code Snippet

https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/leaves/TradingAccount.sol#L557
https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/leaves/TradingAccount.sol#L448

Tool used

Manual Review

Recommendation

In TradingAccount.withdrawMarginUsd, the calculation for requiredMarginInCollateralX18 should round up the division. This would ensure that the else case is entered after which would set isMissingMargin = true and ensure the loop to the next collateral type continues.