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

Missing access control on the distributeAssets() function allows anyone to call it with arbitrary arguments which can lead to loss of funds

iamandreiski

Summary

LiquidationPool::distributeAssets() function is meant to be called by LiquidationPoolManager::runLiquidation() with the list of assets which are based on the TokenManager's tokens array, their addresses and balance, as well as the HUNDERED_PC constant and the collateral rate, as the two other arguments but due to missing access control, the distributeAssets() function can be called by an arbitrary user with arbitrary arguments which can break the accounting system and lead to loss of funds.

Vulnerability Details

LiquidationPool::distributeAssets() function should be called with the ILiquidationPoolManager.Asset[] array of assets calculated and called by the runLiquidation()function, together with the HUNDRED_PC and collateralRate() constants.

function distributeAssets(ILiquidationPoolManager.Asset[] memory _assets, uint256 _collateralRate, uint256 _hundredPC) external payable {
consolidatePendingStakes();
(,int256 priceEurUsd,,,) = Chainlink.AggregatorV3Interface(eurUsd).latestRoundData();
uint256 stakeTotal = getStakeTotal();
uint256 burnEuros;
uint256 nativePurchased;
for (uint256 j = 0; j < holders.length; j++) {
Position memory _position = positions[holders[j]];
uint256 _positionStake = stake(_position);
if (_positionStake > 0) {
for (uint256 i = 0; i < _assets.length; i++) {
ILiquidationPoolManager.Asset memory asset = _assets[i];
if (asset.amount > 0) {
(,int256 assetPriceUsd,,,) = Chainlink.AggregatorV3Interface(asset.token.clAddr).latestRoundData();
uint256 _portion = asset.amount * _positionStake / stakeTotal;
uint256 costInEuros = _portion * 10 ** (18 - asset.token.dec) * uint256(assetPriceUsd) / uint256(priceEurUsd)
* _hundredPC / _collateralRate;
if (costInEuros > _position.EUROs) {
_portion = _portion * _position.EUROs / costInEuros;
costInEuros = _position.EUROs;
}
_position.EUROs -= costInEuros;
rewards[abi.encodePacked(_position.holder, asset.token.symbol)] += _portion;
burnEuros += costInEuros;
if (asset.token.addr == address(0)) {
nativePurchased += _portion;
} else {
IERC20(asset.token.addr).safeTransferFrom(manager, address(this), _portion);
}
}
}
}
positions[holders[j]] = _position;
}
if (burnEuros > 0) IEUROs(EUROs).burn(address(this), burnEuros);
returnUnpurchasedNative(_assets, nativePurchased);
}
​

distributeAssets() should be called only by runLiquidation()

But due to missing access control which would be the onlyManager modifier or a require statement which would require the msg.sender to be the LiquidationManager address, distributeAssets can be called by anyone with arbitrary arguments which can significantly impact the distribution/burning of assets and lead to loss of funds.

Proof of Concept

  • Alice is a malicious user which decides to call the distributeAssets function with arbitrary arguments:

  • For the sake of the example, let's assume the function is called with ETH as _assets (1 ETH), a _collateralRate of "1" and a _hundredPc of 10_000.

  • stakeTotal is 20_000e18;

  • Assume that there are 2 holders in the pool.

  • If we take an arbitrary holder from the holders, with the assumption that he has TST 6_000e18 and EUROs 4_000e18 as stakes, 4_000e18 will be his stake.

  • _portion = 1e18 * 4_000e18 / 20_000e18 = 2e17 (or 0.2e18);

  • costInEuros = 2000e18(EUR Price) * 10_000 / 1 = 20_000_000e18. (or 2e25)

  • if-clause, since the cost is higher than the position:

  • _portion = 0.2e18 * 4_000e18 / 2e25 = 4e13 or 0.00004 in decimal representation.

  • costInEuros = 4_000e18.

  • _position.EUROs = 4_000e18 - 4_000e18 = 0

  • rewards = 4e13 of the ETH = 0.08 EUROs

  • burnEuros = 4000 EUROs.
    ....

This is just one example of how we can manipulate the protocol using native ETH to burn more tokens than the value we've sent and modify the user's balances/rewards causing catastrophic impact.

This won't work with ERC-20 tokens as it uses safeTransferFrom which will require approval from the manager (in runLiquidation()).

Impact

Calling the distributeAssets() function with arbitrary values using ETH as the assets (+ sending some amount of ETH), but changing the HUNDRED_PC and/or collateralRate() can significantly impact the calculations of the costInEuros variable, which would impact the EUROs position, the amount of tokens that need to be burned, as well as the amount of tokens which are going to be sent to the manager contract. This can negatively impact calculations and balances and cause a significant loss for the LiquidationManager and the holders in the LiquidationPool.

Tools Used

  • Manual Review

Recommendations

Add the OnlyManager modifier to 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.