Low Risk (L4): logic in getter estimateSpendUntilTimestamp is miscalculated

[petrovska-petro]

Severity: if it is used externally can lead to false conclusions in the figure and core level does not have critical impact.

Context: while reviewing the test "should calculate spend for timestamp correctly" the numerical expectations looks incorrect, since the the token used for testing has 6 decimals

const currentTime = Math.floor(Date.now() / 1000);
const oneWeekInSeconds = 7 * 24 * 60 * 60;
const timestampOneWeekFromNow = currentTime + oneWeekInSeconds;

let spend = await injector.estimateSpendUntilTimestamp(timestampOneWeekFromNow)
expect(spend).to.equal(500000000000400000000n); // @audit why this number??????

Recommendation: first it is worth investigating why that value is expected in the test 500000000000400000000n, while the distribution it is simply 200000000/week for 2 weeks as per the suite setter.

Also, in estimateSpendUntilTimestamp seems to signal that would like to estimate the spend up to a specific timestamp, in the suite the value after a week would like to be checked for a single gauge, the expectation there would be 200000000 for the single week, and if we would check for 2 weeks (timestampOneWeekFromNow * 2) the expectation would be 400000000, but the values in the test does not seem to expect the same figures, which highlights that a miscalculation is happening in the loop.

- for (uint256 j = target.periodNumber; j < target.maxPeriods; j++) {
+ for (uint256 j; j < target.maxPeriods; j++) {
-   if (block.timestamp + (j * 604800) <= timestamp) { 
+   if (block.timestamp + ((j + 1) * 604800) <= timestamp) {
        spendUntilTimestamp += target.amountPerPeriod;
    }
}

And, it is not considering the factor that the gauge programStartTimestamp in the getter at all, which can lead again to miscalculations reported by that method. That is something still to be included in the code snipper above

In another note, seems the method is basically a getter it is worth reconsidering its name convention to getEstimateSpendUntilTimestamp and follow the pattern.

Review stage

Balancer Maxis:

• Acknowledge
• Fixed

Onchainification Labs:

• Acknowledge correction/mitigation

[Tritium-VLK]

The test seems wrong.. You are right.

I don't understand the second part.

We want to know how much money will leave the injector before a given timestamp. Why would we look at how much money is scheduled to leave the injector by 1 week after that?

[petrovska-petro]

I don't understand the second part.

Regarding the programStartTimestamp consideration the point is that if you'd like to know how much tokens are leaving the injector by specific timestamp, there should be a consideration in checking if actually for gauges[i] has started streaming before that timestamp or not. Otherwise, it will consider that the tokens as they had left the injector contract already, but since could happen that programStartTimestamp > target timestamp for a specific gauge, then the amountPerPeriod should not be sum into spendUntilTimestamp. In other words, that consideration should be added into the code to calculate properly the spendUntilTimestamp output.

Why would we look at how much money is scheduled to leave the injector by 1 week after that?

We're referring to the scenario of the test, where the constant timestampOneWeekFromNow seems to signal that a week from now how much has being emitted, not a week after the 1st. The example it was more to illustrate the error in the test.

[Tritium-VLK]

You're right. This is wrong. See #46. It's an unnecessary function that adds confusion.

[petrovska-petro]

concerning method was removed in #46 and its associates tests. this issue is now irrelevant, safe to close