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

Attackers can DOS liquidation upkeep by burning their accountNft

forgebyola

Summary

Liquidation action by the keeper can be DOSsed by attackers, causing a crucial mechanism to fail. This can be done on a single upkeep or for multiple upkeeps.

Vulnerability Details

Accounts are liquidatable if they no longer have sufficient collateral or margin to keep their active positions open. This can occur if a position accrues significant loses. In such a case, to avoid bad debt from accrueing in the system, the keeper networks are triggered to perform upkeep. Liquidatable accounts within a range of accountIds are aggregated, and forwarded to the keepers to liquidate them through LiquidationKeeper::performUpkeep.

function performUpkeep(bytes calldata peformData) external override onlyForwarder {
uint128[] memory accountsToBeLiquidated = abi.decode(peformData, (uint128[]));
LiquidationKeeperStorage storage self = _getLiquidationKeeperStorage();
(IPerpsEngine perpsEngine) = (self.perpsEngine);
​
perpsEngine.liquidateAccounts(accountsToBeLiquidated);
}

Since these liquidations are performed in a loop, a failure in liquidating any of these accounts would result in DOS of the liquidation process. Because of this, the implementation is done to avoid reverting through most of the liquidation process.
However, there is a way an account can fail to be liquidated.

AccountNfts are Erc721s with an overriden _update function to transfer owner of the account when a transfer transaction occurs.

/// @notice Used by the Account NFT contract to notify an account transfer.
/// @dev Can only be called by the Account NFT contract.
/// @dev It updates the Trading Account stored access control data.
/// @param to The recipient of the account transfer.
/// @param tradingAccountId The trading account id.
function notifyAccountTransfer(address to, uint128 tradingAccountId) external {
_onlyTradingAccountToken();
​
TradingAccount.Data storage tradingAccount = TradingAccount.loadExisting(tradingAccountId);
tradingAccount.owner = to;
}

This function is also called on minting and burning.
The owner of the trading account can burn their accountNfts at any time. When this is done, notifyAccountTransfer updates the new owner to address(0).

  • file: Erc721::burn

​
function _burn(uint256 tokenId) internal {
address previousOwner = _update(address(0), tokenId, address(0));
if (previousOwner == address(0)) {
revert ERC721NonexistentToken(tokenId);
}
}

Within liquidateAccounts, the account is loaded by calling TradingAccount.loadExisting. This function reverts if the account owner is address(0).

function loadExisting(uint128 tradingAccountId) internal view returns (Data storage tradingAccount) {
tradingAccount = load(tradingAccountId);
if (tradingAccount.owner == address(0)) {
@> revert Errors.AccountNotFound(tradingAccountId, msg.sender);
}
}

The attacker can DOS liquidations by creating an account with negliglible collateral or enough to open a trade, open a bad trade such that they become liquidatable. When their account is to be liquidated by the keeper, they can frontrun the performUpkeep call by burning their accountNft which transfers the owner to address(0). This causes this function call by the keeper to fail. The entire process of checkLiquidation and forwarding to the keeper would have to be repeated. Subsequent liquidation upkeep can be DOSsed as well.
This process is one which can be repeated by the attacker or group of attackers for as long as required.

Impact

Liquidation by the keepers which is a crucial component of the system can be denied for as long as required by attackers.

Tools Used

Manual Review

Recommendations

Perform TradingAccount.loadExisting in a try/catch block. If a revert is caught, continue to skip the liquidation of that account instead of revert.

- TradingAccount.Data storage tradingAccount = TradingAccount.loadExisting(ctx.tradingAccountId);
+ try {
TradingAccount.Data storage tradingAccount = TradingAccount.loadExisting(ctx.tradingAccountId);
} catch {
continue;
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

fillOffchainOrders reverts everything if a single order fails one of the multiple checks

If you send 1 cancel and 1 create it should still run the cancel, not revert everything.

Support

FAQs

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