Repository: interlay/interbtc (language: rust)

Issue:
Loans: redeem not releasing correct amount due to rounding
In both `redeem`, the underlying amount is first converted to the lend token amount, after which the to-be released collateral is calculated by converting the lend token back to the underlying amount. This can have the consequence that the released collateral is less than what was requested in the `redeem` call. We should refactor to avoid the second conversion


Discussion / hints:

Fix patch:
diff --git a/crates/currency/src/amount.rs b/crates/currency/src/amount.rs
index f0eb220cf0..ad2be57c77 100644
--- a/crates/currency/src/amount.rs
+++ b/crates/currency/src/amount.rs
@@ -224,6 +224,11 @@ mod math {
             Ok(if self.le(other)? { self.clone() } else { other.clone() })
         }
 
+        pub fn max(&self, other: &Self) -> Result<Self, DispatchError> {
+            ensure!(self.currency_id == other.currency_id, Error::<T>::InvalidCurrency);
+            Ok(if self.ge(other)? { self.clone() } else { other.clone() })
+        }
+
         pub fn lt(&self, other: &Self) -> Result<bool, DispatchError> {
             ensure!(self.currency_id == other.currency_id, Error::<T>::InvalidCurrency);
             Ok(self.amount < other.amount)
diff --git a/crates/loans/src/lib.rs b/crates/loans/src/lib.rs
index a3b0a9902a..8d17853e2d 100644
--- a/crates/loans/src/lib.rs
+++ b/crates/loans/src/lib.rs
@@ -76,6 +76,9 @@ mod types;
 
 mod default_weights;
 
+#[cfg(test)]
+use mocktopus::macros::mockable;
+
 pub const REWARD_SUB_ACCOUNT: &[u8; 7] = b"farming";
 pub const INCENTIVE_SUB_ACCOUNT: &[u8; 9] = b"incentive";
 
@@ -87,6 +90,7 @@ type CurrencyId<T> = <T as orml_tokens::Config>::CurrencyId;
 type BalanceOf<T> = <T as currency::Config>::Balance;
 
 /// Lending-specific methods on Amount
+#[cfg_attr(test, mockable)]
 trait LendingAmountExt {
     fn to_lend_token(&self) -> Result<Self, DispatchError>
     where
@@ -96,6 +100,7 @@ trait LendingAmountExt {
         Self: Sized;
 }
 
+#[cfg_attr(test, mockable)]
 impl<T: Config> LendingAmountExt for Amount<T> {
     fn to_lend_token(&self) -> Result<Self, DispatchError> {
         let lend_token_id = Pallet::<T>::lend_token_id(self.currency())?;
@@ -1007,23 +1012,17 @@ pub mod pallet {
             let who = ensure_signed(origin)?;
             ensure!(!redeem_amount.is_zero(), Error::<T>::InvalidAmount);
 
-            let lend_token_id = Self::lend_token_id(asset_id)?;
-            // if the receiver has collateral locked
-            let deposit = Pallet::<T>::account_deposits(lend_token_id, &who);
-            let amount = Amount::<T>::new(redeem_amount, asset_id);
-            if !deposit.is_zero() {
-                // Withdraw the `lend_tokens` from the borrow collateral, so they are redeemable.
-                // This assumes that a user cannot have both `free` and `locked` lend tokens at
-                // the same time (for the purposes of lending and borrowing).
-                let collateral = Self::recompute_collateral_amount(&amount)?;
-                Self::do_withdraw_collateral(&who, &collateral)?;
-            }
-            Self::do_redeem(&who, &amount)?;
+            let underlying = Amount::<T>::new(redeem_amount, asset_id);
+            let voucher = underlying.to_lend_token()?;
+
+            Self::do_redeem(&who, &underlying, &voucher)?;
 
             Ok(().into())
         }
 
         /// The caller redeems their entire lend token balance in exchange for the underlying asset.
+        /// Note: this will fail if the account needs some of the collateral for backing open borrows,
+        /// or if any of the lend tokens are used by other pallets (e.g. used as vault collateral)
         ///
         /// - `asset_id`: the asset to be redeemed.
         #[pallet::call_index(11)]
@@ -1031,44 +1030,13 @@ pub mod pallet {
         #[transactional]
         pub fn redeem_all(origin: OriginFor<T>, asset_id: CurrencyId<T>) -> DispatchResultWithPostInfo {
             let who = ensure_signed(origin.clone())?;
-            // This function is an almost 1:1 duplicate of the logic in `do_redeem`.
-            // It could be refactored to compute the redeemable underlying
-            // with `Self::recompute_underlying_amount(&Self::free_lend_tokens(asset_id, &who)?)?`
-            // but that would cause the `accrue_interest_works_after_redeem_all` unit test to fail with
-            // left: `1000000000003607`,
-            // right: `1000000000003608`'
-
-            // Chaining `calc_underlying_amount` and `calc_collateral_amount` continuously decreases
-            // an amount because of rounding down, and having the current function call `do_redeem`
-            // would perform three conversions: lend_token -> token -> lend_token -> token.
-            // Calling `do_redeem_voucher` directly only performs one conversion: lend_token -> token,
-            // avoiding this edge case.
-            // TODO: investigate whether it is possible to implement the conversion functions
-            // with guarantees that this always holds:
-            // `calc_underlying_amount(calc_collateral_amount(x)) = calc_collateral_amount(calc_underlying_amount(x))`
-            // Use the `converting_to_and_from_collateral_should_not_change_results` unit test to achieve this.
-            // If there are leftover lend_tokens after a `redeem_all` (because of rounding down), it would make it
-            // impossible to enforce "collateral toggle" state transitions.
-            Self::ensure_active_market(asset_id)?;
 
-            let lend_token_id = Self::lend_token_id(asset_id)?;
-            // if the receiver has collateral locked
-            let deposit = Pallet::<T>::account_deposits(lend_token_id, &who);
-            if !deposit.is_zero() {
-                // then withdraw all collateral
-                Self::withdraw_all_collateral(origin, asset_id)?;
-            }
+            let voucher = Self::balance(Self::lend_token_id(asset_id)?, &who);
+            let underlying = &voucher.to_underlying()?;
 
-            // `do_redeem()` logic duplicate:
-            Self::accrue_interest(asset_id)?;
-            let lend_tokens = Self::free_lend_tokens(asset_id, &who)?;
-            ensure!(!lend_tokens.is_zero(), Error::<T>::InvalidAmount);
-            let redeem = Self::do_redeem_voucher(&who, lend_tokens)?;
-            Self::deposit_event(Event::<T>::Redeemed {
-                account_id: who,
-                currency_id: asset_id,
-                amount: redeem.amount(),
-            });
+            // note: we use total balance rather than underlying.to_lend_token, s.t. the account
+            // is left neatly with 0 balance
+            Self::do_redeem(&who, &underlying, &voucher)?;
 
             Ok(().into())
         }
@@ -1313,17 +1281,16 @@ pub mod pallet {
             Self::ensure_active_market(asset_id)?;
             Self::accrue_interest(asset_id)?;
 
-            let redeem_amount = Amount::new(redeem_amount, asset_id);
-            let voucher = redeem_amount.to_lend_token()?;
+            let underlying = Amount::new(redeem_amount, asset_id);
 
-            let redeem = Self::do_redeem_voucher(&from, voucher)?;
+            Self::do_redeem(&from, &underlying, &underlying.to_lend_token()?)?;
 
-            redeem.transfer(&from, &receiver)?;
+            underlying.transfer(&from, &receiver)?;
 
             Self::deposit_event(Event::<T>::IncentiveReservesReduced {
                 receiver,
                 currency_id: asset_id,
-                amount: redeem.amount(),
+                amount: underlying.amount(),
             });
             Ok(().into())
         }
@@ -1458,6 +1425,9 @@ impl<T: Config> Pallet<T> {
             redeemer,
             voucher.amount(),
         );
+
+        ensure!(!voucher.is_zero(), Error::<T>::InvalidAmount);
+
         if Self::balance(voucher.currency(), redeemer).lt(&voucher)? {
             return Err(Error::<T>::InsufficientDeposit.into());
         }
@@ -1475,29 +1445,6 @@ impl<T: Config> Pallet<T> {
         Ok(())
     }
 
-    #[require_transactional]
-    pub fn do_redeem_voucher(who: &T::AccountId, voucher: Amount<T>) -> Result<Amount<T>, DispatchError> {
-        let asset_id = Self::underlying_id(voucher.currency())?;
-
-        Self::redeem_allowed(who, &voucher)?;
-        Self::update_reward_supply_index(asset_id)?;
-        Self::distribute_supplier_reward(asset_id, who)?;
-
-        let redeem_amount = voucher.to_underlying()?;
-
-        // Need to first `lock_on` in order to `burn_from` because:
-        // 1) only the `free` lend_tokens are redeemable
-        // 2) `burn_from` can only be called on locked tokens.
-        voucher.lock_on(who)?;
-        voucher.burn_from(who)?;
-
-        redeem_amount
-            .transfer(&Self::account_id(), who)
-            .map_err(|_| Error::<T>::InsufficientCash)?;
-
-        Ok(redeem_amount)
-    }
-
     /// Borrower shouldn't borrow more than their total collateral value allows
     fn borrow_allowed(borrower: &T::AccountId, borrow: &Amount<T>) -> DispatchResult {
         Self::ensure_under_borrow_cap(borrow)?;
@@ -2089,18 +2036,39 @@ impl<T: Config> LoansTrait<CurrencyId<T>, AccountIdOf<T>, Amount<T>> for Pallet<
         Ok(())
     }
 
-    fn do_redeem(supplier: &AccountIdOf<T>, amount: &Amount<T>) -> Result<(), DispatchError> {
-        let asset_id = amount.currency();
+    fn do_redeem(supplier: &AccountIdOf<T>, underlying: &Amount<T>, voucher: &Amount<T>) -> Result<(), DispatchError> {
+        let asset_id = underlying.currency();
+
+        // if the receiver has collateral locked
+        if !Pallet::<T>::account_deposits(voucher.currency(), &supplier).is_zero() {
+            // Withdraw the `lend_tokens` from the borrow collateral, so they are redeemable.
+            // This assumes that a user cannot have both `free` and `locked` lend tokens at
+            // the same time (for the purposes of lending and borrowing).
+            Self::do_withdraw_collateral(&supplier, &voucher)?;
+        }
+
         Self::ensure_active_market(asset_id)?;
         Self::accrue_interest(asset_id)?;
 
-        let voucher = amount.to_lend_token()?;
+        Self::redeem_allowed(supplier, &voucher)?;
+
+        Self::update_reward_supply_index(asset_id)?;
+        Self::distribute_supplier_reward(asset_id, supplier)?;
+
+        // Need to first `lock_on` in order to `burn_from` because:
+        // 1) only the `free` lend_tokens are redeemable
+        // 2) `burn_from` can only be called on locked tokens.
+        voucher.lock_on(supplier)?;
+        voucher.burn_from(supplier)?;
+
+        underlying
+            .transfer(&Self::account_id(), supplier)
+            .map_err(|_| Error::<T>::InsufficientCash)?;
 
-        let redeem = Self::do_redeem_voucher(supplier, voucher)?;
         Self::deposit_event(Event::<T>::Redeemed {
             account_id: supplier.clone(),
             currency_id: asset_id,
-            amount: redeem.amount(),
+            amount: underlying.amount(),
         });
         Ok(())
     }
diff --git a/crates/traits/src/loans.rs b/crates/traits/src/loans.rs
index 4124853a70..d3df296f6a 100644
--- a/crates/traits/src/loans.rs
+++ b/crates/traits/src/loans.rs
@@ -28,7 +28,7 @@ pub trait LoansApi<CurrencyId, AccountId, Amount> {
     fn do_deposit_collateral(supplier: &AccountId, lend_tokens: &Amount) -> Result<(), DispatchError>;
     fn do_withdraw_collateral(supplier: &AccountId, voucher: &Amount) -> Result<(), DispatchError>;
     fn do_repay_borrow(borrower: &AccountId, borrow: &Amount) -> Result<(), DispatchError>;
-    fn do_redeem(supplier: &AccountId, amount: &Amount) -> Result<(), DispatchError>;
+    fn do_redeem(supplier: &AccountId, amount: &Amount, voucher: &Amount) -> Result<(), DispatchError>;
     fn recompute_underlying_amount(lend_tokens: &Amount) -> Result<Amount, DispatchError>;
     fn underlying_id(lend_token_id: CurrencyId) -> Result<CurrencyId, DispatchError>;
     fn recompute_collateral_amount(underlying: &Amount) -> Result<Amount, DispatchError>;


Test patch:
diff --git a/crates/loans/src/tests.rs b/crates/loans/src/tests.rs
index 3007d3bf27..ea564e697d 100644
--- a/crates/loans/src/tests.rs
+++ b/crates/loans/src/tests.rs
@@ -24,7 +24,7 @@ mod market;
 use currency::Amount;
 use frame_support::{assert_noop, assert_ok};
 
-use mocktopus::mocking::Mockable;
+use mocktopus::mocking::{MockResult, Mockable};
 use sp_runtime::{
     traits::{CheckedDiv, One, Saturating},
     FixedU128, Permill,
@@ -1474,3 +1474,48 @@ fn redeeming_full_amount_leaves_no_leftover() {
         assert_eq!(Loans::reserved_lend_tokens(DOT, &ALICE).unwrap().is_zero(), true);
     })
 }
+
+#[test]
+fn redeem_amount_matches_freed_underlying() {
+    new_test_ext().execute_with(|| {
+        let amount_to_mint = 2_123_123_123;
+        let amount_to_redeem = Amount::<Test>::new(1_00_000_000, DOT);
+        assert_ok!(Loans::mint(RuntimeOrigin::signed(ALICE), DOT, amount_to_mint));
+        assert_ok!(Loans::deposit_all_collateral(RuntimeOrigin::signed(ALICE), DOT));
+
+        let pre_redeem_balance = Tokens::free_balance(DOT, &ALICE);
+        let inner_exchange_rate = FixedU128::from_inner(20000000000000001);
+
+        // Force rounding error in conversion
+        Amount::<Test>::to_lend_token.mock_safe(move |x| {
+            MockResult::Return(Ok(x
+                .checked_mul(&inner_exchange_rate.clone())
+                .unwrap()
+                .set_currency(Loans::lend_token_id(x.currency()).unwrap())))
+        });
+        Amount::<Test>::to_underlying.mock_safe(move |x| {
+            MockResult::Return(Ok(x
+                .checked_div(&inner_exchange_rate.clone())
+                .unwrap()
+                .set_currency(Loans::underlying_id(x.currency()).unwrap())))
+        });
+
+        // sanity check: converting the lend token and back to underlying gives indeed a different result
+        assert_ne!(
+            amount_to_redeem.to_lend_token().unwrap().to_underlying().unwrap(),
+            amount_to_redeem
+        );
+
+        assert_ok!(Loans::redeem(
+            RuntimeOrigin::signed(ALICE),
+            DOT,
+            amount_to_redeem.amount()
+        ));
+
+        // before #930 this used to be off by one.
+        assert_eq!(
+            Tokens::free_balance(DOT, &ALICE),
+            pre_redeem_balance + amount_to_redeem.amount()
+        );
+    })
+}
diff --git a/crates/loans/src/tests/interest_rate.rs b/crates/loans/src/tests/interest_rate.rs
index e69b098ca1..0c1e2d3166 100644
--- a/crates/loans/src/tests/interest_rate.rs
+++ b/crates/loans/src/tests/interest_rate.rs
@@ -187,7 +187,7 @@ fn accrue_interest_works_after_redeem() {
                 .saturating_mul_int(Loans::account_deposits(Loans::lend_token_id(Token(DOT)).unwrap(), &BOB).amount()),
             0,
         );
-        assert_eq!(Tokens::balance(Token(DOT), &ALICE), 819999999999999);
+        assert_eq!(Tokens::balance(Token(DOT), &ALICE), 820000000000000);
     })
 }