proposeRedemption() should use the current price, not a cached price

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibShortRecord.sol#L27-L28 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L56-L211

Vulnerability details

Summary

Redemption proposals should be processed using an up to date price rather than a cached price which may be stale

Vulnerability Details

In the docs it is stated here:

shortRecords that are invalid to be proposed: if it's greater than the max allowable CR

proposeRedemption() uses a cached price p.oraclePrice to calculate the collateral Ratio of Short Records being proposed. The cached price may be using stale data so the CRs in the list of proposals may now have different CRs which violate the C.MAX_REDEMPTION_CR check and therefore should never have been included in the slate.

    function proposeRedemption(
        address asset,
        MTypes.ProposalInput[] calldata proposalInput,
        uint88 redemptionAmount,
        uint88 maxRedemptionFee
    ) external isNotFrozen(asset) nonReentrant {

        // SOME CODE

        p.oraclePrice = LibOracle.getPrice(p.asset);

        // SOME CODE

        p.currentCR = currentSR.getCollateralRatio(p.oraclePrice);
        if (p.previousCR > p.currentCR || p.currentCR >= C.MAX_REDEMPTION_CR) continue;

        // SOME CODE
    }

Impact

Invalid Short Records are added to the slate. Paying off their debt increases their CR beyond the max allowed level which can affect stability of the protocol.

Tools Used

Manual Review Foundry Testing

Recommendations

Use getSavedOrSpotOraclePrice() which will use getPrice() if it's not stale to save gas or else return the spot price.

    function proposeRedemption(
        address asset,
        MTypes.ProposalInput[] calldata proposalInput,
        uint88 redemptionAmount,
        uint88 maxRedemptionFee
    ) external isNotFrozen(asset) nonReentrant {

        // SOME CODE

-       p.oraclePrice = LibOracle.getPrice(p.asset);
+       p.oraclePrice = LibOracle.getSavedOrSpotOraclePrice(p.asset);

        // SOME CODE

        p.currentCR = currentSR.getCollateralRatio(p.oraclePrice);
        if (p.previousCR > p.currentCR || p.currentCR >= C.MAX_REDEMPTION_CR) continue;

        // SOME CODE
    }

Assessed type

Oracle

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #114

[raymondfam]

See #128.

[c4-judge]

hansfriese marked the issue as satisfactory