Skip to content

Latest commit

 

History

History
100 lines (64 loc) · 2.86 KB

072.md

File metadata and controls

100 lines (64 loc) · 2.86 KB

Jovial Teal Butterfly

High

Precision loss in BondToken::getIndexedUserAmount function due to division by SHARES_DECIMALS in every iteration.

Summary

BondToken::getIndexedUserAmount is used to get shares owned by an user. In the function the shares is calculated as - https://github.com/sherlock-audit/2024-12-plaza-finance/blob/14a962c52a8f4731bbe4655a2f6d0d85e144c7c2/plaza-evm/src/BondToken.sol#L195

    for (uint256 i = userPool.lastUpdatedPeriod; i < period; i++) {
      shares += (balance * globalPool.previousPoolAmounts[i].sharesPerToken).toBaseUnit(SHARES_DECIMALS);
    }

the toBaseUnit does following -

  function toBaseUnit(uint256 amount, uint8 decimals) internal pure returns (uint256) {
    return amount / (10 ** decimals);
  }

But this approach can lead to, precision loss, as the division is done for each iteration instead of whole.

for example consider a loop of 2 iteration only -

1st iteration

balance = 105555 globalPool.previousPoolAmounts[0].sharesPerToken = 2000

shares = (109999*2000)/ 10**6 = 2199

2nd iteration

balance = 105555 globalPool.previousPoolAmounts[1].sharesPerToken = 4000

shares = 2199 + (109999*4000)/ 10**6 = 2199 + 4399 = 6598

If the final product is divide by the shares then the precession loss will be comparatively less.

i.e.

[(109999*2000) + (109999*4000)]/10**6 = 6599

It can be clearly seen there is loss of 1 unit.

As for the above the amount taken are very small and only of 2 iterations, but in real life BOND_ETH has 18 decimals and the length of loop could be very large, in that case the precision loss could be pretty high.

Another analogy -

current architecture - 1.7/1 +1.4/1 = 2 if divided the whole - [1.7 + 1.4]/1 = 3

Leading to loss by 1 uint. This analogy could also be applied.

Root Cause

Division of all elements by SHARES_DECIMALS for each iteration of for loop.

Internal Pre-conditions

No response

External Pre-conditions

No response

Attack Path

No response

Impact

  1. Can lead to less number of shares calculation for user.
  2. The share of an user is stored in a mapping userAssets[user].
  3. In Distributer::claim the shares of an user is fetched via the function
    uint256 shares = bondToken.getIndexedUserAmount(msg.sender, balance, currentPeriod)
                              .normalizeAmount(bondToken.decimals(), IERC20(couponToken).safeDecimals());
  1. as the updated userAssets[user] is less as it should be due to precession, the user will claim less reward.

PoC

No response

Mitigation

Avoid dividing for each iteration of loop, instead divide the fincal product by SHARES_DECIMAL something like -

    for (uint256 i = userPool.lastUpdatedPeriod; i < period; i++) {
      shares += (balance * globalPool.previousPoolAmounts[i].sharesPerToken);
    }
    return shares.toBaseUnit(SHARES_DECIMALS);