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

Deleting CollateralTypes from the CollateralLiquidationPriority allows traders to be liquidated for free and getting back their full collateral as if they were not liquidated.

0xstalin
## Summary
Removing a CollateralType from the CollateralLiquidationPriority causes that accounts can be liquidated for free without being charged for their losses, and, traders are able to get back a 100% of their collateral after a liquidation.
​
​
## Vulnerability Details
[When deducting margin from an account to cover a liquidation](https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/leaves/TradingAccount.sol#L488-L578), the order of liquidation is determined by the `collateralLiquidationPriority`, it will iterate over all the collaterals in this list and will try to deduct the exact amount to be deducted, or at least, the most that can be deducted (in case the user's collateral is less than what it should be deducted).
​
[When collateral is removed from the `collateralLiquidationPriority`](https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/GlobalConfigurationBranch.sol#L271-L280), [users are not allowed to continue depositing that asset as collateral for their accounts.](https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/TradingAccountBranch.sol#L342-L343) But, a secondary effect of removing a collateral from the `collateralLiquidationPriority` is that tis collateral won't be able to be deducted when deducting margin from an account.
​
While the collateralType was part of the `collateralLiquidationPriority`, users could have deposited that collateral in their accounts, and, they could have opened positions.
​
**The problem with the existing logic to deduct margin is that once collateral is removed from the `collateralLiquidationPriority`, even though an account has only that collateral as its collateralBalance, the deduct margin logic won't deduct such a collateral from the user**, and, because liquidations try to remove as most collateral as it could without reverting, this allows for free liquidations where the user's bad investements would be removed for free without paying for it, and, after the account has been liquidated, the account's owner will be able to withdraw all the collateral from its account.
​
### Coded PoC
On this PoC, we simulate a scenario where a specific collateral is part of the `collateralLiquidationPriority`, and, an accout deposits some of it to open a position. After some time, this collateral is removed from the `collateralLiquidationPriority`, and, the user's account gets liquidable. After liquidating the account, the account's owner proceeds to withdraw a 100% of its collateral, as if the account would have never been liquidated.
​
Add the below PoC in the [`liquidateAccounts.t.sol`](https://github.com/Cyfrin/2024-07-zaros/blob/main/test/integration/perpetuals/liquidation-branch/liquidateAccounts/liquidateAccounts.t.sol) test file.
​
Run the PoC with the next command: `forge test --match-test test_FreeLiquidationWhenCollateralIsRemovedFromCollateralLiquidationPriorityPoC -vvvv`
​
<details>
<summary><b>Expand to see PoC</b></summary>
<br>
​
```
function test_FreeLiquidationWhenCollateralIsRemovedFromCollateralLiquidationPriorityPoC()
external
givenTheSenderIsARegisteredLiquidator
whenTheAccountsIdsArrayIsNotEmpty
givenAllAccountsExist
{
uint256 marketId = 3;
uint256 secondMarketId = 5;
bool isLong = true;
uint256 timeDelta = 1 weeks;
​
TestFuzz_GivenThereAreLiquidatableAccountsInTheArray_Context memory ctx;
​
ctx.fuzzMarketConfig = getFuzzMarketConfig(marketId);
ctx.secondMarketConfig = getFuzzMarketConfig(secondMarketId);
​
vm.assume(ctx.fuzzMarketConfig.marketId != ctx.secondMarketConfig.marketId);
​
uint256 amountOfTradingAccounts = 1;
​
ctx.marginValueUsd = 10_000e18 / amountOfTradingAccounts;
ctx.initialMarginRate = ctx.fuzzMarketConfig.imr;
​
deal({ token: address(usdz), to: users.naruto.account, give: ctx.marginValueUsd });
​
// last account id == 0
ctx.accountsIds = new uint128[](amountOfTradingAccounts);
​
ctx.accountMarginValueUsd = ctx.marginValueUsd / (amountOfTradingAccounts);
​
{
ctx.tradingAccountId = createAccountAndDeposit(ctx.accountMarginValueUsd, address(usdz));
​
openPosition(
ctx.fuzzMarketConfig,
ctx.tradingAccountId,
ctx.initialMarginRate,
ctx.accountMarginValueUsd / 2,
isLong
);
​
openPosition(
ctx.secondMarketConfig,
ctx.tradingAccountId,
ctx.secondMarketConfig.imr,
ctx.accountMarginValueUsd / 2,
isLong
);
​
ctx.accountsIds[0] = ctx.tradingAccountId;
​
deal({ token: address(usdz), to: users.naruto.account, give: ctx.marginValueUsd });
}
​
//@audit => Removing usdz from the LiquidationPriority.
changePrank({ msgSender: users.owner.account });
perpsEngine.removeCollateralFromLiquidationPriority(address(usdz));
​
setAccountsAsLiquidatable(ctx.fuzzMarketConfig, isLong);
setAccountsAsLiquidatable(ctx.secondMarketConfig, isLong);
​
uint256 usdzCollateralOwnedByLiquidatedAccount_BeforeLiquidation = perpsEngine.getAccountMarginCollateralBalance(ctx.tradingAccountId,address(usdz)).intoUint256();
​
//@audit => Underwater user is liquidated
//@audit-issue => Because the user's collateral was removed from the liquidationPriority, the liquidation will be made for free
//@audit-issue => The negative PnL of the user will be erased for free without taking any of his collateral.
changePrank({ msgSender: liquidationKeeper });
skip(timeDelta);
perpsEngine.liquidateAccounts(ctx.accountsIds);
​
// console2.log("usdz collateral in perp engine after liquidation: ", usdz.balanceOf(address(perpsEngine)));
​
uint256 usdzCollateralOwnedByLiquidatedAccount_AfterLiquidation = perpsEngine.getAccountMarginCollateralBalance(ctx.tradingAccountId,address(usdz)).intoUint256();
//@audit => None of the liquidated user's collateral was seized during the liquidation
assertEq(usdzCollateralOwnedByLiquidatedAccount_BeforeLiquidation, usdzCollateralOwnedByLiquidatedAccount_AfterLiquidation);
​
uint256 usdzLiquidatedUserBalance_BeforeWithdrawing = usdz.balanceOf(address(users.naruto.account));
​
//@audit => User is now able to withdraw all the deposited collateral. His losses were liquidated for free and now it can get back all the original deposited collateral regardless of the loss on his trading!
changePrank({ msgSender: users.naruto.account });
perpsEngine.withdrawMargin(ctx.tradingAccountId,address(usdz),usdzCollateralOwnedByLiquidatedAccount_AfterLiquidation);
​
uint256 usdzLiquidatedUserBalance_AfterWithdrawing = usdz.balanceOf(address(users.naruto.account));
​
assert(usdzLiquidatedUserBalance_AfterWithdrawing > usdzLiquidatedUserBalance_BeforeWithdrawing);
​
}
​
```
​
</details>
</br>
​
​
## Impact
Traders are able to be liquidated without getting their collateral deducted from their accounts.
​
## Tools Used
Manual Audit & Foundry
​
## Recommendations
In order to preserve the logic of deducting margin based on a priority list, so that collaterals with the best loanToValue are deducted until the last, I'd recommend to add the below logic after the iteration over the collateralLiquidationPriority is over, the idea is to handle the scenario when a collatearl was removed from the collateralLiquidationPriority, but, the account could have that collateral as part of its marginCollateralBalance.
​
[`TradingAccount.deductAccountMaring() function`](https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/leaves/TradingAccount.sol#L488-L578)
```
function deductAccountMargin(
Data storage self,
FeeRecipients.Data memory feeRecipients,
UD60x18 pnlUsdX18,
UD60x18 settlementFeeUsdX18,
UD60x18 orderFeeUsdX18
)
internal
returns (UD60x18 marginDeductedUsdX18)
{
// working data
DeductAccountMarginContext memory ctx;
​
...
​
// loop through configured collateral types
for (uint256 i; i < cachedCollateralLiquidationPriorityLength; i++) {
...
}
​
// output total margin deducted
marginDeductedUsdX18 =
ctx.settlementFeeDeductedUsdX18.add(ctx.orderFeeDeductedUsdX18).add(ctx.pnlDeductedUsdX18);
​
//@audit => Recommendation to mitigate this bug
​
//@audit => If isMissingMargin remained in the default value and marginDeductedUsdX18 is 0, that means that the account had no collateral from the collateralLiquidationPriority
if (ctx.isMissingMargin == false && marginDeductedUsdX18.eq(0)) {
UD60x18 totalToDeductX18 = pnlUsdX18.add(settlementFeeUsdX18).add(orderFeeUsdX18);
​
//@audit => Iterate over the exact list of collateral of the account been liquidated!
uint256 cachedMarginCollateralBalanceLength = self.marginCollateralBalanceX18.length();
for (uint256 i; i < cachedMarginCollateralBalanceLength; i++) {
(address collateralType, uint256 balanceX18) = self.marginCollateralBalanceX18.at(i);
​
//@audit => Deduct from this collateral what it needs/can be deducted!
​
//@audit => Make sure to recompute `marginDeductedUsdX18` to add the amount of collateral deducted from the collateral that was not part of the collateralLiquidationPriority.
​
}
​
}
​
}
```
Updates

Lead Judging Commences

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

removal of collateral types from the liquidation priority list will affect affect the liquidation process of the existing positions

Appeal created

0xstalin Submitter
over 1 year ago
inallhonesty Lead Judge
over 1 year ago
0xstalin Submitter
over 1 year ago
inallhonesty Lead Judge
over 1 year ago
inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

removal of collateral types from the liquidation priority list will affect affect the liquidation process of the existing positions

Support

FAQs

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

Give us feedback!