code423n4 commented 1 year ago

Lines of code

https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/cbeth/CBETHCollateral.sol#L63

Vulnerability details

Whenever refresh() is called for a collateral it does a few checks, one of them is to ensure the collateral didn't depge. It does so by calling tryPrice() and checking that the returned parameter pegPrice (which is supposed to represent the current price of the collateral on the market) is within the limits of the peg. The issue here is that CBETHCollateral.tryPrice() always returns a constant 1 for pegPrice, meaning a depeg event wouldn't be detected.

Impact

Asset wouldn't be marked as iffy/disabled in case of a depeg, one of the impacts of that is that issuance would still be possible during a depeg. This would cause a loss of assets, since when the protocol would attempt to cover the deficit caused by the depeg it'd have a bigger deficit to cover for (this would probably come at the expense of RSR stakers or other RToken holders, depending on the case).

Proof of Concept

tryPrice() returns targetPerRef for pegPrice:

    function tryPrice()
        external
        view
        override
        returns (
            uint192 low,
            uint192 high,
            uint192 pegPrice
        )
    {
        // {UoA/tok} = {UoA/ref} * {ref/tok}
        uint192 p = chainlinkFeed.price(oracleTimeout).mul(
            refPerTokChainlinkFeed.price(refPerTokChainlinkTimeout)
        );
        uint192 err = p.mul(oracleError, CEIL);

        high = p + err;
        low = p - err;
        // assert(low <= high); obviously true just by inspection

        pegPrice = targetPerRef(); // {target/ref} ETH/ETH is always 1
    }

targetPerRef always returns 1:

    /// @return {target/ref} Quantity of whole target units per whole reference unit in the peg
    function targetPerRef() public view virtual returns (uint192) {
        return FIX_ONE;
    }

Recommended Mitigation Steps

Use an oracle for pegPrice