trustline-inc / probity

A transparent asset-based lending protocol implemented for the EVM.
https://docs.trustline.co
Other
3 stars 1 forks source link

Teller: would more equity be created than debt if currentUtilization is much lower than`lastUtilization` #250

Closed shine2lay closed 2 years ago

shine2lay commented 2 years ago

In Teller, we use lastUtilization in equityAccumulator calculation even though the we are getting current totalDebt and totalEquity values. I am not quite sure why lastUtilization is being used here and i think it may potentially create more equity than debt if current utilization ratio is much lower than lastUtilization i think. Will need to test in order to verify this.

mrosendin commented 2 years ago

I'm a bit confused -

The current Teller contract tracks lastUtilization and lastUpdated for each asset - why?

The utilization tracks the Aurei in the treasury, and last updated tracks when the accumulators were last updated. The accumulators are used to calculate Aurei balances, so they do not need to be within the Asset struct. I believe same goes for protocolFee. Maybe I am missing something, but it is not clear to me what the purpose of Teller's asset data struct

mrosendin commented 2 years ago

The previous comment may be a separate issue - to address this specific issue:

In version prior to the rework, the operations for lending and borrowing (what we call modifyEquity and modifyDebt now) updated the accumulators within the implementation. Current implementation doesn't do this. I don't think it was an issue in the prior implementation, so maybe can can just call Teller.updateAccumulators in modifyDebt and modifyEquity to keep the lastUtilization always correct?

shine2lay commented 2 years ago

I'm a bit confused -

The current Teller contract tracks lastUtilization and lastUpdated for each asset - why?

This is a part of the code that you wrote that we just pulled in from earlier version, I understand the need for lastUpdated because it gives it the time frame for how long it's been since the accumulator has been updated. I think lastUtilization should be removed in favor of calculation of utilization on the spot.

The utilization tracks the Aurei in the treasury, and last updated tracks when the accumulators were last updated. The accumulators are used to calculate Aurei balances, so they do not need to be within the Asset struct. I believe same goes for protocolFee. Maybe I am missing something, but it is not clear to me what the purpose of Teller's asset data struct

I believe lastUpdated is necessary because each asset is initiated at different time period.

maybe can can just call Teller.updateAccumulators in modifyDebt and modifyEquity to keep the lastUtilization always correct?

I think we can just get rid of lastUtilization in favor of calculating utilizationRatio within the updateAccumulator

mrosendin commented 2 years ago

This is a part of the code that you wrote that we just pulled in from earlier version, I understand the need for lastUpdated because it gives it the time frame for how long it's been since the accumulator has been updated. I think lastUtilization should be removed in favor of calculation of utilization on the spot.

According to the git blame, you added the struct for Asset (back then called Collateral):

https://github.com/trustline-inc/probity/blame/0548679dc254bbb5f732e9747bde27196ed1be8b/contracts/probity/Teller.sol#L42

I believe lastUpdated is necessary because each asset is initiated at different time period.

No, lastUpdated is tracking when the rate accumulator last updated. I am not sure what it has to do with any of the supported assets because it is related to the internal system rate for Aurei. Unless there is a reason for the Asset struct that I am not seeing, we should remove the struct mapping and just use top-level uint256 variables

I think we can just get rid of lastUtilization in favor of calculating utilizationRatio within the updateAccumulator

It would not be accurate unless it's also called during modifyDebt and modifyEquity, because someone can borrow all the supply, and the APR won't change until someone calls Teller.updateAccumulators, so technically he can borrow all of the supply at 1% APR. He can then call Teller.updateAccumulators and immediately repay the loan, so then everyone is stuck paying 100% APR.

mrosendin commented 2 years ago

I'm still investigating, now into Vault.updateAccumulators. I think the divergence here is that you added the struct to represent the Aurei backed by assets. Sorry about my previous comment about lastUpdated as it seems each asset now has its own rate accumulators. My apologies.

shine2lay commented 2 years ago

It would not be accurate unless it's also called during modifyDebt and modifyEquity, because someone can borrow all the supply, and the APR won't change until someone calls Teller.updateAccumulators, so technically he can borrow all of the supply at 1% APR. He can then call Teller.updateAccumulators and immediately repay the loan, so then everyone is stuck paying 100% APR.

In the case you described, equity position holders has incentive to call updateAccumulator right after the borrower has taken out the new debt. In the other extreme where debt position holder who has paid off 100% of the supply has incentive to call updateAccumulators right after to make sure the APR drops.

I think this should be taken care of with the lib to do these calls atomic and not be apart of the vault engine.

mrosendin commented 2 years ago

In the case you described, equity position holders has incentive to call updateAccumulator right after the borrower has taken out the new debt. In the other extreme where debt position holder who has paid off 100% of the supply has incentive to call updateAccumulators right after to make sure the APR drops.

But then we would need to build some sort of tool to show how far the interest rate has strayed since the last update, so that users can evaluate the cost/benefit for the decision to update the accumulator.

I think this should be taken care of with the lib to do these calls atomic and not be apart of the vault engine.

I agree that the lib would be the best way to go IMO. The lib is a priority feature