Skip to content

Conversation

@Debugger022
Copy link
Contributor

@Debugger022 Debugger022 commented Nov 3, 2025

NOTE: The changes from this PR have already been merged into the liquidation improvements PR #604. Therefore, we will be closing this PR soon.

This PR updates the repayAmount calculation of the VToken.

From this to this:
vars.repayAmount = repayAmount >= vars.accountBorrows ? vars.accountBorrows : repayAmount;

This change will bring the following impacts:

1. Liquidation Process (VToken.sol - liquidateBorrowFresh)

function liquidateBorrowFresh(...) internal returns (uint, uint) {
    // This directly calls repayBorrowFresh
    (uint repayBorrowError, uint actualRepayAmount) = repayBorrowFresh(liquidator, borrower, repayAmount);
    
    // OLD: actualRepayAmount could exceed borrower's debt
    // NEW: actualRepayAmount is guaranteed ≤ borrower's debt
}

2. Public Repayment Functions (VBep20.sol)

function repayBorrow(uint repayAmount) external returns (uint) {
    // Calls repayBorrowInternal -> repayBorrowFresh
    // OLD: Users had to calculate exact debt or use type(uint256).max
    // NEW: Users can safely over-estimate repayment amounts
}

function repayBorrowBehalf(address borrower, uint repayAmount) external returns (uint) {
    // Same direct impact - prevents over-repayment when repaying for others
}

3. RepayBorrow Event Emission

// In repayBorrowFresh function
emit RepayBorrow(payer, borrower, vars.actualRepayAmount, vars.accountBorrowsNew, vars.totalBorrowsNew);

// OLD: actualRepayAmount could be larger than what was actually needed
// NEW: actualRepayAmount reflects the capped amount (actual debt paid)

4. Mathematical Calculations in repayBorrowFresh

// These calculations are now guaranteed safe from underflow
(vars.mathErr, vars.accountBorrowsNew) = subUInt(vars.accountBorrows, vars.actualRepayAmount);
(vars.mathErr, vars.totalBorrowsNew) = subUInt(totalBorrows, vars.actualRepayAmount);

// OLD: Risk of underflow if actualRepayAmount > accountBorrows
// NEW: No underflow risk since actualRepayAmount ≤ accountBorrows

@github-actions
Copy link

github-actions bot commented Nov 3, 2025

Code Coverage

Package Line Rate Branch Rate Health
contracts 100% 100%
contracts.Admin 88% 41%
contracts.Comptroller 100% 90%
contracts.Comptroller.Diamond 95% 61%
contracts.Comptroller.Diamond.facets 80% 67%
contracts.Comptroller.Diamond.interfaces 100% 100%
contracts.Comptroller.Types 100% 100%
contracts.Comptroller.legacy 100% 100%
contracts.Comptroller.legacy.Diamond 0% 0%
contracts.Comptroller.legacy.Diamond.facets 0% 0%
contracts.Comptroller.legacy.Diamond.interfaces 100% 100%
contracts.DelegateBorrowers 100% 89%
contracts.Governance 68% 45%
contracts.InterestRateModels 74% 59%
contracts.Lens 41% 34%
contracts.Liquidator 83% 60%
contracts.Oracle 100% 100%
contracts.PegStability 88% 84%
contracts.Swap 91% 58%
contracts.Swap.interfaces 100% 100%
contracts.Swap.lib 81% 53%
contracts.Tokens 100% 100%
contracts.Tokens.Prime 96% 72%
contracts.Tokens.Prime.Interfaces 100% 100%
contracts.Tokens.Prime.libs 90% 76%
contracts.Tokens.VAI 82% 53%
contracts.Tokens.VRT 20% 9%
contracts.Tokens.VTokens 65% 49%
contracts.Tokens.VTokens.legacy 0% 0%
contracts.Tokens.VTokens.legacy.Utils 0% 0%
contracts.Tokens.XVS 19% 8%
contracts.Utils 53% 30%
contracts.VAIVault 50% 45%
contracts.VRTVault 49% 36%
contracts.XVSVault 63% 50%
contracts.external 100% 100%
contracts.lib 89% 71%
Summary 55% (3529 / 6360) 41% (1320 / 3186)

@Debugger022 Debugger022 marked this pull request as ready for review November 4, 2025 06:33
@Debugger022 Debugger022 self-assigned this Nov 4, 2025
Copy link
Contributor

@fred-venus fred-venus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@fred-venus
Copy link
Contributor

fred-venus commented Nov 11, 2025

I had a question in terms of the saying in the description

function liquidateBorrowFresh(...) internal returns (uint, uint) {
    // This directly calls repayBorrowFresh
    (uint repayBorrowError, uint actualRepayAmount) = repayBorrowFresh(liquidator, borrower, repayAmount);
    
    // OLD: actualRepayAmount could exceed borrower's debt
    // NEW: actualRepayAmount is guaranteed ≤ borrower's debt


   ...
   
   (vars.mathErr, vars.accountBorrowsNew) = subUInt(vars.accountBorrows, vars.actualRepayAmount);
        ensureNoMathError(vars.mathErr);

   (vars.mathErr, vars.totalBorrowsNew) = subUInt(totalBorrows, vars.actualRepayAmount);
        ensureNoMathError(vars.mathErr);
}

The existing implementation for liquidator who has provided a bigger (bigger than actual debt) value here, it will get revert because the subsequent overflow check

then is the check already unnecessary after we perform this improvement ?

@Debugger022
Copy link
Contributor Author

Debugger022 commented Nov 11, 2025

@fred-venus It is advisable to keep a check for safety and clarity. The performance impact is minimal, and it offers valuable protection against edge cases and any future changes.

@fred-venus fred-venus force-pushed the develop branch 6 times, most recently from f8786ac to d1378e1 Compare December 12, 2025 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants