hifi-finance / hifi

Monorepo implementing the Hifi fixed-rate, fixed-term lending protocol
https://app.hifi.finance
Other
105 stars 15 forks source link

fix(flash-swap): `repayAmount` calculation in `liquidateBorrow(...)` #59

Closed scorpion9979 closed 2 years ago

scorpion9979 commented 2 years ago

We can't use the debt amount returned by getDebtAmount(...), as it could yield more collateral than actually exists in the vault in the edge case of a multi-collateral single-bond vault. And we also can't use the repay amount that is calculated by getRepayAmount(...), as it could return an amount larger than the entire debt amount in the edge case of a single collateral multi-bond vault.

The fix calculates hypotheticalRepayAmount from the vault's collateralAmount using getRepayAmount(...), then compares it with debtAmount and uses it as a cap for the repay amount to be passed to liquidateBorrow(...).

PaulRBerg commented 2 years ago

The fix calculates hypotheticalRepayAmount from the vault's collateralAmount using getRepayAmount(...), then compares it with debtAmount and uses it as a cap

Let me get this right. You are doing this to account for the single collateral multi-bond situation, aren't you? That is, you are making debtAmount the cap because if there are multiple bonds, hypotheticalRepayAmount could be more way bigger than what can be repaid for one single bond.

scorpion9979 commented 2 years ago

The fix calculates hypotheticalRepayAmount from the vault's collateralAmount using getRepayAmount(...), then compares it with debtAmount and uses it as a cap

Let me get this right. You are doing this to account for the single collateral multi-bond situation, aren't you? That is, you are making debtAmount the cap because if there are multiple bonds, hypotheticalRepayAmount could be more way bigger than what can be repaid for one single bond.

I would say that the single collateral multi-bond situation is an extreme case where this happens. But yeah the idea is that for some vaults, the calculated hypotheticalRepayAmount could be bigger than a single bond's entire debt amount.

PaulRBerg commented 2 years ago

for some vaults, the calculated hypotheticalRepayAmount could be bigger than a single bond's entire debt amount.

I don't understand how hypotheticalRepayAmount can be bigger than debtAmount if the vault is not a multi-bond one. Sure, there can be two-collateral/ three-bond vaults, but that is both a multi-collateral and a multi-bond vault.

scorpion9979 commented 2 years ago

for some vaults, the calculated hypotheticalRepayAmount could be bigger than a single bond's entire debt amount.

I don't understand how hypotheticalRepayAmount can be bigger than debtAmount if the vault is not a multi-bond one. Sure, there can be two-collateral/ three-bond vaults, but that is both a multi-collateral and a multi-bond vault.

Yes, hypotheticalRepayAmount can be bigger than debtAmount in two cases:

  1. Multi-collateral and a multi-bond vaults.
  2. Single-collateral multi-bond vaults
PaulRBerg commented 2 years ago

Cool. Unfortunately I can't merge this until all of the added code is tested :)

Last time we hurried with merging and that resulted in introducing a bug.

PaulRBerg commented 2 years ago

I don't think this implementation is correct. I just ran the tests and the "when the collateral ratio is lower than 110%" test is failing.

The reason is that the repay amount gets lowered to the value returned by getRepayAmount:

mintedHTokenAmount 10000000000000000000000
collateralAmount 100000000
debtAmount 10000000000000000000000
hypotheticalRepayAmount 9090909090909090909090
repayAmount 9090909090909090909090
truncatedRepayAmount 9090909090909090909090

But the minted hToken amount would have normally triggered a revert. This may be an unintended consequence of your algorithm.

scorpion9979 commented 2 years ago

t. I just ran the tests and the "when the collateral ratio is lower than 110%" test is failing.

The reason is that the repay a

Actually, this is intended. The assumption here is that the client would pass a swapAmount that is offsetted by 1 * 10^(-1*underlyingDecimals) to ensure minting more htokens than needed to liquidate the entire position leaving no debt dust. Previously, the revert could be triggered because if the debt repay amount passed to liquidatedBorrow(...) is more than the debt amount needed to be burned to withdraw the entire collateral, it would yield seizableCollateralAmount that is larger than the actual collateral amount held in the vault, causing the a revert with BalanceSheet__LiquidateBorrowInsufficientCollateral. But now since the max possible value for the repay amount passed to liquidateBorrow(...) is capped by the debt amount needed to be burned to liquidate the entire collateral, it is not possible to encounter that error.

PaulRBerg commented 2 years ago

Gotcha.

But now since the max possible value for the repay amount passed to liquidateBorrow(...) is capped by the debt amount needed to be burned to liquidate the entire collateral, it is not possible to encounter that error.

I guess you meant that it is not possible to encounter that error through the flash swap contract.

PaulRBerg commented 2 years ago

I added tests and refactored the Solidity code so that it's clearer what it does. Can you add your review, @scorpion9979?

PaulRBerg commented 2 years ago

I addressed all of your concerns in the latest commit, @scorpion9979.