sherlock-audit / 2023-02-surge-judging

4 stars 1 forks source link

peanuts - liquidate ()should handle the case if the repaid amount is greater than the loan amount #265

Closed github-actions[bot] closed 1 year ago

github-actions[bot] commented 1 year ago

peanuts

medium

liquidate ()should handle the case if the repaid amount is greater than the loan amount

Summary

liquidate() should handle the case if the repaid amount is greater than the loan amount.

Vulnerability Detail

When liquidating loans in Surge, the liquidator can call liquidate() and pay the borrowers debt partially or fully in exchange for collateral. If liquidator intends to repay the debt in full, then he should set the amount to type(uint).max. If not, the borrower will set an arbitrary amount. However, the liquidator does not know the debt of the borrower until getDebtOf() is called. There may be cases when the amount he intends to pay back is greater than the loan amount. If that is the case, the borrower should also be able to repay in full.

    function liquidate(address borrower, uint amount) external {
        uint _loanTokenBalance = LOAN_TOKEN.balanceOf(address(this));
        (address _feeRecipient, uint _feeMantissa) = FACTORY.getFee();
        (  
            uint _currentTotalSupply,
            uint _accruedFeeShares,
            uint _currentCollateralRatioMantissa,
            uint _currentTotalDebt
        ) = getCurrentState(
            _loanTokenBalance,
            _feeMantissa,
            lastCollateralRatioMantissa,
            totalSupply,
            lastAccrueInterestTime,
            lastTotalDebt
        );

        uint collateralBalance = collateralBalanceOf[borrower];
        uint _debtSharesSupply = debtSharesSupply;
        uint userDebt = getDebtOf(debtSharesBalanceOf[borrower], _debtSharesSupply, _currentTotalDebt);
        uint userCollateralRatioMantissa = userDebt * 1e18 / collateralBalance;
        require(userCollateralRatioMantissa > _currentCollateralRatioMantissa, "Pool: borrower not liquidatable");

        address _borrower = borrower; // avoid stack too deep
        uint _amount = amount; // avoid stack too deep
        uint _shares;
        uint collateralReward;
@--audit only checks type(uint).max and _amount == userDebt but should also check _amount > userDebt
        if(_amount == type(uint).max || _amount == userDebt) {
            collateralReward = collateralBalance;
            _shares = debtSharesBalanceOf[_borrower];
            _amount = userDebt;
        } else {
            uint userInvertedCollateralRatioMantissa = collateralBalance * 1e18 / userDebt;
            collateralReward = _amount * userInvertedCollateralRatioMantissa / 1e18; // rounds down
            _shares = tokenToShares(_amount, _currentTotalDebt, _debtSharesSupply, false);
        }
        _currentTotalDebt -= _amount;

If amount > userDebt, then the else part will execute and the transaction will revert because of underflow.

A similar issue can be seen here: https://github.com/sherlock-audit/2023-01-cooler-judging/issues/327

Impact

liquidation will underflow if _amount is greater than userDebt.

Code Snippet

https://github.com/sherlock-audit/2023-02-surge/blob/main/surge-protocol-v1/src/Pool.sol#L504-L530

Tool used

Manual Review

Recommendation

Recommend updating the logic to take care of cases where _amount > userDebt

-        if(_amount == type(uint).max || _amount == userDebt) {
+        if(_amount == type(uint).max || _amount >= userDebt) {
            collateralReward = collateralBalance;
            _shares = debtSharesBalanceOf[_borrower];
            _amount = userDebt;