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

Lack of flexibility in collateral liquidation priority could lead to accounting errors and increase traders exposure

greed

Summary

The protocol is able to modify the collateralLiquidationPriority set to re-order the collateral addresses that will be collected first during liquidation.

It is done through the GlobalConfigurationBranch::configureCollateralLiquidationPriority() function which adds the provided collateral address at the end of the set.

In case one of the collaterals that are trying to be added to the set already exists, the transaction will revert.

https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/leaves/GlobalConfiguration.sol#L101-L113

function configureCollateralLiquidationPriority(Data storage self, address[] memory collateralTypes) internal {
uint256 cachedCollateralTypesCached = collateralTypes.length;
​
for (uint256 i; i < cachedCollateralTypesCached; i++) {
if (collateralTypes[i] == address(0)) {
revert Errors.ZeroInput("collateralType");
}
​
if (!self.collateralLiquidationPriority.add(collateralTypes[i])) {
revert Errors.MarginCollateralAlreadyInPriority(collateralTypes[i]);
}
}
}

Vulnerability Details

In case the whole collateral liquidation priority has to be changed, the current implementation doesn't offer enough flexibility to do it in a single transaction, can be quite a burden to reorder and could cause unexpected behaviors including accounting errors.

The protocol admins would have to remove all the collaterals one by one, executing GlobalConfigurationBranch::removeCollateralFromLiquidationPriority() multiple times to remove them from the collateralLiquidationPriority set before pushing them back in the new order with GlobalConfigurationBranch::configureCollateralLiquidationPriority()

During the multiple executions of removeCollateralFromLiquidationPriority() the protocol doesn't stop operating and liquidations must occur which collects collateral based upon the collateralLiquidationPriority that is temporarly unstable and unreliable.

Impact

Some accounts could be liquidated and their collateral being collected in an unexpected order leading to user being more exposed to the volatility of his collateral basket.

Liquidations can occur in an unexpected manner resulting in funds being stuck in the contract and fees not collected.

The following coded PoC demonstrates an issue where a liquidation occurs during a collateral priority reordering and results in fees not being collected by the respective recipients (ending up stuck in the contract) as well as an accounting error in the trader's account.

The test can be pasted in test\integration\perpetuals\liquidation-branch\liquidateAccounts\liquidateAccounts.t.sol

Make sure to import UD60x18 and SD59x18 if needed

function testAccountingErrorsDuringPriorityReorder() external {
// give naruto some tokens
uint256 USER_STARTING_BALANCE = 10e18;
int128 USER_POS_SIZE_DELTA = 10e18;
deal({ token: address(weEth), to: users.naruto.account, give: USER_STARTING_BALANCE });
​
// currently in the test suite during liquidations, the assets will be collected in the following order :
// USDz - USDC - WETH - WBTC - wstETH - weETH
changePrank({ msgSender: users.naruto.account });
uint128 tradingAccountId = createAccountAndDeposit(USER_STARTING_BALANCE, address(weEth));
​
changePrank({ msgSender: users.naruto.account });
openManualPosition(BTC_USD_MARKET_ID, BTC_USD_STREAM_ID, MOCK_BTC_USD_PRICE, tradingAccountId, USER_POS_SIZE_DELTA);
​
updateMockPriceFeed(BTC_USD_MARKET_ID, MOCK_BTC_USD_PRICE / 10); // BTC / 10
​
// account is now liquidatable
uint128[] memory liquidatableAccountsIds = perpsEngine.checkLiquidatableAccounts(0, 1);
assertEq(1, liquidatableAccountsIds.length);
assertEq(liquidatableAccountsIds[0], 1);
​
// but at the same time, admins want to do some changes on the priority set so it ends up like this :
// USDz - USDC - wstETH - weETH - WETH - WBTC
changePrank({ msgSender: users.owner.account });
perpsEngine.removeCollateralFromLiquidationPriority(address(weEth));
perpsEngine.removeCollateralFromLiquidationPriority(address(wstEth));
// during the reorder, keepers trigger the account liquidation
// cache data to be compared after liquidation
uint256 perpsEngineWeEthBalance = weEth.balanceOf(address(perpsEngine));
(SD59x18 marginBalanceUsdX18, UD60x18 initialMarginUsdX18, UD60x18 maintenanceMarginUsdX18, SD59x18 availableMarginUsdX18) =
perpsEngine.getAccountMarginBreakdown(tradingAccountId);
​
changePrank({ msgSender: liquidationKeeper });
perpsEngine.liquidateAccounts(liquidatableAccountsIds);
​
// even though liquidation occurred, nothing has been transferred to the fee recipients ; funds stayed in the contract
assertEq(
perpsEngineWeEthBalance,
weEth.balanceOf(address(perpsEngine))
);
​
// liquidation did not move funds but still changed account margins
(
SD59x18 marginBalanceUsdX18After,
UD60x18 initialMarginUsdX18After,
UD60x18 maintenanceMarginUsdX18After,
SD59x18 availableMarginUsdX18After
) = perpsEngine.getAccountMarginBreakdown(tradingAccountId);
assert(marginBalanceUsdX18After.neq(marginBalanceUsdX18));
assert(initialMarginUsdX18After.neq(initialMarginUsdX18));
assert(maintenanceMarginUsdX18After.neq(maintenanceMarginUsdX18));
assert(availableMarginUsdX18After.neq(availableMarginUsdX18));
​
// the last collateral are removed
changePrank({ msgSender: users.owner.account });
perpsEngine.removeCollateralFromLiquidationPriority(address(wBtc));
perpsEngine.removeCollateralFromLiquidationPriority(address(wEth));
​
// now the owner can add the collateral tokens in the right order
address[] memory tokensToAdd = new address[](4);
tokensToAdd[0] = address(wstEth);
tokensToAdd[1] = address(weEth);
tokensToAdd[2] = address(wEth);
tokensToAdd[3] = address(wBtc);
perpsEngine.configureCollateralLiquidationPriority(tokensToAdd);
}

Tools Used

Manual review

Recommendations

Implement a function that is responsible for re-ordering the collateralLiquidationPriority storage in a single transaction or re-implement configureCollateralLiquidationPriority() to allow a more flexible re-ordering.

Updates

Lead Judging Commences

inallhonesty Lead Judge
over 1 year ago
inallhonesty Lead Judge over 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.

Give us feedback!