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

Inconsistent handling of account ID equals to 0 between LiquidationBranch::checkLiquidatableAccounts and LiquidationBranch::liquidateAccounts

0x5chn0uf

Summary

The LiquidationBranch::checkLiquidatableAccounts function can return an array containing zero values (0) for account IDs when the checked accounts is not liquidatable.

However, the LiquidationBranch::liquidateAccounts function, which is expected to process these account IDs, assumes that zero values should never occur and includes a comment stating:

// sanity check for non-sensical accountId; should never be true
if (ctx.tradingAccountId == 0) continue;

While the LiquidationBranch::liquidateAccounts function does skip over these zero values, and while there is no part of the code that uses the two functions consequently as it is intended for the keepers, the inconsistency between the two functions can lead to unnecessary gas consumption and potential confusion if the liquidator doesn't know they need to sanitize the array.

Vulnerability Details

function test_chackLiquidationReturn() public {
MarketConfig memory fuzzMarketConfig = getFuzzMarketConfig(DOGE_USD_MARKET_ID);
​
MockPriceFeed priceFeed = MockPriceFeed(fuzzMarketConfig.priceAdapter);
uint256 initialPrice = 1e16; // 0.01 USD
priceFeed.updateMockPrice(initialPrice);
​
// Setup multiple users with large amounts of USDC
deal({ token: address(usdc), to: users.naruto.account, give: 10_000_000e6 }); // 10 million USDC
deal({ token: address(usdc), to: users.sasuke.account, give: 10_000_000e6 }); // 10 million USDC
deal({ token: address(usdc), to: users.sakura.account, give: 10_000_000e6 }); // 10 million USDC
deal({ token: address(usdc), to: users.madara.account, give: 10_000_000e6 }); // 10 million USDC
​
changePrank({ msgSender: users.naruto.account });
uint128 tradingAccountId = createAccountAndDeposit(10_000_000e6, address(usdc));
changePrank({ msgSender: users.sasuke.account });
uint128 sasukeAccountId = createAccountAndDeposit(10_000_000e6, address(usdc));
changePrank({ msgSender: users.sakura.account });
uint128 sakuraAccountId = createAccountAndDeposit(10_000_000e6, address(usdc));
changePrank({ msgSender: users.madara.account });
uint128 madaraAccountId = createAccountAndDeposit(10_000_000e6, address(usdc));
​
uint256 initialMarginRate = fuzzMarketConfig.imr;
uint256 marginValueUsd = 10_000e18;
​
// Open positions for each user
changePrank({ msgSender: users.naruto.account });
openPosition(fuzzMarketConfig, tradingAccountId, initialMarginRate, marginValueUsd, true);
changePrank({ msgSender: users.sasuke.account });
openPosition(fuzzMarketConfig, sasukeAccountId, initialMarginRate, marginValueUsd, false);
changePrank({ msgSender: users.sakura.account });
openPosition(fuzzMarketConfig, sakuraAccountId, initialMarginRate, marginValueUsd, true);
changePrank({ msgSender: users.madara.account });
openPosition(fuzzMarketConfig, madaraAccountId, initialMarginRate, marginValueUsd, false);
​
// Price hasn't still moved
​
// Check for liquidations
uint128[] memory liquidatableAccountsIds = perpsEngine.checkLiquidatableAccounts(0, 4);
assertEq(liquidatableAccountsIds.length, 4); // <-- Here we can wrongly assume there are four liquidatable accounts
}

Impact

It does not lead to any direct security risks or unintended state changes. However, it may result in inefficient gas usage when processing liquidations, as the LiquidationBranch::liquidateAccounts function still iterates over zero values before skipping them.

It could also be source of potential confusion for integrators who may assume that the all the returned liquidatableAccountsIds are accounts IDs that should be liquidated.

Tools Used

Foundry

Recommendations

Exclude the 0 IDs from the liquidatableAccountsIds variable.

function checkLiquidatableAccounts(
uint256 lowerBound,
uint256 upperBound
)
external
view
returns (uint128[] memory liquidatableAccountsIds)
{
// Prepare output array size
liquidatableAccountsIds = new uint128[](upperBound - lowerBound);
+ uint256 validAccountsCount;
​
// ...
for (uint256 i = lowerBound; i < upperBound; i++) {
// ...
// Account can be liquidated if requiredMargin > marginBalance
if (TradingAccount.isLiquidatable(requiredMaintenanceMarginUsdX18, marginBalanceUsdX18)) {
- liquidatableAccountsIds[i] = tradingAccountId;
+ liquidatableAccountsIds[validAccountsCount++] = tradingAccountId;
}
}
​
+ // Resize the liquidatableAccountsIds array to the actual number of valid accounts
+ assembly {
+ mstore(liquidatableAccountsIds, validAccountsCount)
+ }
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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