Replace borrow index

[pakim249CAL]

Pull Request

Issue(s) fixed

This pull request fixes #1173

[pakim249CAL]

In the process of making this PR, I've discovered that replacing exchangeRateStored with a cached value is not guaranteed to be the same.

https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/CToken.sol#L293

The exchange rate can suffer minor rounding errors if any funds are supplied or borrowed in the same transaction as can be seen in this code, as the exchange rate is a function of the total supply and borrows in the cToken contract.

And so only a select few instances of this are cached. Specifically, only the ones that are right after an index update, as this index should be correct since there has been no pool interaction to mess with the index at these points.

Borrow index is replaceable with a cached value just fine though.

[MerlinEgalite]

Changing the base of the PR.

[MerlinEgalite]

That makes me think: should we update each time the indexes instead? or check that the lastPoolSupplyIndex is the same as the stored one (meaning that there was no update)?
Since here there could be a discrepancy between the exchangeRateStored used in the code and the one used in the IRM, right?

[Rubilmax]

If I'm not mistaken, the issue impacts the IRM as well: we took for granted that calling accrueInterest was updating the supply & borrow indexes and that afterwards, supply & borrow indexes were constant within a given block.

But here, we learn that the theorem holds only for the borrow index. So we should rework the IRM to use exchangeRateStored instead of the cached supply pool index (and anywhere else), or ignore this. I don't think ignoring this should be considered, as the difference can be major: totalCash & totalBorrows can be completely manipulated by the user and thus incurr a large difference between exchangeRateStored & lastSupplyPoolIndex

[QGarchery]

Yes we should definitely investigate this, here is a first step towards this:
https://github.com/morpho-labs/morpho-yellowpaper/issues/117

The issue is linked to #997, it seems like we missed this implication at the time.

I'm not sure we should use exchangeRateStored everywhere, maybe we should use a cached value that is fixed in the block, to go back to the previous, expected behavior. In fact, I'm not even sure that this is really an issue

[MerlinEgalite]

If we don't use exchangeRateStored that means we should compute the P2P index each time to be consistent, right? Can we estimate the loss implied by that?

I think the borrowIndex is not subject to this, so only the supply index is affected. And I think, we just tend to underestimate the supply index, so we tend to underestimate what Morpho has on the pool, and what Morpho owes to suppliers. Am I missing something?

[Rubilmax]

I approve the changes in this PR. Isn't it an optimization though? Is it the last optimization we're merging and deploying?
Because it's in contradiction with the measures we've taken recently, which goal is to decrease the nb of changes we apply to the deployed code (or I may have misunderstood)

This optimization is safe to me, but still: do we want to pursue such work in the future?

I didn't see exchangeRateStored was replaced with lastSupplyPoolIndex. I need to think about this

[Rubilmax]

so we tend to underestimate what Morpho has on the pool, and what Morpho owes to suppliers. Am I missing something?

I disagree with this: with the changes of this PR, it seems Morpho could be overestimating the supplier's balance on the pool (because we are dividing the amount by the cached index)

For this reason, I consider this change unsafe (I may be mistaken)

---

We could in parallel think about what I highlight just above:

Isn't it an optimization though? Is it the last optimization we're merging and deploying?
Because it's in contradiction with the measures we've taken recently, which goal is to decrease the nb of changes we apply to the deployed code (or I may have misunderstood)

do we want to pursue such work in the future?

We may not want to pursue such optimizations anymore (because it's not necessary to Morpho for now)

[MathisGD]

If my understanding is correct, the issue is that exchangeRateStored is not always exactly constant during a block. This means that if we do some accounting with our cached exchange rate called earlier in the block, we may do some wrong accounting. But if we constantly call exchangeRateStored (which is the case right now), everything is ok.

Then either we only use the cached borrowIndex, either we close this PR (which would go in the direction that we decided to take, as mentioned by Romain).

[MathisGD]

Not mergeable atm for the reasons mentioned above.

[MerlinEgalite]

There are other things I want to clarify too:

• If the cached value is considered as wrong, every getter returning this value could be considered as such, correct? What's the impact for integrators? What's the worst-case scenario if someone can manipulate the exchangeRateStored?
• What's the impact on the P2P indexes computation? As the value will not change during a block?

[MathisGD]

What's the impact on the P2P indexes computation? As the value will not change during a block?

I think that it is ok for P2P indexes computations.

[Rubilmax]

I think that it is ok for P2P indexes computations.

Because we are using exchangeRateStored

[MerlinEgalite]

I think that it is ok for P2P indexes computations.

Because we are using exchangeRateStored

Yes but within a block P2P indexes are fixed while exchangeRateStored isn't.

[MathisGD]

Yes but within a block P2P indexes are fixed while exchangeRateStored isn't.

Ok you are right, let's not make too quick assumptions, we need to make sure that it is not breaking anything. I'm notably thinking of 100% delta situations, were this could cause issues.

[MerlinEgalite]

Yes but within a block P2P indexes are fixed while exchangeRateStored isn't.

Ok you are right, let's not make too quick assumptions, we need to make sure that it is not breaking anything. I'm notably thinking of 100% delta situations, were this could cause issues.

So is it a R&D topic or should the solidity do the research?

[MathisGD]

As you want, we have not started anything yet.

[MerlinEgalite]

Ok after some time on this, I don't think there's any issue with the exchangeRateStored and the cached P2P indexes, since the case where the exchange rate is pumped is already covered in the IRM and the other computations are done with exchangeRateStored that will be used on the pool.

[pakim249CAL]

Ok after some time on this, I don't think there's any issue with the exchangeRateStored and the cached P2P indexes, since the case where the exchange rate is pumped is already covered in the IRM and the other computations are done with exchangeRateStored that will be used on the pool.

Just to be sure then, this PR is good as is?

[MerlinEgalite]

Just to be sure then, this PR is good as is?

No we must use the exchangeRateStored instead of the cached one to mirror the pool behavior IMO. I think this is the same for the borrowIndex but this must be confirmed

[MathisGD]

LGTM !

[MerlinEgalite]

LGTM 🎉

[MerlinEgalite]

• exchangeRateStored can changed whithin a block
• borrowIndex is updated only one at the beginning of the block