Revert in the `computeRoi` function due to `ln`calculation

[hats-bug-reporter[bot]]

Github username: @0xmahdirostami Submission hash (on-chain): 0x7c402da62ea6108b6870add5bbba4ee087008c31a0397daa5ac077b560fe55be Severity: high

Description: Description\ The computeRoi function uses computeCvgExpected. In the computeCvgExpected function, If composedFunctionis 1, it uses ln:

else if (composedFunction == 1) {
            cvgExpected = ABDKMathQuad
                .ln(timeRatio)
                .div(ABDKMathQuad.ln(ABDKMathQuad.fromUInt(totalOutToken / 10 ** 18)))
                .add(ABDKMathQuad.fromUInt(1));

also as :0 < timeRatio < 1we have -∞ < ln(timeRatio) <0 , so we do ln(timeRatio) / ln(totalOutToken / 10 ** 18) to have
-1<ABDKMathQuad.ln(timeRatio).div(ABDKMathQuad.ln(ABDKMathQuad.fromUInt(totalOutToken / 10 ** 18))) < 0,
but if ln(totalOutToken / 10 ** 18) is less than ln(timeRatio) so ABDKMathQuad.ln(timeRatio).div(ABDKMathQuad.ln(ABDKMathQuad.fromUInt(totalOutToken / 10 ** 18))) will become greater than -1 and cvgExpected becomes negative, so it causes revert in computeRoi.

Impact\ Revert in computeRoi may cause a lot of problems as computeRoi is used in _depositRoi(is used in getBondView) and _computeCvgBondUsdPrice( is used in deposit and getBondView)

• which makes deposit revert untill ln(totalOutToken / 10 ** 18) becomes bigger than unsigned ln(timeRatio).

Attachments

1. Proof of Concept (PoC) File

   In Bond Calculator tests: VESTING_TIME is: 43200, so log(1/43200) = -15.3987436919, so if ln(totalOutToken / 10 ** 18) < 15.3987436919, it will be reverted.
   ln(43200) = 15.3987436919 so if totalOutToken / 10 ** 18 < 43200, it will be reverted.

test:

  const VESTING_TIME = 43200; // Half a day

  const INCR = 2000;

  before(async () => {
    helper = new TestHelper();
    const { contracts, users } = await loadFixture(deployBondCalculatorFixture);

    bondCalculatorContract = contracts.bondCalculatorContract;
  });
  it("Should compute NumberTokenReal/NumberTokenExpected ratio", async () => {
    let ntrNtcRatio = await bondCalculatorContract.computeNtrDivNtc(
      1,
      VESTING_TIME,
+      1,
+      ethers.parseEther("43000"),
      ethers.parseEther("1000")
    );
    console.log(ntrNtcRatio);

$ npx hardhat test --grep "Calculator"

Output:

  Bond Calculator tests

    1) Should compute NumberTokenReal/NumberTokenExpected ratio
    ✓ Should compute the bond ROI

And if totalOutToken / 10 ** 18 > 43200, it will be passed.

test:

  const VESTING_TIME = 43200; // Half a day

  const INCR = 2000;

  before(async () => {
    helper = new TestHelper();
    const { contracts, users } = await loadFixture(deployBondCalculatorFixture);

    bondCalculatorContract = contracts.bondCalculatorContract;
  });
  it("Should compute NumberTokenReal/NumberTokenExpected ratio", async () => {
    let ntrNtcRatio = await bondCalculatorContract.computeNtrDivNtc(
      1,
      VESTING_TIME,
+      1,
+      ethers.parseEther("44000"),
      ethers.parseEther("1000")
    );
    console.log(ntrNtcRatio);

$ npx hardhat test --grep "Calculator"

Output:

  Bond Calculator tests
13243060n
    ✓ Should compute NumberTokenReal/NumberTokenExpected ratio
    ✓ Should compute the bond ROI

2. Revised Code File (Optional)

   Recommendation, Make sure that ln(totalOutToken / 10 ** 18) is bigger than unsigned ln(timeRatio).
   For example:
   

   • if time is 43200 so ln(totalOutToken / 10 ** 18) must be bigger than unsigned ln(1/43200) => 15.3987436919
   • if time is 432000 so ln(totalOutToken / 10 ** 18) must be bigger than unsigned ln(1/432000) => 18.7206717868

[0xmahdirostami]

Sorry, here ABDKMathQuad.ln(timeRatio).div(ABDKMathQuad.ln(ABDKMathQuad.fromUInt(totalOutToken / 10 ** 18))) will become greater than -1. Instead of greater, it should be lower. By greater, I mean unsigned greater.

[walk-on-me]

Hello, Thanks a lot for your attention.

This is a nice catch.

In fact with the function is : f(t) = (ln(t/T) / ln(N))+1

As we know, ln(x) with x ]0;1] is defined on ]-infinite; 0]

Here is a representation of the function https://www.desmos.com/calculator/epedwlev4z?lang=fr

On this representation, we can see that :

• The smaller N is, the more time the curve will stay under 0
• The bigger T is, the more time the curve will stay under 0

This time the curve stays under 0 is the time where the bond is not available. The only risks is that the function will revert. Users will not be able to deposit in the bond during the first S seconds of the bonds. So no funds will be lost or blocked.

As the impact is verry small ( example on the desmos link => 10s of outage at the begining of the bond )

We cannot afford an high severity for it but more a Low.

The fix we found is that, if we find a negative result we'll pass 0 now.

[0xmahdirostami]

@walk-on-me The fix sounds good to me. But for the label, I should mention that:

1. the impact is temporarily disabling the contract's function, and the likelihood is high, Its severity should be medium.
2. In the example on the demos link => 10s of outage at the begining of the bond BUT as you said it could be so much more, for example, if N=10 ETH and VESTING_TIME will be 1 day → 2.4 hours of outage

[walk-on-me]

After discussion with @0xmahdirostami

We agree that for a more general usage of this contract, this outage at the beginning of the bond can be more problematic

( ex of 10 ETH on 1 day )

This issue passes so to medium