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

Update market configuration lacking key sanity check

impish

Description

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/GlobalConfigurationBranch.sol#L475-L538

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/leaves/MarketConfiguration.sol#L37-L49

Updating Market Configuration lacks a key sanity check that may lead to an unstable liquidation market wherein nearly all user accounts are liquidatable. Specifically, self.maintenanceMarginRateX18 could potentially be updated to a very high value, bricking tradingAccount::isLiquidatable functionality.

Impact

While this described condition is rare and may only arise from some form of input error from the owner of GlobalConfigurationBranch, the potential impact is high. Considered the low probability of such conditions arising, It would be fair to categorize as a Medium.

Vulnerability Details

Below are all the sanity checks for update within GlobalConfigurationBranch::updatePerpMarketConfiguration, where we only check for zero input for params.maintenanceMarginRateX18.

if (abi.encodePacked(params.name).length == 0) {
revert Errors.ZeroInput("name");
}
if (abi.encodePacked(params.symbol).length == 0) {
revert Errors.ZeroInput("symbol");
}
if (params.priceAdapter == address(0)) {
revert Errors.ZeroInput("priceAdapter");
}
if (params.maintenanceMarginRateX18 == 0) {
revert Errors.ZeroInput("maintenanceMarginRateX18");
}
if (params.maxOpenInterest == 0) {
revert Errors.ZeroInput("maxOpenInterest");
}
if (params.maxSkew == 0) {
revert Errors.ZeroInput("maxSkew");
}
if (params.initialMarginRateX18 == 0) {
revert Errors.ZeroInput("initialMarginRateX18");
}
if (params.initialMarginRateX18 <= params.maintenanceMarginRateX18) {
revert Errors.InitialMarginRateLessOrEqualThanMaintenanceMarginRate();
}
if (params.skewScale == 0) {
revert Errors.ZeroInput("skewScale");
}
if (params.minTradeSizeX18 == 0) {
revert Errors.ZeroInput("minTradeSizeX18");
}
if (params.maxFundingVelocity == 0) {
revert Errors.ZeroInput("maxFundingVelocity");
}
if (params.priceFeedHeartbeatSeconds == 0) {
revert Errors.ZeroInput("priceFeedHeartbeatSeconds");
}

Note that we are not checking for extreme values of params.maintenanceMarginRateX18.

Within tradingAccount::isLiquidatable the below is used to evaluate if an account is liquidatable.

return requiredMaintenanceMarginUsdX18.intoSD59x18().gt(marginBalanceUsdX18);

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/leaves/TradingAccount.sol#L243-L256

requiredMaintenanceMarginUsdX18 is calculated by summing all of the positionalMaintenanceMarginUsdX18 within TradingAccount.sol::getAccountMarginRequirementUsdAndUnrealizedPnlUsd.

function getAccountMarginRequirementUsdAndUnrealizedPnlUsd(
Data storage self,
uint128 targetMarketId,
SD59x18 sizeDeltaX18
)
internal
view
returns (
UD60x18 requiredInitialMarginUsdX18,
UD60x18 requiredMaintenanceMarginUsdX18,
SD59x18 accountTotalUnrealizedPnlUsdX18
)
{
// if an existing position is being changed, perform some additional processing
if (targetMarketId != 0) {
...
(UD60x18 positionInitialMarginUsdX18, UD60x18 positionMaintenanceMarginUsdX18) = Position
.getMarginRequirement(
notionalValueX18,
ud60x18(perpMarket.configuration.initialMarginRateX18),
ud60x18(perpMarket.configuration.maintenanceMarginRateX18)
);
...
// update cumulative outputs
requiredMaintenanceMarginUsdX18 = requiredMaintenanceMarginUsdX18.add(positionMaintenanceMarginUsdX18);
}
}

Within Position.sol::getMarginRequirement each positionMaintenanceMarginUsdX18is calculated by multiplying the notionalValueX18 by maintenanceMarginRateX18. It is returned as maintenanceMarginRateX18 in getMarginRequirement, but is assigned to positionalMaintenanceMarginUsdX18 within tradingAccount.sol::getAccountMarginRequirementUsdAndUnrealizedPnlUsd.

function getMarginRequirement(
UD60x18 notionalValueX18,
UD60x18 initialMarginRateX18,
UD60x18 maintenanceMarginRateX18
)
internal
pure
returns (UD60x18 initialMarginUsdX18, UD60x18 maintenanceMarginUsdX18)
{
initialMarginUsdX18 = notionalValueX18.mul(initialMarginRateX18);
maintenanceMarginUsdX18 = notionalValueX18.mul(maintenanceMarginRateX18);
}

Note that if perpMarket.configuration.maintenanceMarginRateX18 is initialized to a high value within MarketConfiguration.sol::update, all positions would have a very high coefficient to their notionalValueX18. This would lead totradingAccount::isLiquidatable returning true for nearly all participating users due to a very high value of requiredMaintenanceMarginUsdX18.

Tools Used

Manual Review

Mitigation

Implement a revert for when params.maintenanceMarginRateX18 is a nonesensically high number alongside the other revert sanity checks in updatePerpMarketConfiguration

Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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