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

Protocol Fails to Liquidate Accounts Properly

Reentrants

Summary

The checkLiquidatableAccounts function is designed to assist the LiquidationKeeper and other actors in identifying liquidatable accounts by processing them in manageable batches. Due to the extensive volume of trading over time, numerous trading accounts may become undercollateralized. A single call to check all liquidatable accounts would fail due to gas limitations, making batch processing essential. However, an out-of-bounds error occurs due to incorrect array indexing. This error prevents the protocol from correctly identifying and processing liquidatable accounts.

Vulnerability Details

The function checkLiquidatableAccounts creates an array liquidatableAccountsIds with a size of upperBound - lowerBound. The loop iterates from lowerBound to upperBound, using i as the index. For example, if lowerBound is 5 and upperBound is 10, the array size is 5. When i exceeds this size, it causes an out-of-bounds error, preventing the protocol from correctly identifying and processing liquidatable accounts.

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/LiquidationBranch.sol#L42-L86

function checkLiquidatableAccounts(
uint256 lowerBound,
uint256 upperBound
)
external
view
returns (uint128[] memory liquidatableAccountsIds)
{
// prepare output array size
@> liquidatableAccountsIds = new uint128[](upperBound - lowerBound);
...
// iterate over active accounts within given bounds
@> for (uint256 i = lowerBound; i < upperBound; i++) {
...
// account can be liquidated if requiredMargin > marginBalance
if (TradingAccount.isLiquidatable(requiredMaintenanceMarginUsdX18, marginBalanceUsdX18)) {
@> liquidatableAccountsIds[i] = tradingAccountId;
}
}
}

PoC

  1. Add the following test on checkLiquidatableAccounts.t.sol and run:

    forge test --match-test testGivenLowerBoundGreaterThanZero_transactionWillRevert

// import "forge-std/StdError.sol";
function testGivenLowerBoundGreaterThanZero_transactionWillRevert() public {
// pre conditions
// 1. system has 10 open positions. Positions from 5 to 10 are liquidatable.
uint256 USER_STARTING_BALANCE = 100_000e6;
int128 USER_POS_SIZE_DELTA = 10e18;
int128 USER_POS_NOT_LIQUIDATABLE_SIZE_DELTA = 1e18;
​
deal({ token: address(usdc), to: users.naruto.account, give: USER_STARTING_BALANCE * 10 });
changePrank({ msgSender: users.naruto.account });
​
// open 5 positions that are not liquidatable
for (uint256 i = 0; i < 5; i++) {
uint128 tradingAccountId = createAccountAndDeposit(USER_STARTING_BALANCE, address(usdc));
openManualPosition(BTC_USD_MARKET_ID, BTC_USD_STREAM_ID, MOCK_BTC_USD_PRICE, tradingAccountId, USER_POS_NOT_LIQUIDATABLE_SIZE_DELTA);
skip(1 minutes);
}
​
// open 5 positions that are liquidatable
for (uint256 i = 0; i < 5; i++) {
uint128 tradingAccountId = createAccountAndDeposit(USER_STARTING_BALANCE, address(usdc));
openManualPosition(BTC_USD_MARKET_ID, BTC_USD_STREAM_ID, MOCK_BTC_USD_PRICE, tradingAccountId, USER_POS_SIZE_DELTA);
skip(1 minutes);
}
// action: drop the price to make the last 5 positions liquidatable
updateMockPriceFeed(BTC_USD_MARKET_ID, MOCK_BTC_USD_PRICE/2);
​
// post condition: check not liquidatable positions from 0 to 5
uint128[] memory notLiquidatableAccountIds = perpsEngine.checkLiquidatableAccounts(0, 5);
​
// works as expected
for (uint256 i = 0; i < notLiquidatableAccountIds.length; i++) {
// should return 0 as positions are not liquidatable
assertEq(notLiquidatableAccountIds[i], 0);
}
// post condition: check liquidatable positions from 5 to 10
// panic: array out-of-bounds access (0x32)
vm.expectRevert(stdError.indexOOBError);
uint128[] memory liquidatableAccountIds = perpsEngine.checkLiquidatableAccounts(5, 10);
}

Output: after trying to check liquidatable accounts from (5,10) the function reverts with out-of-bounds.

[PASS] testGivenLowerBoundGreaterThanZero_transactionWillRevert() (gas: 7561246)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 14.43ms (7.94ms CPU time)

Impact

  • DoS on Liquidation: The protocol can't identify and liquidate under-collateralized accounts properly.

  • Bad debt: Increases the risk of bad debt in the system due to unliquidated positions.

  • Liquidation Keeper fees: Causes failed transactions, wasting fees to run the Keeper.

Tools Used

Manual Review & Foundry

Recommendations

By adjusting the index calculation to i - lowerBound, we ensure that values are correctly assigned within the bounds of the array, preventing out-of-bounds errors and allowing the liquidation process to function correctly

// checkLiquidatableAccounts function
​
if (TradingAccount.isLiquidatable(requiredMaintenanceMarginUsdX18, marginBalanceUsdX18)) {
- liquidatableAccountsIds[i] = tradingAccountId
+ liquidatableAccountsIds[i - lowerBound] = tradingAccountId;
}
Updates

Lead Judging Commences

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

`LiquidationBranch::checkLiquidatableAccounts()` executes `for` loop where `liquidatableAccountsIds[i] = tradingAccountId;` gives out of bounds if `lowerBound != 0`

Support

FAQs

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

Give us feedback!