The Standard
The Standard
DeFiHardhat
20,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Liquidations can be DOSed in LiquidationPoolManager

0xasen

Summary

Liquidations can easily be DOSed by a user that creates too many pending stakes.

Vulnerability Details

Let's take a look at the user flow when liquidating a vault in LiquidationPoolManager.sol.
Anybody can call the runLiquidation() function which should result in liquidating a vault that is undercollateralized:

function runLiquidation(uint256 _tokenId) external {
ISmartVaultManager manager = ISmartVaultManager(smartVaultManager);
manager.liquidateVault(_tokenId);
​
distributeFees(); <--
​
ITokenManager.Token[] memory tokens = ITokenManager(manager.tokenManager()).getAcceptedTokens();
ILiquidationPoolManager.Asset[] memory assets = new ILiquidationPoolManager.Asset[](tokens.length);
uint256 ethBalance;
​
for (uint256 i = 0; i < tokens.length; i++) {
//transfer of assets
}
LiquidationPool(pool).distributeAssets{value: ethBalance}(assets, manager.collateralRate(), manager.HUNDRED_PC());
forwardRemainingRewards(tokens);
}

We'll concentrate on the call to the distributeFees() function:

function distributeFees() public {
IERC20 eurosToken = IERC20(EUROs);
uint256 _feesForPool = eurosToken.balanceOf(address(this)) * poolFeePercentage / HUNDRED_PC;
if (_feesForPool > 0) {
eurosToken.approve(pool, _feesForPool);
LiquidationPool(pool).distributeFees(_feesForPool); <--
}
eurosToken.transfer(protocol, eurosToken.balanceOf(address(this)));
}

As you can see, if there are fees to be distributed we're calling distributeFees in LiquidationPool:

function distributeFees(uint256 _amount) external onlyManager {
uint256 tstTotal = getTstTotal();
if (tstTotal > 0) {
IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _amount);
for (uint256 i = 0; i < holders.length; i++) {
address _holder = holders[i];
positions[_holder].EUROs += _amount * positions[_holder].TST / tstTotal;
}
for (uint256 i = 0; i < pendingStakes.length; i++) {
pendingStakes[i].EUROs += _amount * pendingStakes[i].TST / tstTotal;
}
}
}

As you can see, the function is looping over all the holders and all the pending stakes. A malicious user could easily create a lot of pending stakes and DOS the distributeFees function making it revert and by extension, reverting the liquidation of the vault.

A pending stake is created via increasePosition in the LiquidationPool contract and is not removed from the pending stakes array until 24hrs have passed. This can be checked in the consolidatePendingStakes function.

So even if there's not a malicious user, simply from increased usage and a lot of users increasing their positions a DOS can happen.

Impact

Impossible liquidation of vaults even if they are undercollateralized and/or with bad debt.

Tools Used

Manual review

Recommendations

The whole design should be should be improved. Right now, there is too much looping in these functions - over the pending stakes and over the holders, which are not bound in any way.
A possible solution could be to limit the number of pending stakes a user can open at the same time and bound the holders at some reasonable amount. However, I'm not sure this is the best way to mitigate it.

Updates

Lead Judging Commences

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

pendingstake-dos

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

pendingstake-high

Support

FAQs

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