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

Scaling error allows low decimals tokens to bypass the margin requirements during withdrawal

greed

Summary

Zaros allows users to deposit multiple assets as collateral that will be used for their trades.

One of these assets is Wrapped Bitcoin (WBTC) which is an ERC20 that consists in 8 decimals to represent 1 unit. This means 1e8 in balance represents 1 WBTC.

After creating a trading account, users can deposit collateral to back their positions and start trading on Zaros, taking longs or shorts positions on the market they wish.

Once a position is opened traders are allowed to withdraw their collateral as long as they still satisfy the margin requirements of their positions after the withdrawal.

This is done using the TradingAccountBranch::withdrawMargin() function and the margin requirement is insured by the TradingAccount::validateMarginRequirement() internal function.

https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/TradingAccountBranch.sol#L358-L398

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

Vulnerability Details

In order to handle low decimals tokens everywhere in the protocol, Zaros starts by scaling such token amount to 18 decimals so rounding errors are avoided.

The issue arises during withdrawals because the USD price of the collateral is calculated based upon the scaled amount and not the actual amount.

The TradingAccountBranch::withdrawMargin() function takes the following parameters :

  • uint128 tradingAccountId : the user's account ID

  • address collateralType : the token the user wants to withdraw

  • uint256 amount : the balance of tokens the user wants to withdraw

It starts by scaling the given amount to 18 decimals and verifies the account holds enough collateral before subtracting it from the account storage (this collateral was also scaled to 18 decimals during deposit meaning this is compared correctly) :

UD60x18 amountX18 = marginCollateralConfiguration.convertTokenAmountToUd60x18(amount);
​
_requireEnoughMarginCollateral(tradingAccount, collateralType, amountX18);
​
tradingAccount.withdraw(collateralType, amountX18);

After the storage has been updated, the contract will cache the different margin requirements, get the USD value of the collateral held by the account before verifying the collateral still holds the margin requirements

(UD60x18 requiredInitialMarginUsdX18,, SD59x18 accountTotalUnrealizedPnlUsdX18) =
tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);
​
SD59x18 marginBalanceUsdX18 = tradingAccount.getMarginBalanceUsd(accountTotalUnrealizedPnlUsdX18);
​
tradingAccount.validateMarginRequirement(requiredInitialMarginUsdX18, marginBalanceUsdX18, UD60x18_ZERO);

If we take a look at the getMarginBalanceUsd() function, it loops through all the collateral held by the account, gets the scaled amount of the token deposited and multiplies it by its USD price and the LTV ratio.

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

function getMarginBalanceUsd(
Data storage self,
SD59x18 activePositionsUnrealizedPnlUsdX18
)
internal
view
returns (SD59x18 marginBalanceUsdX18)
{
// cache colllateral length
uint256 cachedMarginCollateralBalanceLength = self.marginCollateralBalanceX18.length();
​
// iterate over every collateral account has deposited
for (uint256 i; i < cachedMarginCollateralBalanceLength; i++) {
// read key/value from storage for current iteration
(address collateralType, uint256 balance) = self.marginCollateralBalanceX18.at(i);
​
// load collateral margin config for this collateral type
MarginCollateralConfiguration.Data storage marginCollateralConfiguration =
MarginCollateralConfiguration.load(collateralType);
​
// calculate the collateral's "effective" balance as:
// collateral_price * deposited_balance * collateral_loan_to_value_ratio
> UD60x18 adjustedBalanceUsdX18 = marginCollateralConfiguration.getPrice().mul(ud60x18(balance)).mul(
> ud60x18(marginCollateralConfiguration.loanToValue)
);
​
// add this account's "effective" collateral balance to cumulative output
marginBalanceUsdX18 = marginBalanceUsdX18.add(adjustedBalanceUsdX18.intoSD59x18());
}
​
// finally add the unrealized PNL to the cumulative output
marginBalanceUsdX18 = marginBalanceUsdX18.add(activePositionsUnrealizedPnlUsdX18);
}

When dealing with low decimals assets, the adjustedBalanceUsdX18 should be down scaled to the token's real decimals number otherwise the collateral price will be way higher than it actually is.

For WBTC since the token amount has been scaled from 8 decimals to 18, the price returned will have 10 additional decimals.

Here is a coded PoC that can be pasted in test\integration\perpetuals\perp-market-branch\getFundingRate\getFundingRate.t.sol that demonstrates a trader opening a position then withdrawing its entire collateral balance. After the operations, the position still exists and can't be liquidated even though the asset being longed crashes to 1 wei.

function testMarginBtcBypassWithdrawAndUnliquidatable_ETH_Market() external {
uint256 btcBalance = 1e8;
​
deal({ token: address(wBtc), to: users.naruto.account, give: btcBalance });
​
vm.startPrank(users.naruto.account);
uint128 tradingAccountId = createAccountAndDeposit(btcBalance, address(wBtc));
​
// user longs ETH with his BTC collateral
openManualPosition(ETH_USD_MARKET_ID, ETH_USD_STREAM_ID, MOCK_ETH_USD_PRICE, tradingAccountId, 500 ether);
​
// user withdraws all his collateral, bypassing the margin requirements
perpsEngine.withdrawMargin(tradingAccountId, address(wBtc), 99999999);
// user retrieves almost his entire collateral, leaving 1 wei of dust in the contract
assertEq(wBtc.balanceOf(address(perpsEngine)), 1);
assertEq(wBtc.balanceOf(users.naruto.account), 99999999);
skip(5 days);
​
// ETH crashes to 1 wei
updateMockPriceFeed(ETH_USD_MARKET_ID, 1);
// even though the collateral has been withdrawn, the funding rate has still increased and still contributes to the market
SD59x18 fundingRate = perpsEngine.getFundingRate(ETH_USD_MARKET_ID);
assertNotEq(fundingRate.intoUint256(), 0);
​
// his margins also still exist
(SD59x18 marginBalanceUsdX18, UD60x18 initialMarginUsdX18, UD60x18 maintenanceMarginUsdX18, SD59x18 availableMarginUsdX18) =
perpsEngine.getAccountMarginBreakdown(tradingAccountId);
assertNotEq(marginBalanceUsdX18.intoUint256(), 0);
assertNotEq(initialMarginUsdX18.intoUint256(), 0);
assertNotEq(maintenanceMarginUsdX18.intoUint256(), 0);
assertNotEq(availableMarginUsdX18.intoUint256(), 0);
​
// still, the account is not liquidatable
uint128[] memory liquidatableAccountsIds = perpsEngine.checkLiquidatableAccounts(0, 1);
assertEq(1, liquidatableAccountsIds.length);
assertEq(liquidatableAccountsIds[0], 0);
}

Impact

The estimated USD value of a collateral with low decimals is wrong.

Users are able to bypass the margin requirements during margin withdrawal using low decimals assets, leaving their position persistent and influencing the market through their contribution to the funding rate at no cost.

Moreover, the liquidation requirements rely on the same flawed metric making this kind of positions impossible to liquidate.

Tools Used

Manual review

Recommendations

Make sure the TradingAccount::getMarginBalanceUsd() function down scales to stored collateral margin balance to the appropriate number of decimals.

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

Appeal created

greed Submitter
about 1 year ago
blckhv Auditor
about 1 year ago
inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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