The Standard

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

Lack of access control in the ```LiquidationPool::distributeAssets``` causes a staker to steal all the rewards for other stakers.

Summary

When a vault is liquidated the protocol distribute the assets of that vault to all the stakers as rewards(more like buying at discounted price).
This is done through the distributeAssets function in the LiquidationPool contract which updates the rewards mapping for the staker which he can later claim it.

Vulnerability Details

Whenever a liquidation occurs it is called from the LiquidationPoolManager::runLiquidation which will liquidate the vault and send all the assets to the LiquidationPoolManager contract and then distribute it to the stakers by approving and calling the distributeAssets function.
When rewards are distributed it is not directly sent to the stakers but we map the amount with their address and the LiquidationPool contract still holds the rewards which the stakers can later claim it.

The problem is there is no access control in the distributAssets function:

function distributeAssets(ILiquidationPoolManager.Asset[] memory _assets, uint256 _collateralRate, uint256 _hundredPC) external payable {

this means anyone can call it with arbitrary values.

Example:

  • Say the LiquidationPool currently holds 10,000 eth in unclaimed rewards.

  • A malicious staker could call the distributeAssets function with the native currency as the asset to increase his rewards mapping and claim the rewards and repeat until he has drained all the rewards. Although he may need to burn some of his EUROs.
    This will work only for the native currency because with the native currency the attacker doesn't have to send any value but with erc20 tokens the function will try to transfer form the manager contract which is the LiquidationPoolManager contract that might not have approved the LiquidationPool.

if (asset.token.addr == address(0)) {
nativePurchased += _portion;
} else {
IERC20(asset.token.addr).safeTransferFrom(manager, address(this), _portion);
}

The function calls returnUnpurchasedNative(_assets, nativePurchased); but this will not be a problem for the attacker because if we look at the function it sends the native currency from the liquidationPool contract to the manager and the attacker doesn't have to spend any eth(in ethereum).
See:

function returnUnpurchasedNative(ILiquidationPoolManager.Asset[] memory _assets, uint256 _nativePurchased) private {
for (uint256 i = 0; i < _assets.length; i++) {
if (_assets[i].token.addr == address(0) && _assets[i].token.symbol != bytes32(0)) {
(bool _sent,) = manager.call{value: _assets[i].amount - _nativePurchased}("");
require(_sent);
}
}
}

Or
An attacker who is not even a staker can:

  • Call distributeAssets with the asset as the native currency and asset.amount as a very large number.

  • Increase the rewards for every staker by very large number.

  • And when users try to claim their rewards the claimRewards function will revert because the contract doesn't have enough native currency which will result in users not being able to claim even the other assets while his EUROs staked were also lost.

Impact

A staker can drain all the native currency held by the LiquidationPool contract as rewards and cause the claimRewards function to revert.

An attacker can increase the rewards count of the native asset for all stakers which the contract doesn't have which will DOS the claimRewards function and as a result the stakers lost EUROs and rewards of the all the asset tokens being unclaimable.

Impact

A staker can drain all the native currency held by the LiquidationPool contract as rewards and cause the claimRewards function to revert.

An attacker can increase the rewards count of the native asset for all stakers which the contract doesn't have which will DOS the claimRewards function and as a result the stakers lost EUROs and rewards of the all the asset tokens being unclaimable and lock all the assets in the contract.
Even if the users want to unstake here their position.EUROs already got decreased inside the distributeAssets function.
_position.EUROs -= costInEuros;
positions[holders[j]] = _position;

Tools Used

manual.

Recommendations

Add onlyManager modifier in the distributeAssets function.

Updates

Lead Judging Commences

hrishibhat Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

distributeAssets-issue

Support

FAQs

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

Give us feedback!