0x52 - User is unable to partially payback loan if they aren't able to post enough isoUSD to bring them back to minOpeningMargin

[github-actions]

0x52

high

User is unable to partially payback loan if they aren't able to post enough isoUSD to bring them back to minOpeningMargin

Summary

The only way for a user to reduce their debt is to call closeLoan. If the amount repaid does not bring the user back above minOpeningMargin then the transaction will revert. This is problematic for users that wish to repay their debt but don't have enough to get back to minOpeningMargin as it could lead to unfair liquidations.

Vulnerability Detail

    if(outstandingisoUSD >= TENTH_OF_CENT){ //ignore leftover debts less than $0.001
        uint256 collateralLeft = collateralPosted[_collateralAddress][msg.sender] - _collateralToUser;
        uint256 colInUSD = priceCollateralToUSD(currencyKey, collateralLeft); 
        uint256 borrowMargin = (outstandingisoUSD * minOpeningMargin) / LOAN_SCALE;
        require(colInUSD > borrowMargin , "Remaining debt fails to meet minimum margin!");
    }

The checks above are done when a user calls closeLoan. This ensures that the user's margin is back above minOpeningMargin before allowing them to remove any collateral. This is done as a safeguard to block loans users from effectively opening loans at lower than desired margin. This has the unintended consequence that as user cannot pay off any of their loan if they do not increase their loan back above minOpeningMargin. This could prevent users from being able to save a loan that is close to liquidation causing them to get liquidated when they otherwise would have paid off their loan.

Impact

User is unable to make partial repayments if their payment does not increase margin enough

Code Snippet

https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Isomorph/contracts/Vault_Synths.sol#L197-L248

Tool used

Manual Review

Recommendation

I recommend adding a separate function that allows users to pay off their loan without removing any collateral:

function paybackLoan(
    address _collateralAddress,
    uint256 _USDToVault
    ) external override whenNotPaused 
    {
    _collateralExists(_collateralAddress);
    _closeLoanChecks(_collateralAddress, 0, _USDToVault);
    //make sure virtual price is related to current time before fetching collateral details
    //slither-disable-next-line reentrancy-vulnerabilities-1
    _updateVirtualPrice(block.timestamp, _collateralAddress);
    (   
        bytes32 currencyKey,
        uint256 minOpeningMargin,
        ,
        ,
        ,
        uint256 virtualPrice,
        
    ) = _getCollateral(_collateralAddress);
    //check for frozen or paused collateral
    _checkIfCollateralIsActive(currencyKey);

    uint256 isoUSDdebt = (isoUSDLoanAndInterest[_collateralAddress][msg.sender] * virtualPrice) / LOAN_SCALE;
    require( isoUSDdebt >= _USDToVault, "Trying to return more isoUSD than borrowed!");
    uint256 outstandingisoUSD = isoUSDdebt - _USDToVault;

    uint256 collateral = collateralPosted[_collateralAddress][msg.sender];
    uint256 colInUSD = priceCollateralToUSD(currencyKey, collateral); 
    uint256 borrowMargin = (outstandingisoUSD * liquidatableMargin) / LOAN_SCALE;
    require(colInUSD > borrowMargin , "Liquidation margin not met!");
    
    //record paying off loan principle before interest
    //slither-disable-next-line uninitialized-local-variables
    uint256 interestPaid;
    uint256 loanPrinciple = isoUSDLoaned[_collateralAddress][msg.sender];
    if( loanPrinciple >= _USDToVault){
        //pay off loan principle first
        isoUSDLoaned[_collateralAddress][msg.sender] = loanPrinciple - _USDToVault;
    }
    else{
        interestPaid = _USDToVault - loanPrinciple;
        //loan principle is fully repaid so record this.
        isoUSDLoaned[_collateralAddress][msg.sender] = 0;
    }
    //update mappings with reduced amounts
    isoUSDLoanAndInterest[_collateralAddress][msg.sender] = isoUSDLoanAndInterest[_collateralAddress][msg.sender] - ((_USDToVault * LOAN_SCALE) / virtualPrice);
    emit ClosedLoan(msg.sender, _USDToVault, currencyKey, 0);
    //Now all effects are handled, transfer the assets so we follow CEI pattern
    _decreaseLoan(_collateralAddress, 0, _USDToVault, interestPaid);
}

[kree-dotcom]

Applied fix to Vault_Synth here kree-dotcom/isomorph@6b0879e
Vault_Velo and Vault_Lyra fixed here kree-dotcom/isomorph@7b113ea

change the closeLoan() check to

if((outstandingisoUSD > 0) && (_collateralToUser > 0)){ //leftover debt must meet minOpeningMargin if requesting collateral back
uint256 collateralLeft = ...
...
}

This way checks are only triggered if the user is repaying some of their loan and requesting capital back. This allows us to prevent people from opening loans with a lower than minOpeningMargin but allows them to reduce their loan without needing to match the minOpeningMargin.

Note Vault_Velo has to use a slightly different method because we are handling NFTs instead of ERC20s. I have checked the _calculateProposedReturnedCapital() function we are relying on will also work fine if given an array of non-owned NFTs (i.e. the user is not receiving any collateral back just reducing their loan) by returning 0.

Fix typo in Vault_Velo kree-dotcom/isomorph@67b2f98

[IAm0x52]

Fixes look good. Only values the collateral if there is debt and the user is removing collateral