Breeje - Multiplication after Division can cause larger Precision loss

[sherlock-admin]

Breeje

medium

Multiplication after Division can cause larger Precision loss

Summary

There are multiple instances in the code where multiplication is done on the result of division.

Vulnerability Detail

In several critical calculations like calculating the payouts or current market price, multiplication is done on the result of division which can lead to larger precision loss.

Instances:

1. payoutFor method in BondBaseOSDA.sol:

File: BondBaseOSDA.sol

548:    uint256 fee = amount_.mulDiv(_teller.getFee(referrer_), 1e5);
549:    uint256 payout = (amount_ - fee).mulDiv(terms[id_].scale, marketPrice(id_));

Link to Code

2. _currentMarketPrice method in BondBaseOSDA.sol:

File: BondBaseOSDA.sol

Instance 1:

431:    uint256 price = term.oracle.currentPrice(id_).mulDivUp(
440:    price = price * term.oracleConversion;

Instance 2:

451:    uint256 expectedCapacity = initialCapacity.mulDiv(timeRemaining, uint256(term.length));
472:    (term.decaySpeed * (expectedCapacity - market.capacity)) /
477:    uint256 factor = (term.decaySpeed * (market.capacity - expectedCapacity)) /
482:    return price.mulDivUp(adjustment, ONE_HUNDRED_PERCENT);

Link to Code

3. maxAmountAccepted method in BondBaseOFDA:

File: BondBaseOFDA.sol

512:    uint256 quoteCapacity = market.capacityInQuote
513:        ? market.capacity
514:        : market.capacity.mulDiv(price, term.scale);
515:    uint256 maxQuote = market.maxPayout.mulDiv(price, term.scale);
516:    uint256 amountAccepted = quoteCapacity < maxQuote ? quoteCapacity : maxQuote;

523:    uint256 estimatedFee = amountAccepted.mulDiv(

Link to Code

Impact

Larger Precision Loss.

Code Snippet

Given above.

Tool used

Manual Review

Recommendation

Multiply all the numerators first and then divide it with the product of all the denominator to get the least possible precision loss.

[Oighty]

In general, I think this is mitigated by the scaling we use, but I'll try to address these individually.

1. The fee is calculated by a mulDiv with the feePercent multiplied first and then divided by 100%. We have to finish this calculation here because it is used in a subtraction on the next line.

2.1. This calculation could increase precision slightly by multiplying the oracleConversion prior to implementing the discount. Will fix this.

2.2. expectedCapacity has to be pre-computed because it's used in a subtraction operation. Additionally, there is an addition or subtraction in the adjustment calculation so the values must be pre-computed before the final operation. I don't think the precision of these can increase. Even if it did, the code would be much less legible.

3. I don't see how the precision can be improved because the maxQuote and quoteCapacity amounts have to be pre-computed to allow for the comparison to get amountAccepted. Perhaps if you did the comparison and then recalculated the right one, but the precision loss is negligible and the amount we're calculating here is conservative anyways.

[UsmannK]

Escalate for 10 USDC

The watson does not show any impact or measurements of the supposed precision loss. The sponsor says that they think the issue is mitigated and provides reasoning. They say "I don't see how the precision can be improved".

No impact is shown in the report or observed by the sponsor. Report lacks POC and does not show any particular negative effects of the arithmetic. This issue should not be a medium according to the Sherlock guidance. The guidance for "Medium" states:

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost.

No such scenario has been shown.

[sherlock-admin]

Escalate for 10 USDC

The watson does not show any impact or measurements of the supposed precision loss. The sponsor says that they think the issue is mitigated and provides reasoning. They say "I don't see how the precision can be improved".

No impact is shown in the report or observed by the sponsor. Report lacks POC and does not show any particular negative effects of the arithmetic. This issue should not be a medium according to the Sherlock guidance. The guidance for "Medium" states:

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost.

No such scenario has been shown.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Breeje16]

Escalate for 10 USDC

The impact is legit given the sponsors confirmed the finding and are going to Fix it. Especially in the second case, the Precision loss could have been avoided but it was not done.

The impact is quite understood in these cases. It's different in different cases. In the second case, _currentMarketPrice is going to return the value of price lower than what it should return because of precision loss. As price is a critical parameter, it impacts every methods which uses this price in the BondBaseOSDA.sol e.g: User can get more payout tokens in purchaseBond because of lesser price value than actual.

File: BondBaseOSDA.sol

378:    payout = amount_.mulDiv(term.scale, price); // @audit 'price' value contains precision loss.

Given that the precision loss issue is legit here which requires a fix and it has critical usage, it should be considered a valid medium finding which even sponsors confirmed. They are going to fix it too.

Even in the past, such findings have been rewarded as medium: Link

[sherlock-admin]

Escalate for 10 USDC

The impact is legit given the sponsors confirmed the finding and are going to Fix it. Especially in the second case, the Precision loss could have been avoided but it was not done.

The impact is quite understood in these cases. It's different in different cases. In the second case, _currentMarketPrice is going to return the value of price lower than what it should return because of precision loss. As price is a critical parameter, it impacts every methods which uses this price in the BondBaseOSDA.sol e.g: User can get more payout tokens in purchaseBond because of lesser price value than actual.

File: BondBaseOSDA.sol

378:    payout = amount_.mulDiv(term.scale, price); // @audit 'price' value contains precision loss.

Given that the precision loss issue is legit here which requires a fix and it has critical usage, it should be considered a valid medium finding which even sponsors confirmed. They are going to fix it too.

Even in the past, such findings have been rewarded as medium: Link

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Oighty]

Issue fixed here: https://github.com/Bond-Protocol/bonds/pull/50

[xiaoming9090]

Fixed in https://github.com/Bond-Protocol/bonds/pull/50

[hrishibhat]

Escalation accepted

Given that there are no details/calculations presented on the loss this issue could potentially cause, considering this issue a low severity.

[sherlock-admin]

Escalation accepted

Given that there are no details/calculations presented on the loss this issue could potentially cause, considering this issue a low severity.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.