Ch_301 - `getPositionRisk()` will return a wrong value of risk

[sherlock-admin]

Ch_301

high

getPositionRisk() will return a wrong value of risk

Summary

In order to interact with SPELL the users need to lend() some collateral which is known as Isolated Collateral and the SoftVault will deposit them into Compound protocol to generate some lending interest (to earn passive yield)

Vulnerability Detail

to liquidate a position this function isLiquidatable() should return true

    function isLiquidatable(uint256 positionId) public view returns (bool) {
        return
            getPositionRisk(positionId) >=
            banks[positions[positionId].underlyingToken].liqThreshold;
    }

and it is subcall to getPositionRisk()

    function getPositionRisk(
        uint256 positionId
    ) public view returns (uint256 risk) {
        uint256 pv = getPositionValue(positionId);          
        uint256 ov = getDebtValue(positionId);             
        uint256 cv = getIsolatedCollateralValue(positionId);

        if (
            (cv == 0 && pv == 0 && ov == 0) || pv >= ov // Closed position or Overcollateralized position
        ) {
            risk = 0;
        } else if (cv == 0) {
            // Sth bad happened to isolated underlying token
            risk = Constants.DENOMINATOR;
        } else {
            risk = ((ov - pv) * Constants.DENOMINATOR) / cv;
        }
    }

as we can see the cv is a critical value in terms of the calculation of risk the cv is returned by getIsolatedCollateralValue()

    function getIsolatedCollateralValue(
        uint256 positionId
    ) public view override returns (uint256 icollValue) {
        Position memory pos = positions[positionId];
        // NOTE: exchangeRateStored has 18 decimals.
        uint256 underlyingAmount;
        if (_isSoftVault(pos.underlyingToken)) {
            underlyingAmount =                                              
                (ICErc20(banks[pos.debtToken].bToken).exchangeRateStored() * 
                    pos.underlyingVaultShare) /
                Constants.PRICE_PRECISION; 
        } else {
            underlyingAmount = pos.underlyingVaultShare;
        }
        icollValue = oracle.getTokenValue(
            pos.underlyingToken,
            underlyingAmount
        );
    }

and it uses exchangeRateStored() to ask Compound (CToken.sol) for the exchange rate from CToken contract

This function does not accrue interest before calculating the exchange rate

so the getPositionRisk() will return a wrong value of risk because the interest does not accrue for this position

Impact

the user (position) could get liquidated even if his position is still healthy

Code Snippet

https://github.com/compound-finance/compound-protocol/blob/master/contracts/CToken.sol#LL270C1-L286C6

    /**
     * @notice Accrue interest then return the up-to-date exchange rate
     * @return Calculated exchange rate scaled by 1e18
     */
    function exchangeRateCurrent() override public nonReentrant returns (uint) {
        accrueInterest();
        return exchangeRateStored();
    }

    /**
     * @notice Calculates the exchange rate from the underlying to the CToken
     * @dev This function does not accrue interest before calculating the exchange rate
     * @return Calculated exchange rate scaled by 1e18
     */
    function exchangeRateStored() override public view returns (uint) {
        return exchangeRateStoredInternal();
    }

Tool used

Manual Review

Recommendation

You shoud use exchangeRateCurrent() to Accrue interest first.

[Gornutz]

Since we are using a view function we are unable to use exchangeRateCurrent() we have to use exchangeRateStored()

[Ch-301]

Escalate for 10 USDC

The sponsor confirms that. so the user could get liquidated even in case his position is still healthy. I believe the rules are clear on that He decided to not fix it but the risk still exists

[sherlock-admin]

Escalate for 10 USDC

The sponsor confirms that. so the user could get liquidated even in case his position is still healthy. I believe the rules are clear on that He decided to not fix it but the risk still exists

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[ctf-sec]

Can be a valid medium

[hrishibhat]

Escalation accepted

Valid medium Although the difference in the interest accumulated here can be very low as it updates slowly, although this cannot be exactly quantified, the fact that a position can be liquidated based on outdated value makes it a valid medium.

[sherlock-admin]

Escalation accepted

Valid medium Although the difference in the interest accumulated here can be very low as it updates slowly, although this cannot be exactly quantified, the fact that a position can be liquidated based on outdated value makes it a valid medium.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[IAm0x52]

Sponsor has acknowledged this risk