diff --git a/crates/loans/src/interest.rs b/crates/loans/src/interest.rs index 768ecfb008..5229d3e575 100644 --- a/crates/loans/src/interest.rs +++ b/crates/loans/src/interest.rs @@ -72,7 +72,8 @@ impl Pallet { let total_cash = Self::get_total_cash(asset_id); let mut total_borrows = Self::total_borrows(asset_id); let mut total_reserves = Self::total_reserves(asset_id); - let mut borrow_index = Self::borrow_index(asset_id); + let borrow_index = Self::borrow_index(asset_id); + let mut borrow_index_new = borrow_index; let util = Self::calc_utilization_ratio(&total_cash, &total_borrows, &total_reserves)?; let borrow_rate = market @@ -87,15 +88,24 @@ impl Pallet { let delta_time = now .checked_sub(last_accrued_interest_time) .ok_or(ArithmeticError::Underflow)?; - let interest_accumulated = Self::accrued_interest(borrow_rate, &total_borrows, delta_time)?; - total_borrows = interest_accumulated.checked_add(&total_borrows)?; - total_reserves = interest_accumulated + borrow_index_new = Self::accrue_index(borrow_rate, borrow_index, delta_time)?; + let total_borrows_old = total_borrows.clone(); + // Round `total_borrows` down because it needs to be less than or equal to the sum of + // `current_borrow_balance` to compute lend token exchange rate correctly. If `total_borrows` + // is too big, the exchange rate will also be big and there may not be enough cash in the pallet + // account to allow valid redeems. + // Due to this rounding error, the lend token exchange rate may increase after a repayment + // because the pallet account's cash balance could increase by more than `total_borrows` + total_borrows = Self::borrow_balance_from_old_and_new_index( + &borrow_index, + &borrow_index_new, + total_borrows, + RoundingMode::Down, + )?; + let interest_accummulated = total_borrows.checked_sub(&total_borrows_old)?; + total_reserves = interest_accummulated .map(|x| market.reserve_factor.mul_floor(x)) .checked_add(&total_reserves)?; - - borrow_index = Self::increment_index(borrow_rate, borrow_index, delta_time) - .and_then(|r| r.checked_add(&borrow_index)) - .ok_or(ArithmeticError::Overflow)?; } let exchange_rate = Self::calculate_exchange_rate(&total_supply, &total_cash, &total_borrows, &total_reserves)?; @@ -107,7 +117,7 @@ impl Pallet { util, total_borrows.amount(), total_reserves.amount(), - borrow_index, + borrow_index_new, )) } @@ -161,26 +171,17 @@ impl Pallet { }) } - fn accrued_interest( - borrow_rate: Rate, - amount: &Amount, - delta_time: Timestamp, - ) -> Result, DispatchError> { - let balance = borrow_rate - .checked_mul_int(amount.amount()) - .ok_or(ArithmeticError::Overflow)? - .checked_mul(delta_time.into()) - .ok_or(ArithmeticError::Overflow)? - .checked_div(SECONDS_PER_YEAR.into()) - .ok_or(ArithmeticError::Underflow)?; - Ok(Amount::new(balance, amount.currency())) - } - - fn increment_index(borrow_rate: Rate, index: Rate, delta_time: Timestamp) -> Option { - borrow_rate - .checked_mul(&index)? - .checked_mul(&FixedU128::saturating_from_integer(delta_time))? + pub(crate) fn accrue_index(borrow_rate: Rate, index: Rate, delta_time: Timestamp) -> Result { + // Compound interest: + // new_index = old_index * (1 + annual_borrow_rate / SECONDS_PER_YEAR) ^ delta_time + let rate_fraction = borrow_rate .checked_div(&FixedU128::saturating_from_integer(SECONDS_PER_YEAR)) + .ok_or(ArithmeticError::Underflow)?; + let base_rate = Rate::one() + .checked_add(&rate_fraction) + .ok_or(ArithmeticError::Overflow)?; + let compounded_rate = base_rate.saturating_pow(usize::saturated_from(delta_time)); + Ok(index.checked_mul(&compounded_rate).ok_or(ArithmeticError::Overflow)?) } fn calculate_exchange_rate( diff --git a/crates/loans/src/lib.rs b/crates/loans/src/lib.rs index 70d93d10c6..d9251f6262 100644 --- a/crates/loans/src/lib.rs +++ b/crates/loans/src/lib.rs @@ -57,7 +57,7 @@ use traits::{ pub use default_weights::WeightInfo; pub use orml_traits::currency::{OnDeposit, OnSlash, OnTransfer}; -pub use types::{BorrowSnapshot, EarnedSnapshot, Market, MarketState, RewardMarketState}; +pub use types::{BorrowSnapshot, EarnedSnapshot, Market, MarketState, RewardMarketState, RoundingMode}; #[cfg(feature = "runtime-benchmarks")] mod benchmarking; @@ -1525,11 +1525,8 @@ impl Pallet { let account_borrows_new = account_borrows.checked_sub(&repay_amount)?; let total_borrows = Self::total_borrows(asset_id); - // NOTE: `total_borrows()` uses `u128` to calculate accrued interest, - // while `current_borrow_balance()` uses a `FixedU128` (the `BorrowSnapshot`) to calculate accrued interest. - // As a result, when a user repays all borrows, `total_borrows` may be less than `account_borrows` - // due to rounding, which would cause a `checked_sub` to fail with `ArithmeticError::Underflow`. - // Use `saturating_sub` instead here: + // Use `saturating_sub` here, because it's intended for `total_borrows` to be rounded down, + // such that it is less than or equal to the actual borrower debt. let total_borrows_new = total_borrows.saturating_sub(&repay_amount)?; AccountBorrows::::insert( asset_id, @@ -1551,16 +1548,35 @@ impl Pallet { if snapshot.principal.is_zero() || snapshot.borrow_index.is_zero() { return Ok(Amount::zero(asset_id)); } - // Calculate new borrow balance using the interest index: - // recent_borrow_balance = snapshot.principal * borrow_index / snapshot.borrow_index let principal_amount = Amount::::new(snapshot.principal, asset_id); - let borrow_index_increase = Self::borrow_index(asset_id) - .checked_div(&snapshot.borrow_index) - .ok_or(ArithmeticError::Underflow)?; - // Round up the borrower's debt, to avoid giving out short-term interest-free loans. - let recent_borrow_balance = principal_amount.checked_fixed_point_mul_rounded_up(&borrow_index_increase)?; + // Round borrower debt up to avoid interest-free loans + Self::borrow_balance_from_old_and_new_index( + &snapshot.borrow_index, + &Self::borrow_index(asset_id), + principal_amount, + RoundingMode::Up, + ) + } - Ok(recent_borrow_balance) + pub fn borrow_balance_from_old_and_new_index( + old_index: &FixedU128, + new_index: &FixedU128, + amount: Amount, + rounding_mode: RoundingMode, + ) -> Result, DispatchError> { + // Calculate new borrow balance using the interest index: + // recent_borrow_balance = snapshot.principal * borrow_index / snapshot.borrow_index + let borrow_index_increase = new_index.checked_div(&old_index).ok_or(ArithmeticError::Underflow)?; + let borrow_balance = match rounding_mode { + RoundingMode::Up => amount.checked_fixed_point_mul_rounded_up(&borrow_index_increase)?, + RoundingMode::Down => { + let balance_u128 = borrow_index_increase + .checked_mul_int(amount.amount()) + .ok_or(ArithmeticError::Overflow)?; + Amount::::new(balance_u128, amount.currency()) + } + }; + Ok(borrow_balance) } /// Checks if the liquidation should be allowed to occur diff --git a/crates/loans/src/tests/edge_cases.rs b/crates/loans/src/tests/edge_cases.rs index 67be1a761e..4edb135ba7 100644 --- a/crates/loans/src/tests/edge_cases.rs +++ b/crates/loans/src/tests/edge_cases.rs @@ -34,23 +34,24 @@ fn repay_borrow_all_no_underflow() { assert_ok!(Loans::deposit_all_collateral(RuntimeOrigin::signed(ALICE), Token(KSM))); // Alice borrow only 1/1e5 KSM which is hard to accrue total borrows interest in 100 seconds - assert_ok!(Loans::borrow(RuntimeOrigin::signed(ALICE), Token(KSM), 10_u128.pow(7))); + assert_ok!(Loans::borrow(RuntimeOrigin::signed(ALICE), Token(KSM), 10_u128.pow(5))); - accrue_interest_per_block(Token(KSM), 100, 9); + accrue_interest_per_block(Token(KSM), 100, 1000); assert_eq!( Loans::current_borrow_balance(&ALICE, Token(KSM)).unwrap().amount(), - 10000006 + 100007 ); - // FIXME since total_borrows is too small and we accrue internal on it every 100 seconds - // accrue_interest fails every time - // as you can see the current borrow balance is not equal to total_borrows anymore - assert_eq!(Loans::total_borrows(Token(KSM)).amount(), 10000000); + // `total_borrows` had its interest rounded down to zero each block because the principal + // amount is too small + assert_eq!(Loans::total_borrows(Token(KSM)).amount(), 100000); - // Alice repay all borrow balance. total_borrows = total_borrows.saturating_sub(10000006) = 0. + // Alice repay all borrow balance. total_borrows = total_borrows.saturating_sub(100007) = 0. + // Repaying should not cause an underflow assert_ok!(Loans::repay_borrow_all(RuntimeOrigin::signed(ALICE), Token(KSM))); + assert_eq!(Loans::total_borrows(Token(KSM)).amount(), 0); - assert_eq!(Tokens::balance(Token(KSM), &ALICE), unit(800) - 6); + assert_eq!(Tokens::balance(Token(KSM), &ALICE), unit(800) - 7); assert_eq!( Loans::exchange_rate(Token(DOT)).saturating_mul_int( diff --git a/crates/loans/src/tests/interest_rate.rs b/crates/loans/src/tests/interest_rate.rs index c133752b12..e69b098ca1 100644 --- a/crates/loans/src/tests/interest_rate.rs +++ b/crates/loans/src/tests/interest_rate.rs @@ -2,9 +2,9 @@ use crate::{mock::*, tests::Loans, Markets}; use currency::Amount; use frame_support::assert_ok; use mocktopus::mocking::Mockable; -use primitives::{CurrencyId::Token, Rate, Ratio, DOT, KSM, SECONDS_PER_YEAR}; +use primitives::{CurrencyId::Token, Moment, Rate, Ratio, DOT, KSM}; use sp_runtime::{ - traits::{CheckedDiv, One, Saturating}, + traits::{CheckedDiv, One}, FixedPointNumber, }; use traits::OracleApi; @@ -84,13 +84,13 @@ fn interest_rate_model_works() { assert_eq!(Loans::utilization_ratio(Token(DOT)), util_ratio); let borrow_rate = (jump_rate - base_rate) * util_ratio.into() / jump_utilization.into() + base_rate; - let interest_accumulated: u128 = borrow_rate - .saturating_mul_int(total_borrows) - .saturating_mul(delta_time.into()) - .checked_div(SECONDS_PER_YEAR.into()) - .unwrap(); - total_borrows = interest_accumulated + total_borrows; - assert_eq!(Loans::total_borrows(Token(DOT)).amount(), total_borrows); + let borrow_index_old = borrow_index; + borrow_index = Loans::accrue_index(borrow_rate, borrow_index, delta_time as Moment).unwrap(); + let total_borrows_old = total_borrows; + total_borrows = (borrow_index / borrow_index_old).saturating_mul_int(total_borrows); + let interest_accumulated = total_borrows - total_borrows_old; + let actual_total_borrows = Loans::total_borrows(Token(DOT)).amount(); + assert_eq!(actual_total_borrows, total_borrows); total_reserves = Markets::::get(&Token(DOT)) .unwrap() .reserve_factor @@ -103,26 +103,20 @@ fn interest_rate_model_works() { Loans::exchange_rate(Token(DOT)).into_inner(), (total_cash + total_borrows - total_reserves) * rate_decimal / total_supply ); - let numerator = borrow_index - .saturating_mul(borrow_rate) - .saturating_mul(Rate::saturating_from_integer(delta_time)) - .checked_div(&Rate::saturating_from_integer(SECONDS_PER_YEAR)) - .unwrap(); - borrow_index = numerator + borrow_index; assert_eq!(Loans::borrow_index(Token(DOT)), borrow_index); } - assert_eq!(total_borrows, 100000063926960646826); - assert_eq!(total_reserves, 9589044097001); - assert_eq!(borrow_index, Rate::from_inner(1000000639269606444)); - assert_eq!(Loans::exchange_rate(Token(DOT)), Rate::from_inner(20000005433791654)); + assert_eq!(total_borrows, 100000063926960953257); + assert_eq!(total_reserves, 9589044142967); + assert_eq!(borrow_index, Rate::from_inner(1000000639269609557)); + assert_eq!(Loans::exchange_rate(Token(DOT)), Rate::from_inner(20000005433791681)); // Calculate borrow accrued interest let borrow_principal = (borrow_index / borrow_snapshot.borrow_index).saturating_mul_int(borrow_snapshot.principal); - // TODO: Why subtract `million_unit(200)` here? Accruing interest doesn't fix this. + // The supply interest here is probably the fraction of interest that goes to the reserve let supply_interest = Loans::exchange_rate(Token(DOT)).saturating_mul_int(total_supply) - million_unit(200); - assert_eq!(supply_interest, 54337916540000); - assert_eq!(borrow_principal, 100000063926960644400); + assert_eq!(supply_interest, 54337916810000); + assert_eq!(borrow_principal, 100000063926960955700); assert_eq!(total_borrows / 10000, borrow_principal / 10000); assert_eq!( (total_borrows - million_unit(100) - total_reserves) / 10000, @@ -143,7 +137,7 @@ fn last_accrued_interest_time_should_be_update_correctly() { assert_eq!(Loans::borrow_index(Token(DOT)), Rate::one()); TimestampPallet::set_timestamp(12000); assert_ok!(Loans::mint(RuntimeOrigin::signed(ALICE), Token(DOT), unit(100))); - assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000013318112633),); + assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000013318112698),); }) } @@ -156,7 +150,7 @@ fn accrue_interest_works_after_mint() { assert_eq!(Loans::borrow_index(Token(DOT)), Rate::one()); TimestampPallet::set_timestamp(12000); assert_ok!(Loans::mint(RuntimeOrigin::signed(ALICE), Token(DOT), unit(100))); - assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000013318112633),); + assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000013318112698),); }) } @@ -187,7 +181,7 @@ fn accrue_interest_works_after_redeem() { Token(DOT), amount_to_redeem )); - assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000004756468797),); + assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000004756468801),); assert_eq!( Loans::exchange_rate(Token(DOT)) .saturating_mul_int(Loans::account_deposits(Loans::lend_token_id(Token(DOT)).unwrap(), &BOB).amount()), @@ -209,7 +203,7 @@ fn accrue_interest_works_after_redeem_all() { assert_eq!(Tokens::balance(Token(DOT), &BOB), 980000000000000); TimestampPallet::set_timestamp(12000); assert_ok!(Loans::redeem_all(RuntimeOrigin::signed(BOB), Token(DOT))); - assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000004669977168),); + assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000004669977174),); assert_eq!( Loans::exchange_rate(Token(DOT)) .saturating_mul_int(Loans::account_deposits(Loans::lend_token_id(Token(DOT)).unwrap(), &BOB).amount()), @@ -231,7 +225,7 @@ fn accrue_interest_works_after_repay() { assert_eq!(Loans::borrow_index(Token(DOT)), Rate::one()); TimestampPallet::set_timestamp(12000); assert_ok!(Loans::repay_borrow(RuntimeOrigin::signed(ALICE), Token(DOT), unit(10))); - assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000005707762557),); + assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000005707762564),); }) } @@ -245,7 +239,7 @@ fn accrue_interest_works_after_repay_all() { assert_eq!(Loans::borrow_index(Token(KSM)), Rate::one()); TimestampPallet::set_timestamp(12000); assert_ok!(Loans::repay_borrow_all(RuntimeOrigin::signed(ALICE), Token(KSM))); - assert_eq!(Loans::borrow_index(Token(KSM)), Rate::from_inner(1000000008561643835),); + assert_eq!(Loans::borrow_index(Token(KSM)), Rate::from_inner(1000000008561643864),); assert_eq!(Tokens::balance(Token(KSM), &ALICE), 999999999571917); let borrow_snapshot = Loans::account_borrows(Token(KSM), ALICE); assert_eq!(borrow_snapshot.principal, 0); @@ -277,8 +271,8 @@ fn accrue_interest_works_after_liquidate_borrow() { unit(50), Token(DOT) )); - assert_eq!(Loans::borrow_index(Token(KSM)), Rate::from_inner(1000000013318112633),); - assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000006976141552),); + assert_eq!(Loans::borrow_index(Token(KSM)), Rate::from_inner(1000000013318112698),); + assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000006976141566),); }) } @@ -295,7 +289,7 @@ fn accrue_interest_works_after_recompute_collateral_amount() { 1234, Token(KSM) ))); - assert_eq!(Loans::borrow_index(Token(KSM)), Rate::from_inner(1000000008561643835),); + assert_eq!(Loans::borrow_index(Token(KSM)), Rate::from_inner(1000000008561643864),); }) } @@ -311,7 +305,7 @@ fn accrue_interest_works_after_recompute_underlying_amount() { assert_ok!(Loans::recompute_underlying_amount( &Loans::free_lend_tokens(Token(KSM), &ALICE).unwrap() )); - assert_eq!(Loans::borrow_index(Token(KSM)), Rate::from_inner(1000000008561643835),); + assert_eq!(Loans::borrow_index(Token(KSM)), Rate::from_inner(1000000008561643864),); }) } diff --git a/crates/loans/src/types.rs b/crates/loans/src/types.rs index f42cb6e251..a9d0c2e64e 100644 --- a/crates/loans/src/types.rs +++ b/crates/loans/src/types.rs @@ -14,6 +14,12 @@ pub enum AccountLiquidity { Shortfall(Amount), } +#[derive(Eq, PartialEq, Clone, RuntimeDebug)] +pub enum RoundingMode { + Up, + Down, +} + impl AccountLiquidity { pub fn from_collateral_and_debt( collateral_value: Amount,