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

Division before multiplication incurs unnecessary precision loss

bhilare71

Summary

Precision Loss Due to Division Preceding Multiplication

Vulnerability Details

In LiquidationPool::distributeAssets() in the following line of code :

uint256 costInEuros = _portion * 10 ** (18 - asset.token.dec) * uint256(assetPriceUsd) / uint256(priceEurUsd) * _hundredPC / _collateralRate;

This code calculates the cost in Euros based on several factors. Let's identify the potential precision loss.

1-> _portion * 10 ** (18 - asset.token.dec): This part adjusts the _portion by scaling it according to the token's decimal value.

2-> * uint256(assetPriceUsd): Converts the assetPriceUsd to a uint256.

3-> / uint256(priceEurUsd): Divides the value obtained in step 2 by priceEurUsd. If the result from the multiplication step has a lot of decimal places, dividing by priceEurUsd could truncate the result.

4-> * _hundredPC: Multiplying by _hundredPC may further amplify precision loss, especially if the result from the division was not an exact integer.

5-> / _collateralRate: Divides the result from step 4 by _collateralRate.

Impact

The issue lies in the order of operations. Intermediate results can become very large or have many decimal places, leading to a loss of precision in the final result.

Tools Used

Manual reviewing of the code

Recommendations

It's advisable to rearrange the operations to perform multiplication before division. This involves calculating the intermediate results separately before combining them.
Below changes in order of operation can be used instead:

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;
+ uint256 costInEuros = (_portion * 10 ** (18 - asset.token.dec) * uint256(assetPriceUsd) * _hundredPC) / (uint256(priceEurUsd) * _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);
}

This arrangement ensures that multiplication is done before division, reducing the risk of precision loss.

Updates

Lead Judging Commences

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

precision

Support

FAQs

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